| Message ID | 1458056124-8024-10-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 15, 2016 at 03:35:05PM +0000, Andrew Cooper wrote: > All of this information will be used by the toolstack to make informed > levelling decisions for VMs, and by Xen to sanity check toolstack-provided > information. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>> On 15.03.16 at 16:35, <andrew.cooper3@citrix.com> wrote: > +static void __init calculate_hvm_featureset(void) > +{ > + unsigned int i; > + const uint32_t *hvm_featuremask; > + > + if ( !hvm_enabled ) > + return; > + > + hvm_featuremask = hvm_funcs.hap_supported ? > + hvm_hap_featuremask : hvm_shadow_featuremask; I know I asked about this before, and it still puzzles me. Could you add some explanation of this to the commit message or a comment? > + for ( i = 0; i < FSCAPINTS; ++i ) > + hvm_featureset[i] = host_featureset[i] & hvm_featuremask[i]; > + > + /* Unconditionally claim to be able to set the hypervisor bit. */ > + __set_bit(X86_FEATURE_HYPERVISOR, hvm_featureset); > + > + /* > + * Allow the toolstack to set HTT and CMP_LEGACY. These bits affect how > + * to interpret topology information in other cpuid leaves. > + */ > + __set_bit(X86_FEATURE_HTT, pv_featureset); > + __set_bit(X86_FEATURE_CMP_LEGACY, pv_featureset); Copy-n-paste mistake again (pv feature set in hvm function)? With these taken care of Acked-by: Jan Beulich <jbeulich@suse.com> Jan
On 18/03/16 17:09, Jan Beulich wrote: >>>> On 15.03.16 at 16:35, <andrew.cooper3@citrix.com> wrote: >> +static void __init calculate_hvm_featureset(void) >> +{ >> + unsigned int i; >> + const uint32_t *hvm_featuremask; >> + >> + if ( !hvm_enabled ) >> + return; >> + >> + hvm_featuremask = hvm_funcs.hap_supported ? >> + hvm_hap_featuremask : hvm_shadow_featuremask; > I know I asked about this before, and it still puzzles me. Could you > add some explanation of this to the commit message or a comment? I am not sure what more I can say about it. The toolstack needs the be able to see the difference between a guest started in shadow mode on hap hardware, to be able to correctly calculate whether it can migrate to hap-incapable hardware. (Specifically - this is a usecase that I must support.) There is no "get_cpuid_policy" hypercall, so a toolstack cannot query what a guest can actually see, after Xen's dynamic checks have taken place. Implementing get_cpuid_policy depends on Xen having a full model of cpuid state, which is too much work to be rolled together into this series. Like all of the dynamic checks later, it is only an intermediate step, and I do have plans to remove them longterm when Xen has a better model of cpuid. ~Andrew
>>> On 22.03.16 at 12:23, <andrew.cooper3@citrix.com> wrote: > On 18/03/16 17:09, Jan Beulich wrote: >>>>> On 15.03.16 at 16:35, <andrew.cooper3@citrix.com> wrote: >>> +static void __init calculate_hvm_featureset(void) >>> +{ >>> + unsigned int i; >>> + const uint32_t *hvm_featuremask; >>> + >>> + if ( !hvm_enabled ) >>> + return; >>> + >>> + hvm_featuremask = hvm_funcs.hap_supported ? >>> + hvm_hap_featuremask : hvm_shadow_featuremask; >> I know I asked about this before, and it still puzzles me. Could you >> add some explanation of this to the commit message or a comment? > > I am not sure what more I can say about it. > > The toolstack needs the be able to see the difference between a guest > started in shadow mode on hap hardware, to be able to correctly > calculate whether it can migrate to hap-incapable hardware. Difference to what? A HAP guest? How would that difference be invisible if you surfaced two feature sets? Even more - with just one feature set exposed, how would the tool stack see that very difference? > There is no "get_cpuid_policy" hypercall, so a toolstack cannot query > what a guest can actually see, after Xen's dynamic checks have taken place. > > Implementing get_cpuid_policy depends on Xen having a full model of > cpuid state, which is too much work to be rolled together into this series. > > Like all of the dynamic checks later, it is only an intermediate step, > and I do have plans to remove them longterm when Xen has a better model > of cpuid. I understand this is subject to further changes down the road. But we all know that getting there may take time, so getting things right for the time until then is quite necessary (the more that we're going to release 4.7 in such intermediate state aiui). Jan
On 22/03/16 12:39, Jan Beulich wrote: >>>> On 22.03.16 at 12:23, <andrew.cooper3@citrix.com> wrote: >> On 18/03/16 17:09, Jan Beulich wrote: >>>>>> On 15.03.16 at 16:35, <andrew.cooper3@citrix.com> wrote: >>>> +static void __init calculate_hvm_featureset(void) >>>> +{ >>>> + unsigned int i; >>>> + const uint32_t *hvm_featuremask; >>>> + >>>> + if ( !hvm_enabled ) >>>> + return; >>>> + >>>> + hvm_featuremask = hvm_funcs.hap_supported ? >>>> + hvm_hap_featuremask : hvm_shadow_featuremask; >>> I know I asked about this before, and it still puzzles me. Could you >>> add some explanation of this to the commit message or a comment? >> I am not sure what more I can say about it. >> >> The toolstack needs the be able to see the difference between a guest >> started in shadow mode on hap hardware, to be able to correctly >> calculate whether it can migrate to hap-incapable hardware. > Difference to what? A HAP guest? How would that difference be > invisible if you surfaced two feature sets? Even more - with just > one feature set exposed, how would the tool stack see that very > difference? At the moment, a toolstack creates a domain, and has no clue what the domain can actually see in cpuid. In particular, it can't retrieve the "lost bits" which Xen dynamically disables. As a result, creating a shadow domain on a hap-capable host results in misinformation about which features the guest was advertised at boot. This in turn leads to an erronious decision that the domain can't be migrated to a hap-incapable host. > >> There is no "get_cpuid_policy" hypercall, so a toolstack cannot query >> what a guest can actually see, after Xen's dynamic checks have taken place. >> >> Implementing get_cpuid_policy depends on Xen having a full model of >> cpuid state, which is too much work to be rolled together into this series. >> >> Like all of the dynamic checks later, it is only an intermediate step, >> and I do have plans to remove them longterm when Xen has a better model >> of cpuid. > I understand this is subject to further changes down the road. But > we all know that getting there may take time, so getting things > right for the time until then is quite necessary (the more that we're > going to release 4.7 in such intermediate state aiui). This is all an internal implementation detail. It (very deliberately) doesn't manifest in the public API. It exists only internally to Xen, and in the libxc wrapper exposing the autogenerated information. ~Andrew
>>> On 22.03.16 at 15:37, <andrew.cooper3@citrix.com> wrote: > On 22/03/16 12:39, Jan Beulich wrote: >>>>> On 22.03.16 at 12:23, <andrew.cooper3@citrix.com> wrote: >>> On 18/03/16 17:09, Jan Beulich wrote: >>>>>>> On 15.03.16 at 16:35, <andrew.cooper3@citrix.com> wrote: >>>>> +static void __init calculate_hvm_featureset(void) >>>>> +{ >>>>> + unsigned int i; >>>>> + const uint32_t *hvm_featuremask; >>>>> + >>>>> + if ( !hvm_enabled ) >>>>> + return; >>>>> + >>>>> + hvm_featuremask = hvm_funcs.hap_supported ? >>>>> + hvm_hap_featuremask : hvm_shadow_featuremask; >>>> I know I asked about this before, and it still puzzles me. Could you >>>> add some explanation of this to the commit message or a comment? >>> I am not sure what more I can say about it. >>> >>> The toolstack needs the be able to see the difference between a guest >>> started in shadow mode on hap hardware, to be able to correctly >>> calculate whether it can migrate to hap-incapable hardware. >> Difference to what? A HAP guest? How would that difference be >> invisible if you surfaced two feature sets? Even more - with just >> one feature set exposed, how would the tool stack see that very >> difference? > > At the moment, a toolstack creates a domain, and has no clue what the > domain can actually see in cpuid. In particular, it can't retrieve the > "lost bits" which Xen dynamically disables. One more reason to expose the shadow and HAP feature sets separately, it would seem to me. Then the tool stack can have a clue. Jan > As a result, creating a shadow domain on a hap-capable host results in > misinformation about which features the guest was advertised at boot. > This in turn leads to an erronious decision that the domain can't be > migrated to a hap-incapable host. > >> >>> There is no "get_cpuid_policy" hypercall, so a toolstack cannot query >>> what a guest can actually see, after Xen's dynamic checks have taken place. >>> >>> Implementing get_cpuid_policy depends on Xen having a full model of >>> cpuid state, which is too much work to be rolled together into this series. >>> >>> Like all of the dynamic checks later, it is only an intermediate step, >>> and I do have plans to remove them longterm when Xen has a better model >>> of cpuid. >> I understand this is subject to further changes down the road. But >> we all know that getting there may take time, so getting things >> right for the time until then is quite necessary (the more that we're >> going to release 4.7 in such intermediate state aiui). > > This is all an internal implementation detail. It (very deliberately) > doesn't manifest in the public API. > > It exists only internally to Xen, and in the libxc wrapper exposing the > autogenerated information. > > ~Andrew
On 22/03/16 14:52, Jan Beulich wrote: >>>> On 22.03.16 at 15:37, <andrew.cooper3@citrix.com> wrote: >> On 22/03/16 12:39, Jan Beulich wrote: >>>>>> On 22.03.16 at 12:23, <andrew.cooper3@citrix.com> wrote: >>>> On 18/03/16 17:09, Jan Beulich wrote: >>>>>>>> On 15.03.16 at 16:35, <andrew.cooper3@citrix.com> wrote: >>>>>> +static void __init calculate_hvm_featureset(void) >>>>>> +{ >>>>>> + unsigned int i; >>>>>> + const uint32_t *hvm_featuremask; >>>>>> + >>>>>> + if ( !hvm_enabled ) >>>>>> + return; >>>>>> + >>>>>> + hvm_featuremask = hvm_funcs.hap_supported ? >>>>>> + hvm_hap_featuremask : hvm_shadow_featuremask; >>>>> I know I asked about this before, and it still puzzles me. Could you >>>>> add some explanation of this to the commit message or a comment? >>>> I am not sure what more I can say about it. >>>> >>>> The toolstack needs the be able to see the difference between a guest >>>> started in shadow mode on hap hardware, to be able to correctly >>>> calculate whether it can migrate to hap-incapable hardware. >>> Difference to what? A HAP guest? How would that difference be >>> invisible if you surfaced two feature sets? Even more - with just >>> one feature set exposed, how would the tool stack see that very >>> difference? >> At the moment, a toolstack creates a domain, and has no clue what the >> domain can actually see in cpuid. In particular, it can't retrieve the >> "lost bits" which Xen dynamically disables. > One more reason to expose the shadow and HAP feature sets > separately, it would seem to me. Then the tool stack can have > a clue. When "get_cpuid" works properly, it will no longer be needed, which is specifically why I am not putting it in the API. The toolstack will be able to construct an arbitrary domain, call get_cpuid to see exactly what the guest will see, and use that as the basis of migrateability decisions. It is just awkward that this information is needed at the moment when a suitable interface from Xen isn't available. ~Andrew
>>> On 22.03.16 at 16:01, <andrew.cooper3@citrix.com> wrote: > On 22/03/16 14:52, Jan Beulich wrote: >>>>> On 22.03.16 at 15:37, <andrew.cooper3@citrix.com> wrote: >>> On 22/03/16 12:39, Jan Beulich wrote: >>>>>>> On 22.03.16 at 12:23, <andrew.cooper3@citrix.com> wrote: >>>>> On 18/03/16 17:09, Jan Beulich wrote: >>>>>>>>> On 15.03.16 at 16:35, <andrew.cooper3@citrix.com> wrote: >>>>>>> +static void __init calculate_hvm_featureset(void) >>>>>>> +{ >>>>>>> + unsigned int i; >>>>>>> + const uint32_t *hvm_featuremask; >>>>>>> + >>>>>>> + if ( !hvm_enabled ) >>>>>>> + return; >>>>>>> + >>>>>>> + hvm_featuremask = hvm_funcs.hap_supported ? >>>>>>> + hvm_hap_featuremask : hvm_shadow_featuremask; >>>>>> I know I asked about this before, and it still puzzles me. Could you >>>>>> add some explanation of this to the commit message or a comment? >>>>> I am not sure what more I can say about it. >>>>> >>>>> The toolstack needs the be able to see the difference between a guest >>>>> started in shadow mode on hap hardware, to be able to correctly >>>>> calculate whether it can migrate to hap-incapable hardware. >>>> Difference to what? A HAP guest? How would that difference be >>>> invisible if you surfaced two feature sets? Even more - with just >>>> one feature set exposed, how would the tool stack see that very >>>> difference? >>> At the moment, a toolstack creates a domain, and has no clue what the >>> domain can actually see in cpuid. In particular, it can't retrieve the >>> "lost bits" which Xen dynamically disables. >> One more reason to expose the shadow and HAP feature sets >> separately, it would seem to me. Then the tool stack can have >> a clue. > > When "get_cpuid" works properly, it will no longer be needed, which is > specifically why I am not putting it in the API. This is a sysctl iirc, so not set in stone if added. Jan > The toolstack will be able to construct an arbitrary domain, call > get_cpuid to see exactly what the guest will see, and use that as the > basis of migrateability decisions. > > It is just awkward that this information is needed at the moment when a > suitable interface from Xen isn't available. > > ~Andrew
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c index 77e008a..174cfa0 100644 --- a/xen/arch/x86/cpuid.c +++ b/xen/arch/x86/cpuid.c @@ -1,14 +1,174 @@ #include <xen/init.h> #include <xen/lib.h> #include <asm/cpuid.h> +#include <asm/hvm/hvm.h> +#include <asm/hvm/vmx/vmcs.h> +#include <asm/processor.h> const uint32_t known_features[] = INIT_KNOWN_FEATURES; const uint32_t special_features[] = INIT_SPECIAL_FEATURES; +static const uint32_t __initconst pv_featuremask[] = INIT_PV_FEATURES; +static const uint32_t __initconst hvm_shadow_featuremask[] = INIT_HVM_SHADOW_FEATURES; +static const uint32_t __initconst hvm_hap_featuremask[] = INIT_HVM_HAP_FEATURES; + +uint32_t __read_mostly raw_featureset[FSCAPINTS]; +uint32_t __read_mostly pv_featureset[FSCAPINTS]; +uint32_t __read_mostly hvm_featureset[FSCAPINTS]; + +static void __init sanitise_featureset(uint32_t *fs) +{ + unsigned int i; + + for ( i = 0; i < FSCAPINTS; ++i ) + { + /* Clamp to known mask. */ + fs[i] &= known_features[i]; + } + + /* + * Sort out shared bits. We are constructing a featureset which needs to + * be applicable to a cross-vendor case. Intel strictly clears the common + * bits in e1d, while AMD strictly duplicates them. + * + * We duplicate them here to be compatible with AMD while on Intel, and + * rely on logic closer to the guest to make the featureset stricter if + * emulating Intel. + */ + fs[FEATURESET_e1d] = ((fs[FEATURESET_1d] & CPUID_COMMON_1D_FEATURES) | + (fs[FEATURESET_e1d] & ~CPUID_COMMON_1D_FEATURES)); +} + +static void __init calculate_raw_featureset(void) +{ + unsigned int max, tmp; + + max = cpuid_eax(0); + + if ( max >= 1 ) + cpuid(0x1, &tmp, &tmp, + &raw_featureset[FEATURESET_1c], + &raw_featureset[FEATURESET_1d]); + if ( max >= 7 ) + cpuid_count(0x7, 0, &tmp, + &raw_featureset[FEATURESET_7b0], + &raw_featureset[FEATURESET_7c0], + &tmp); + if ( max >= 0xd ) + cpuid_count(0xd, 1, + &raw_featureset[FEATURESET_Da1], + &tmp, &tmp, &tmp); + + max = cpuid_eax(0x80000000); + if ( (max >> 16) != 0x8000 ) + return; + + if ( max >= 0x80000001 ) + cpuid(0x80000001, &tmp, &tmp, + &raw_featureset[FEATURESET_e1c], + &raw_featureset[FEATURESET_e1d]); + if ( max >= 0x80000007 ) + cpuid(0x80000007, &tmp, &tmp, &tmp, + &raw_featureset[FEATURESET_e7d]); + if ( max >= 0x80000008 ) + cpuid(0x80000008, &tmp, + &raw_featureset[FEATURESET_e8b], + &tmp, &tmp); +} + +static void __init calculate_pv_featureset(void) +{ + unsigned int i; + + for ( i = 0; i < FSCAPINTS; ++i ) + pv_featureset[i] = host_featureset[i] & pv_featuremask[i]; + + /* Unconditionally claim to be able to set the hypervisor bit. */ + __set_bit(X86_FEATURE_HYPERVISOR, pv_featureset); + + /* + * Allow the toolstack to set HTT and CMP_LEGACY. These bits affect how + * to interpret topology information in other cpuid leaves. + */ + __set_bit(X86_FEATURE_HTT, pv_featureset); + __set_bit(X86_FEATURE_CMP_LEGACY, pv_featureset); + + sanitise_featureset(pv_featureset); +} + +static void __init calculate_hvm_featureset(void) +{ + unsigned int i; + const uint32_t *hvm_featuremask; + + if ( !hvm_enabled ) + return; + + hvm_featuremask = hvm_funcs.hap_supported ? + hvm_hap_featuremask : hvm_shadow_featuremask; + + for ( i = 0; i < FSCAPINTS; ++i ) + hvm_featureset[i] = host_featureset[i] & hvm_featuremask[i]; + + /* Unconditionally claim to be able to set the hypervisor bit. */ + __set_bit(X86_FEATURE_HYPERVISOR, hvm_featureset); + + /* + * Allow the toolstack to set HTT and CMP_LEGACY. These bits affect how + * to interpret topology information in other cpuid leaves. + */ + __set_bit(X86_FEATURE_HTT, pv_featureset); + __set_bit(X86_FEATURE_CMP_LEGACY, pv_featureset); + + /* + * Xen can provide an APIC emulation to HVM guests even if the host's APIC + * isn't enabled. + */ + __set_bit(X86_FEATURE_APIC, hvm_featureset); + + /* + * On AMD, PV guests are entirely unable to use 'sysenter' as Xen runs in + * long mode (and init_amd() has cleared it out of host capabilities), but + * HVM guests are able if running in protected mode. + */ + if ( (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) && + test_bit(X86_FEATURE_SEP, raw_featureset) ) + __set_bit(X86_FEATURE_SEP, hvm_featureset); + + /* + * With VT-x, some features are only supported by Xen if dedicated + * hardware support is also available. + */ + if ( cpu_has_vmx ) + { + if ( !(vmx_vmexit_control & VM_EXIT_CLEAR_BNDCFGS) || + !(vmx_vmentry_control & VM_ENTRY_LOAD_BNDCFGS) ) + __clear_bit(X86_FEATURE_MPX, hvm_featureset); + + if ( !cpu_has_vmx_xsaves ) + __clear_bit(X86_FEATURE_XSAVES, hvm_featureset); + + if ( !cpu_has_vmx_pcommit ) + __clear_bit(X86_FEATURE_PCOMMIT, hvm_featureset); + } + + sanitise_featureset(hvm_featureset); +} + +void __init calculate_featuresets(void) +{ + calculate_raw_featureset(); + calculate_pv_featureset(); + calculate_hvm_featureset(); +} + static void __init __maybe_unused build_assertions(void) { BUILD_BUG_ON(ARRAY_SIZE(known_features) != FSCAPINTS); BUILD_BUG_ON(ARRAY_SIZE(special_features) != FSCAPINTS); + BUILD_BUG_ON(ARRAY_SIZE(pv_featuremask) != FSCAPINTS); + BUILD_BUG_ON(ARRAY_SIZE(hvm_shadow_featuremask) != FSCAPINTS); + BUILD_BUG_ON(ARRAY_SIZE(hvm_hap_featuremask) != FSCAPINTS); } /* diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index a8bf2c9..c9e52fe 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -50,6 +50,7 @@ #include <asm/nmi.h> #include <asm/alternative.h> #include <asm/mc146818rtc.h> +#include <asm/cpuid.h> /* opt_nosmp: If true, secondary processors are ignored. */ static bool_t __initdata opt_nosmp; @@ -1494,6 +1495,8 @@ void __init noreturn __start_xen(unsigned long mbi_p) "Multiple initrd candidates, picking module #%u\n", initrdidx); + calculate_featuresets(); + /* * Temporarily clear SMAP in CR4 to allow user-accesses in construct_dom0(). * This saves a large number of corner cases interactions with diff --git a/xen/include/asm-x86/cpuid.h b/xen/include/asm-x86/cpuid.h index 0ecf357..5041bcd 100644 --- a/xen/include/asm-x86/cpuid.h +++ b/xen/include/asm-x86/cpuid.h @@ -6,12 +6,29 @@ #define FSCAPINTS FEATURESET_NR_ENTRIES +#define FEATURESET_1d 0 /* 0x00000001.edx */ +#define FEATURESET_1c 1 /* 0x00000001.ecx */ +#define FEATURESET_e1d 2 /* 0x80000001.edx */ +#define FEATURESET_e1c 3 /* 0x80000001.ecx */ +#define FEATURESET_Da1 4 /* 0x0000000d:1.eax */ +#define FEATURESET_7b0 5 /* 0x00000007:0.ebx */ +#define FEATURESET_7c0 6 /* 0x00000007:0.ecx */ +#define FEATURESET_e7d 7 /* 0x80000007.edx */ +#define FEATURESET_e8b 8 /* 0x80000008.ebx */ + #ifndef __ASSEMBLY__ #include <xen/types.h> extern const uint32_t known_features[FSCAPINTS]; extern const uint32_t special_features[FSCAPINTS]; +extern uint32_t raw_featureset[FSCAPINTS]; +#define host_featureset boot_cpu_data.x86_capability +extern uint32_t pv_featureset[FSCAPINTS]; +extern uint32_t hvm_featureset[FSCAPINTS]; + +void calculate_featuresets(void); + #endif /* __ASSEMBLY__ */ #endif /* !__X86_CPUID_H__ */
All of this information will be used by the toolstack to make informed levelling decisions for VMs, and by Xen to sanity check toolstack-provided information. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> v3: * Move as much as possible into .init. * Fix the handing of the shared bits for the cross-vendor case. * Fix extended check. --- xen/arch/x86/cpuid.c | 160 ++++++++++++++++++++++++++++++++++++++++++++ xen/arch/x86/setup.c | 3 + xen/include/asm-x86/cpuid.h | 17 +++++ 3 files changed, 180 insertions(+)