Re: [PATCH v9 06/26] x86/fpu/xstate: Calculate and remember dynamic XSTATE buffer sizes

Borislav Petkov
2021-08-12T16:36:25Z

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.