Messages in this thread | | | Date | Wed, 9 Oct 2019 18:59:21 +0200 | From | Borislav Petkov <> | Subject | Re: [PATCH 3/6] x86/fpu/xstate: Separate user and supervisor xfeatures mask |
| |
On Wed, Sep 25, 2019 at 08:10:19AM -0700, Yu-cheng Yu wrote: > Before the introduction of XSAVES supervisor states, 'xfeatures_mask' is > used at various places to determine XSAVE buffer components and XCR0 bits. > It contains only user xstates. To support supervisor xstates, it is > necessary to separate user and supervisor xstates: > > - First, change 'xfeatures_mask' to 'xfeatures_mask_all', which represents > the full set of bits that should ever be set in a kernel XSAVE buffer. > - Introduce xfeature_mask_supervisor() and xfeatures_mask_user() to
xfeatures_mask_supervisor() ^
's' is missing.
> extract relevant xfeatures from xfeatures_mask_all. > > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Similar problem here. Fenghua's SOB means what exactly?
You're probably trying to say that he co-developed it but then it needs to state that this way:
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com> Co-developed-by: Yu-cheng Yu <yu-cheng.yu@intel.com> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com> > Reviewed-by: Tony Luck <tony.luck@intel.com> > --- > arch/x86/include/asm/fpu/internal.h | 2 +- > arch/x86/include/asm/fpu/xstate.h | 8 ++- > arch/x86/kernel/fpu/signal.c | 15 ++++-- > arch/x86/kernel/fpu/xstate.c | 78 +++++++++++++++++------------ > 4 files changed, 65 insertions(+), 38 deletions(-) > > diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h > index 4c95c365058a..f56ad1248b5d 100644 > --- a/arch/x86/include/asm/fpu/internal.h > +++ b/arch/x86/include/asm/fpu/internal.h > @@ -92,7 +92,7 @@ static inline void fpstate_init_xstate(struct xregs_state *xsave) > * XRSTORS requires these bits set in xcomp_bv, or it will > * trigger #GP: > */ > - xsave->header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT | xfeatures_mask; > + xsave->header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT | xfeatures_mask_all; > } > > static inline void fpstate_init_fxstate(struct fxregs_state *fx) > diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h > index 014c386deaa3..5954bd97da16 100644 > --- a/arch/x86/include/asm/fpu/xstate.h > +++ b/arch/x86/include/asm/fpu/xstate.h > @@ -51,7 +51,13 @@ > #define REX_PREFIX > #endif > > -extern u64 xfeatures_mask; > +extern u64 xfeatures_mask_all; > + > +static inline u64 xfeatures_mask_user(void) > +{ > + return xfeatures_mask_all & SUPPORTED_XFEATURES_MASK_USER; > +}
Please group this function along with xfeature_is_user(), xfeature_is_supervisor() and xfeature_mask_supervisor() in the xstate.h header as they're very similar in what they do.
> + > extern u64 xstate_fx_sw_bytes[USER_XSTATE_FX_SW_WORDS]; > > extern void __init update_regset_xstate_info(unsigned int size,
...
> @@ -731,16 +742,22 @@ void __init fpu__init_system_xstate(void) > return; > } > > + /* > + * Find user xstates supported by the processor. > + */ > cpuid_count(XSTATE_CPUID, 0, &eax, &ebx, &ecx, &edx); > - xfeatures_mask = eax + ((u64)edx << 32); > + xfeatures_mask_all = eax + ((u64)edx << 32); > + > + /* Place supervisor features in xfeatures_mask_all here */ >
Remove that newline.
> - if ((xfeatures_mask & XFEATURE_MASK_FPSSE) != XFEATURE_MASK_FPSSE) { > + if ((xfeatures_mask_user() & XFEATURE_MASK_FPSSE) != XFEATURE_MASK_FPSSE) { > /* > * This indicates that something really unexpected happened > * with the enumeration. Disable XSAVE and try to continue > * booting without it. This is too early to BUG(). > */ > - pr_err("x86/fpu: FP/SSE not present amongst the CPU's xstate features: 0x%llx.\n", xfeatures_mask); > + pr_err("x86/fpu: FP/SSE not present amongst the CPU's xstate features: 0x%llx.\n", > + xfeatures_mask_all); > goto out_disable; > } > > @@ -749,10 +766,10 @@ void __init fpu__init_system_xstate(void) > */ > for (i = 0; i < ARRAY_SIZE(xsave_cpuid_features); i++) { > if (!boot_cpu_has(xsave_cpuid_features[i])) > - xfeatures_mask &= ~BIT(i); > + xfeatures_mask_all &= ~BIT_ULL(i); > } > > - xfeatures_mask &= fpu__get_supported_xfeatures_mask(); > + xfeatures_mask_all &= fpu__get_supported_xfeatures_mask(); > > /* Enable xstate instructions to be able to continue with initialization: */ > fpu__init_cpu_xstate(); > @@ -764,16 +781,16 @@ void __init fpu__init_system_xstate(void) > * Update info used for ptrace frames; use standard-format size and no > * supervisor xstates: > */ > - update_regset_xstate_info(fpu_user_xstate_size, > - xfeatures_mask & SUPPORTED_XFEATURES_MASK_USER); > + update_regset_xstate_info(fpu_user_xstate_size, xfeatures_mask_user()); > > fpu__init_prepare_fx_sw_frame(); > setup_init_fpu_buf(); > setup_xstate_comp(); > print_xstate_offset_size(); > > - pr_info("x86/fpu: Enabled xstate features 0x%llx, context size is %d bytes, using '%s' format.\n", > - xfeatures_mask, > + pr_info("x86/fpu: Enabled xstate features 0x%llx: user 0x%llx, context size is %d bytes, using '%s' format.\n", > + xfeatures_mask_all, > + xfeatures_mask_user(),
Why do we need to dump the user features separately?
> fpu_kernel_xstate_size, > boot_cpu_has(X86_FEATURE_XSAVES) ? "compacted" : "standard"); > return;
-- Regards/Gruss, Boris.
https://people.kernel.org/tglx/notes-about-netiquette
| |