diff mbox series

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

Message ID 20181006001928.28097-1-andi@firstfloor.org (mailing list archive)
State New, archived
Headers show
Series [v1,1/2] x86/cpufeature: Add facility to match microcode revisions | expand

Commit Message

Andi Kleen Oct. 6, 2018, 12:19 a.m. UTC 
From: Andi Kleen <ak@linux.intel.com>

For bug workarounds or checks it is useful to check for specific
microcode versions. Add a new table format to check for steppings
with min/max microcode revisions.

This does not change the existing x86_cpu_id because it's an ABI
shared with modutils, and also has quite difference requirements,
as in no wildcards, but everything has to be matched exactly.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/include/asm/cpu_device_id.h | 22 ++++++++++++++
 arch/x86/kernel/cpu/match.c          | 43 ++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)

Comments

Thomas Gleixner Oct. 6, 2018, 2:14 p.m. UTC | #1 
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
Thomas Gleixner Oct. 6, 2018, 2:40 p.m. UTC | #2 
On Fri, 5 Oct 2018, Andi Kleen wrote:
> Some time ago KVM added a workaround for PEBS events leaking
> into guests. This uses the KVM entry/exit list to add an extra
> disable of the PEBS_ENABLE MSR.
> 
> Intel also added a fix for this issue to microcode updates on
> Haswell/Broadwell/Skylake.
> 
> It turns out using the MSR entry/exit list makes VM exits
> significantly slower. The list is only needed for disabling
> PEBS, because the GLOBAL_CTRL change gets optimized by
> KVM into changing the VMCS.
> 
> This patch checks for the microcode updates that have the microcode

# grep "This patch" Documentation/process

> fix for leaking PEBS, and disables the extra entry/exit list
> entry for PEBS_ENABLE. In addition we always clear the
> GLOBAL_CTRL for the PEBS counter while running in the guest,
> which is enough to make them never fire at the wrong
> side of the host/guest transition.
>  
> +#define IUCODE(model, step, rev) \
> +	{ X86_VENDOR_INTEL, 6, model, step, rev, 0, 0 }

So we are going to have this kind of defines on every usage site. Please
put these macros into the corresponding header file.

Also this wants to be named INTEL_MIN_UCODE() so it's clear what this is
about.

Thanks,

	tglx
Thomas Gleixner Oct. 6, 2018, 4:10 p.m. UTC | #3 
On Sat, 6 Oct 2018, Thomas Gleixner wrote:

> 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;

Why do we need max_ucode and driver_data? I can't find an existing example
where this would be useful. If we need it later then it can be added
incrementaly.

Thanks,

	tglx
Thomas Gleixner Oct. 6, 2018, 4:13 p.m. UTC | #4 
On Sat, 6 Oct 2018, Thomas Gleixner wrote:

> On Fri, 5 Oct 2018, Andi Kleen wrote:
> > Some time ago KVM added a workaround for PEBS events leaking
> > into guests. This uses the KVM entry/exit list to add an extra
> > disable of the PEBS_ENABLE MSR.
> > 
> > Intel also added a fix for this issue to microcode updates on
> > Haswell/Broadwell/Skylake.
> > 
> > It turns out using the MSR entry/exit list makes VM exits
> > significantly slower. The list is only needed for disabling
> > PEBS, because the GLOBAL_CTRL change gets optimized by
> > KVM into changing the VMCS.
> > 
> > This patch checks for the microcode updates that have the microcode
> 
> # grep "This patch" Documentation/process
> 
> > fix for leaking PEBS, and disables the extra entry/exit list
> > entry for PEBS_ENABLE. In addition we always clear the
> > GLOBAL_CTRL for the PEBS counter while running in the guest,
> > which is enough to make them never fire at the wrong
> > side of the host/guest transition.
> >  
> > +#define IUCODE(model, step, rev) \
> > +	{ X86_VENDOR_INTEL, 6, model, step, rev, 0, 0 }
> 
> So we are going to have this kind of defines on every usage site. Please
> put these macros into the corresponding header file.
> 
> Also this wants to be named INTEL_MIN_UCODE() so it's clear what this is
> about.

And please use designated initializers. That way the unused fields do not
need explicit zeroing and any change of the data structure either just
works or the compiler catches the breakage.

Thanks,

	tglx
Andi Kleen Oct. 6, 2018, 6:15 p.m. UTC | #5 
On Sat, Oct 06, 2018 at 04:14:54PM +0200, Thomas Gleixner wrote:
> 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.

The matcher can be used to match specific hardware steppings by setting
the min/max_ucode to 0 or specific microcode revisions 
(which are associated with steppings) 

> > +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.

We still support the old microcode interface that allows updates
per CPU, and also it could happen during CPU hotplug.

> 
> > +	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().

In my case the caller, but yes should be documented.

-Andi
Borislav Petkov Oct. 6, 2018, 6:39 p.m. UTC | #6 
On Sat, Oct 06, 2018 at 11:15:07AM -0700, Andi Kleen wrote:
> The matcher can be used to match specific hardware steppings by setting
> the min/max_ucode to 0 or specific microcode revisions 
> (which are associated with steppings)

This better be explained unambiguously.

> We still support the old microcode interface that allows updates
> per CPU, and also it could happen during CPU hotplug.

There are no per CPU microcode updates anymore - it is all or none.

It is actually your microcoders who came up with a bunch of restrictions
like quiescing the cores from doing *anything*, blocking hotplug,
prohibiting updates if a subset of the cores is not online and still not
guaranteeing it'll work all the time because <reasons>.

The actually very simple reason being it is just too late for microcode
update when the machine is up. Where all I wanna do is rip the damn
thing out completely.
Thomas Gleixner Oct. 7, 2018, 5:32 a.m. UTC | #7 
On Sat, 6 Oct 2018, Andi Kleen wrote:
> On Sat, Oct 06, 2018 at 04:14:54PM +0200, Thomas Gleixner wrote:
> > 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.
> 
> The matcher can be used to match specific hardware steppings by setting
> the min/max_ucode to 0 or specific microcode revisions 
> (which are associated with steppings)

I can see your point, but calling x86_match_ucode() to match the stepping
of a CPU is really not intuitive. Can we please have functions and data
structures which have a clear purpose and are not overloaded in obscure
ways?

Thanks,

	tglx
diff mbox series

Patch

diff --git a/arch/x86/include/asm/cpu_device_id.h b/arch/x86/include/asm/cpu_device_id.h
index baeba0567126..bf2222d5438c 100644
--- a/arch/x86/include/asm/cpu_device_id.h
+++ b/arch/x86/include/asm/cpu_device_id.h
@@ -11,4 +11,26 @@ 
 
 extern const struct x86_cpu_id *x86_match_cpu(const struct x86_cpu_id *match);
 
+/*
+ * Match specific microcodes or steppings.
+ *
+ * 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;
+	__u16 family;
+	__u16 model;
+	__u16 stepping;
+	__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);
+
+	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)
+{
+	int cpu;
+	const struct x86_ucode_id *all_m = NULL;
+	bool first = true;
+
+	for_each_online_cpu(cpu) {
+		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;
+}