| Message ID | 20221012082609.922631-1-yang.zhong@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | target/i386: Switch back XFRM value | expand |
On Wed, 2022-10-12 at 04:26 -0400, Yang Zhong wrote: > The previous patch wrongly replaced FEAT_XSAVE_XCR0_{LO|HI} with > FEAT_XSAVE_XSS_{LO|HI} in CPUID(EAX=12,ECX=1):ECX, which made SGX ^ Nit: both ECX and EDX are wrongly set, but not only ECX. > enclave only supported SSE and x87 feature(xfrm=0x3). Is this true? Perhaps I am missing something, but it seems env- >features[FEAT_XSAVE_XCR0_LO] only includes LBR bit, which is bit 15. /* Calculate XSAVE components based on the configured CPU feature flags */ static void x86_cpu_enable_xsave_components(X86CPU *cpu) { ... env->features[FEAT_XSAVE_XSS_LO] = mask & CPUID_XSTATE_XSS_MASK; ... } /* CPUID feature bits available in XSS */ #define CPUID_XSTATE_XSS_MASK (XSTATE_ARCH_LBR_MASK) > > Fixes: 301e90675c3f ("target/i386: Enable support for XSAVES based features") > > Signed-off-by: Yang Zhong <yang.zhong@linux.intel.com> > --- > target/i386/cpu.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index ad623d91e4..19aaed877b 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -5584,8 +5584,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, > } else { > *eax &= env->features[FEAT_SGX_12_1_EAX]; > *ebx &= 0; /* ebx reserve */ > - *ecx &= env->features[FEAT_XSAVE_XSS_LO]; > - *edx &= env->features[FEAT_XSAVE_XSS_HI]; > + *ecx &= env->features[FEAT_XSAVE_XCR0_LO]; > + *edx &= env->features[FEAT_XSAVE_XCR0_HI]; > > /* FP and SSE are always allowed regardless of XSAVE/XCR0. */ > *ecx |= XSTATE_FP_MASK | XSTATE_SSE_MASK; The code looks good: Reviewed-by: Kai Huang <kai.huang@intel.com>
On Wed, Oct 12, 2022 at 09:59:04AM +0000, Huang, Kai wrote: > On Wed, 2022-10-12 at 04:26 -0400, Yang Zhong wrote: > > The previous patch wrongly replaced FEAT_XSAVE_XCR0_{LO|HI} with > > FEAT_XSAVE_XSS_{LO|HI} in CPUID(EAX=12,ECX=1):ECX, which made SGX > ^ > > Nit: both ECX and EDX are wrongly set, but not only ECX. > Yes, I will change to CPUID(EAX=12,ECX=1):{ECX,EDX}, thanks! > > enclave only supported SSE and x87 feature(xfrm=0x3). > > Is this true? Perhaps I am missing something, but it seems env- > >features[FEAT_XSAVE_XCR0_LO] only includes LBR bit, which is bit 15. We printed the XFRM value from SGX SDK to find this issue. > > /* Calculate XSAVE components based on the configured CPU feature flags */ > static void x86_cpu_enable_xsave_components(X86CPU *cpu) > { > ... > env->features[FEAT_XSAVE_XSS_LO] = mask & CPUID_XSTATE_XSS_MASK; > ... > } > > /* CPUID feature bits available in XSS */ > #define CPUID_XSTATE_XSS_MASK (XSTATE_ARCH_LBR_MASK) > > > > > Fixes: 301e90675c3f ("target/i386: Enable support for XSAVES based features") > > > > Signed-off-by: Yang Zhong <yang.zhong@linux.intel.com> > > --- > > target/i386/cpu.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > > index ad623d91e4..19aaed877b 100644 > > --- a/target/i386/cpu.c > > +++ b/target/i386/cpu.c > > @@ -5584,8 +5584,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, > > } else { > > *eax &= env->features[FEAT_SGX_12_1_EAX]; > > *ebx &= 0; /* ebx reserve */ > > - *ecx &= env->features[FEAT_XSAVE_XSS_LO]; > > - *edx &= env->features[FEAT_XSAVE_XSS_HI]; > > + *ecx &= env->features[FEAT_XSAVE_XCR0_LO]; > > + *edx &= env->features[FEAT_XSAVE_XCR0_HI]; > > > > /* FP and SSE are always allowed regardless of XSAVE/XCR0. */ > > *ecx |= XSTATE_FP_MASK | XSTATE_SSE_MASK; > > The code looks good: > > Reviewed-by: Kai Huang <kai.huang@intel.com> >
On Thu, 2022-10-13 at 02:23 -0400, Yang Zhong wrote: > > > enclave only supported SSE and x87 feature(xfrm=0x3). > > > > Is this true? Perhaps I am missing something, but it seems env- > > > features[FEAT_XSAVE_XCR0_LO] only includes LBR bit, which is bit 15. > > We printed the XFRM value from SGX SDK to find this issue. I don't know how you added the print, but the exact value that set to SGX CPUID is irrelevant, as it is wrong anyway. The value can also differ when you run on different machines, etc. IMHO in changelog we just need to point out the fact that the XSAVE enabling patch wrongly messed up with SGX CPUID and this patch fixes that.
diff --git a/target/i386/cpu.c b/target/i386/cpu.c index ad623d91e4..19aaed877b 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -5584,8 +5584,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, } else { *eax &= env->features[FEAT_SGX_12_1_EAX]; *ebx &= 0; /* ebx reserve */ - *ecx &= env->features[FEAT_XSAVE_XSS_LO]; - *edx &= env->features[FEAT_XSAVE_XSS_HI]; + *ecx &= env->features[FEAT_XSAVE_XCR0_LO]; + *edx &= env->features[FEAT_XSAVE_XCR0_HI]; /* FP and SSE are always allowed regardless of XSAVE/XCR0. */ *ecx |= XSTATE_FP_MASK | XSTATE_SSE_MASK;
The previous patch wrongly replaced FEAT_XSAVE_XCR0_{LO|HI} with FEAT_XSAVE_XSS_{LO|HI} in CPUID(EAX=12,ECX=1):ECX, which made SGX enclave only supported SSE and x87 feature(xfrm=0x3). Fixes: 301e90675c3f ("target/i386: Enable support for XSAVES based features") Signed-off-by: Yang Zhong <yang.zhong@linux.intel.com> --- target/i386/cpu.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)