Christoph Egger
2010-Sep-01 14:54 UTC
[Xen-devel] [PATCH 01/13] Nested Virtualization: tools
Signed-off-by: Christoph Egger <Christoph.Egger@amd.com> -- ---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
Dong, Eddie
2010-Sep-03 07:54 UTC
[Xen-devel] RE: [PATCH 01/13] Nested Virtualization: tools
Dong, Eddie wrote:> # HG changeset patch > # User cegger > # Date 1283345869 -7200 > tools: Add nestedhvm guest config option > > diff -r 80ef08613ec2 -r ecec3d163efa tools/libxc/xc_cpuid_x86.c > --- a/tools/libxc/xc_cpuid_x86.c > +++ b/tools/libxc/xc_cpuid_x86.c > @@ -30,7 +30,7 @@ > #define set_bit(idx, dst) ((dst) |= (1u << ((idx) & 31))) > > #define DEF_MAX_BASE 0x0000000du > -#define DEF_MAX_EXT 0x80000008u > +#define DEF_MAX_EXT 0x8000000auHow can this make Intel CPU happy? You may refer to my previous comments in V2.> > static int hypervisor_is_64bit(xc_interface *xch) > { > @@ -78,7 +78,7 @@ static void xc_cpuid_brand_get(char *str > static void amd_xc_cpuid_policy( > xc_interface *xch, domid_t domid, > const unsigned int *input, unsigned int *regs, > - int is_pae) > + int is_pae, int is_nestedhvm) > { > switch ( input[0] ) > { > @@ -97,6 +97,7 @@ static void amd_xc_cpuid_policy( > /* Filter all other features according to a whitelist. */ > regs[2] &= ((is_64bit ? bitmaskof(X86_FEATURE_LAHF_LM) : 0) | > bitmaskof(X86_FEATURE_CMP_LEGACY) | > + (is_nestedhvm ? bitmaskof(X86_FEATURE_SVME) : 0) > | bitmaskof(X86_FEATURE_ALTMOVCR) | > bitmaskof(X86_FEATURE_ABM) | > bitmaskof(X86_FEATURE_SSE4A) | > @@ -121,13 +122,43 @@ static void amd_xc_cpuid_policy( > */ > regs[2] = ((regs[2] & 0xf000u) + 1) | ((regs[2] & 0xffu) << > 1) | 1u; break; > + > + case 0x8000000a: { > + uint32_t edx; > + > + if (!is_nestedhvm) { > + regs[0] = regs[1] = regs[2] = regs[3] = 0; > + break; > + } > + > +#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; > + break; > + } > + > } > } > > static void intel_xc_cpuid_policy( > xc_interface *xch, domid_t domid, > const unsigned int *input, unsigned int *regs, > - int is_pae) > + int is_pae, int is_nestedhvm) > { > switch ( input[0] ) > { > @@ -161,6 +192,11 @@ static void intel_xc_cpuid_policy( > /* Mask AMD Number of Cores information. */ > regs[2] = 0; > break; > + > + case 0x8000000a: > + /* Clear AMD SVM feature bits */ > + regs[0] = regs[1] = regs[2] = regs[3] = 0; > + break;ditto.> } > } > > @@ -169,12 +205,17 @@ static void xc_cpuid_hvm_policy( > const unsigned int *input, unsigned int *regs) > { > char brand[13]; > + unsigned long nestedhvm; > unsigned long pae; > int is_pae; > + int is_nestedhvm; > > xc_get_hvm_param(xch, domid, HVM_PARAM_PAE_ENABLED, &pae); > is_pae = !!pae; > > + xc_get_hvm_param(xch, domid, HVM_PARAM_NESTEDHVM, &nestedhvm); > + is_nestedhvm = !!nestedhvm; > + > switch ( input[0] ) > { > case 0x00000000: > @@ -260,6 +301,7 @@ static void xc_cpuid_hvm_policy( > case 0x80000004: /* ... continued */ > case 0x80000005: /* AMD L1 cache/TLB info (dumped by Intel > policy) */ case 0x80000006: /* AMD L2/3 cache/TLB info ; Intel > L2 cache features */ > + case 0x8000000a: /* AMD SVM feature bits */Should this be in amd_xc_cpuid_policy?> break; > > default: > @@ -269,9 +311,9 @@ static void xc_cpuid_hvm_policy( > > xc_cpuid_brand_get(brand); > if ( strstr(brand, "AMD") ) > - amd_xc_cpuid_policy(xch, domid, input, regs, is_pae); > + amd_xc_cpuid_policy(xch, domid, input, regs, is_pae, > is_nestedhvm); else > - intel_xc_cpuid_policy(xch, domid, input, regs, is_pae); > + intel_xc_cpuid_policy(xch, domid, input, regs, is_pae, > is_nestedhvm); > > } >Thx, Eddie _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2010-Sep-03 08:11 UTC
Re: [Xen-devel] RE: [PATCH 01/13] Nested Virtualization: tools
On 03/09/2010 08:54, "Dong, Eddie" <eddie.dong@intel.com> wrote:>> #define DEF_MAX_BASE 0x0000000du >> -#define DEF_MAX_EXT 0x80000008u >> +#define DEF_MAX_EXT 0x8000000au > > How can this make Intel CPU happy? > You may refer to my previous comments in V2.Perhaps this should be made dependent on the CPUID brand string, like some other bits in this file? K. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Dong, Eddie
2010-Sep-03 08:13 UTC
RE: [Xen-devel] RE: [PATCH 01/13] Nested Virtualization: tools
Keir Fraser wrote:> On 03/09/2010 08:54, "Dong, Eddie" <eddie.dong@intel.com> wrote: > >>> #define DEF_MAX_BASE 0x0000000du >>> -#define DEF_MAX_EXT 0x80000008u >>> +#define DEF_MAX_EXT 0x8000000au >> >> How can this make Intel CPU happy? >> You may refer to my previous comments in V2. > > Perhaps this should be made dependent on the CPUID brand string, like > some other bits in this file? > > K.Agree:) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Andre Przywara
2010-Sep-03 09:14 UTC
Re: [Xen-devel] RE: [PATCH 01/13] Nested Virtualization: tools
Dong, Eddie wrote:> Dong, Eddie wrote: >> # HG changeset patch >> # User cegger >> # Date 1283345869 -7200 >> tools: Add nestedhvm guest config option >> >> diff -r 80ef08613ec2 -r ecec3d163efa tools/libxc/xc_cpuid_x86.c >> --- a/tools/libxc/xc_cpuid_x86.c >> +++ b/tools/libxc/xc_cpuid_x86.c >> @@ -30,7 +30,7 @@ >> #define set_bit(idx, dst) ((dst) |= (1u << ((idx) & 31))) >> >> #define DEF_MAX_BASE 0x0000000du >> -#define DEF_MAX_EXT 0x80000008u >> +#define DEF_MAX_EXT 0x8000000au > > How can this make Intel CPU happy? > You may refer to my previous comments in V2.Correct me if I am wrong, but this is only a max boundary: tools/libxc/xc_cpuid_x86.c:234 case 0x80000000: if ( regs[0] > DEF_MAX_EXT ) regs[0] = DEF_MAX_EXT; break; So if an Intel CPU returns 0x80000008 here, this will be in the regs[0] field and thus any higher value in DEF_MAX_EXT does not affect the guest''s CPUID response. So as long as Intel CPUs don''t return higher values which don''t match the AMD assignment (which is extremely unlikely), extending DEF_MAX_EXT is fine. Regards, Andre. -- Andre Przywara AMD-OSRC (Dresden) Tel: x29712 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Dong, Eddie
2010-Sep-07 00:54 UTC
RE: [Xen-devel] RE: [PATCH 01/13] Nested Virtualization: tools
Andre Przywara wrote:> Dong, Eddie wrote: >> Dong, Eddie wrote: >>> # HG changeset patch >>> # User cegger >>> # Date 1283345869 -7200 >>> tools: Add nestedhvm guest config option >>> >>> diff -r 80ef08613ec2 -r ecec3d163efa tools/libxc/xc_cpuid_x86.c >>> --- a/tools/libxc/xc_cpuid_x86.c >>> +++ b/tools/libxc/xc_cpuid_x86.c >>> @@ -30,7 +30,7 @@ >>> #define set_bit(idx, dst) ((dst) |= (1u << ((idx) & 31))) >>> >>> #define DEF_MAX_BASE 0x0000000du >>> -#define DEF_MAX_EXT 0x80000008u >>> +#define DEF_MAX_EXT 0x8000000au >> >> How can this make Intel CPU happy? >> You may refer to my previous comments in V2. > Correct me if I am wrong, but this is only a max boundary: > tools/libxc/xc_cpuid_x86.c:234 > case 0x80000000: > if ( regs[0] > DEF_MAX_EXT ) > regs[0] = DEF_MAX_EXT; > break; > So if an Intel CPU returns 0x80000008 here, this will be in the > regs[0] field and thus any higher value in DEF_MAX_EXT does not > affect the guest''s CPUID response. > So as long as Intel CPUs don''t return higher values which don''t match > the AMD assignment (which is extremely unlikely), extending > DEF_MAX_EXT is fine. >But it is called as MAX_EXT and will cause some unreasonable setup of leaves. May you split the MACRO to _AMD & _INTEL, or a dynamic variable depending on CPU brand like Keir suggested? Thx, Eddie _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Andre Przywara
2010-Sep-07 09:44 UTC
Re: [Xen-devel] RE: [PATCH 01/13] Nested Virtualization: tools
Dong, Eddie wrote:> Andre Przywara wrote: >> Dong, Eddie wrote: >>> Dong, Eddie wrote: >>>> # HG changeset patch >>>> # User cegger >>>> # Date 1283345869 -7200 >>>> tools: Add nestedhvm guest config option >>>> >>>> diff -r 80ef08613ec2 -r ecec3d163efa tools/libxc/xc_cpuid_x86.c >>>> --- a/tools/libxc/xc_cpuid_x86.c >>>> +++ b/tools/libxc/xc_cpuid_x86.c >>>> @@ -30,7 +30,7 @@ >>>> #define set_bit(idx, dst) ((dst) |= (1u << ((idx) & 31))) >>>> >>>> #define DEF_MAX_BASE 0x0000000du >>>> -#define DEF_MAX_EXT 0x80000008u >>>> +#define DEF_MAX_EXT 0x8000000au >>> How can this make Intel CPU happy? >>> You may refer to my previous comments in V2. >> Correct me if I am wrong, but this is only a max boundary: >> tools/libxc/xc_cpuid_x86.c:234 >> case 0x80000000: >> if ( regs[0] > DEF_MAX_EXT ) >> regs[0] = DEF_MAX_EXT; >> break; >> So if an Intel CPU returns 0x80000008 here, this will be in the >> regs[0] field and thus any higher value in DEF_MAX_EXT does not >> affect the guest''s CPUID response. >> So as long as Intel CPUs don''t return higher values which don''t match >> the AMD assignment (which is extremely unlikely), extending >> DEF_MAX_EXT is fine. >> > But it is called as MAX_EXT and will cause some unreasonable setup of leaves.Where? If DEF_MAX_EXT would be 0x8FFFFFFF, CPUID would still return 0x80000008 on Intel CPUs. I don''t see any leaves setup because of a changed DEF_MAX_EXT value. CPUID will just return the smaller value of (DEF_MAX_EXT,native CPUID), any leafs beyond the value will not be populated in xc_cpuid_apply_policy() and thus will not appear in the HV''s struct domain->arch.cpuids array used for delivering CPUID results to guests.> May you split the MACRO to _AMD & _INTEL, or a dynamic variable depending on CPU brand like Keir suggested?I guess that is not needed. The leaf is properly handled in the {amd,intel}_xc_cpuid_policy() filters, which will only be called on the respective CPUs. Regards, Andre. -- Andre Przywara AMD-Operating System Research Center (OSRC), Dresden, Germany Tel: +49 351 448-3567-12 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Dong, Eddie
2010-Sep-07 12:39 UTC
RE: [Xen-devel] RE: [PATCH 01/13] Nested Virtualization: tools
Andre Przywara wrote:> Dong, Eddie wrote: >> Andre Przywara wrote: >>> Dong, Eddie wrote: >>>> Dong, Eddie wrote: >>>>> # HG changeset patch >>>>> # User cegger >>>>> # Date 1283345869 -7200 >>>>> tools: Add nestedhvm guest config option >>>>> >>>>> diff -r 80ef08613ec2 -r ecec3d163efa tools/libxc/xc_cpuid_x86.c >>>>> --- a/tools/libxc/xc_cpuid_x86.c >>>>> +++ b/tools/libxc/xc_cpuid_x86.c >>>>> @@ -30,7 +30,7 @@ >>>>> #define set_bit(idx, dst) ((dst) |= (1u << ((idx) & 31))) >>>>> >>>>> #define DEF_MAX_BASE 0x0000000du >>>>> -#define DEF_MAX_EXT 0x80000008u >>>>> +#define DEF_MAX_EXT 0x8000000au >>>> How can this make Intel CPU happy? >>>> You may refer to my previous comments in V2. >>> Correct me if I am wrong, but this is only a max boundary: >>> tools/libxc/xc_cpuid_x86.c:234 >>> case 0x80000000: >>> if ( regs[0] > DEF_MAX_EXT ) >>> regs[0] = DEF_MAX_EXT; >>> break; >>> So if an Intel CPU returns 0x80000008 here, this will be in the >>> regs[0] field and thus any higher value in DEF_MAX_EXT does not >>> affect the guest''s CPUID response. >>> So as long as Intel CPUs don''t return higher values which don''t >>> match the AMD assignment (which is extremely unlikely), extending >>> DEF_MAX_EXT is fine. >>> >> But it is called as MAX_EXT and will cause some unreasonable setup >> of leaves. > Where? If DEF_MAX_EXT would be 0x8FFFFFFF, CPUID would still return > 0x80000008 on Intel CPUs. I don''t see any leaves setup because of a > changed DEF_MAX_EXT value. CPUID will just return the smaller value of > (DEF_MAX_EXT,native CPUID), any leafs beyond the value will not be > populated in xc_cpuid_apply_policy() and thus will not appear in the > HV''s struct domain->arch.cpuids array used for delivering CPUID > results to guests.Well, what does DEF_MAX_EXT mean then? Isn''t it mean default maximum Extended Function CPUID Information? If yes, you shouldn''t mislead readers to think virtual Intel CPU has 0x8..A Extended Function CPUID Information now.> > >> May you split the MACRO to _AMD & _INTEL, or a dynamic variable >> depending on CPU brand like Keir suggested? > I guess that is not needed. The leaf is properly handled in the > {amd,intel}_xc_cpuid_policy() filters, which will only be called on > the respective CPUs.Are you assuming that future Intel processor won''t implement any leaf higher than 0x8...8/A either? Software should follow SDM as more as possible. Thx, Eddie _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel