Re: [PATCH RFC v2 6/9] x86/fpu/apx: Define APX state component

[Sohil Mehta]

Overall, the patch looks good to me.

Reviewed-by: Sohil Mehta <sohil.mehta@xxxxxxxxx>

Some minor nits below.

On 3/20/2025 4:42 PM, Chang S. Bae wrote:
> The Advanced Performance Extensions (APX) feature flag was previously
> defined. 

It's unnecessary to say this. You could directly start with. Advanced
Performance Extensions (APX) is associated with...

> This feature is associated with a new state component number 19.
> To support APX, it is essential to define this xstate component and
> implement the necessary sanity checks.
> 

It might be more precise to say what support is being added.

Maybe something like,
During context switch, to support saving and restoring of APX registers
using XSAVE, it is essential...

> Define the new component number, state name, and those register data
> type. Then, extend the size checker to validate the register data type
> and explicitly set the APX feature flag as a dependency for the new
> component in xsave_cpuid_features[].
> 
> Signed-off-by: Chang S. Bae <chang.seok.bae@xxxxxxxxx>
> ---
> RFC-V1 -> RFC-V2:
> * Remove the ordering table change, as it is now dynamically populated.
> ---
>  arch/x86/include/asm/fpu/types.h | 9 +++++++++
>  arch/x86/kernel/fpu/xstate.c     | 3 +++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/arch/x86/include/asm/fpu/types.h b/arch/x86/include/asm/fpu/types.h
> index de16862bf230..97310df3ea13 100644
> --- a/arch/x86/include/asm/fpu/types.h
> +++ b/arch/x86/include/asm/fpu/types.h
> @@ -125,6 +125,7 @@ enum xfeature {
>  	XFEATURE_RSRVD_COMP_16,
>  	XFEATURE_XTILE_CFG,
>  	XFEATURE_XTILE_DATA,
> +	XFEATURE_APX,
>  
>  	XFEATURE_MAX,
>  };
> @@ -145,6 +146,7 @@ enum xfeature {
>  #define XFEATURE_MASK_LBR		(1 << XFEATURE_LBR)
>  #define XFEATURE_MASK_XTILE_CFG		(1 << XFEATURE_XTILE_CFG)
>  #define XFEATURE_MASK_XTILE_DATA	(1 << XFEATURE_XTILE_DATA)
> +#define XFEATURE_MASK_APX		(1 << XFEATURE_APX)
>  
>  #define XFEATURE_MASK_FPSSE		(XFEATURE_MASK_FP | XFEATURE_MASK_SSE)
>  #define XFEATURE_MASK_AVX512		(XFEATURE_MASK_OPMASK \
> @@ -303,6 +305,13 @@ struct xtile_data {
>  	struct reg_1024_byte		tmm;
>  } __packed;
>  
> +/*
> + * State component 19: 8B extended general purpose register.
> + */
> +struct apx_state {
> +	u64				egpr[16];
> +} __packed;
> +

The below comment makes it seem the APX component is inserted out of
order. But, it's the PASID component that is actually out of order.

I'll send a cleanup patch separately. It could be applied before or
after this series.

>  /*
>   * State component 10 is supervisor state used for context-switching the
>   * PASID state.
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index 46c45e2f2a5a..2a270683a762 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -63,6 +63,7 @@ static const char *xfeature_names[] =
>  	"unknown xstate feature",
>  	"AMX Tile config",
>  	"AMX Tile data",
> +	"APX registers",
>  	"unknown xstate feature",
>  };
>  
> @@ -81,6 +82,7 @@ static unsigned short xsave_cpuid_features[] __initdata = {
>  	[XFEATURE_CET_USER]			= X86_FEATURE_SHSTK,
>  	[XFEATURE_XTILE_CFG]			= X86_FEATURE_AMX_TILE,
>  	[XFEATURE_XTILE_DATA]			= X86_FEATURE_AMX_TILE,
> +	[XFEATURE_APX]				= X86_FEATURE_APX,
>  };
>  
>  static unsigned int xstate_offsets[XFEATURE_MAX] __ro_after_init =
> @@ -570,6 +572,7 @@ static bool __init check_xstate_against_struct(int nr)
>  	case XFEATURE_XTILE_CFG:  return XCHECK_SZ(sz, nr, struct xtile_cfg);
>  	case XFEATURE_CET_USER:	  return XCHECK_SZ(sz, nr, struct cet_user_state);
>  	case XFEATURE_XTILE_DATA: check_xtile_data_against_struct(sz); return true;
> +	case XFEATURE_APX:        return XCHECK_SZ(sz, nr, struct apx_state);

Can we insert the new APX case before XFEATURE_XTILE_DATA? These cases
are not really in numerical order. That way, the switch case is more
consistent and easier to read.

>  	default:
>  		XSTATE_WARN_ON(1, "No structure for xstate: %d\n", nr);
>  		return false;