Christoph Egger
2010-Oct-15 13:10 UTC
[Xen-devel] [PATCH 01/13] Nested Virtualization: tools
-- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85609 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Christoph Egger
2010-Nov-12 18:40 UTC
[Xen-devel] [PATCH 01/13] Nested Virtualization: tools
-- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85609 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tim Deegan
2010-Nov-16 11:37 UTC
Re: [Xen-devel] [PATCH 01/13] Nested Virtualization: tools
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? Tim. -- Tim Deegan <Tim.Deegan@citrix.com> Principal Software Engineer, Xen Platform Team Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Christoph Egger
2010-Nov-16 11:52 UTC
Re: [Xen-devel] [PATCH 01/13] Nested Virtualization: tools
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. Christoph -- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85609 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
George Dunlap
2010-Nov-16 12:52 UTC
Re: [Xen-devel] [PATCH 01/13] Nested Virtualization: tools
On Tue, Nov 16, 2010 at 11:52 AM, Christoph Egger <Christoph.Egger@amd.com> 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 think Tim is suggesting this: regs[3] &= SVM_FEATURE_NPT|SVM_FEATURE_LBRV|... Which will have the same effect. -George> > Christoph > > > -- > ---to satisfy European Law for business letters: > Advanced Micro Devices GmbH > Einsteinring 24, 85609 Dornach b. Muenchen > Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd > Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen > Registergericht Muenchen, HRB Nr. 43632 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tim Deegan
2010-Nov-16 13:03 UTC
Re: [Xen-devel] [PATCH 01/13] Nested Virtualization: tools
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@citrix.com> Principal Software Engineer, Xen Platform Team Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Christoph Egger
2010-Nov-16 13:10 UTC
Re: [Xen-devel] [PATCH 01/13] Nested Virtualization: tools
On Tuesday 16 November 2010 14:03:40 Tim Deegan wrote:> 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?Thanks for clarification (and the other people who guessed right). I changed that in my local tree. Christoph -- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85609 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel