Re: [Xen-devel] [PATCH 01/13] Nested Virtualization: tools

[Tim Deegan <Tim.Deegan@xxxxxxxxxx>]

At 11:52 +0000 on 16 Nov (1289908371), Christoph Egger wrote:
> On Tuesday 16 November 2010 12:37:06 Tim Deegan wrote:
> > Hi,
> >
> > At 18:40 +0000 on 12 Nov (1289587225), Christoph Egger wrote:
> > > +#define SVM_FEATURE_NPT            0x00000001
> > > +#define SVM_FEATURE_LBRV           0x00000002
> > > +#define SVM_FEATURE_SVML           0x00000004
> > > +#define SVM_FEATURE_NRIPS          0x00000008
> > > +#define SVM_FEATURE_PAUSEFILTER    0x00000400
> > > +
> > > +        /* Only passthrough SVM features which are implemented */
> > > +        edx = 0;
> > > +        if (regs[3] & SVM_FEATURE_NPT)
> > > +            edx |= SVM_FEATURE_NPT;
> > > +        if (regs[3] & SVM_FEATURE_LBRV)
> > > +            edx |= SVM_FEATURE_LBRV;
> > > +        if (regs[3] & SVM_FEATURE_NRIPS)
> > > +            edx |= SVM_FEATURE_NRIPS;
> > > +        if (regs[3] & SVM_FEATURE_PAUSEFILTER)
> > > +            edx |= SVM_FEATURE_PAUSEFILTER;
> > > +
> > > +        regs[3] = edx;
> >
> > Minor niggle - why isn't this just a single &= operation?
> 
> The l1 guest shouldn't see upcoming svm features yet.
> They will be added here when support for them is implemented.

I meant: why don't you or together the feature flags you support
(which should probably be defined in a header file with the other CPUID
bits, btw) and just 'regs[3] &= SVM_FEAURE_FOO|SVM_FEATURE_BAR|...'
instead of using ten lines of code?

It's just a coding style niggle, not a logic error. 

Tim.


-- 
Tim Deegan <Tim.Deegan@xxxxxxxxxx>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel