| Message ID | 1458056124-8024-21-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 15.03.16 at 16:35, <andrew.cooper3@citrix.com> wrote: > @@ -87,6 +88,129 @@ static void update_domain_cpuid_info(struct domain *d, > d->arch.x86_model = (ctl->eax >> 4) & 0xf; > if ( d->arch.x86 >= 0x6 ) > d->arch.x86_model |= (ctl->eax >> 12) & 0xf0; > + > + if ( is_pv_domain(d) && ((levelling_caps & LCAP_1cd) == LCAP_1cd) ) > + { > + uint64_t mask = cpuidmask_defaults._1cd; > + uint32_t ecx = ctl->ecx & pv_featureset[FEATURESET_1c]; > + uint32_t edx = ctl->edx & pv_featureset[FEATURESET_1d]; > + > + /* > + * Must expose hosts HTT value so a guest using native CPU can DYM "native CPUID" here? > + * correctly interpret other leaves which cannot be masked. > + */ > + edx &= ~cpufeat_mask(X86_FEATURE_HTT); > + if ( cpu_has_htt ) > + edx |= cpufeat_mask(X86_FEATURE_HTT); > + > + switch ( boot_cpu_data.x86_vendor ) > + { > + case X86_VENDOR_INTEL: > + mask &= ((uint64_t)edx << 32) | ecx; > + break; > + > + case X86_VENDOR_AMD: > + mask &= ((uint64_t)ecx << 32) | edx; > + > + /* Fast-forward bits - Must be set. */ > + if (ecx & cpufeat_mask(X86_FEATURE_XSAVE)) Missing blanks inside parens. Also I think the comment should be expanded to include the fact that the need to do this here but not in the Intel case was empirically(?) determined - just in case something isn't quite right with this on some future hardware, and readers (possibly including ourselves) wonder. > + case X86_VENDOR_AMD: > + /* > + * Must expose hosts CMP_LEGACY value so a guest using native > + * CPU can correctly interpret other leaves which cannot be CPUID again? > + * masked. > + */ > + ecx &= ~cpufeat_mask(X86_FEATURE_CMP_LEGACY); > + if ( cpu_has_cmp_legacy ) > + ecx |= cpufeat_mask(X86_FEATURE_CMP_LEGACY); > + > + mask &= ((uint64_t)ecx << 32) | edx; > + > + /* Fast-forward bits - Must be set. */ > + ecx = 0; > + edx = cpufeat_mask(X86_FEATURE_APIC); > + > + mask |= ((uint64_t)ecx << 32) | edx; This certainly looks strange (as in more complicated than it needs to be) - why not just mask |= cpufeat_mask(X86_FEATURE_APIC)? Jan
On 21/03/16 17:06, Jan Beulich wrote: >>>> On 15.03.16 at 16:35, <andrew.cooper3@citrix.com> wrote: >> @@ -87,6 +88,129 @@ static void update_domain_cpuid_info(struct domain *d, >> d->arch.x86_model = (ctl->eax >> 4) & 0xf; >> if ( d->arch.x86 >= 0x6 ) >> d->arch.x86_model |= (ctl->eax >> 12) & 0xf0; >> + >> + if ( is_pv_domain(d) && ((levelling_caps & LCAP_1cd) == LCAP_1cd) ) >> + { >> + uint64_t mask = cpuidmask_defaults._1cd; >> + uint32_t ecx = ctl->ecx & pv_featureset[FEATURESET_1c]; >> + uint32_t edx = ctl->edx & pv_featureset[FEATURESET_1d]; >> + >> + /* >> + * Must expose hosts HTT value so a guest using native CPU can > DYM "native CPUID" here? Yes. > >> + * correctly interpret other leaves which cannot be masked. >> + */ >> + edx &= ~cpufeat_mask(X86_FEATURE_HTT); >> + if ( cpu_has_htt ) >> + edx |= cpufeat_mask(X86_FEATURE_HTT); >> + >> + switch ( boot_cpu_data.x86_vendor ) >> + { >> + case X86_VENDOR_INTEL: >> + mask &= ((uint64_t)edx << 32) | ecx; >> + break; >> + >> + case X86_VENDOR_AMD: >> + mask &= ((uint64_t)ecx << 32) | edx; >> + >> + /* Fast-forward bits - Must be set. */ >> + if (ecx & cpufeat_mask(X86_FEATURE_XSAVE)) > Missing blanks inside parens. Also I think the comment should be > expanded to include the fact that the need to do this here but > not in the Intel case was empirically(?) determined It was empirically determined that AMD did have to behave like this. > - just in case > something isn't quite right with this on some future hardware, > and readers (possibly including ourselves) wonder. Intel and AMD masking MSRs are fundamentally different, and function differently. Intel MSRs are documented strictly an & mask, and experimentally, are applied before OSXSAVE/APIC are fast-forwarded from hardware. AMD MSRs are documented strictly as an override, with a caution to avoid setting bits which aren't actually supported in the hardware, and experimentally need the fast-forward bits set if fast-forwarding is to occur. I suppose I could split this up and move this logic into cpu/{amd,intel}.c as appropriate, and call a vendor specific function to appropriately translate a guest cpuid value into a mask value. However, a lot of common logic would end up needing to be duplicated. > >> + case X86_VENDOR_AMD: >> + /* >> + * Must expose hosts CMP_LEGACY value so a guest using native >> + * CPU can correctly interpret other leaves which cannot be > CPUID again? Yes > >> + * masked. >> + */ >> + ecx &= ~cpufeat_mask(X86_FEATURE_CMP_LEGACY); >> + if ( cpu_has_cmp_legacy ) >> + ecx |= cpufeat_mask(X86_FEATURE_CMP_LEGACY); >> + >> + mask &= ((uint64_t)ecx << 32) | edx; >> + >> + /* Fast-forward bits - Must be set. */ >> + ecx = 0; >> + edx = cpufeat_mask(X86_FEATURE_APIC); >> + >> + mask |= ((uint64_t)ecx << 32) | edx; > This certainly looks strange (as in more complicated than it needs to > be) - why not just mask |= cpufeat_mask(X86_FEATURE_APIC)? Copy and paste from the leaf 1 case. I specifically wanted to avoid directly manipulating the mask value, as it is already confusing enough which way ecx and edx are. ~Andrew
>>> On 22.03.16 at 17:37, <andrew.cooper3@citrix.com> wrote: > On 21/03/16 17:06, Jan Beulich wrote: >>>>> On 15.03.16 at 16:35, <andrew.cooper3@citrix.com> wrote: >>> + switch ( boot_cpu_data.x86_vendor ) >>> + { >>> + case X86_VENDOR_INTEL: >>> + mask &= ((uint64_t)edx << 32) | ecx; >>> + break; >>> + >>> + case X86_VENDOR_AMD: >>> + mask &= ((uint64_t)ecx << 32) | edx; >>> + >>> + /* Fast-forward bits - Must be set. */ >>> + if (ecx & cpufeat_mask(X86_FEATURE_XSAVE)) >> Missing blanks inside parens. Also I think the comment should be >> expanded to include the fact that the need to do this here but >> not in the Intel case was empirically(?) determined > > It was empirically determined that AMD did have to behave like this. > >> - just in case >> something isn't quite right with this on some future hardware, >> and readers (possibly including ourselves) wonder. > > Intel and AMD masking MSRs are fundamentally different, and function > differently. > > Intel MSRs are documented strictly an & mask, and experimentally, are > applied before OSXSAVE/APIC are fast-forwarded from hardware. > > AMD MSRs are documented strictly as an override, with a caution to avoid > setting bits which aren't actually supported in the hardware, and > experimentally need the fast-forward bits set if fast-forwarding is to > occur. > > I suppose I could split this up and move this logic into > cpu/{amd,intel}.c as appropriate, and call a vendor specific function to > appropriately translate a guest cpuid value into a mask value. However, > a lot of common logic would end up needing to be duplicated. I'm certainly against code duplication. And just to re-iterate - all I have been asking for was to write some of what you've said in a source comment. Jan
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c index b34a295..8db8f3b 100644 --- a/xen/arch/x86/domctl.c +++ b/xen/arch/x86/domctl.c @@ -36,6 +36,7 @@ #include <asm/xstate.h> #include <asm/debugger.h> #include <asm/psr.h> +#include <asm/cpuid.h> static int gdbsx_guest_mem_io(domid_t domid, struct xen_domctl_gdbsx_memio *iop) { @@ -87,6 +88,129 @@ static void update_domain_cpuid_info(struct domain *d, d->arch.x86_model = (ctl->eax >> 4) & 0xf; if ( d->arch.x86 >= 0x6 ) d->arch.x86_model |= (ctl->eax >> 12) & 0xf0; + + if ( is_pv_domain(d) && ((levelling_caps & LCAP_1cd) == LCAP_1cd) ) + { + uint64_t mask = cpuidmask_defaults._1cd; + uint32_t ecx = ctl->ecx & pv_featureset[FEATURESET_1c]; + uint32_t edx = ctl->edx & pv_featureset[FEATURESET_1d]; + + /* + * Must expose hosts HTT value so a guest using native CPU can + * correctly interpret other leaves which cannot be masked. + */ + edx &= ~cpufeat_mask(X86_FEATURE_HTT); + if ( cpu_has_htt ) + edx |= cpufeat_mask(X86_FEATURE_HTT); + + switch ( boot_cpu_data.x86_vendor ) + { + case X86_VENDOR_INTEL: + mask &= ((uint64_t)edx << 32) | ecx; + break; + + case X86_VENDOR_AMD: + mask &= ((uint64_t)ecx << 32) | edx; + + /* Fast-forward bits - Must be set. */ + if (ecx & cpufeat_mask(X86_FEATURE_XSAVE)) + ecx = cpufeat_mask(X86_FEATURE_OSXSAVE); + else + ecx = 0; + edx = cpufeat_mask(X86_FEATURE_APIC); + + mask |= ((uint64_t)ecx << 32) | edx; + break; + } + + d->arch.pv_domain.cpuidmasks->_1cd = mask; + } + break; + + case 6: + if ( is_pv_domain(d) && ((levelling_caps & LCAP_6c) == LCAP_6c) ) + { + uint64_t mask = cpuidmask_defaults._6c; + + if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD ) + mask &= (~0ULL << 32) | ctl->ecx; + + d->arch.pv_domain.cpuidmasks->_6c = mask; + } + break; + + case 7: + if ( ctl->input[1] != 0 ) + break; + + if ( is_pv_domain(d) && ((levelling_caps & LCAP_7ab0) == LCAP_7ab0) ) + { + uint64_t mask = cpuidmask_defaults._7ab0; + uint32_t eax = ctl->eax; + uint32_t ebx = ctl->ebx & pv_featureset[FEATURESET_7b0]; + + if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD ) + mask &= ((uint64_t)eax << 32) | ebx; + + d->arch.pv_domain.cpuidmasks->_7ab0 = mask; + } + break; + + case 0xd: + if ( ctl->input[1] != 1 ) + break; + + if ( is_pv_domain(d) && ((levelling_caps & LCAP_Da1) == LCAP_Da1) ) + { + uint64_t mask = cpuidmask_defaults.Da1; + uint32_t eax = ctl->eax & pv_featureset[FEATURESET_Da1]; + + if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ) + mask &= (~0ULL << 32) | eax; + + d->arch.pv_domain.cpuidmasks->Da1 = mask; + } + break; + + case 0x80000001: + if ( is_pv_domain(d) && ((levelling_caps & LCAP_e1cd) == LCAP_e1cd) ) + { + uint64_t mask = cpuidmask_defaults.e1cd; + uint32_t ecx = ctl->ecx & pv_featureset[FEATURESET_e1c]; + uint32_t edx = ctl->edx & pv_featureset[FEATURESET_e1d]; + + /* If emulating Intel, clear the duplicated features in e1d. */ + if ( d->arch.x86_vendor == X86_VENDOR_INTEL ) + edx &= ~CPUID_COMMON_1D_FEATURES; + + switch ( boot_cpu_data.x86_vendor ) + { + case X86_VENDOR_INTEL: + mask &= ((uint64_t)edx << 32) | ecx; + break; + + case X86_VENDOR_AMD: + /* + * Must expose hosts CMP_LEGACY value so a guest using native + * CPU can correctly interpret other leaves which cannot be + * masked. + */ + ecx &= ~cpufeat_mask(X86_FEATURE_CMP_LEGACY); + if ( cpu_has_cmp_legacy ) + ecx |= cpufeat_mask(X86_FEATURE_CMP_LEGACY); + + mask &= ((uint64_t)ecx << 32) | edx; + + /* Fast-forward bits - Must be set. */ + ecx = 0; + edx = cpufeat_mask(X86_FEATURE_APIC); + + mask |= ((uint64_t)ecx << 32) | edx; + break; + } + + d->arch.pv_domain.cpuidmasks->e1cd = mask; + } break; } } diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h index 50414e3..79236ea 100644 --- a/xen/include/asm-x86/cpufeature.h +++ b/xen/include/asm-x86/cpufeature.h @@ -81,6 +81,7 @@ #define cpu_has_xsavec boot_cpu_has(X86_FEATURE_XSAVEC) #define cpu_has_xgetbv1 boot_cpu_has(X86_FEATURE_XGETBV1) #define cpu_has_xsaves boot_cpu_has(X86_FEATURE_XSAVES) +#define cpu_has_cmp_legacy boot_cpu_has(X86_FEATURE_CMP_LEGACY) #define cpu_has_hypervisor boot_cpu_has(X86_FEATURE_HYPERVISOR) enum _cache_type {
This allows PV domains with different featuresets to observe different values from a native cpuid instruction, on supporting hardware. It is important to leak the host view of HTT and CMP_LEGACY through to guests, even though they could be hidden. These flags affect how to interpret other cpuid leaves which are not maskable. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> v3: * Only set a shadow cpumask if it is available in hardware. This causes fewer branches in the context switch. * Fix interaction between fastforward bits and override MSR. * Fix up the cross-vendor case. * Fix the host view of HTT/CMP_LEGACY. v2: * Use switch() rather than if/elseif chain * Clamp to static PV featuremask --- xen/arch/x86/domctl.c | 124 +++++++++++++++++++++++++++++++++++++++ xen/include/asm-x86/cpufeature.h | 1 + 2 files changed, 125 insertions(+)