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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <22a47339-efd7-4df1-9b0a-14e5d6f59b67@intel.com>
Date: Fri, 11 Apr 2025 15:54:26 -0700
From: Sohil Mehta <sohil.mehta@...el.com>
To: "Chang S. Bae" <chang.seok.bae@...el.com>, <linux-kernel@...r.kernel.org>
CC: <x86@...nel.org>, <tglx@...utronix.de>, <mingo@...hat.com>,
	<bp@...en8.de>, <dave.hansen@...ux.intel.com>
Subject: Re: [PATCH RFC v2 6/9] x86/fpu/apx: Define APX state component

Overall, the patch looks good to me.

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

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@...el.com>
> ---
> 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;