Messages in this thread Patch in this message | | | Date | Thu, 12 Aug 2021 18:36:54 +0200 | From | Borislav Petkov <> | Subject | Re: [PATCH v9 06/26] x86/fpu/xstate: Calculate and remember dynamic XSTATE buffer sizes |
| |
On Fri, Jul 30, 2021 at 07:59:37AM -0700, Chang S. Bae wrote: > The CPUID instruction separately enumerates sizes and alignments of > individual xfeatures. It independently enumerates the required size of an > entire XSAVE buffer to store all enabled features. > > calculate_xstate_sizes() currently uses the individual feature > size/alignment enumeration to independently recalculate the required XSAVE > buffer size. This is compared against the CPUID-provided value. > > Extend the function to accept an option to exclude dynamic states. With > that, calculate the maximum size that contains all the enabled states, and > the minimum size that fits in the statically-allocated buffer by excluding > dynamic states.
This explains *what* this patch does but not *why*. *What* I can more or less see but for *why* I'd need my crystal ball...
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c > index 12caf1a56ce0..cd709408efb5 100644 > --- a/arch/x86/kernel/fpu/xstate.c > +++ b/arch/x86/kernel/fpu/xstate.c > @@ -591,24 +591,28 @@ static void check_xstate_against_struct(int nr) > } > } > > -/* > - * This essentially double-checks what the cpu told us about > - * how large the XSAVE buffer needs to be. We are recalculating > - * it to be safe.
Why are you removing that comment? Are we not recalculating anymore?
> +/** > + * calculate_xstate_size - Calculate the xstate per-task buffer size. > + * > + * Independent XSAVE features allocate their own buffers and are always > + * excluded. Only the size of the buffer for task->fpu is checked here. > * > - * Independent XSAVE features allocate their own buffers and are not > - * covered by these checks. Only the size of the buffer for task->fpu > - * is checked here. > + * @include_dynamic_states: A knob to include dynamic states or not. > + * > + * Return: The calculated xstate size. > */ > -static void do_extra_xstate_size_checks(void) > +static unsigned int calculate_xstate_size(bool include_dynamic_states) > { > - int paranoid_xstate_size = FXSAVE_SIZE + XSAVE_HDR_SIZE; > + unsigned int xstate_size = FXSAVE_SIZE + XSAVE_HDR_SIZE; > int i; > > for (i = FIRST_EXTENDED_XFEATURE; i < XFEATURE_MAX; i++) { > if (!xfeature_enabled(i)) > continue; > > + if (!include_dynamic_states && (xfeatures_mask_user_dynamic & BIT_ULL(i)))
The order should be flipped: if (dynamic_state and !include_dynamic_states)
> + continue; > + > check_xstate_against_struct(i); > /* > * Supervisor state components can be managed only by > @@ -619,7 +623,7 @@ static void do_extra_xstate_size_checks(void) > > /* Align from the end of the previous feature */ > if (xfeature_is_aligned(i)) > - paranoid_xstate_size = ALIGN(paranoid_xstate_size, 64); > + xstate_size = ALIGN(xstate_size, 64); > /* > * The offset of a given state in the non-compacted > * format is given to us in a CPUID leaf. We check > @@ -627,18 +631,15 @@ static void do_extra_xstate_size_checks(void) > * setup_xstate_features(). XSAVES uses compacted format. > */ > if (!cpu_feature_enabled(X86_FEATURE_XSAVES)) > - paranoid_xstate_size = xfeature_uncompacted_offset(i); > + xstate_size = xfeature_uncompacted_offset(i); > /* > * The compacted-format offset always depends on where > * the previous state ended. > */ > - paranoid_xstate_size += xfeature_size(i); > + xstate_size += xfeature_size(i); > } > - /* > - * The size accounts for all the possible states reserved in the > - * per-task buffer. Check against the maximum size. > - */ > - XSTATE_WARN_ON(paranoid_xstate_size != get_xstate_config(XSTATE_MAX_SIZE)); > + > + return xstate_size; > } > >
<--- You can remove one of the newlines here, while at it.
> @@ -723,7 +724,7 @@ static bool is_supported_xstate_size(unsigned int test_xstate_size) > static int __init init_xstate_size(void) > { > /* Recompute the context size for enabled features: */ > - unsigned int possible_xstate_size; > + unsigned int possible_xstate_size, xstate_size; > unsigned int xsave_size; > > xsave_size = get_xsave_size(); > @@ -734,23 +735,23 @@ static int __init init_xstate_size(void) > possible_xstate_size = xsave_size; > > /* > - * The size accounts for all the possible states reserved in the > - * per-task buffer. Set the maximum with this value. > + * Calculate xstate size for all the possible states by setting > + * 'true' to include dynamic states.
"Calculate the maximum xstate size, including the dynamic states."
> Cross-check with the CPUID- > + * provided size and record it. > */ > + xstate_size = calculate_xstate_size(true); > + XSTATE_WARN_ON(possible_xstate_size != xstate_size); > set_xstate_config(XSTATE_MAX_SIZE, possible_xstate_size); > > - /* Perform an extra check for the maximum size. */ > - do_extra_xstate_size_checks(); > - > /* > - * Set the minimum to be the same as the maximum. The dynamic > - * user states are not supported yet. > + * Calculate the xstate size without dynamic states by setting > + * 'false' to exclude dynamic states.
"Calculate the minimum xstate size, i.e., excluding the dynamic xstates."
> Ensure the size fits in > + * the statically-allocated buffer and record it. > */ > - set_xstate_config(XSTATE_MIN_SIZE, possible_xstate_size); > - > - /* Ensure the minimum size fits in the statically-allocated buffer: */ > - if (!is_supported_xstate_size(get_xstate_config(XSTATE_MIN_SIZE))) > + xstate_size = calculate_xstate_size(false); > + if (!is_supported_xstate_size(xstate_size)) > return -EINVAL;
<---- newline here.
> + set_xstate_config(XSTATE_MIN_SIZE, xstate_size); > > /* > * User space is always in standard format. > -- > 2.17.1 >
-- Regards/Gruss, Boris.
https://people.kernel.org/tglx/notes-about-netiquette
| |