Re: [PATCH RFC v2 7/9] x86/fpu/apx: Disallow conflicting MPX presence

[Sohil Mehta <sohil.mehta@intel.com>]

On 3/20/2025 4:42 PM, Chang S. Bae wrote:
> APX is introduced as xstate component 19, following AMX. However, in the
> non-compacted format, its offset overlaps with the space previously
> occupied by the now-deprecated MPX:
> 
>   45fc24e89b7c ("x86/mpx: remove MPX from arch/x86")
> 

Would it be useful to describe why and how this is possible?

My knowledge on this fairly limited, is it because the size of the APX
register state is the same or less than the legacy MPX state?

Also, what other options does the kernel have to handle this?

There was an earlier email from Dave that describes the trade-offs.
Would it be useful to insert some form of summary here or in a different
patch?
https://lore.kernel.org/lkml/674db309-e206-4bb4-bf99-b3c39bff7973@intel.com/


> To prevent conflicts, the kernel must ensure the CPU never expose both
> features at the same time. If so, it indicates unreliable hardware. In
> such cases, XSAVE should be disabled entirely as a precautionary measure.
> 
> Add a sanity check to detect this condition and disable XSAVE if an
> invalid hardware configuration is identified.
> 
> Note: MPX state components remain enabled on legacy systems solely for
> KVM guest support.
> 

I didn't understand this properly. Can you please explain more? Is there
an impact or restriction that this imposes on a combination of legacy
and modern components?
Old KVM — new Guest kernel
 	or
New KVM — Old guest kernel

> Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
> ---
>  arch/x86/kernel/fpu/xstate.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index 2a270683a762..0d68d5c4bc48 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -814,6 +814,17 @@ void __init fpu__init_system_xstate(unsigned int legacy_size)
>  		goto out_disable;
>  	}
>  
> +	if (fpu_kernel_cfg.max_features & XFEATURE_MASK_APX &&
> +	    fpu_kernel_cfg.max_features & (XFEATURE_MASK_BNDREGS | XFEATURE_MASK_BNDCSR)) {
> +		/*
> +		 * This is a problematic CPU configuration where two
> +		 * conflicting state components are both enumerated.
> +		 */
> +		pr_err("x86/fpu: both APX and MPX present in the CPU's xstate features: 0x%llx.\n",
> +		       fpu_kernel_cfg.max_features);

s/both/Both

For the user, this statement doesn't necessarily seem like an error
message, nor does it say what action the kernel takes.
IIUC, there is no other print that says the kernel is disabling XSAVE
once it jumps to out_disable?

It might be useful to add a "disabling XSAVE" print at the end of this
statement, like the other error messages in the same function.

With this change,
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>

> +		goto out_disable;
> +	}
> +
>  	fpu_kernel_cfg.independent_features = fpu_kernel_cfg.max_features &
>  					      XFEATURE_MASK_INDEPENDENT;
>