x86/cpufeature: Add facility to match microcode revisions
PATCH 1/2 x86/cpufeature: Add facility to match microcode revisions
From: Andi Kleen <[email protected]>
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.
--- arch/x86/include/asm/cpu_device_id.h | 22 ++++++++++++++ arch/x86/kernel/cpu/match.c | 43 ++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+)
arch/x86/include/asm/cpu_device_id.h +22 -0
@@ -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
arch/x86/kernel/cpu/match.c +43 -0
46 lines not on this page · raw message
Signature · 1 line
2.17.1
Quoted 2 lines · Andi Kleen
+/*
+ * 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.
arch/x86/include/asm/cpu_device_id.h line 22
Quoted 7 lines
+ *
+ * vendor/family/model/stepping must be all set.
+ * min_ucode/max_ucode/driver_data are optional and can be 0.
+ */arch/x86/include/asm/cpu_device_id.h +3
-0+ +struct x86_ucode_id { +__u16 vendor;
__uXX are usually UAPI types. Please use the regular kernel uXX types instead.
arch/x86/include/asm/cpu_device_id.h line 25
Quoted 3 lines
arch/x86/include/asm/cpu_device_id.h +3
-0+__u16 family; +__u16 model; +__u16 stepping;
Why u16? The corresponding members in cpuinfo_x86 are 8bit wide so why wasting memory for these tables?
Quoted 16 lines
arch/x86/include/asm/cpu_device_id.h +10
-0+__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 arch/x86/kernel/cpu/match.c +0
-0
arch/x86/kernel/cpu/match.c const struct x86_cpu_id *x86_match_cpu(const struct x86_cpu_id *match) · line 56
Quoted 9 lines
arch/x86/kernel/cpu/match.c +6
-0@@ -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.
Quoted 10 lines
arch/x86/kernel/cpu/match.c +10
-0+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)
arch/x86/kernel/cpu/match.c const struct x86_cpu_id *x86_match_cpu(const struct x86_cpu_id *match) · line 77
Quoted 10 lines
arch/x86/kernel/cpu/match.c +10
-0+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.
arch/x86/kernel/cpu/match.c const struct x86_cpu_id *x86_match_cpu(const struct x86_cpu_id *match) · line 83
Quoted 5 lines
arch/x86/kernel/cpu/match.c +5
-0+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().
arch/x86/kernel/cpu/match.c const struct x86_cpu_id *x86_match_cpu(const struct x86_cpu_id *match) · line 92
Quoted 9 lines
arch/x86/kernel/cpu/match.c +9
-0+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
Quoted 26 lines · Thomas Gleixner
Quoted 2 lines · Andi Kleen
+/*
+ * 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.
arch/x86/include/asm/cpu_device_id.h line 22
Quoted 7 lines
+ *
+ * vendor/family/model/stepping must be all set.
+ * min_ucode/max_ucode/driver_data are optional and can be 0.
+ */arch/x86/include/asm/cpu_device_id.h +3
-0+ +struct x86_ucode_id { +__u16 vendor;
__uXXare usually UAPI types. Please use the regular kernel uXX types instead.arch/x86/include/asm/cpu_device_id.h line 25
Quoted 3 lines
arch/x86/include/asm/cpu_device_id.h +3
-0+__u16 family; +__u16 model; +__u16 stepping;Why u16? The corresponding members in
cpuinfo_x86are 8bit wide so why wasting memory for these tables?
Quoted 3 lines
arch/x86/include/asm/cpu_device_id.h line 28
Quoted 3 lines
arch/x86/include/asm/cpu_device_id.h +3
-0+__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
Quoted 7 lines · Thomas Gleixner
Quoted 2 lines · Andi Kleen
+/*
+ * 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)
Quoted 6 lines
Quoted 1 line
+const struct
x86_ucode_id*x86_match_ucode_all(const structx86_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.
Quoted 9 lines
arch/x86/kernel/cpu/match.c const struct x86_cpu_id *x86_match_cpu(const struct x86_cpu_id *match) · line 83
Quoted 6 lines
arch/x86/kernel/cpu/match.c +5
-0+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 alockdep_assert_cpus_held().
In my case the caller, but yes should be documented.
-Andi
Borislav Petkov
maintainerQuoted 3 lines · Andi Kleen
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.
Quoted 2 lines
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.
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Quoted 5 lines · Andi Kleen
Quoted 4 lines · Thomas Gleixner
Quoted 2 lines · Andi Kleen
+/*
+ * Match specific microcodes or steppings.
Quoted 7 lines
Quoted 3 lines
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
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH v1 2/2] perf/x86/kvm: Avoid unnecessary work in guest filtering
Quoted 7 lines · Andi Kleen
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_ENABLEMSR.Intel also added a fix for this issue to microcode updates on Haswell/Broadwell/Skylake.
Quoted 6 lines
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_CTRLchange gets optimized by KVM into changing the VMCS.This patch checks for the microcode updates that have the microcode
# grep "This patch" Documentation/process
Review
Quoted 8 lines
fix for leaking PEBS, and disables the extra entry/exit list entry for
PEBS_ENABLE. In addition we always clear theGLOBAL_CTRLfor 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.(quoted diff) +2
-0+#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
Re: [PATCH v1 2/2] perf/x86/kvm: Avoid unnecessary work in guest filtering
Quoted 23 lines · Thomas Gleixner
Quoted 7 lines · Andi Kleen
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_ENABLEMSR.Intel also added a fix for this issue to microcode updates on Haswell/Broadwell/Skylake.
Quoted 6 lines
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_CTRLchange gets optimized by KVM into changing the VMCS.This patch checks for the microcode updates that have the microcode
# grep "This patch" Documentation/processQuoted 6 lines
fix for leaking PEBS, and disables the extra entry/exit list entry for
PEBS_ENABLE. In addition we always clear theGLOBAL_CTRLfor 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.
Quoted 8 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+#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