Re: [PATCH 01/13] x86/CPU/AMD: Add ZenX generations flags

[Tom Lendacky]

On 11/20/23 04:41, Borislav Petkov wrote:
> From: "Borislav Petkov (AMD)" <bp@xxxxxxxxx>
>
> Add X86_FEATURE flags for each Zen generation. They should be used from
> now on instead of checking f/m/s.
>
> Signed-off-by: Borislav Petkov (AMD) <bp@xxxxxxxxx>
> ---
>   arch/x86/include/asm/cpufeatures.h |  6 ++-
>   arch/x86/kernel/cpu/amd.c          | 70 +++++++++++++++++++++++++++++-
>   2 files changed, 72 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index 4af140cf5719..6f6cf49e9891 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -218,7 +218,7 @@
>   #define X86_FEATURE_IBRS		( 7*32+25) /* Indirect Branch Restricted Speculation */
>   #define X86_FEATURE_IBPB		( 7*32+26) /* Indirect Branch Prediction Barrier */
>   #define X86_FEATURE_STIBP		( 7*32+27) /* Single Thread Indirect Branch Predictors */
> -#define X86_FEATURE_ZEN			(7*32+28) /* "" CPU based on Zen microarchitecture */
> +#define X86_FEATURE_ZEN			( 7*32+28) /* "" CPU based on Zen microarchitecture */
>   #define X86_FEATURE_L1TF_PTEINV		( 7*32+29) /* "" L1TF workaround PTE inversion */
>   #define X86_FEATURE_IBRS_ENHANCED	( 7*32+30) /* Enhanced IBRS */
>   #define X86_FEATURE_MSR_IA32_FEAT_CTL	( 7*32+31) /* "" MSR IA32_FEAT_CTL configured */
> @@ -308,10 +308,12 @@
>   #define X86_FEATURE_SMBA		(11*32+21) /* "" Slow Memory Bandwidth Allocation */
>   #define X86_FEATURE_BMEC		(11*32+22) /* "" Bandwidth Monitoring Event Configuration */
>   #define X86_FEATURE_USER_SHSTK		(11*32+23) /* Shadow stack support for user mode applications */
> -
>   #define X86_FEATURE_SRSO		(11*32+24) /* "" AMD BTB untrain RETs */
>   #define X86_FEATURE_SRSO_ALIAS		(11*32+25) /* "" AMD BTB untrain RETs through aliasing */
>   #define X86_FEATURE_IBPB_ON_VMEXIT	(11*32+26) /* "" Issue an IBPB only on VMEXIT */
> +#define X86_FEATURE_ZEN2		(11*32+27) /* "" CPU based on Zen2 microarchitecture */
> +#define X86_FEATURE_ZEN3		(11*32+28) /* "" CPU based on Zen3 microarchitecture */
> +#define X86_FEATURE_ZEN4		(11*32+29) /* "" CPU based on Zen4 microarchitecture */
>   
>   /* Intel-defined CPU features, CPUID level 0x00000007:1 (EAX), word 12 */
>   #define X86_FEATURE_AVX_VNNI		(12*32+ 4) /* AVX VNNI instructions */
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index a7eab05e5f29..fa6ba63ca7e2 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -616,6 +616,49 @@ static void bsp_init_amd(struct cpuinfo_x86 *c)
>   	}
>   
>   	resctrl_cpu_detect(c);
> +
> +	/* Figure out Zen generations: */
> +	switch (c->x86) {
> +	case 0x17: {
> +		switch (c->x86_model) {
> +		case 0x00 ... 0x2f:
> +		case 0x50 ... 0x5f:
> +			setup_force_cpu_cap(X86_FEATURE_ZEN);
> +			break;
> +		case 0x30 ... 0x4f:
> +		case 0x60 ... 0x7f:
> +		case 0x90 ... 0x91:
> +		case 0xa0 ... 0xaf:
> +			setup_force_cpu_cap(X86_FEATURE_ZEN2);
> +			break;
> +		default:
> +			goto warn;

Previously just being family 17h or 19h would get X86_FEATURE_ZEN set. 
With this, if the model check doesn't match, you won't get any 
X86_FEATURE_ZEN* set. Should you do set X86_FEATURE_ZEN here, e.g. lowest 
common denominator for the family?

> +		}
> +		break;
> +	}
> +	case 0x19: {
> +		switch (c->x86_model) {
> +		case 0x00 ... 0x0f:
> +		case 0x20 ... 0x5f:
> +			setup_force_cpu_cap(X86_FEATURE_ZEN3);
> +			break;
> +		case 0x10 ... 0x1f:
> +		case 0x60 ... 0xaf:
> +			setup_force_cpu_cap(X86_FEATURE_ZEN4);
> +			break;
> +		default:

Ditto here to set X86_FEATURE_ZEN3?

Thanks,
Tom

> +			goto warn;
> +		}
> +		break;
> +	}
> +	default:
> +		break;
> +	}
> +
> +	return;
> +
> +warn:
> +	WARN_ONCE(1, "Family 0x%x, model: 0x%x??\n", c->x86, c->x86_model);
>   }
>   
>   static void early_detect_mem_encrypt(struct cpuinfo_x86 *c)
> @@ -974,8 +1017,6 @@ void init_spectral_chicken(struct cpuinfo_x86 *c)
>   
>   static void init_amd_zn(struct cpuinfo_x86 *c)
>   {
> -	set_cpu_cap(c, X86_FEATURE_ZEN);
> -
>   #ifdef CONFIG_NUMA
>   	node_reclaim_distance = 32;
>   #endif
> @@ -997,6 +1038,22 @@ static void init_amd_zn(struct cpuinfo_x86 *c)
>   	}
>   }
>   
> +static void init_amd_zen(struct cpuinfo_x86 *c)
> +{
> +}
> +
> +static void init_amd_zen2(struct cpuinfo_x86 *c)
> +{
> +}
> +
> +static void init_amd_zen3(struct cpuinfo_x86 *c)
> +{
> +}
> +
> +static void init_amd_zen4(struct cpuinfo_x86 *c)
> +{
> +}
> +
>   static bool cpu_has_zenbleed_microcode(void)
>   {
>   	u32 good_rev = 0;
> @@ -1077,6 +1134,15 @@ static void init_amd(struct cpuinfo_x86 *c)
>   	case 0x19: init_amd_zn(c); break;
>   	}
>   
> +	if (boot_cpu_has(X86_FEATURE_ZEN))
> +		init_amd_zen(c);
> +	else if (boot_cpu_has(X86_FEATURE_ZEN2))
> +		init_amd_zen2(c);
> +	else if (boot_cpu_has(X86_FEATURE_ZEN3))
> +		init_amd_zen3(c);
> +	else if (boot_cpu_has(X86_FEATURE_ZEN4))
> +		init_amd_zen4(c);
> +
>   	/*
>   	 * Enable workaround for FXSAVE leak on CPUs
>   	 * without a XSaveErPtr feature