Re: [PATCH v1 1/2] x86/cpufeature: Add facility to match microcode revisions

[Thomas Gleixner]

On Fri, 5 Oct 2018, Andi Kleen wrote:
> +/*
> + * Match specific microcodes or steppings.

What means microcodes or steppings? If you mean microcode revisions then
please spell it out and use it all over the place. steppings is confusing
at best as its associated to the CPU stepping.

> + *
> + * vendor/family/model/stepping must be all set.
> + * min_ucode/max_ucode/driver_data are optional and can be 0.
> + */
> +
> +struct x86_ucode_id {
> +	__u16 vendor;

__uXX are usually UAPI types. Please use the regular kernel uXX
types instead.

> +	__u16 family;
> +	__u16 model;
> +	__u16 stepping;

Why u16? The corresponding members in cpuinfo_x86 are 8bit wide so why
wasting memory for these tables?

> +	__u32 min_ucode;
> +	__u32 max_ucode;
> +	kernel_ulong_t driver_data;
> +};
> +
> +extern const struct x86_ucode_id *
> +x86_match_ucode_cpu(int cpu, const struct x86_ucode_id *match);
> +extern const struct x86_ucode_id *
> +x86_match_ucode_all(const struct x86_ucode_id *match);
> +
>  #endif
> diff --git a/arch/x86/kernel/cpu/match.c b/arch/x86/kernel/cpu/match.c
> index 3fed38812eea..f29a21b2809c 100644
> --- a/arch/x86/kernel/cpu/match.c
> +++ b/arch/x86/kernel/cpu/match.c
> @@ -48,3 +48,46 @@ const struct x86_cpu_id *x86_match_cpu(const struct x86_cpu_id *match)
>  	return NULL;
>  }
>  EXPORT_SYMBOL(x86_match_cpu);
> +
> +const struct x86_ucode_id *x86_match_ucode_cpu(int cpu,
> +					       const struct x86_ucode_id *match)
> +{
> +	const struct x86_ucode_id *m;
> +	struct cpuinfo_x86 *c = &cpu_data(cpu);

Please use reverse fir tree ordering for variable declarations

	struct cpuinfo_x86 *c = &cpu_data(cpu);
	const struct x86_ucode_id *m;

It's simpler to parse.

> +	for (m = match; m->vendor | m->family | m->model; m++) {
> +		if (c->x86_vendor != m->vendor)
> +			continue;
> +		if (c->x86 != m->family)
> +			continue;
> +		if (c->x86_model != m->model)
> +			continue;
> +		if (c->x86_stepping != m->stepping)
> +			continue;
> +		if (m->min_ucode && c->microcode < m->min_ucode)
> +			continue;
> +		if (m->max_ucode && c->microcode > m->max_ucode)
> +			continue;
> +		return m;
> +	}
> +	return NULL;
> +}
> +
> +/* Check all CPUs */
> +const struct x86_ucode_id *x86_match_ucode_all(const struct x86_ucode_id *match)

Can you please name that so it's obvious that this checks all cpus, but
aside of that, why would a system ever end up with different microcode
revisions at all? The changelog is not mentioning any reason for this
function and "/* Check all CPUs */" is not helpful either.

> +	int cpu;
> +	const struct x86_ucode_id *all_m = NULL;
> +	bool first = true;
> +
> +	for_each_online_cpu(cpu) {

What guarantees that CPUs cannot be plugged? You either need to have
cpus_read_lock() in this function or a lockdep_assert_cpus_held().

> +		const struct x86_ucode_id *m = x86_match_ucode_cpu(cpu, match);
> +
> +		if (first)
> +			all_m = m;
> +		else if (m != all_m)
> +			return NULL;
> +		first = false;
> +	}
> +	return all_m;

Thanks,

	tglx