| Message ID | 574EA4F0.5030607@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 01.06.16 at 11:03, <andrew.cooper3@citrix.com> wrote: > While this does work, it undoes some of the work I started with my cpuid > improvements in 4.7 > > Does the attached patch also resolve your issue? While that's much better than the original, I don't think it's quite enough. The rest of the domain policy should be taken into account (and I think I had suggested to do so during review of your CPUID rework series), i.e. this can't be calculated once for every domain. And then, as said in reply to the original patch, handle_xsetbv()'s checking should be generalized from the special casing of PKRU (or if we don't want that, then that special case would better get removed for consistency reasons). Jan
Y, I think it works well, and more better. to Luwei: you can test if the problem is solved. On Wed, 2016-06-01 at 10:03 +0100, Andrew Cooper wrote: > On 01/06/16 05:58, Luwei Kang wrote: > > CPUID.0XD.0X0.EAX is from machine value for dom0, and dom0 kernel will xsetbv > > with xfeatures_mask that is from CPUID.0XD.0X0.EAX, but handle_xsetbv has > > ingored XSTATE_PKRU with hardware protection fault emulation, so dom0 kernel > > will crash on skylake machine with PKRU support. > > > > Signed-off-by: Luwei Kang <luwei.kang@intel.com> > > --- > > xen/arch/x86/traps.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c > > index 1ef8401..5e72e44 100644 > > --- a/xen/arch/x86/traps.c > > +++ b/xen/arch/x86/traps.c > > @@ -1100,6 +1100,9 @@ void pv_cpuid(struct cpu_user_regs *regs) > > */ > > if ( !is_control_domain(currd) && !is_hardware_domain(currd) ) > > cpuid_count(leaf, subleaf, &tmp, &b, &tmp, &tmp); > > + > > + /* PV is not supported by XSTATE_PKRU. */ > > + a &= ~XSTATE_PKRU; > > break; > > } > > > > While this does work, it undoes some of the work I started with my cpuid > improvements in 4.7 > > Does the attached patch also resolve your issue? > > ~Andrew
On Wed, Jun 01, 2016 at 10:03:44AM +0100, Andrew Cooper wrote: > > static void __init calculate_hvm_featureset(void) > @@ -179,7 +206,7 @@ static void __init calculate_hvm_featureset(void) > __clear_bit(X86_FEATURE_PCOMMIT, hvm_featureset); > } > > - sanitise_featureset(hvm_featureset); > + sanitise_featureset(hvm_featureset, & hvm_xfeature_mask); Nit: extraneous space after "&". Wei.
On 01/06/16 10:30, Wei Liu wrote: > On Wed, Jun 01, 2016 at 10:03:44AM +0100, Andrew Cooper wrote: >> >> static void __init calculate_hvm_featureset(void) >> @@ -179,7 +206,7 @@ static void __init calculate_hvm_featureset(void) >> __clear_bit(X86_FEATURE_PCOMMIT, hvm_featureset); >> } >> >> - sanitise_featureset(hvm_featureset); >> + sanitise_featureset(hvm_featureset, & hvm_xfeature_mask); > Nit: extraneous space after "&". Yeah - I noticed that and fixed it up after posting the email. ~Andrew
Thank you Andrew Cooper, this patch indeed resolve my issue and two point need modify.
The code need move ahead of "break;"
@@ -1101,6 +1101,9 @@ void pv_cpuid(struct cpu_user_regs *regs)
if ( !is_control_domain(currd) && !is_hardware_domain(currd) )
cpuid_count(leaf, subleaf, &tmp, &b, &tmp, &tmp);
break;
+
+ a &= (uint32_t)pv_xfeature_mask;
+ d &= (uint32_t)(pv_xfeature_mask >> 32);
}
extraneous space after "&".
- sanitise_featureset(hvm_featureset);
+ sanitise_featureset(hvm_featureset, & hvm_xfeature_mask);
-----Original Message-----
From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
Sent: Wednesday, June 1, 2016 5:04 PM
To: Kang, Luwei <luwei.kang@intel.com>; xen-devel@lists.xen.org
Cc: jbeulich@suse.com; Han, Huaitong <huaitong.han@intel.com>; Wang, Yong Y <yong.y.wang@intel.com>
Subject: Re: [PATCH] x86/cpuid: fix dom0 crash on skylake machine
On 01/06/16 05:58, Luwei Kang wrote:
> CPUID.0XD.0X0.EAX is from machine value for dom0, and dom0 kernel will
> xsetbv with xfeatures_mask that is from CPUID.0XD.0X0.EAX, but
> handle_xsetbv has ingored XSTATE_PKRU with hardware protection fault
> emulation, so dom0 kernel will crash on skylake machine with PKRU support.
>
> Signed-off-by: Luwei Kang <luwei.kang@intel.com>
> ---
> xen/arch/x86/traps.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c index
> 1ef8401..5e72e44 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1100,6 +1100,9 @@ void pv_cpuid(struct cpu_user_regs *regs)
> */
> if ( !is_control_domain(currd) && !is_hardware_domain(currd) )
> cpuid_count(leaf, subleaf, &tmp, &b, &tmp, &tmp);
> +
> + /* PV is not supported by XSTATE_PKRU. */
> + a &= ~XSTATE_PKRU;
> break;
> }
>
While this does work, it undoes some of the work I started with my cpuid improvements in 4.7
Does the attached patch also resolve your issue?
~Andrew
On 01/06/16 11:54, Kang, Luwei wrote: > Thank you Andrew Cooper, this patch indeed resolve my issue and two point need modify. > > The code need move ahead of "break;" > @@ -1101,6 +1101,9 @@ void pv_cpuid(struct cpu_user_regs *regs) > if ( !is_control_domain(currd) && !is_hardware_domain(currd) ) > cpuid_count(leaf, subleaf, &tmp, &b, &tmp, &tmp); > break; > + > + a &= (uint32_t)pv_xfeature_mask; > + d &= (uint32_t)(pv_xfeature_mask >> 32); > } Ah of course. That is quite a silly mistake on my behalf. > > extraneous space after "&". > - sanitise_featureset(hvm_featureset); > + sanitise_featureset(hvm_featureset, & hvm_xfeature_mask); I had already spotted and fixed this. I will collect all feedback and post a formal patch to the list. ~Andrew
From 3cbb2a63fe368ac185e483b9ef5a8504340a3702 Mon Sep 17 00:00:00 2001 From: Andrew Cooper <andrew.cooper3@citrix.com> Date: Wed, 1 Jun 2016 10:00:12 +0100 Subject: [PATCH] xen/x86: Clip guests view of xfeature_mask at the per-domain maximum --- xen/arch/x86/cpuid.c | 33 ++++++++++++++++++++++++++++++--- xen/arch/x86/hvm/hvm.c | 3 +++ xen/arch/x86/traps.c | 3 +++ xen/include/asm-x86/cpuid.h | 2 ++ 4 files changed, 38 insertions(+), 3 deletions(-) diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c index e1e0e44..0a75d4a 100644 --- a/xen/arch/x86/cpuid.c +++ b/xen/arch/x86/cpuid.c @@ -4,6 +4,7 @@ #include <asm/hvm/hvm.h> #include <asm/hvm/vmx/vmcs.h> #include <asm/processor.h> +#include <asm/xstate.h> const uint32_t known_features[] = INIT_KNOWN_FEATURES; const uint32_t special_features[] = INIT_SPECIAL_FEATURES; @@ -17,11 +18,14 @@ 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) +uint64_t __read_mostly pv_xfeature_mask, __read_mostly hvm_xfeature_mask; + +static void __init sanitise_featureset(uint32_t *fs, uint64_t *xfeature_mask) { /* for_each_set_bit() uses unsigned longs. Extend with zeroes. */ uint32_t disabled_features[ ROUNDUP(FSCAPINTS, sizeof(unsigned long)/sizeof(uint32_t))] = {}; + uint64_t mask = 0; unsigned int i; for ( i = 0; i < FSCAPINTS; ++i ) @@ -62,6 +66,29 @@ static void __init sanitise_featureset(uint32_t *fs) */ fs[FEATURESET_e1d] = ((fs[FEATURESET_1d] & CPUID_COMMON_1D_FEATURES) | (fs[FEATURESET_e1d] & ~CPUID_COMMON_1D_FEATURES)); + + /* + * Calculate the maximum applicable xfeature mask, based on the + * featureset. + */ + if ( test_bit(X86_FEATURE_XSAVE, fs) ) + { + mask = XSTATE_SSE | XSTATE_FP; + + if ( test_bit(X86_FEATURE_AVX, fs) ) + mask |= XSTATE_YMM; + + if ( test_bit(X86_FEATURE_MPX, fs) ) + mask |= XSTATE_BNDREGS | XSTATE_BNDCSR; + + if ( test_bit(X86_FEATURE_PKU, fs) ) + mask |= XSTATE_PKRU; + + if ( test_bit(X86_FEATURE_LWP, fs) ) + mask |= XSTATE_LWP; + } + + *xfeature_mask = mask; } static void __init calculate_raw_featureset(void) @@ -119,7 +146,7 @@ static void __init calculate_pv_featureset(void) __set_bit(X86_FEATURE_X2APIC, pv_featureset); __set_bit(X86_FEATURE_CMP_LEGACY, pv_featureset); - sanitise_featureset(pv_featureset); + sanitise_featureset(pv_featureset, &pv_xfeature_mask); } static void __init calculate_hvm_featureset(void) @@ -179,7 +206,7 @@ static void __init calculate_hvm_featureset(void) __clear_bit(X86_FEATURE_PCOMMIT, hvm_featureset); } - sanitise_featureset(hvm_featureset); + sanitise_featureset(hvm_featureset, & hvm_xfeature_mask); } void __init calculate_featuresets(void) diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 5040a5c..8678da9 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -3448,6 +3448,9 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx, if ( (_eax + _ebx) > *ebx ) *ebx = _eax + _ebx; } + + *eax &= (uint32_t)hvm_xfeature_mask; + *edx &= (uint32_t)(hvm_xfeature_mask >> 32); } if ( count == 1 ) diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c index 1ef8401..81935b8 100644 --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -1101,6 +1101,9 @@ void pv_cpuid(struct cpu_user_regs *regs) if ( !is_control_domain(currd) && !is_hardware_domain(currd) ) cpuid_count(leaf, subleaf, &tmp, &b, &tmp, &tmp); break; + + a &= (uint32_t)pv_xfeature_mask; + d &= (uint32_t)(pv_xfeature_mask >> 32); } case 1: diff --git a/xen/include/asm-x86/cpuid.h b/xen/include/asm-x86/cpuid.h index 9a21c25..231c6d4 100644 --- a/xen/include/asm-x86/cpuid.h +++ b/xen/include/asm-x86/cpuid.h @@ -29,6 +29,8 @@ extern uint32_t raw_featureset[FSCAPINTS]; extern uint32_t pv_featureset[FSCAPINTS]; extern uint32_t hvm_featureset[FSCAPINTS]; +extern uint64_t pv_xfeature_mask, hvm_xfeature_mask; + void calculate_featuresets(void); const uint32_t *lookup_deep_deps(uint32_t feature); -- 2.1.4