Re: [PATCH 1/5] x86/ras: Merge Intel and AMD ppin_init() functions

[Borislav Petkov <bp@alien8.de>]

On Fri, Jan 07, 2022 at 02:54:38PM -0800, Tony Luck wrote:

Make that subject prefix "x86/cpu" please. Same for patches 2 and 3.

Patch 4 is probably topology/sysfs - at least this is what past patches
say. If you're not sure about the prefix, use git log on the files
you're touching.

> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 0083464de5e3..a1e29c0844d1 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -88,6 +88,80 @@ EXPORT_SYMBOL_GPL(get_llc_id);
>  /* L2 cache ID of each logical CPU */
>  DEFINE_PER_CPU_READ_MOSTLY(u16, cpu_l2c_id) = BAD_APICID;
>  
> +static struct ppin_info {
> +	int	feature;
> +	int	msr_ppin_ctl;
> +	int	msr_ppin;

That thing is unused in this patch and only accessed in patch 3. Please
add it there.

> +} ppin_info[] = {
> +	[X86_VENDOR_INTEL] = {
> +		.feature = X86_FEATURE_INTEL_PPIN,
> +		.msr_ppin_ctl = MSR_PPIN_CTL,
> +		.msr_ppin = MSR_PPIN
> +	},
> +	[X86_VENDOR_AMD] = {

X86_VENDOR_AMD is index 2 not 1 so ppin_info[1] is empty.

I wouldn't mind swapping the numbers here in a pre-patch:

#define X86_VENDOR_CYRIX        1
#define X86_VENDOR_AMD          2

nothing would depend on those naked numbers, right? :)

> +		.feature = X86_FEATURE_AMD_PPIN,
> +		.msr_ppin_ctl = MSR_AMD_PPIN_CTL,
> +		.msr_ppin = MSR_AMD_PPIN
> +	},
> +};
> +
> +static const struct x86_cpu_id ppin_cpuids[] = {
> +	X86_MATCH_VENDOR_FEATURE(AMD, X86_FEATURE_AMD_PPIN, &ppin_info[X86_VENDOR_AMD]),

X86_MATCH_FEATURE() I guess.

> +	/* Legacy models without CPUID enumeration */
> +	X86_MATCH_INTEL_FAM6_MODEL(IVYBRIDGE_X, &ppin_info[X86_VENDOR_INTEL]),
> +	X86_MATCH_INTEL_FAM6_MODEL(HASWELL_X, &ppin_info[X86_VENDOR_INTEL]),
> +	X86_MATCH_INTEL_FAM6_MODEL(BROADWELL_D, &ppin_info[X86_VENDOR_INTEL]),
> +	X86_MATCH_INTEL_FAM6_MODEL(BROADWELL_X, &ppin_info[X86_VENDOR_INTEL]),
> +	X86_MATCH_INTEL_FAM6_MODEL(SKYLAKE_X, &ppin_info[X86_VENDOR_INTEL]),
> +	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_X, &ppin_info[X86_VENDOR_INTEL]),
> +	X86_MATCH_INTEL_FAM6_MODEL(SAPPHIRERAPIDS_X, &ppin_info[X86_VENDOR_INTEL]),
> +	X86_MATCH_INTEL_FAM6_MODEL(XEON_PHI_KNL, &ppin_info[X86_VENDOR_INTEL]),
> +	X86_MATCH_INTEL_FAM6_MODEL(XEON_PHI_KNM, &ppin_info[X86_VENDOR_INTEL]),
> +
> +	{}
> +};
> +
> +static void ppin_init(struct cpuinfo_x86 *c)
> +{
> +	const struct x86_cpu_id *id;
> +	unsigned long long val;
> +	struct ppin_info *info;
> +
> +	id = x86_match_cpu(ppin_cpuids);
> +	if (!id)
> +		return;
> +
> +	/*
> +	 * Testing the prescence of the MSR is not enough. Need to check

Unknown word [prescence] in comment.
Suggestions: ['presence', 'prescience', 'putrescence', 'prepubescence', 'excrescence', 'concrescence']

> +	 * that the PPIN_CTL allows reading of the PPIN.
> +	 */
> +	info = (struct ppin_info *)id->driver_data;
> +
> +	if (rdmsrl_safe(info->msr_ppin_ctl, &val))
> +		goto clear_ppin;
> +
> +	if ((val & 3UL) == 1UL) {
> +		/* PPIN locked in disabled mode */
> +		goto clear_ppin;
> +	}
> +
> +	/* If PPIN is disabled, try to enable */
> +	if (!(val & 2UL)) {
> +		wrmsrl_safe(info->msr_ppin_ctl,  val | 2UL);
> +		rdmsrl_safe(info->msr_ppin_ctl, &val);
> +	}
> +
> +	/* Is the enable bit set? */
> +	if (val & 2UL) {
> +		set_cpu_cap(c, info->feature);
> +		return;
> +	}
> +
> +clear_ppin:
> +	clear_cpu_cap(c, info->feature);
> +}
> +
>  /* correctly size the local cpu masks */
>  void __init setup_cpu_local_masks(void)
>  {
> @@ -1655,6 +1729,8 @@ static void identify_cpu(struct cpuinfo_x86 *c)
>  			c->x86_capability[i] |= boot_cpu_data.x86_capability[i];
>  	}
>  
> +	ppin_init(c);

I can't say that I'm crazy about all those miscellaneous
feature-initializing functions sprinkled allround here but I don't have
a better idea ... yet.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette