x86/microcode: 4.11 queue
Subsystems (2)
Borislav Petkov
maintainercover x86/microcode: 4.11 queue
From: Borislav Petkov <[email protected]>
Hi,
so this is more of a lessons-learned pile after the rewriting of the whole patch loading method and not caching addresses. It largely simplifies the loader - just look at the diffstat - without any functionality loss (I hope :-)). The driver is also very much readable now with unified, common paths where possible.
What is more, 6/13 reworks the whole AMD container parsing into much more readable separation of functionality and functions doing only one thing properly.
13/13 is also another nice simplification for the AP update path which looks almost straightforward in comparison with what we had before.
All has been tested on a lot of boxes and in different configurations:
* builtin vs initrd microcode
* old microcode_ctl application method for Intel
* "echo 1 > /sys/devices/system/cpu/microcode/reload" late method
* CONFIG_RANDOMIZE_MEMORY
both on Intel and AMD machines.
Please queue for 4.11.
Thanks.
Borislav Petkov (13):
x86/microcode/intel: Drop stashed AP patch pointer optimization x86/microcode: Use own MSR accessors x86/microcode/AMD: Clean up find_equiv_id() x86/microcode/AMD: Shorten function parameter's name x86/microcode/AMD: Extend the container struct x86/microcode/AMD: Rework container parsing x86/microcode: Decrease CPUID use
x86/microcode/AMD: Get rid of global this_equiv_id x86/microcode/AMD: Use find_microcode_in_initrd() x86/microcode/AMD: Check patch level only on the BSP x86/microcode/AMD: Unify load_ucode_amd_ap() x86/microcode/AMD: Simplify saving from initrd x86/microcode/AMD: Remove AP scanning optimization
arch/x86/include/asm/microcode.h | 29 +- arch/x86/include/asm/microcode_amd.h | 2 - arch/x86/include/asm/microcode_intel.h | 4 +- arch/x86/kernel/cpu/microcode/amd.c | 501 +++++++++++---------------------- arch/x86/kernel/cpu/microcode/core.c | 81 ++++-- arch/x86/kernel/cpu/microcode/intel.c | 13 +- 6 files changed, 248 insertions(+), 382 deletions(-)
Signature · 1 line
2.11.0
Borislav Petkov
maintainerPATCH 1/13 x86/microcode/intel: Drop stashed AP patch pointer optimization
From: Borislav Petkov <[email protected]>
This was meant to save us the scanning of the microcode containter in the initrd since the first AP had already done that but it can also hurt us:
Imagine a single hyperthreaded CPU (Intel(R) Atom(TM) CPU N270 for example) which updates the microcode on the BSP but since the microcode engine is shared between the two threads, the update on CPU1 doesn't happen because it has already happened on CPU0 and we don't find a newer microcode revision.
Which doesn't set the intel_ucode_patch pointer and at initrd jettisoning time and we don't save the microcode patch for later application.
Now, when we suspend to RAM, the loaded microcode gets cleared so we need to reload but there's no patch saved in the cache.
Removing this optimization fixes this issue and all is fine and dandy.
--- arch/x86/kernel/cpu/microcode/intel.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)
arch/x86/kernel/cpu/microcode/intel.c +1 -8
@@ -41,7 +41,7 @@static const char ucode_path[] ="kernel/x86/microcode/GenuineIntel.bin" ; -/* Current microcode patch used in early patching */ +/* Current microcode patch used in early patching on the APs. */ struct microcode_intel *intel_ucode_patch; static inline bool cpu_signatures_match(unsigned int s1, unsigned int p1,
@@ -607,12 +607,6 @@ int __init save_microcode_in_initrd_intel(void)struct ucode_cpu_info uci;struct cpio_data cp; -/* -* AP loading didn't find any microcode patch, no need to save anything. -*/ -if (!intel_ucode_patch ||IS_ERR (intel_ucode_patch)) -return 0 ; -if (!load_builtin_intel_microcode (&cp)) cp =find_microcode_in_initrd (ucode_path, false);
@@ -628,7 +622,6 @@ int __init save_microcode_in_initrd_intel(void)return 0 ; } -/* * @res_patch, output: a pointer to the patch we found. */
Signature · 1 line
2.11.0
Re: [PATCH 01/13] x86/microcode/intel: Drop stashed AP patch pointer optimization
Quoted 16 lines · Borislav Petkov
From: Borislav Petkov <[email protected]>
This was meant to save us the scanning of the microcode containter in the initrd since the first AP had already done that but it can also hurt us:
Imagine a single hyperthreaded CPU (Intel(R) Atom(TM) CPU N270 for example) which updates the microcode on the BSP but since the microcode engine is shared between the two threads, the update on CPU1 doesn't happen because it has already happened on CPU0 and we don't find a newer microcode revision.
Which doesn't set the
intel_ucode_patchpointer and at initrd jettisoning time and we don't save the microcode patch for later application.
Quoted 6 lines
Now, when we suspend to RAM, the loaded microcode gets cleared so we need to reload but there's no patch saved in the cache.
Removing this optimization fixes this issue and all is fine and dandy.
Signed-off-by Borislav Petkov <[email protected]>
So this one needs to go into x86/urgent
Borislav Petkov
maintainerPATCH 2/13 x86/microcode: Use own MSR accessors
In reply to Borislav PetkovFrom: Borislav Petkov <[email protected]>
Having tracepoints to the MSR accessors makes them unsuitable for early microcode loading: think 32-bit before paging is enabled and us chasing pointers to test whether a tracepoint is enabled or not. Results in a reliable triple fault.
Thus, define microcode loader-specific MSR accessors which do the bare minimum of reading or writing an MSR and nothing more.
--- arch/x86/include/asm/microcode.h | 29 +++++++++++++++++++++-------- arch/x86/include/asm/microcode_intel.h | 4 ++-- arch/x86/kernel/cpu/microcode/amd.c | 6 +++--- arch/x86/kernel/cpu/microcode/intel.c | 4 ++-- 4 files changed, 28 insertions(+), 15 deletions(-)
arch/x86/include/asm/microcode.h +21 -8
@@ -5,20 +5,33 @@#include <linux/earlycpio.h> #include <linux/initrd.h> -#define native_rdmsr (msr, val1, val2) \ +static inline unsigned long long __rdmsr (unsigned int msr) +{ +DECLARE_ARGS (val, low, high); + + asm volatile("1: rdmsr\n" + "2:\n" + : EAX_EDX_RET(val, low, high) : "c" (msr)); +return EAX_EDX_VAL (val, low, high); +} + +#define microcode_rdmsr (msr, val1, val2) \ do { \ - u64 __val = native_read_msr((msr)); \ + u64 __val = __rdmsr((msr)); \ (void)((val1) = (u32)__val); \ (void)((val2) = (u32)(__val >> 32)); \ } while (0) -#define native_wrmsr (msr, low, high) \ - native_write_msr(msr, low, high) +static inline void microcode_wrmsr (unsigned int msr,u64 val) +{ +u32 low, high; + + low = (u32 )val; + high = (u32)(val >>32 ); -#define native_wrmsrl (msr, val) \ - native_write_msr((msr), \ - (u32)((u64)(val)), \ - (u32)((u64)(val) >> 32)) + asmvolatile ("wrmsr\n" + ::"c" (msr),"a" (low),"d" (high) :"memory" ); +}struct ucode_patch {struct list_head plist;
arch/x86/include/asm/microcode_intel.h +2 -2
@@ -56,13 +56,13 @@ static inline u32 intel_get_microcode_revision(void) {u32 rev, dummy; -native_wrmsrl (MSR_IA32_UCODE_REV ,0 ); +microcode_wrmsr (MSR_IA32_UCODE_REV ,0 );/* As documented in the SDM: Do a CPUID 1 here */ native_cpuid_eax (1 );/* get the current revision from MSR 0x8B */ -native_rdmsr (MSR_IA32_UCODE_REV , dummy, rev); +microcode_rdmsr (MSR_IA32_UCODE_REV , dummy, rev);return rev; }
arch/x86/kernel/cpu/microcode/amd.c +3 -3
@@ -198,10 +198,10 @@ static int __apply_microcode_amd(struct microcode_amd *mc_amd) {u32 rev, dummy; -native_wrmsrl (MSR_AMD64_PATCH_LOADER , (u64)(long)&mc_amd->hdr.data_code); +microcode_wrmsr (MSR_AMD64_PATCH_LOADER , (u64)(long)&mc_amd->hdr.data_code);/* verify patch application was successful */ -native_rdmsr (MSR_AMD64_PATCH_LEVEL , rev, dummy); +microcode_rdmsr (MSR_AMD64_PATCH_LEVEL , rev, dummy);if (rev != mc_amd->hdr.patch_id)return -1 ;
@@ -656,7 +656,7 @@ bool check_current_patch_level(u32 *rev, bool early)bool ret = false;u32 *levels; -native_rdmsr (MSR_AMD64_PATCH_LEVEL , lvl, dummy); +microcode_rdmsr (MSR_AMD64_PATCH_LEVEL , lvl, dummy);if (IS_ENABLED (CONFIG_X86_32 ) && early) levels = (u32 *)__pa_nodebug (&final_levels);
arch/x86/kernel/cpu/microcode/intel.c +2 -2
@@ -387,7 +387,7 @@ static int collect_cpu_info_early(struct ucode_cpu_info *uci)if ((model >=5 ) || (family >6 )) {/* get processor flags from MSR 0x17 */ -native_rdmsr (MSR_IA32_PLATFORM_ID , val[0 ], val[1 ]); +microcode_rdmsr (MSR_IA32_PLATFORM_ID , val[0 ], val[1 ]); csig.pf =1 << ((val[1 ] >>18 ) &7 ); }
@@ -582,7 +582,7 @@ static int apply_microcode_early(struct ucode_cpu_info *uci, bool early)return 0 ;/* write microcode via MSR 0x79 */ -native_wrmsrl (MSR_IA32_UCODE_WRITE , (unsigned long )mc->bits); +microcode_wrmsr (MSR_IA32_UCODE_WRITE , (unsigned long )mc->bits); rev =intel_get_microcode_revision ();if (rev != mc->hdr.rev)
Signature · 1 line
2.11.0
Re: [PATCH 02/13] x86/microcode: Use own MSR accessors
Quoted 33 lines · Borislav Petkov
arch/x86/include/asm/microcode.h +16
-4@@ -5,20 +5,33 @@#include <linux/earlycpio.h> #include <linux/initrd.h> -#define native_rdmsr (msr, val1, val2) \ +static inline unsigned long long __rdmsr (unsigned int msr) +{ +DECLARE_ARGS (val, low, high); + + asm volatile("1: rdmsr\n" + "2:\n" + : EAX_EDX_RET(val, low, high) : "c" (msr)); +return EAX_EDX_VAL (val, low, high); +} + +#define microcode_rdmsr (msr, val1, val2) \ do { \ - u64 __val = native_read_msr((msr)); \ + u64 __val = __rdmsr((msr)); \ (void)((val1) = (u32)__val); \ (void)((val2) = (u32)(__val >> 32)); \ } while (0) -#define native_wrmsr (msr, low, high) \ - native_write_msr(msr, low, high) +static inline void microcode_wrmsr (unsigned int msr,u64 val) +{ +u32 low, high; +
arch/x86/include/asm/microcode.h line 34
Quoted 10 lines
arch/x86/include/asm/microcode.h +5
-4+ low = (u32 )val; + high = (u32)(val >>32 ); -#define native_wrmsrl (msr, val) \ - native_write_msr((msr), \ - (u32)((u64)(val)), \ - (u32)((u64)(val) >> 32)) + asmvolatile ("wrmsr\n" + ::"c" (msr),"a" (low),"d" (high) :"memory" ); +}
msr.h already has:
wrmsr_notrace() so adding wrmsrl_notrace() to it should be a nobrainer.
Providing rdmsr_notrace() in msr.h is a natural extension of the existing interfaces.
That would get rid of all the extra microcode specific MSR accessors which are just yet another copy of stuff in msr.h.
Thanks,
tglx
Borislav Petkov
maintainerRe: [PATCH 02/13] x86/microcode: Use own MSR accessors
Quoted 2 lines · Thomas Gleixner
That would get rid of all the extra microcode specific MSR accessors which are just yet another copy of stuff in
msr.h.
Well, I did think about reusing those but last time I did, they received those tracepoints (apparently, we're sprinkling dumb tracepoints left and right because good ol' staring at the code is just too hard) which simply doesn't work on 32-bit before paging is enabled.
Then, __native_write_msr_notrace() has exception handling which doesn't work before paging has been enabled on 32-bit - this is when the 32-bit microcode update path happens due to paging hardware bugs in CPUs which are fixed in microcode. So we must run that early on 32-bit.
So before someone decides to add more "functionality" to the generic MSR accessors and break the microcode loader once more, I'd really really prefer to have private accessors. They're small enough so shouldn't be that much of a bloat.
Thanks.
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Re: [PATCH 02/13] x86/microcode: Use own MSR accessors
Quoted 9 lines · Borislav Petkov
Quoted 2 lines · Thomas Gleixner
That would get rid of all the extra microcode specific MSR accessors which are just yet another copy of stuff in
msr.h.Well, I did think about reusing those but last time I did, they received those tracepoints (apparently, we're sprinkling dumb tracepoints left and right because good ol' staring at the code is just too hard) which simply doesn't work on 32-bit before paging is enabled.
Quoted 4 lines
Then,
__native_write_msr_notrace()has exception handling which doesn't work before paging has been enabled on 32-bit - this is when the 32-bit microcode update path happens due to paging hardware bugs in CPUs which are fixed in microcode. So we must run that early on 32-bit.
Well, the exception handling is irrelevant in that case. If the MSR access succeeds then the exception handling is not invoked. If it fails then it does not matter much whether you die from the MSR #GP or from the exception handling attempt #GP.
Quoted 4 lines
So before someone decides to add more "functionality" to the generic MSR accessors and break the microcode loader once more, I'd really really prefer to have private accessors. They're small enough so shouldn't be that much of a bloat.
I can understand that, but we better have real native functions in msr.h which do exactly what they are supposed to do: read or write the MSR, nothing else. Add a comment to those function which should be immutable and threaten that offenders will be slapped with stinking trouts or frozen sharks.
We really want to have a single place for all of this MSR stuff, because following your argumentation will be a perfect precedence for more private implementations.
Thanks,
tglx
Borislav Petkov
maintainerRe: [PATCH 02/13] x86/microcode: Use own MSR accessors
Quoted 3 lines · Thomas Gleixner
We really want to have a single place for all of this MSR stuff, because following your argumentation will be a perfect precedence for more private implementations.
Yes, you're so right. Ok, how about this untested thing. The warning should be saucy enough :-)
---
From: Borislav Petkov <[email protected]>
Date: Tue, 17 Jan 2017 23:29:10 +0100
Subject: [PATCH] x86/MSR: Carve out bare minimum accessors
Add __rdmsr() and __wrmsr() which *only* read and write an MSR with exception handling. Those are going to be used in early code, like the microcode loader, which cannot stomach tracing code piggybacking on the MSR operation.
While at it, get rid of __native_write_msr_notrace().
--- arch/x86/include/asm/apic.h | 2 +- arch/x86/include/asm/msr.h | 51 +++++++++++++++++++++++++++------------------ 2 files changed, 32 insertions(+), 21 deletions(-)
arch/x86/include/asm/apic.h +1 -1
@@ -195,7 +195,7 @@ static inline void native_apic_msr_write(u32 reg, u32 v)static inline void native_apic_msr_eoi_write (u32 reg,u32 v) { -wrmsr_notrace (APIC_BASE_MSR + (APIC_EOI >>4 ),APIC_EOI_ACK ,0 ); +__wrmsr (APIC_BASE_MSR + (APIC_EOI >>4 ),APIC_EOI_ACK ,0 ); }static inline u32 native_apic_msr_read (u32 reg)
arch/x86/include/asm/msr.h +31 -20
78 lines not on this page · raw message
Signature · 7 lines
2.11.0
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Re: [PATCH 02/13] x86/microcode: Use own MSR accessors
Quoted 10 lines · Borislav Petkov
From: Borislav Petkov <[email protected]>
Date: Tue, 17 Jan 2017 23:29:10 +0100
Subject: [PATCH] x86/MSR: Carve out bare minimum accessorsAdd
__rdmsr()and__wrmsr()which *only* read and write an MSR with exception handling. Those are going to be used in early code, like the microcode loader, which cannot stomach tracing code piggybacking on the MSR operation.While at it, get rid of
__native_write_msr_notrace().
Looks good.
Borislav Petkov
maintainerPATCH 3/13 x86/microcode/AMD: Clean up find_equiv_id()
In reply to Borislav PetkovFrom: Borislav Petkov <[email protected]>
No need to have it marked "inline" - let gcc decide. Also, shorten the argument name and simplify while-test.
No functionality change.
--- arch/x86/kernel/cpu/microcode/amd.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
arch/x86/kernel/cpu/microcode/amd.c +5 -6
@@ -97,17 +97,16 @@ static size_t compute_container_size(u8 *data, u32 total_size)return size; } -static inline u16 find_equiv_id (struct equiv_cpu_entry *equiv_cpu_table, -unsigned int sig) +static u16 find_equiv_id (struct equiv_cpu_entry *equiv_table,u32 sig) {int i =0 ; -if (!equiv_cpu_table) +if (!equiv_table)return 0 ; -while (equiv_cpu_table[i].installed_cpu !=0 ) { -if (sig == equiv_cpu_table[i].installed_cpu) -return equiv_cpu_table[i].equiv_cpu; +while (equiv_table[i].installed_cpu) { +if (sig == equiv_table[i].installed_cpu) +return equiv_table[i].equiv_cpu; i++; }
Signature · 1 line
2.11.0
Re: [PATCH 03/13] x86/microcode/AMD: Clean up find_equiv_id()
Quoted 28 lines · Borislav Petkov
From: Borislav Petkov <[email protected]>
No need to have it marked "inline" - let gcc decide. Also, shorten the argument name and simplify while-test.
No functionality change.
Signed-off-by Borislav Petkov <[email protected]>--- arch/x86/kernel/cpu/microcode/amd.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)arch/x86/kernel/cpu/microcode/amd.c +2
-3@@ -97,17 +97,16 @@ static size_t compute_container_size(u8 *data, u32 total_size)return size; } -static inline u16 find_equiv_id (struct equiv_cpu_entry *equiv_cpu_table, -unsigned int sig) +static u16 find_equiv_id (struct equiv_cpu_entry *equiv_table,u32 sig) {int i =0 ; -if (!equiv_cpu_table) +if (!equiv_table)
arch/x86/kernel/cpu/microcode/amd.c static size_t compute_container_size(u8 *data, u32 total_size) · line 111
Quoted 10 lines
arch/x86/kernel/cpu/microcode/amd.c +3
-3return 0 ; -while (equiv_cpu_table[i].installed_cpu !=0 ) { -if (sig == equiv_cpu_table[i].installed_cpu) -return equiv_cpu_table[i].equiv_cpu; +while (equiv_table[i].installed_cpu) { +if (sig == equiv_table[i].installed_cpu) +return equiv_table[i].equiv_cpu; i++;
While you are at it, please rewrite that thing as a for loop, which is the obvious correct thing:
for (i = 0; equiv_table[i].installed_cpu; i++)
and even simpler:
for (; equiv_table->installed_cpu; equiv_table++)
Hmm?
Thanks,
tglx
Borislav Petkov
maintainerRe: [PATCH 03/13] x86/microcode/AMD: Clean up find_equiv_id()
Quoted 3 lines · Thomas Gleixner
and even simpler:
for (; equiv_table->installed_cpu; equiv_table++)
Very nice, thanks!
Here's v1.1:
---
From: Borislav Petkov <[email protected]>
Date: Sun, 18 Dec 2016 12:05:50 +0100
Subject: [PATCH -v1.1 03/13] x86/microcode/AMD: Clean up find_equiv_id()
No need to have it marked "inline" - let gcc decide. Also, shorten the argument name and simplify while-test.
While at it, make it into a proper for-loop and simplify it even more, as tglx suggests.
No functionality change.
--- arch/x86/kernel/cpu/microcode/amd.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-)
arch/x86/kernel/cpu/microcode/amd.c +4 -12
@@ -97,20 +97,12 @@ static size_t compute_container_size(u8 *data, u32 total_size)return size; } -static inline u16 find_equiv_id (struct equiv_cpu_entry *equiv_cpu_table, -unsigned int sig) +static u16 find_equiv_id (struct equiv_cpu_entry *equiv_table,u32 sig) { -int i =0 ; - -if (!equiv_cpu_table) -return 0 ; +for (; equiv_table && equiv_table->installed_cpu; equiv_table++) +if (sig == equiv_table->installed_cpu) +return equiv_table->equiv_cpu; -while (equiv_cpu_table[i].installed_cpu !=0 ) { -if (sig == equiv_cpu_table[i].installed_cpu) -return equiv_cpu_table[i].equiv_cpu; - - i++; - }return 0 ; }
Signature · 7 lines
2.11.0
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Re: [PATCH 03/13] x86/microcode/AMD: Clean up find_equiv_id()
arch/x86/kernel/cpu/microcode/amd.c static size_t compute_container_size(u8 *data, u32 total_size) · line 104
Quoted 3 lines · Borislav Petkov
arch/x86/kernel/cpu/microcode/amd.c +3
-0+for (; equiv_table && equiv_table->installed_cpu; equiv_table++) +if (sig == equiv_table->installed_cpu) +return equiv_table->equiv_cpu;
This would be perfect if you just kept the braces around the for loop.
for (; cond; incr) do_something();
parses perfectly fine as it matches the expectation of a single line statement following the for().
for (; cond; incr) if (othercond) do_something();
not so much because we expect a single line statement due to the lack of a opening brace after the for()
for (; cond; incr) {
if (othercond)
do_something();
}That's how it parses best. The opening brace after the for() tells us: here comes a multiline statement. And the inner if (othercond) w/o the opening brace tells: here comes a single line statement.
Reading code/patches very much depends on patterns and structuring. If they are consistent the reading flow is undisturbed.
Thanks,
tglx
Borislav Petkov
maintainerRe: [PATCH 03/13] x86/microcode/AMD: Clean up find_equiv_id()
Quoted 6 lines · Thomas Gleixner
That's how it parses best. The opening brace after the
for()tells us: here comes a multiline statement. And the inner if (othercond) w/o the opening brace tells: here comes a single line statement.Reading code/patches very much depends on patterns and structuring. If they are consistent the reading flow is undisturbed.
Yeah, very true.
---
From: Borislav Petkov <[email protected]>
Date: Sun, 18 Dec 2016 12:05:50 +0100
Subject: [PATCH] x86/microcode/AMD: Clean up find_equiv_id()
No need to have it marked "inline" - let gcc decide. Also, shorten the argument name and simplify while-test.
While at it, make it into a proper for-loop and simplify it even more, as tglx suggests.
No functionality change.
--- arch/x86/kernel/cpu/microcode/amd.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)
arch/x86/kernel/cpu/microcode/amd.c +5 -12
@@ -97,20 +97,13 @@ static size_t compute_container_size(u8 *data, u32 total_size)return size; } -static inline u16 find_equiv_id (struct equiv_cpu_entry *equiv_cpu_table, -unsigned int sig) +static u16 find_equiv_id (struct equiv_cpu_entry *equiv_table,u32 sig) { -int i =0 ; - -if (!equiv_cpu_table) -return 0 ; - -while (equiv_cpu_table[i].installed_cpu !=0 ) { -if (sig == equiv_cpu_table[i].installed_cpu) -return equiv_cpu_table[i].equiv_cpu; - - i++; +for (; equiv_table && equiv_table->installed_cpu; equiv_table++) { +if (sig == equiv_table->installed_cpu) +return equiv_table->equiv_cpu; } +return 0 ; }
Signature · 7 lines
2.11.0
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Borislav Petkov
maintainerPATCH 4/13 x86/microcode/AMD: Shorten function parameter's name
In reply to Borislav PetkovFrom: Borislav Petkov <[email protected]>
The whole driver calls this "mc", do that here too.
No functionality change.
--- arch/x86/kernel/cpu/microcode/amd.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
arch/x86/kernel/cpu/microcode/amd.c +3 -3
@@ -193,15 +193,15 @@ find_proper_container(u8 *ucode, size_t size, struct container *ret_cont)return eq_id; } -static int __apply_microcode_amd (struct microcode_amd *mc_amd) +static int __apply_microcode_amd (struct microcode_amd *mc) {u32 rev, dummy; -microcode_wrmsr (MSR_AMD64_PATCH_LOADER , (u64)(long)&mc_amd->hdr.data_code); +microcode_wrmsr (MSR_AMD64_PATCH_LOADER , (u64)(long)&mc->hdr.data_code);/* verify patch application was successful */ microcode_rdmsr (MSR_AMD64_PATCH_LEVEL , rev, dummy); -if (rev != mc_amd->hdr.patch_id) +if (rev != mc->hdr.patch_id)return -1 ;return 0 ;
Signature · 1 line
2.11.0
Re: [PATCH 04/13] x86/microcode/AMD: Shorten function parameter's name
Quoted 7 lines · Borislav Petkov
From: Borislav Petkov <[email protected]>
The whole driver calls this "mc", do that here too.
No functionality change.
Signed-off-by Borislav Petkov <[email protected]>
Borislav Petkov
maintainerPATCH 5/13 x86/microcode/AMD: Extend the container struct
In reply to Borislav PetkovFrom: Borislav Petkov <[email protected]>
Make it into a container descriptor which is being passed around and stores important info like the matching container and the patch for the current CPU.
Later patches will use this and thus get rid of a double container parsing.
--- arch/x86/kernel/cpu/microcode/amd.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)
arch/x86/kernel/cpu/microcode/amd.c +13 -9
@@ -42,9 +42,13 @@ static struct equiv_cpu_entry *equiv_cpu_table;/* * This points to the current valid container of microcode patches which we will -* save from the initrd/builtin before jettisoning its contents. +* save from the initrd/builtin before jettisoning its contents. @mc is the +* microcode patch we found to match. */ -struct container { +struct cont_desc { +struct microcode_amd *mc; +u32 psize; +u16 eq_id;u8 *data;size_t size; } cont;
@@ -119,9 +123,9 @@ static u16 find_equiv_id(struct equiv_cpu_entry *equiv_table, u32 sig)* table or 0 if none found. */ static u16 -find_proper_container (u8 *ucode,size_t size,struct container *ret_cont) +find_proper_container (u8 *ucode,size_t size,struct cont_desc *desc) { -struct container ret = {NULL ,0 }; +struct cont_desc ret = {0 };u32 eax, ebx, ecx, edx;struct equiv_cpu_entry *eq;int offset, left;
@@ -164,7 +168,7 @@ find_proper_container(u8 *ucode, size_t size, struct container *ret_cont)*/ left = ret.size - offset; - *ret_cont = ret; + *desc = ret;return eq_id; }
@@ -219,11 +223,11 @@ static int __apply_microcode_amd(struct microcode_amd *mc)* Returns true if container found (sets @ret_cont), false otherwise. */ static bool apply_microcode_early_amd (void *ucode,size_t size,bool save_patch, -struct container *ret_cont) +struct cont_desc *desc) {u8 (*patch)[PATCH_MAX_SIZE ];u32 rev, *header, *new_rev; -struct container ret; +struct cont_desc ret;int offset, left;u16 eq_id =0 ;u8 *data;
@@ -276,8 +280,8 @@ static bool apply_microcode_early_amd(void *ucode, size_t size, bool save_patch, left -= offset; } -if (ret_cont) - *ret_cont = ret; +if (desc) + *desc = ret;return true; }
Signature · 1 line
2.11.0
Re: [PATCH 05/13] x86/microcode/AMD: Extend the container struct
arch/x86/kernel/cpu/microcode/amd.c static struct equiv_cpu_entry *equiv_cpu_table; · line 53
Quoted 7 lines · Borislav Petkov
arch/x86/kernel/cpu/microcode/amd.c +4
-1-struct container { +struct cont_desc { +struct microcode_amd *mc; +u32 psize; +u16 eq_id;u8 *data;size_t size;
While at it can you please structure the members in tabular fashion?
struct cont_desc {
struct microcode_amd *mc;
u32 psize;
...Other than that:
Borislav Petkov
maintainerPATCH 6/13 x86/microcode/AMD: Rework container parsing
In reply to Borislav PetkovFrom: Borislav Petkov <[email protected]>
It was pretty clumsy before and the whole work of parsing the microcode containers was spread around the functions wrongly.
Clean it up so that there's a main scan_containers() function which iterates over the microcode blob and picks apart the containers glued together. For each container, it calls a parse_container() helper which concentrates on one container only: sanity-checking, parsing, counting microcode patches in there, etc.
It makes much more sense now and it is actually very readable. Oh, and we luvz a diffstat removing more crap than adding.
--- arch/x86/kernel/cpu/microcode/amd.c | 241 ++++++++++++++++-------------------- 1 file changed, 108 insertions(+), 133 deletions(-)
arch/x86/kernel/cpu/microcode/amd.c +108 -133
@@ -64,43 +64,6 @@ static u16 this_equiv_id;static const char ucode_path[]__maybe_unused ="kernel/x86/microcode/AuthenticAMD.bin" ; -static size_t compute_container_size (u8 *data,u32 total_size) -{ -size_t size =0 ; -u32 *header = (u32 *)data; - -if (header[0 ] !=UCODE_MAGIC || - header[1 ] !=UCODE_EQUIV_CPU_TABLE_TYPE ||/* type */ - header[2 ] ==0 )/* size */ -return size; - - size = header[2 ] +CONTAINER_HDR_SZ ; - total_size -= size; - data += size; - -while (total_size) { -u16 patch_size; - - header = (u32 *)data; - -if (header[0 ] !=UCODE_UCODE_TYPE ) -break ; - -/* -* Sanity-check patch size. -*/ - patch_size = header[1 ]; -if (patch_size >PATCH_MAX_SIZE ) -break ; - - size += patch_size +SECTION_HDR_SIZE ; - data += patch_size +SECTION_HDR_SIZE ; - total_size -= patch_size +SECTION_HDR_SIZE ; - } - -return size; -} -static u16 find_equiv_id (struct equiv_cpu_entry *equiv_table,u32 sig) {int i =0 ;
@@ -121,80 +84,109 @@ static u16 find_equiv_id(struct equiv_cpu_entry *equiv_table, u32 sig)* This scans the ucode blob for the proper container as we can have multiple * containers glued together. Returns the equivalence ID from the equivalence * table or 0 if none found. +* Returns the amount of bytes consumed while scanning. @desc contains all the +* data we're going to use in later stages of the application. */ -static u16 -find_proper_container (u8 *ucode,size_t size,struct cont_desc *desc) +static ssize_t parse_container (u8 *ucode,ssize_t size,struct cont_desc *desc) { -struct cont_desc ret = {0 }; -u32 eax, ebx, ecx, edx;struct equiv_cpu_entry *eq; -int offset, left; -u16 eq_id =0 ; -u32 *header; -u8 *data; +ssize_t orig_size = size; +u32 *hdr = (u32 *)ucode; +u32 eax, ebx, ecx, edx; +u16 eq_id; +u8 *buf; - data = ucode; - left = size; - header = (u32 *)data; +/* Am I looking at an equivalence table header? */ +if (hdr[0 ] !=UCODE_MAGIC || + hdr[1 ] !=UCODE_EQUIV_CPU_TABLE_TYPE || + hdr[2 ] ==0 ) { + desc->eq_id =0 ; +return CONTAINER_HDR_SZ ; + } + buf = ucode; -/* find equiv cpu table */ -if (header[0 ] !=UCODE_MAGIC || - header[1 ] !=UCODE_EQUIV_CPU_TABLE_TYPE ||/* type */ - header[2 ] ==0 )/* size */ -return eq_id; + eq = (struct equiv_cpu_entry *)(buf +CONTAINER_HDR_SZ ); - eax =0x00000001 ; + eax =1 ; ecx =0 ;native_cpuid (&eax, &ebx, &ecx, &edx); -while (left >0 ) { - eq = (struct equiv_cpu_entry *)(data +CONTAINER_HDR_SZ ); +/* Find the equivalence ID of our CPU in this table: */ + eq_id =find_equiv_id (eq, eax); - ret.data = data; + buf += hdr[2 ] +CONTAINER_HDR_SZ ; + size -= hdr[2 ] +CONTAINER_HDR_SZ ; -/* Advance past the container header */ - offset = header[2 ] +CONTAINER_HDR_SZ ; - data += offset; - left -= offset; +/* +* Scan through the rest of the container to find where it ends. We do +* some basic sanity-checking too. +*/ +while (size >0 ) { +struct microcode_amd *mc; +u32 patch_size; - eq_id =find_equiv_id (eq, eax); -if (eq_id) { - ret.size =compute_container_size (ret.data, left + offset); + hdr = (u32 *)buf; -/* -* truncate how much we need to iterate over in the -* ucode update loop below -*/ - left = ret.size - offset; +if (hdr[0 ] !=UCODE_UCODE_TYPE ) +break ; - *desc = ret; -return eq_id; +/* Sanity-check patch size. */ + patch_size = hdr[1 ]; +if (patch_size >PATCH_MAX_SIZE ) { +/* Something corrupted the container, invalidate it. */ + eq_id =0 ; +break ; } -/* -* support multiple container files appended together. if this -* one does not have a matching equivalent cpu entry, we fast -* forward to the next container file. -*/ -while (left >0 ) { - header = (u32 *)data; +/* Skip patch section header: */ + buf +=SECTION_HDR_SIZE ; + size -=SECTION_HDR_SIZE ; -if (header[0 ] ==UCODE_MAGIC && - header[1 ] ==UCODE_EQUIV_CPU_TABLE_TYPE ) -break ; - - offset = header[1 ] +SECTION_HDR_SIZE ; - data += offset; - left -= offset; + mc = (struct microcode_amd *)buf; +if (eq_id == mc->hdr.processor_rev_id) { + desc->psize = patch_size; + desc->mc = mc; } -/* mark where the next microcode container file starts */ - offset = data - (u8 *)ucode; - ucode = data; + buf += patch_size; + size -= patch_size; + } + +/* +* If we have found an eq_id, it means we're looking at the container +* which has a patch for this CPU so return 0 to mean, @ucode already +* points to it and it will be parsed later. Otherwise, we return the +* size we scanned so that we can advance to the next container in the +* buffer. +*/ +if (eq_id) { + desc->eq_id = eq_id; + desc->data = ucode; + desc->size = orig_size - size; + +return 0 ; } -return eq_id; +return orig_size - size; +} + +/* +* Scan the ucode blob for the proper container as we can have multiple +* containers glued together. +*/ +static void scan_containers (u8 *ucode,size_t size,struct cont_desc *desc) +{ +ssize_t rem = size; + +while (rem >=0 ) { +ssize_t s =parse_container (ucode, rem, desc); +if (!s) +return ; + + ucode += s; + rem -= s; + } }static int __apply_microcode_amd (struct microcode_amd *mc)
@@ -220,17 +212,16 @@ static int __apply_microcode_amd(struct microcode_amd *mc)* load_microcode_amd() to save equivalent cpu table and microcode patches in * kernel heap memory. * -* Returns true if container found (sets @ret_cont), false otherwise. +* Returns true if container found (sets @desc), false otherwise. */ static bool apply_microcode_early_amd (void *ucode,size_t size,bool save_patch, -struct cont_desc *desc) +struct cont_desc *ret_desc) { +struct cont_desc desc = {0 };u8 (*patch)[PATCH_MAX_SIZE ]; -u32 rev, *header, *new_rev; -struct cont_desc ret; -int offset, left; -u16 eq_id =0 ; -u8 *data; +struct microcode_amd *mc; +u32 rev, *new_rev; +bool ret = false;#ifdef CONFIG_X86_32 new_rev = (u32 *)__pa_nodebug (&ucode_new_rev);
@@ -241,49 +232,33 @@ static bool apply_microcode_early_amd(void *ucode, size_t size, bool save_patch,#endif if (check_current_patch_level (&rev, true)) -return false; - - eq_id =find_proper_container (ucode, size, &ret); -if (!eq_id) -return false; - - this_equiv_id = eq_id; - header = (u32 *)ret.data; - -/* We're pointing to an equiv table, skip over it. */ - data = ret.data + header[2 ] +CONTAINER_HDR_SZ ; - left = ret.size - (header[2 ] +CONTAINER_HDR_SZ ); - -while (left >0 ) { -struct microcode_amd *mc; +return ret; - header = (u32 *)data; -if (header[0 ] !=UCODE_UCODE_TYPE ||/* type */ - header[1 ] ==0 )/* size */ -break ; +scan_containers (ucode, size, &desc); +if (!desc.eq_id) +return ret; - mc = (struct microcode_amd *)(data +SECTION_HDR_SIZE ); + this_equiv_id = desc.eq_id; -if (eq_id == mc->hdr.processor_rev_id && rev < mc->hdr.patch_id) { + mc = desc.mc; +if (!mc) +return ret; -if (!__apply_microcode_amd (mc)) { - rev = mc->hdr.patch_id; - *new_rev = rev; +if (rev >= mc->hdr.patch_id) +return ret; -if (save_patch) -memcpy (patch, mc,min_t (u32, header[1 ],PATCH_MAX_SIZE )); - } - } +if (!__apply_microcode_amd (mc)) { + *new_rev = mc->hdr.patch_id; + ret = true; - offset = header[1 ] +SECTION_HDR_SIZE ; - data += offset; - left -= offset; +if (save_patch) +memcpy (patch, mc,min_t (u32, desc.psize,PATCH_MAX_SIZE )); } -if (desc) - *desc = ret; +if (ret_desc) + *ret_desc = desc; -return true; +return ret; }static bool get_builtin_microcode (struct cpio_data *cp,unsigned int family)
@@ -402,6 +377,7 @@ void load_ucode_amd_ap(unsigned int family) }if (!apply_microcode_early_amd (cp.data, cp.size, false, &cont)) { + cont.data =NULL ; cont.size =-1 ;return ; }
@@ -440,7 +416,6 @@ int __init save_microcode_in_initrd_amd(unsigned int fam) {enum ucode_state ret;int retval =0 ; -u16 eq_id;if (!cont.data) {if (IS_ENABLED (CONFIG_X86_32 ) && (cont.size !=-1 )) {
@@ -456,8 +431,8 @@ int __init save_microcode_in_initrd_amd(unsigned int fam)return -EINVAL ; } - eq_id =find_proper_container (cp.data, cp.size, &cont); -if (!eq_id) { +scan_containers (cp.data, cp.size, &cont); +if (!cont.eq_id) { cont.size =-1 ;return -EINVAL ; }
Signature · 1 line
2.11.0
Re: [PATCH 06/13] x86/microcode/AMD: Rework container parsing
arch/x86/kernel/cpu/microcode/amd.c static u16 find_equiv_id(struct equiv_cpu_entry *equiv_table, u32 sig) · line 116
Quoted 2 lines · Borislav Petkov
arch/x86/kernel/cpu/microcode/amd.c +2
-0+/* Find the equivalence ID of our CPU in this table: */ + eq_id =find_equiv_id (eq, eax);
So here we figure out whether the blob claims to have a patch for that cpu.
arch/x86/kernel/cpu/microcode/amd.c static u16 find_equiv_id(struct equiv_cpu_entry *equiv_table, u32 sig) · line 124
Quoted 4 lines
arch/x86/kernel/cpu/microcode/amd.c +4
-0+/* +* Scan through the rest of the container to find where it ends. We do +* some basic sanity-checking too. +*/
Stupid question. Why do we need to walk through that blob if we already know that it does not contain a patch for this cpu, i.e. eq_id == 0 ?
I assume it has to do with the multiple containers glued together in the blob, but that should be mentioned in the comment.
Quoted 39 lines
arch/x86/kernel/cpu/microcode/amd.c +15
-18+while (size >0 ) { +struct microcode_amd *mc; +u32 patch_size; - eq_id =find_equiv_id (eq, eax); -if (eq_id) { - ret.size =compute_container_size (ret.data, left + offset); + hdr = (u32 *)buf; -/* -* truncate how much we need to iterate over in the -* ucode update loop below -*/ - left = ret.size - offset; +if (hdr[0 ] !=UCODE_UCODE_TYPE ) +break ; - *desc = ret; -return eq_id; +/* Sanity-check patch size. */ + patch_size = hdr[1 ]; +if (patch_size >PATCH_MAX_SIZE ) { +/* Something corrupted the container, invalidate it. */ + eq_id =0 ; +break ; } -/* -* support multiple container files appended together. if this -* one does not have a matching equivalent cpu entry, we fast -* forward to the next container file. -*/ -while (left >0 ) { - header = (u32 *)data; +/* Skip patch section header: */ + buf +=SECTION_HDR_SIZE ; + size -=SECTION_HDR_SIZE ; - if (header[0] == UCODE_MAGIC &&
arch/x86/kernel/cpu/microcode/amd.c static u16 find_equiv_id(struct equiv_cpu_entry *equiv_table, u32 sig) · line 149
Quoted 10 lines
arch/x86/kernel/cpu/microcode/amd.c +4
-6- header[1] == UCODE_EQUIV_CPU_TABLE_TYPE) -break ; - - offset = header[1 ] +SECTION_HDR_SIZE ; - data += offset; - left -= offset; + mc = (struct microcode_amd *)buf; +if (eq_id == mc->hdr.processor_rev_id) { + desc->psize = patch_size; + desc->mc = mc;
So here you set patch_size and mc when the eq_id is matching. I assume we continue the scan for the same reason as we do the scan for eq_id = 0, right?
Quoted 7 lines
arch/x86/kernel/cpu/microcode/amd.c +2
-3} -/* mark where the next microcode container file starts */ - offset = data - (u8 *)ucode; - ucode = data; + buf += patch_size; + size -= patch_size;
arch/x86/kernel/cpu/microcode/amd.c static u16 find_equiv_id(struct equiv_cpu_entry *equiv_table, u32 sig) · line 163
Quoted 10 lines
arch/x86/kernel/cpu/microcode/amd.c +10
-0+ } + +/* +* If we have found an eq_id, it means we're looking at the container +* which has a patch for this CPU so return 0 to mean, @ucode already +* points to it and it will be parsed later. Otherwise, we return the +* size we scanned so that we can advance to the next container in the +* buffer. +*/ +if (eq_id) {
Now this one is dangerous. If the blob is corrupted we might have exited the loop above due to
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (hdr[0] != UCODE_UCODE_TYPE) + break;
before the eq_id matching happened. In that case we return success, but desc->psize and desc->mc are not set. Not what you want, right?
arch/x86/kernel/cpu/microcode/amd.c static bool apply_microcode_early_amd(void *ucode, size_t size, bool save_patch, · line 244
Quoted 11 lines
arch/x86/kernel/cpu/microcode/amd.c +5
-1+ desc->eq_id = eq_id; + desc->data = ucode; + desc->size = orig_size - size; + +return 0 ;@@ -241,49 +232,33 @@ static bool apply_microcode_early_amd(void *ucode, size_t size, bool save_patch,#endif if (check_current_patch_level (&rev, true)) -return false;
So this becomes
Review static bool apply_microcode_early_amd(void *ucode, size_t size, bool save_patch, · line 235
Quoted 1 line
(quoted diff) +1
-0+ return ret;
which is not really better than the original code. I think we really should only use the variable when there is something which can change between two points, but that's my personal preference and up to you :)
arch/x86/kernel/cpu/microcode/amd.c static bool apply_microcode_early_amd(void *ucode, size_t size, bool save_patch, · line 241
Quoted 5 lines
arch/x86/kernel/cpu/microcode/amd.c +3
-1+if (!desc.eq_id) +return ret; - mc = (struct microcode_amd *)(data +SECTION_HDR_SIZE ); + this_equiv_id = desc.eq_id;
Why are you storing the id when you don't have an idea whether the patch is actually available and useable? There might be a proper reason, but w/o a comment or access to the microcode crystalball it's hard to tell.
arch/x86/kernel/cpu/microcode/amd.c void load_ucode_amd_ap(unsigned int family) · line 381
Quoted 7 lines
arch/x86/kernel/cpu/microcode/amd.c +1
-0static bool get_builtin_microcode (struct cpio_data *cp,unsigned int family)@@ -402,6 +377,7 @@ void load_ucode_amd_ap(unsigned int family) }if (!apply_microcode_early_amd (cp.data, cp.size, false, &cont)) { + cont.data =NULL ; cont.size =-1 ;
What's the point of fiddling with the local variable at all if we return right away?
Quoted 11 lines
arch/x86/kernel/cpu/microcode/amd.c +0
-1return ; }@@ -440,7 +416,6 @@ int __init save_microcode_in_initrd_amd(unsigned int fam) {enum ucode_state ret;int retval =0 ; -u16 eq_id;if (!cont.data) {if (IS_ENABLED (CONFIG_X86_32 ) && (cont.size !=-1 )) {
arch/x86/kernel/cpu/microcode/amd.c int __init save_microcode_in_initrd_amd(unsigned int fam) · line 436
Quoted 8 lines
arch/x86/kernel/cpu/microcode/amd.c +2
-2@@ -456,8 +431,8 @@ int __init save_microcode_in_initrd_amd(unsigned int fam)return -EINVAL ; } - eq_id =find_proper_container (cp.data, cp.size, &cont); -if (!eq_id) { +scan_containers (cp.data, cp.size, &cont); +if (!cont.eq_id) { cont.size =-1 ;
Ditto. That might be fixed in a seperate patch because thats existing code.
Review int __init save_microcode_in_initrd_amd(unsigned int fam) · line 437
Quoted 1 line
(quoted diff) +0
-0return -EINVAL;
Thanks,
tglx
Borislav Petkov
maintainerRe: [PATCH 06/13] x86/microcode/AMD: Rework container parsing
Quoted 5 lines · Thomas Gleixner
Stupid question. Why do we need to walk through that blob if we already know that it does not contain a patch for this cpu, i.e.
eq_id== 0 ?I assume it has to do with the multiple containers glued together in the blob, but that should be mentioned in the comment.
It is mentioned a bit further down in the comment:
/*
* If we have found an eq_id, it means we're looking at the container
* which has a patch for this CPU so return 0 to mean, @ucode already
* points to it and it will be parsed later. Otherwise, we return the
* size we scanned so that we can advance to the next container in the
* buffer.
*/
Quoted 2 lines
So here you set
patch_sizeand mc when theeq_idis matching. I assume we continue the scan for the same reason as we do the scan foreq_id= 0, right?
Yes. Also, theoretically, some later container might hold an even newer patch so we will update that pointer when we get there.
Quoted 8 lines
Now this one is dangerous. If the blob is corrupted we might have exited the loop above due to
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (hdr[0] != UCODE_UCODE_TYPE) + break;before the
eq_idmatching happened. In that case we return success, butdesc->psizeanddesc->mcare not set. Not what you want, right?
Ah, good catch. I guess I'll have to do here:
/* Something corrupted the container, invalidate it. */
eq_id = 0;
break;too.
Quoted 6 lines
So this becomes
Review
Quoted 1 line
(quoted diff) +1
-0+ return ret;which is not really better than the original code. I think we really should only use the variable when there is something which can change between two points, but that's my personal preference and up to you :)
Ok, changed. This check goes away later anyway - it gets moved to the BSP early path.
Quoted 3 lines
Why are you storing the id when you don't have an idea whether the patch is actually available and useable? There might be a proper reason, but w/o a comment or access to the microcode crystalball it's hard to tell.
That is gone later, as you discovered yourself.
The thing is, it is hard to change things in this driver one at a time without breaking it so I had to restrain myself.
Updated and untested version below. Testing is scheduled for the coming days :-)
---
From: Borislav Petkov <[email protected]>
Date: Sun, 18 Dec 2016 20:53:09 +0100
Subject: [PATCH] x86/microcode/AMD: Rework container parsing
It was pretty clumsy before and the whole work of parsing the microcode containers was spread around the functions wrongly.
Clean it up so that there's a main scan_containers() function which iterates over the microcode blob and picks apart the containers glued together. For each container, it calls a parse_container() helper which concentrates on one container only: sanity-checking, parsing, counting microcode patches in there, etc.
It makes much more sense now and it is actually very readable. Oh, and we luvz a diffstat removing more crap than adding.
--- arch/x86/kernel/cpu/microcode/amd.c | 242 ++++++++++++++++-------------------- 1 file changed, 110 insertions(+), 132 deletions(-)
arch/x86/kernel/cpu/microcode/amd.c +110 -132
@@ -64,43 +64,6 @@ static u16 this_equiv_id;static const char ucode_path[]__maybe_unused ="kernel/x86/microcode/AuthenticAMD.bin" ; -static size_t compute_container_size (u8 *data,u32 total_size) -{ -size_t size =0 ; -u32 *header = (u32 *)data; - -if (header[0 ] !=UCODE_MAGIC || - header[1 ] !=UCODE_EQUIV_CPU_TABLE_TYPE ||/* type */ - header[2 ] ==0 )/* size */ -return size; - - size = header[2 ] +CONTAINER_HDR_SZ ; - total_size -= size; - data += size; - -while (total_size) { -u16 patch_size; - - header = (u32 *)data; - -if (header[0 ] !=UCODE_UCODE_TYPE ) -break ; - -/* -* Sanity-check patch size. -*/ - patch_size = header[1 ]; -if (patch_size >PATCH_MAX_SIZE ) -break ; - - size += patch_size +SECTION_HDR_SIZE ; - data += patch_size +SECTION_HDR_SIZE ; - total_size -= patch_size +SECTION_HDR_SIZE ; - } - -return size; -} -static u16 find_equiv_id (struct equiv_cpu_entry *equiv_table,u32 sig) {for (; equiv_table && equiv_table->installed_cpu; equiv_table++) {
@@ -115,80 +78,112 @@ static u16 find_equiv_id(struct equiv_cpu_entry *equiv_table, u32 sig)* This scans the ucode blob for the proper container as we can have multiple * containers glued together. Returns the equivalence ID from the equivalence * table or 0 if none found. +* Returns the amount of bytes consumed while scanning. @desc contains all the +* data we're going to use in later stages of the application. */ -static u16 -find_proper_container (u8 *ucode,size_t size,struct cont_desc *desc) +static ssize_t parse_container (u8 *ucode,ssize_t size,struct cont_desc *desc) { -struct cont_desc ret = {0 }; -u32 eax, ebx, ecx, edx;struct equiv_cpu_entry *eq; -int offset, left; -u16 eq_id =0 ; -u32 *header; -u8 *data; +ssize_t orig_size = size; +u32 *hdr = (u32 *)ucode; +u32 eax, ebx, ecx, edx; +u16 eq_id; +u8 *buf; - data = ucode; - left = size; - header = (u32 *)data; +/* Am I looking at an equivalence table header? */ +if (hdr[0 ] !=UCODE_MAGIC || + hdr[1 ] !=UCODE_EQUIV_CPU_TABLE_TYPE || + hdr[2 ] ==0 ) { + desc->eq_id =0 ; +return CONTAINER_HDR_SZ ; + } + buf = ucode; -/* find equiv cpu table */ -if (header[0 ] !=UCODE_MAGIC || - header[1 ] !=UCODE_EQUIV_CPU_TABLE_TYPE ||/* type */ - header[2 ] ==0 )/* size */ -return eq_id; + eq = (struct equiv_cpu_entry *)(buf +CONTAINER_HDR_SZ ); - eax =0x00000001 ; + eax =1 ; ecx =0 ;native_cpuid (&eax, &ebx, &ecx, &edx); -while (left >0 ) { - eq = (struct equiv_cpu_entry *)(data +CONTAINER_HDR_SZ ); - - ret.data = data; +/* Find the equivalence ID of our CPU in this table: */ + eq_id =find_equiv_id (eq, eax); -/* Advance past the container header */ - offset = header[2 ] +CONTAINER_HDR_SZ ; - data += offset; - left -= offset; + buf += hdr[2 ] +CONTAINER_HDR_SZ ; + size -= hdr[2 ] +CONTAINER_HDR_SZ ; - eq_id =find_equiv_id (eq, eax); -if (eq_id) { - ret.size =compute_container_size (ret.data, left + offset); +/* +* Scan through the rest of the container to find where it ends. We do +* some basic sanity-checking too. +*/ +while (size >0 ) { +struct microcode_amd *mc; +u32 patch_size; -/* -* truncate how much we need to iterate over in the -* ucode update loop below -*/ - left = ret.size - offset; + hdr = (u32 *)buf; - *desc = ret; -return eq_id; +if (hdr[0 ] !=UCODE_UCODE_TYPE ) { +/* Invalidate container. */ + eq_id =0 ; +break ; } -/* -* support multiple container files appended together. if this -* one does not have a matching equivalent cpu entry, we fast -* forward to the next container file. -*/ -while (left >0 ) { - header = (u32 *)data; +/* Sanity-check patch size. */ + patch_size = hdr[1 ]; +if (patch_size >PATCH_MAX_SIZE ) { +/* Something corrupted the container, invalidate it. */ + eq_id =0 ; +break ; + } -if (header[0 ] ==UCODE_MAGIC && - header[1 ] ==UCODE_EQUIV_CPU_TABLE_TYPE ) -break ; +/* Skip patch section header: */ + buf +=SECTION_HDR_SIZE ; + size -=SECTION_HDR_SIZE ; - offset = header[1 ] +SECTION_HDR_SIZE ; - data += offset; - left -= offset; + mc = (struct microcode_amd *)buf; +if (eq_id == mc->hdr.processor_rev_id) { + desc->psize = patch_size; + desc->mc = mc; } -/* mark where the next microcode container file starts */ - offset = data - (u8 *)ucode; - ucode = data; + buf += patch_size; + size -= patch_size; + } + +/* +* If we have found an eq_id, it means we're looking at the container +* which has a patch for this CPU so return 0 to mean, @ucode already +* points to it and it will be parsed later. Otherwise, we return the +* size we scanned so that we can advance to the next container in the +* buffer. +*/ +if (eq_id) { + desc->eq_id = eq_id; + desc->data = ucode; + desc->size = orig_size - size; + +return 0 ; } -return eq_id; +return orig_size - size; +} + +/* +* Scan the ucode blob for the proper container as we can have multiple +* containers glued together. +*/ +static void scan_containers (u8 *ucode,size_t size,struct cont_desc *desc) +{ +ssize_t rem = size; + +while (rem >=0 ) { +ssize_t s =parse_container (ucode, rem, desc); +if (!s) +return ; + + ucode += s; + rem -= s; + } }static int __apply_microcode_amd (struct microcode_amd *mc)
@@ -214,17 +209,16 @@ static int __apply_microcode_amd(struct microcode_amd *mc)* load_microcode_amd() to save equivalent cpu table and microcode patches in * kernel heap memory. * -* Returns true if container found (sets @ret_cont), false otherwise. +* Returns true if container found (sets @desc), false otherwise. */ static bool apply_microcode_early_amd (void *ucode,size_t size,bool save_patch, -struct cont_desc *desc) +struct cont_desc *ret_desc) { +struct cont_desc desc = {0 };u8 (*patch)[PATCH_MAX_SIZE ]; -u32 rev, *header, *new_rev; -struct cont_desc ret; -int offset, left; -u16 eq_id =0 ; -u8 *data; +struct microcode_amd *mc; +u32 rev, *new_rev; +bool ret = false;#ifdef CONFIG_X86_32 new_rev = (u32 *)__pa_nodebug (&ucode_new_rev);
@@ -237,47 +231,31 @@ static bool apply_microcode_early_amd(void *ucode, size_t size, bool save_patch,if (check_current_patch_level (&rev, true))return false; - eq_id =find_proper_container (ucode, size, &ret); -if (!eq_id) -return false; - - this_equiv_id = eq_id; - header = (u32 *)ret.data; - -/* We're pointing to an equiv table, skip over it. */ - data = ret.data + header[2 ] +CONTAINER_HDR_SZ ; - left = ret.size - (header[2 ] +CONTAINER_HDR_SZ ); - -while (left >0 ) { -struct microcode_amd *mc; - - header = (u32 *)data; -if (header[0 ] !=UCODE_UCODE_TYPE ||/* type */ - header[1 ] ==0 )/* size */ -break ; +scan_containers (ucode, size, &desc); +if (!desc.eq_id) +return ret; - mc = (struct microcode_amd *)(data +SECTION_HDR_SIZE ); + this_equiv_id = desc.eq_id; -if (eq_id == mc->hdr.processor_rev_id && rev < mc->hdr.patch_id) { + mc = desc.mc; +if (!mc) +return ret; -if (!__apply_microcode_amd (mc)) { - rev = mc->hdr.patch_id; - *new_rev = rev; +if (rev >= mc->hdr.patch_id) +return ret; -if (save_patch) -memcpy (patch, mc,min_t (u32, header[1 ],PATCH_MAX_SIZE )); - } - } +if (!__apply_microcode_amd (mc)) { + *new_rev = mc->hdr.patch_id; + ret = true; - offset = header[1 ] +SECTION_HDR_SIZE ; - data += offset; - left -= offset; +if (save_patch) +memcpy (patch, mc,min_t (u32, desc.psize,PATCH_MAX_SIZE )); } -if (desc) - *desc = ret; +if (ret_desc) + *ret_desc = desc; -return true; +return ret; }static bool get_builtin_microcode (struct cpio_data *cp,unsigned int family)
@@ -396,6 +374,7 @@ void load_ucode_amd_ap(unsigned int family) }if (!apply_microcode_early_amd (cp.data, cp.size, false, &cont)) { + cont.data =NULL ; cont.size =-1 ;return ; }
@@ -434,7 +413,6 @@ int __init save_microcode_in_initrd_amd(unsigned int fam) {enum ucode_state ret;int retval =0 ; -u16 eq_id;if (!cont.data) {if (IS_ENABLED (CONFIG_X86_32 ) && (cont.size !=-1 )) {
@@ -450,8 +428,8 @@ int __init save_microcode_in_initrd_amd(unsigned int fam)return -EINVAL ; } - eq_id =find_proper_container (cp.data, cp.size, &cont); -if (!eq_id) { +scan_containers (cp.data, cp.size, &cont); +if (!cont.eq_id) { cont.size =-1 ;return -EINVAL ; }
Signature · 8 lines
2.11.0
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Borislav Petkov
maintainerRe: [PATCH 06/13] x86/microcode/AMD: Rework container parsing
Quoted 7 lines · Borislav Petkov
Ah, good catch. I guess I'll have to do here:
/* Something corrupted the container, invalidate it. */ eq_id = 0; break;too.
Ok, I should've done the below from the beginning. I should be looking at desc->mc which says whether we have found a patch or not. And then prepare retvals accordingly.
The equivalence id (eq_id) is then used to signal that fact.
And in a future patch, I'll check desc->mc after that function returns and not eq_id. (I had to do eq_id because of the global this_equiv_id but that's gone now too).
/*
* Scan through the rest of the container to find where it ends. We do
* some basic sanity-checking too.
*/
while (size > 0) {
struct microcode_amd *mc;
u32 patch_size;
hdr = (u32 *)buf;
if (hdr[0] != UCODE_UCODE_TYPE)
break;
/* Sanity-check patch size. */
patch_size = hdr[1];
if (patch_size > PATCH_MAX_SIZE)
break;
/* Skip patch section header: */
buf += SECTION_HDR_SIZE;
size -= SECTION_HDR_SIZE;
mc = (struct microcode_amd *)buf;
if (eq_id == mc->hdr.processor_rev_id) {
desc->psize = patch_size;
desc->mc = mc;
}
buf += patch_size;
size -= patch_size;
}
/*
* If we have found a patch (desc->mc), it means we're looking at the
* container which has a patch for this CPU so return 0 to mean, @ucode
* already points to the proper container. Otherwise, we return the size
* we scanned so that we can advance to the next container in the
* buffer.
*/
if (desc->mc) {
desc->eq_id = eq_id;
desc->data = ucode;
desc->size = orig_size - size;
return 0;
}Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Borislav Petkov
maintainerRe: [PATCH 06/13] x86/microcode/AMD: Rework container parsing
Quoted 3 lines · Borislav Petkov
And in a future patch, I'll check
desc->mcafter that function returns and noteq_id. (I had to doeq_idbecause of the globalthis_equiv_idbut that's gone now too).
... which is pretty nice, I can get rid of eq_id now as it is private to the container scanning code now and the callers shouldn't care. Cool.
Now on to test whether that actually makes sense :-)
---
From: Borislav Petkov <[email protected]>
Date: Wed, 18 Jan 2017 15:55:26 +0100
Subject: [PATCH] x86/microcode/AMD: Remove struct cont_desc.eq_id
The equivalence ID was needed outside of the container scanning logic but now, after this has been cleaned up, not anymore. Now, cont_desc.mc is used to denote whether the container we're looking at has the proper microcode patch for this CPU or not.
--- arch/x86/kernel/cpu/microcode/amd.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)
arch/x86/kernel/cpu/microcode/amd.c +4 -10
@@ -49,7 +49,6 @@ struct cont_desc {struct microcode_amd *mc;u32 cpuid_1_eax;u32 psize; -u16 eq_id;u8 *data;size_t size; };
@@ -92,10 +91,8 @@ static ssize_t parse_container(u8 *ucode, ssize_t size, struct cont_desc *desc)/* Am I looking at an equivalence table header? */ if (hdr[0 ] !=UCODE_MAGIC || hdr[1 ] !=UCODE_EQUIV_CPU_TABLE_TYPE || - hdr[2 ] ==0 ) { - desc->eq_id =0 ; + hdr[2 ] ==0 )return CONTAINER_HDR_SZ ; - } buf = ucode;
@@ -147,9 +144,8 @@ static ssize_t parse_container(u8 *ucode, ssize_t size, struct cont_desc *desc)* buffer. */ if (desc->mc) { - desc->eq_id = eq_id; - desc->data = ucode; - desc->size = orig_size - size; + desc->data = ucode; + desc->size = orig_size - size;return 0 ; }
@@ -220,8 +216,6 @@ apply_microcode_early_amd(u32 cpuid_1_eax, void *ucode, size_t size, bool save_p desc.cpuid_1_eax = cpuid_1_eax;scan_containers (ucode, size, &desc); -if (!desc.eq_id) -return ret; mc = desc.mc;if (!mc)
@@ -341,7 +335,7 @@ int __init save_microcode_in_initrd_amd(unsigned int cpuid_1_eax) desc.cpuid_1_eax = cpuid_1_eax;scan_containers (cp.data, cp.size, &desc); -if (!desc.eq_id) +if (!desc.mc)return -EINVAL ; ret = load_microcode_amd(smp_processor_id(), x86_family(cpuid_1_eax),
Signature · 7 lines
2.11.0
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Borislav Petkov
maintainerPATCH 7/13 x86/microcode: Decrease CPUID use
In reply to Borislav PetkovFrom: Borislav Petkov <[email protected]>
Get CPUID(1).EAX value once per CPU and propagate value into the callers instead of conveniently calling it every time.
--- arch/x86/kernel/cpu/microcode/amd.c | 52 +++++++++++++++++------------------- arch/x86/kernel/cpu/microcode/core.c | 38 ++++++++++---------------- 2 files changed, 38 insertions(+), 52 deletions(-)
arch/x86/kernel/cpu/microcode/amd.c +24 -28
@@ -47,6 +47,7 @@ static struct equiv_cpu_entry *equiv_cpu_table;*/ struct cont_desc {struct microcode_amd *mc; +u32 cpuid_1_eax;u32 psize;u16 eq_id;u8 *data;
@@ -92,7 +93,6 @@ static ssize_t parse_container(u8 *ucode, ssize_t size, struct cont_desc *desc)struct equiv_cpu_entry *eq;ssize_t orig_size = size;u32 *hdr = (u32 *)ucode; -u32 eax, ebx, ecx, edx;u16 eq_id;u8 *buf;
@@ -108,12 +108,8 @@ static ssize_t parse_container(u8 *ucode, ssize_t size, struct cont_desc *desc) eq = (struct equiv_cpu_entry *)(buf +CONTAINER_HDR_SZ ); - eax =1 ; - ecx =0 ; -native_cpuid (&eax, &ebx, &ecx, &edx); -/* Find the equivalence ID of our CPU in this table: */ - eq_id =find_equiv_id (eq, eax); + eq_id =find_equiv_id (eq, desc->cpuid_1_eax); buf += hdr[2 ] +CONTAINER_HDR_SZ ; size -= hdr[2 ] +CONTAINER_HDR_SZ ;
@@ -214,8 +210,9 @@ static int __apply_microcode_amd(struct microcode_amd *mc)* * Returns true if container found (sets @desc), false otherwise. */ -static bool apply_microcode_early_amd (void *ucode,size_t size,bool save_patch, -struct cont_desc *ret_desc) +static bool +apply_microcode_early_amd (u32 cpuid_1_eax,void *ucode,size_t size, +bool save_patch,struct cont_desc *ret_desc) {struct cont_desc desc = {0 };u8 (*patch)[PATCH_MAX_SIZE ];
@@ -234,6 +231,8 @@ static bool apply_microcode_early_amd(void *ucode, size_t size, bool save_patch,if (check_current_patch_level (&rev, true))return ret; + desc.cpuid_1_eax = cpuid_1_eax; +scan_containers (ucode, size, &desc);if (!desc.eq_id)return ret;
@@ -276,10 +275,9 @@ static bool get_builtin_microcode(struct cpio_data *cp, unsigned int family)#endif } -void __init load_ucode_amd_bsp (unsigned int family) +void __init load_ucode_amd_bsp (unsigned int cpuid_1_eax) {struct ucode_cpu_info *uci; -u32 eax, ebx, ecx, edx;struct cpio_data cp;const char *path;bool use_pa;
@@ -294,19 +292,16 @@ void __init load_ucode_amd_bsp(unsigned int family) use_pa = false; } -if (!get_builtin_microcode (&cp, family)) +if (!get_builtin_microcode (&cp,x86_family (cpuid_1_eax))) cp =find_microcode_in_initrd (path, use_pa);if (!(cp.data && cp.size))return ; -/* Get BSP's CPUID.EAX(1), needed in load_microcode_amd() */ - eax =1 ; - ecx =0 ; -native_cpuid (&eax, &ebx, &ecx, &edx); - uci->cpu_sig.sig = eax; +/* Needed in load_microcode_amd() */ + uci->cpu_sig.sig = cpuid_1_eax; -apply_microcode_early_amd (cp.data, cp.size, true,NULL ); +apply_microcode_early_amd (cpuid_1_eax, cp.data, cp.size, true,NULL ); }#ifdef CONFIG_X86_32
@@ -317,7 +312,7 @@ void __init load_ucode_amd_bsp(unsigned int family)* In save_microcode_in_initrd_amd() BSP's patch is copied to amd_ucode_patch, * which is used upon resume from suspend. */ -void load_ucode_amd_ap (unsigned int family) +void load_ucode_amd_ap (unsigned int cpuid_1_eax) {struct microcode_amd *mc;struct cpio_data cp;
@@ -328,7 +323,7 @@ void load_ucode_amd_ap(unsigned int family)return ; } -if (!get_builtin_microcode (&cp, family)) +if (!get_builtin_microcode (&cp,x86_family (cpuid_1_eax))) cp =find_microcode_in_initrd ((const char *)__pa_nodebug (ucode_path), true);if (!(cp.data && cp.size))
@@ -338,14 +333,14 @@ void load_ucode_amd_ap(unsigned int family)* This would set amd_ucode_patch above so that the following APs can * use it directly instead of going down this path again. */ - apply_microcode_early_amd(cp.data, cp.size, true, NULL); + apply_microcode_early_amd(cpuid_1_eax, cp.data, cp.size, true, NULL); }#else -void load_ucode_amd_ap (unsigned int family) +void load_ucode_amd_ap (unsigned int cpuid_1_eax) {struct equiv_cpu_entry *eq;struct microcode_amd *mc; -u32 rev, eax; +u32 rev;u16 eq_id;/* 64-bit runs with paging enabled, thus early==false. */
@@ -360,7 +355,7 @@ void load_ucode_amd_ap(unsigned int family)return ; reget: -if (!get_builtin_microcode (&cp, family)) { +if (!get_builtin_microcode (&cp,x86_family (cpuid_1_eax))) {#ifdef CONFIG_BLK_DEV_INITRD cp =find_cpio_data (ucode_path, (void *)initrd_start, initrd_end - initrd_start,NULL );
@@ -376,17 +371,16 @@ void load_ucode_amd_ap(unsigned int family) } } -if (!apply_microcode_early_amd (cp.data, cp.size, false, &cont)) { +if (!apply_microcode_early_amd (cpuid_1_eax, cp.data, cp.size, false, &cont)) { cont.data =NULL ; cont.size =-1 ;return ; } } - eax =cpuid_eax (0x00000001 ); eq = (struct equiv_cpu_entry *)(cont.data +CONTAINER_HDR_SZ ); - eq_id =find_equiv_id (eq, eax); + eq_id =find_equiv_id (eq, cpuid_1_eax);if (!eq_id)return ;
@@ -412,7 +406,7 @@ void load_ucode_amd_ap(unsigned int family)static enum ucode_state load_microcode_amd (int cpu,u8 family,const u8 *data,size_t size); -int __init save_microcode_in_initrd_amd (unsigned int fam) +int __init save_microcode_in_initrd_amd (unsigned int cpuid_1_eax) {enum ucode_state ret;int retval =0 ;
@@ -431,6 +425,8 @@ int __init save_microcode_in_initrd_amd(unsigned int fam)return -EINVAL ; } + cont.cpuid_1_eax = cpuid_1_eax; +scan_containers (cp.data, cp.size, &cont);if (!cont.eq_id) { cont.size =-1 ;
@@ -441,7 +437,7 @@ int __init save_microcode_in_initrd_amd(unsigned int fam)return -EINVAL ; } - ret =load_microcode_amd (smp_processor_id (), fam, cont.data, cont.size); + ret =load_microcode_amd (smp_processor_id (),x86_family (cpuid_1_eax), cont.data, cont.size);if (ret !=UCODE_OK ) retval = -EINVAL ;
arch/x86/kernel/cpu/microcode/core.c +14 -24
@@ -64,10 +64,6 @@ static DEFINE_MUTEX(microcode_mutex);struct ucode_cpu_info ucode_cpu_info[NR_CPUS ]; -/* -* Operations that are run on a target cpu: -*/ -struct cpu_info_ctx {struct cpu_signature *cpu_sig;int err;
@@ -76,7 +72,6 @@ struct cpu_info_ctx {static bool __init check_loader_disabled_bsp (void ) {static const char *__dis_opt_str ="dis_ucode_ldr" ; -u32 a, b, c, d;#ifdef CONFIG_X86_32 const char *cmdline = (const char *)__pa_nodebug (boot_command_line);
@@ -92,16 +87,12 @@ static bool __init check_loader_disabled_bsp(void)if (!have_cpuid_p ())return *res; - a =1 ; - c =0 ; -native_cpuid (&a, &b, &c, &d); -/* * CPUID(1).ECX[31]: reserved for hypervisor use. This is still not * completely accurate as xen pv guests don't see that CPUID bit set but * that's good enough as they don't land on the BSP path anyway. */ -if (c &BIT (31 )) +if (native_cpuid_ecx (1 ) &BIT (31 ))return *res;if (cmdline_find_option_bool (cmdline, option) <=0 )
@@ -131,23 +122,22 @@ bool get_builtin_firmware(struct cpio_data *cd, const char *name)void __init load_ucode_bsp (void ) { -int vendor; -unsigned int family; +unsigned int vendor, cpuid_1_eax;if (check_loader_disabled_bsp ())return ; - vendor =x86_cpuid_vendor (); - family =x86_cpuid_family (); + vendor =x86_cpuid_vendor (); + cpuid_1_eax =native_cpuid_eax (1 );switch (vendor) {case X86_VENDOR_INTEL : -if (family >=6 ) +if (x86_family (cpuid_1_eax) >=6 )load_ucode_intel_bsp ();break ;case X86_VENDOR_AMD : -if (family >=0x10 ) -load_ucode_amd_bsp (family); +if (x86_family (cpuid_1_eax) >=0x10 ) +load_ucode_amd_bsp (cpuid_1_eax);break ;default :break ;
@@ -165,22 +155,22 @@ static bool check_loader_disabled_ap(void)void load_ucode_ap (void ) { -int vendor, family; +unsigned int vendor, cpuid_1_eax;if (check_loader_disabled_ap ())return ; - vendor =x86_cpuid_vendor (); - family =x86_cpuid_family (); + vendor =x86_cpuid_vendor (); + cpuid_1_eax =native_cpuid_eax (1 );switch (vendor) {case X86_VENDOR_INTEL : -if (family >=6 ) +if (x86_family (cpuid_1_eax) >=6 )load_ucode_intel_ap ();break ;case X86_VENDOR_AMD : -if (family >=0x10 ) -load_ucode_amd_ap (family); +if (x86_family (cpuid_1_eax) >=0x10 ) +load_ucode_amd_ap (cpuid_1_eax);break ;default :break ;
@@ -198,7 +188,7 @@ static int __init save_microcode_in_initrd(void)break ;case X86_VENDOR_AMD :if (c->x86 >=0x10 ) -return save_microcode_in_initrd_amd (c->x86); +return save_microcode_in_initrd_amd (cpuid_eax (1 ));break ;default :break ;
Signature · 1 line
2.11.0
Re: [PATCH 07/13] x86/microcode: Decrease CPUID use
Quoted 5 lines · Borislav Petkov
From: Borislav Petkov <[email protected]>
Get CPUID(1).EAX value once per CPU and propagate value into the callers instead of conveniently calling it every time.
Borislav Petkov
maintainerPATCH 8/13 x86/microcode/AMD: Get rid of global this_equiv_id
In reply to Borislav PetkovFrom: Borislav Petkov <[email protected]>
We have a container which we update/prepare each time before applying a patch.
--- arch/x86/kernel/cpu/microcode/amd.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
arch/x86/kernel/cpu/microcode/amd.c +1 -4
@@ -56,7 +56,6 @@ struct cont_desc {static u32 ucode_new_rev;static u8 amd_ucode_patch[PATCH_MAX_SIZE ]; -static u16 this_equiv_id;/* * Microcode patch container file is prepended to the initrd in cpio
@@ -237,8 +236,6 @@ apply_microcode_early_amd(u32 cpuid_1_eax, void *ucode, size_t size,if (!desc.eq_id)return ret; - this_equiv_id = desc.eq_id; - mc = desc.mc;if (!mc)return ret;
@@ -384,7 +381,7 @@ void load_ucode_amd_ap(unsigned int cpuid_1_eax)if (!eq_id)return ; -if (eq_id == this_equiv_id) { +if (eq_id == cont.eq_id) { mc = (struct microcode_amd *)amd_ucode_patch;if (mc && rev < mc->hdr.patch_id) {
Signature · 1 line
2.11.0
Re: [PATCH 08/13] x86/microcode/AMD: Get rid of global this_equiv_id
Quoted 6 lines · Borislav Petkov
From: Borislav Petkov <[email protected]>
We have a container which we update/prepare each time before applying a patch.
Signed-off-by Borislav Petkov <[email protected]>
Ok, this addresses the this_equiv_id storage thing I asked before :)
Borislav Petkov
maintainerPATCH 9/13 x86/microcode/AMD: Use find_microcode_in_initrd()
In reply to Borislav PetkovFrom: Borislav Petkov <[email protected]>
Use the generic helper instead of semi-open-coding the procedure.
--- arch/x86/kernel/cpu/microcode/amd.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)
arch/x86/kernel/cpu/microcode/amd.c +5 -11
@@ -346,17 +346,15 @@ void load_ucode_amd_ap(unsigned int cpuid_1_eax)/* First AP hasn't cached it yet, go through the blob. */ if (!cont.data) { -struct cpio_data cp = {NULL ,0 ,"" }; +struct cpio_data cp;if (cont.size ==-1 )return ; reget:if (!get_builtin_microcode (&cp,x86_family (cpuid_1_eax))) { -#ifdef CONFIG_BLK_DEV_INITRD - cp =find_cpio_data (ucode_path, (void *)initrd_start, - initrd_end - initrd_start,NULL ); -#endif + cp =find_microcode_in_initrd (ucode_path, false); +if (!(cp.data && cp.size)) {/* * Mark it so that other APs do not scan again
@@ -410,13 +408,9 @@ int __init save_microcode_in_initrd_amd(unsigned int cpuid_1_eax)if (!cont.data) {if (IS_ENABLED (CONFIG_X86_32 ) && (cont.size !=-1 )) { -struct cpio_data cp = {NULL ,0 ,"" }; - -#ifdef CONFIG_BLK_DEV_INITRD - cp =find_cpio_data (ucode_path, (void *)initrd_start, - initrd_end - initrd_start,NULL ); -#endif +struct cpio_data cp; + cp =find_microcode_in_initrd (ucode_path, false);if (!(cp.data && cp.size)) { cont.size =-1 ;return -EINVAL ;
Signature · 1 line
2.11.0
Re: [PATCH 09/13] x86/microcode/AMD: Use find_microcode_in_initrd()
Quoted 5 lines · Borislav Petkov
From: Borislav Petkov <[email protected]>
Use the generic helper instead of semi-open-coding the procedure.
Signed-off-by Borislav Petkov <[email protected]>
Borislav Petkov
maintainerPATCH 10/13 x86/microcode/AMD: Check patch level only on the BSP
In reply to Borislav PetkovFrom: Borislav Petkov <[email protected]>
Check final patch levels for AMD only on the BSP. This way, we decide early and only once whether to continue loading or to leave the loader disabled on such systems.
Simplify a lot.
--- arch/x86/include/asm/microcode_amd.h | 2 - arch/x86/kernel/cpu/microcode/amd.c | 77 ++++++------------------------------ arch/x86/kernel/cpu/microcode/core.c | 51 +++++++++++++++++++++--- 3 files changed, 56 insertions(+), 74 deletions(-)
arch/x86/include/asm/microcode_amd.h +0 -2
@@ -54,6 +54,4 @@ static inline int __init save_microcode_in_initrd_amd(unsigned int family) { return -EINVAL; }void reload_ucode_amd (void ) {}#endif - -extern bool check_current_patch_level (u32 *rev,bool early);#endif /* _ASM_X86_MICROCODE_AMD_H */
arch/x86/kernel/cpu/microcode/amd.c +11 -66
132 lines not on this page · raw message
arch/x86/kernel/cpu/microcode/core.c +45 -6
87 lines not on this page · raw message
Signature · 1 line
2.11.0
Re: [PATCH 10/13] x86/microcode/AMD: Check patch level only on the BSP
Quoted 33 lines · Borislav Petkov
From: Borislav Petkov <[email protected]>
Check final patch levels for AMD only on the BSP. This way, we decide early and only once whether to continue loading or to leave the loader disabled on such systems.
Simplify a lot.
Signed-off-by Borislav Petkov <[email protected]>void
__initload_ucode_bsp(void)
{arch/x86/kernel/cpu/microcode/core.c +3
-4-unsigned int vendor, cpuid_1_eax; +unsigned int cpuid_1_eax;if (check_loader_disabled_bsp ())return ; - vendor =x86_cpuid_vendor (); cpuid_1_eax =native_cpuid_eax (1 ); -switch (vendor) { +switch (x86_cpuid_vendor ()) {case X86_VENDOR_INTEL :if (x86_family (cpuid_1_eax) >=6 )load_ucode_intel_bsp ();@@ -155,15 +195,14 @@ static bool check_loader_disabled_ap(void)void load_ucode_ap (void ) { -unsigned int vendor, cpuid_1_eax; +unsigned int cpuid_1_eax;if (check_loader_disabled_ap ())
arch/x86/kernel/cpu/microcode/core.c static bool check_loader_disabled_ap(void) · line 208
Quoted 10 lines
arch/x86/kernel/cpu/microcode/core.c +1
-2return ; - vendor =x86_cpuid_vendor (); cpuid_1_eax =native_cpuid_eax (1 ); -switch (vendor) { +switch (x86_cpuid_vendor ()) {case X86_VENDOR_INTEL :if (x86_family (cpuid_1_eax) >=6 )load_ucode_intel_ap ();
These two hunks look unrelated to $subject. Please seperate them out.
Thanks,
tglx
Borislav Petkov
maintainerPATCH 11/13 x86/microcode/AMD: Unify load_ucode_amd_ap()
In reply to Borislav PetkovFrom: Borislav Petkov <[email protected]>
Use a version for both bitness by adding a helper which does the actual container finding and parsing which can be used on any CPU - BSP or AP. Streamlines the paths more.
--- arch/x86/kernel/cpu/microcode/amd.c | 84 ++++++++++++++----------------------- 1 file changed, 31 insertions(+), 53 deletions(-)
arch/x86/kernel/cpu/microcode/amd.c +31 -53
@@ -271,7 +271,7 @@ static bool get_builtin_microcode(struct cpio_data *cp, unsigned int family)#endif } -void __init load_ucode_amd_bsp (unsigned int cpuid_1_eax) +void __load_ucode_amd (unsigned int cpuid_1_eax,struct cpio_data *ret) {struct ucode_cpu_info *uci;struct cpio_data cp;
@@ -291,95 +291,74 @@ void __init load_ucode_amd_bsp(unsigned int cpuid_1_eax)if (!get_builtin_microcode (&cp,x86_family (cpuid_1_eax))) cp =find_microcode_in_initrd (path, use_pa); -if (!(cp.data && cp.size)) -return ; -/* Needed in load_microcode_amd() */ uci->cpu_sig.sig = cpuid_1_eax; -apply_microcode_early_amd (cpuid_1_eax, cp.data, cp.size, true,NULL ); + *ret = cp; } -#ifdef CONFIG_X86_32 -/* -* On 32-bit, since AP's early load occurs before paging is turned on, we -* cannot traverse cpu_equiv_table and microcode_cache in kernel heap memory. -* So during cold boot, AP will apply_ucode_in_initrd() just like the BSP. -* In save_microcode_in_initrd_amd() BSP's patch is copied to amd_ucode_patch, -* which is used upon resume from suspend. -*/ -void load_ucode_amd_ap (unsigned int cpuid_1_eax) +void __init load_ucode_amd_bsp (unsigned int cpuid_1_eax) { -struct microcode_amd *mc; -struct cpio_data cp; - - mc = (struct microcode_amd *)__pa_nodebug (amd_ucode_patch); -if (mc->hdr.patch_id && mc->hdr.processor_rev_id) { -__apply_microcode_amd (mc); -return ; - } +struct cpio_data cp = { }; -if (!get_builtin_microcode (&cp,x86_family (cpuid_1_eax))) - cp =find_microcode_in_initrd ((const char *)__pa_nodebug (ucode_path), true); +__load_ucode_amd (cpuid_1_eax, &cp);if (!(cp.data && cp.size))return ; -/* -* This would set amd_ucode_patch above so that the following APs can -* use it directly instead of going down this path again. -*/ apply_microcode_early_amd (cpuid_1_eax, cp.data, cp.size, true,NULL ); } -#else +void load_ucode_amd_ap (unsigned int cpuid_1_eax) {struct equiv_cpu_entry *eq;struct microcode_amd *mc; +struct cont_desc *desc;u16 eq_id; +if (IS_ENABLED (CONFIG_X86_32 )) { + mc = (struct microcode_amd *)__pa_nodebug (amd_ucode_patch); + desc = (struct cont_desc *)__pa_nodebug (&cont); + }else { + mc = (struct microcode_amd *)amd_ucode_patch; + desc = &cont; + } +/* First AP hasn't cached it yet, go through the blob. */ -if (!cont.data) { -struct cpio_data cp; +if (!desc->data) { +struct cpio_data cp = { }; -if (cont.size ==-1 ) +if (desc->size ==-1 )return ; reget: -if (!get_builtin_microcode (&cp,x86_family (cpuid_1_eax))) { - cp =find_microcode_in_initrd (ucode_path, false); - -if (!(cp.data && cp.size)) { -/* -* Mark it so that other APs do not scan again -* for no real reason and slow down boot -* needlessly. -*/ - cont.size =-1 ; -return ; - } +__load_ucode_amd (cpuid_1_eax, &cp); +if (!(cp.data && cp.size)) { +/* +* Mark it so that other APs do not scan again for no +* real reason and slow down boot needlessly. +*/ + desc->size =-1 ; +return ; } -if (!apply_microcode_early_amd (cpuid_1_eax, cp.data, cp.size, false, &cont)) { - cont.data =NULL ; - cont.size =-1 ; +if (!apply_microcode_early_amd (cpuid_1_eax, cp.data, cp.size, false, desc)) { + desc->data =NULL ; + desc->size =-1 ;return ; } } - eq = (struct equiv_cpu_entry *)(cont.data +CONTAINER_HDR_SZ ); + eq = (struct equiv_cpu_entry *)(desc->data +CONTAINER_HDR_SZ ); eq_id =find_equiv_id (eq, cpuid_1_eax);if (!eq_id)return ; -if (eq_id == cont.eq_id) { +if (eq_id == desc->eq_id) {u32 rev, dummy;microcode_rdmsr (MSR_AMD64_PATCH_LEVEL , rev, dummy); - - mc = (struct microcode_amd *)amd_ucode_patch; -if (mc && rev < mc->hdr.patch_id) {if (!__apply_microcode_amd (mc)) ucode_new_rev = mc->hdr.patch_id;
@@ -394,7 +373,6 @@ void load_ucode_amd_ap(unsigned int cpuid_1_eax) goto reget; } } -#endif /* CONFIG_X86_32 */ static enum ucode_state load_microcode_amd (int cpu,u8 family,const u8 *data,size_t size);
Signature · 1 line
2.11.0
Re: [PATCH 11/13] x86/microcode/AMD: Unify load_ucode_amd_ap()
arch/x86/kernel/cpu/microcode/amd.c void __init load_ucode_amd_bsp(unsigned int cpuid_1_eax) · line 324
Quoted 13 lines · Borislav Petkov
void
load_ucode_amd_ap(unsigned int cpuid_1_eax)
{arch/x86/kernel/cpu/microcode/amd.c +7
-0struct equiv_cpu_entry *eq;struct microcode_amd *mc; +struct cont_desc *desc;u16 eq_id; +if (IS_ENABLED (CONFIG_X86_32 )) { + mc = (struct microcode_amd *)__pa_nodebug (amd_ucode_patch); + desc = (struct cont_desc *)__pa_nodebug (&cont); + }else { + mc = (struct microcode_amd *)amd_ucode_patch; + desc = &cont;
Bah! Now I realize that 'cont' is not a local variable as I assumed when looking at the other patch. 'cont' is a pretty bad name for a (file) global variable. Can we please use a more obvious name ?
While at it please make that thing static as there cant be a user outside of that file.
arch/x86/kernel/cpu/microcode/amd.c void __init load_ucode_amd_bsp(unsigned int cpuid_1_eax) · line 332
Quoted 11 lines
arch/x86/kernel/cpu/microcode/amd.c +5
-3+ } +/* First AP hasn't cached it yet, go through the blob. */ -if (!cont.data) { -struct cpio_data cp; +if (!desc->data) { +struct cpio_data cp = { }; -if (cont.size ==-1 ) +if (desc->size ==-1 )return ;
I'm not really fond of abusing the size member for this. And that '-1' seems to have different meanings depending on other members. Really not intuitive.
Please introduce a proper state member which tells what the descriptor struct contains, i.e. EMPTY, VALID, INVALID ....
Thanks,
tglx
Re: [PATCH 11/13] x86/microcode/AMD: Unify load_ucode_amd_ap()
Quoted 38 lines · Thomas Gleixner
arch/x86/kernel/cpu/microcode/amd.c void __init load_ucode_amd_bsp(unsigned int cpuid_1_eax) · line 324
Quoted 13 lines · Borislav Petkov
void
load_ucode_amd_ap(unsigned int cpuid_1_eax)
{arch/x86/kernel/cpu/microcode/amd.c +7
-0struct equiv_cpu_entry *eq;struct microcode_amd *mc; +struct cont_desc *desc;u16 eq_id; +if (IS_ENABLED (CONFIG_X86_32 )) { + mc = (struct microcode_amd *)__pa_nodebug (amd_ucode_patch); + desc = (struct cont_desc *)__pa_nodebug (&cont); + }else { + mc = (struct microcode_amd *)amd_ucode_patch; + desc = &cont;Bah! Now I realize that 'cont' is not a local variable as I assumed when looking at the other patch. 'cont' is a pretty bad name for a (file) global variable. Can we please use a more obvious name ?
While at it please make that thing static as there cant be a user outside of that file.
arch/x86/kernel/cpu/microcode/amd.c void __init load_ucode_amd_bsp(unsigned int cpuid_1_eax) · line 332
Quoted 11 lines
arch/x86/kernel/cpu/microcode/amd.c +5
-3+ } +/* First AP hasn't cached it yet, go through the blob. */ -if (!cont.data) { -struct cpio_data cp; +if (!desc->data) { +struct cpio_data cp = { }; -if (cont.size ==-1 ) +if (desc->size ==-1 )return ;I'm not really fond of abusing the size member for this. And that '-1' seems to have different meanings depending on other members. Really not intuitive.
Quoted 2 lines
Please introduce a proper state member which tells what the descriptor struct contains, i.e. EMPTY, VALID, INVALID ....
Reading further through the series I see that you remove all that cruft at the end. Dammit, I never start reading books in the final chapter, maybe I should do that with patch series :)
It might be nice nevertheless to have an initial patch which cleans up the name of that 'cont' variable and that status thing, but it might not be worth the trouble as you remove it at the end anyway, which makes a lot of sense btw. Your call.
Thanks,
tglx
Borislav Petkov
maintainerPATCH 12/13 x86/microcode/AMD: Simplify saving from initrd
In reply to Borislav PetkovFrom: Borislav Petkov <[email protected]>
No need to use the previously stashed info in the container - simply go ahead and parse the initrd once more. It simplifies and streamlines the code a whole lot.
--- arch/x86/kernel/cpu/microcode/amd.c | 43 +++++++++++-------------------------- 1 file changed, 13 insertions(+), 30 deletions(-)
arch/x86/kernel/cpu/microcode/amd.c +13 -30
@@ -379,43 +379,26 @@ load_microcode_amd(int cpu, u8 family, const u8 *data, size_t size);int __init save_microcode_in_initrd_amd (unsigned int cpuid_1_eax) { +struct cont_desc desc;enum ucode_state ret; -int retval =0 ; - -if (!cont.data) { -if (IS_ENABLED (CONFIG_X86_32 ) && (cont.size !=-1 )) { -struct cpio_data cp; - - cp =find_microcode_in_initrd (ucode_path, false); -if (!(cp.data && cp.size)) { - cont.size =-1 ; -return -EINVAL ; - } +struct cpio_data cp; - cont.cpuid_1_eax = cpuid_1_eax; + cp =find_microcode_in_initrd (ucode_path, false); +if (!(cp.data && cp.size)) +return -EINVAL ; -scan_containers (cp.data, cp.size, &cont); -if (!cont.eq_id) { - cont.size =-1 ; -return -EINVAL ; - } + desc.cpuid_1_eax = cpuid_1_eax; - }else -return -EINVAL ; - } +scan_containers (cp.data, cp.size, &desc); +if (!desc.eq_id) +return -EINVAL ; - ret =load_microcode_amd (smp_processor_id (),x86_family (cpuid_1_eax), cont.data, cont.size); + ret =load_microcode_amd (smp_processor_id (),x86_family (cpuid_1_eax), + desc.data, desc.size);if (ret !=UCODE_OK ) - retval = -EINVAL ; - -/* -* This will be freed any msec now, stash patches for the current -* family and switch to patch cache for cpu hotplug, etc later. -*/ - cont.data =NULL ; - cont.size =0 ; +return -EINVAL ; -return retval; +return 0 ; }void reload_ucode_amd (void )
Signature · 1 line
2.11.0
Re: [PATCH 12/13] x86/microcode/AMD: Simplify saving from initrd
Quoted 7 lines · Borislav Petkov
From: Borislav Petkov <[email protected]>
No need to use the previously stashed info in the container - simply go ahead and parse the initrd once more. It simplifies and streamlines the code a whole lot.
Signed-off-by Borislav Petkov <[email protected]>
Borislav Petkov
maintainerPATCH 13/13 x86/microcode/AMD: Remove AP scanning optimization
In reply to Borislav PetkovFrom: Borislav Petkov <[email protected]>
The idea was to not scan the microcode blob on each AP (Application Processor) during boot and thus save us some milliseconds. However, on architectures where the microcode engine is shared between threads, this doesn't work. Here's why:
The microcode on CPU0, i.e., the first thread, gets updated. The second thread, i.e., CPU1, i.e., the first AP walks into load_ucode_amd_ap(), sees that there's no container cached and goes and scans for the proper blob.
It finds it and as a last step of apply_microcode_early_amd(), it tries to apply the patch but that core has already the updated microcode revision which it has received through CPU0's update. So it returns false and we do desc->size = -1 to prevent other APs from scanning.
However, the next AP, CPU2, has a different microcode engine which hasn't been updated yet. The desc->size == -1 test prevents it from scanning the blob anew and we fail to update it.
The fix is much more straight-forward than it looks: the BSP (BootStrapping Processor), i.e., CPU0, caches the microcode patch in amd_ucode_patch. We use that on the AP and try to apply it. In the 99.9999% of cases where we have homogeneous cores - *not* mixed-steppings - the application will be successful and we're good to go.
In the remaining small set of systems, we will simply rescan the blob and find (or not, if none present) the proper patch and apply it then.
--- arch/x86/kernel/cpu/microcode/amd.c | 74 +++++++++---------------------------- 1 file changed, 18 insertions(+), 56 deletions(-)
arch/x86/kernel/cpu/microcode/amd.c +18 -56
@@ -52,7 +52,7 @@ struct cont_desc {u16 eq_id;u8 *data;size_t size; -} cont; +};static u32 ucode_new_rev;static u8 amd_ucode_patch[PATCH_MAX_SIZE ];
@@ -210,8 +210,7 @@ static int __apply_microcode_amd(struct microcode_amd *mc)* Returns true if container found (sets @desc), false otherwise. */ static bool -apply_microcode_early_amd (u32 cpuid_1_eax,void *ucode,size_t size, -bool save_patch,struct cont_desc *ret_desc) +apply_microcode_early_amd (u32 cpuid_1_eax,void *ucode,size_t size,bool save_patch) {struct cont_desc desc = {0 };u8 (*patch)[PATCH_MAX_SIZE ];
@@ -250,9 +249,6 @@ apply_microcode_early_amd(u32 cpuid_1_eax, void *ucode, size_t size,memcpy (patch, mc,min_t (u32, desc.psize,PATCH_MAX_SIZE )); } -if (ret_desc) - *ret_desc = desc; -return ret; }
@@ -302,76 +298,42 @@ void __init load_ucode_amd_bsp(unsigned int cpuid_1_eax)struct cpio_data cp = { };__load_ucode_amd (cpuid_1_eax, &cp); -if (!(cp.data && cp.size))return ; -apply_microcode_early_amd (cpuid_1_eax, cp.data, cp.size, true,NULL ); +apply_microcode_early_amd (cpuid_1_eax, cp.data, cp.size, true); }void load_ucode_amd_ap (unsigned int cpuid_1_eax) { -struct equiv_cpu_entry *eq;struct microcode_amd *mc; -struct cont_desc *desc; -u16 eq_id; +struct cpio_data cp; +u32 *new_rev, rev, dummy;if (IS_ENABLED (CONFIG_X86_32 )) { - mc = (struct microcode_amd *)__pa_nodebug (amd_ucode_patch); - desc = (struct cont_desc *)__pa_nodebug (&cont); + mc = (struct microcode_amd *)__pa_nodebug (amd_ucode_patch); + new_rev = (u32 *)__pa_nodebug (&ucode_new_rev); + }else { - mc = (struct microcode_amd *)amd_ucode_patch; - desc = &cont; + mc = (struct microcode_amd *)amd_ucode_patch; + new_rev = &ucode_new_rev; } -/* First AP hasn't cached it yet, go through the blob. */ -if (!desc->data) { -struct cpio_data cp = { }; - -if (desc->size ==-1 ) -return ; - -reget: -__load_ucode_amd (cpuid_1_eax, &cp); -if (!(cp.data && cp.size)) { -/* -* Mark it so that other APs do not scan again for no -* real reason and slow down boot needlessly. -*/ - desc->size =-1 ; -return ; - } +microcode_rdmsr (MSR_AMD64_PATCH_LEVEL , rev, dummy); -if (!apply_microcode_early_amd (cpuid_1_eax, cp.data, cp.size, false, desc)) { - desc->data =NULL ; - desc->size =-1 ; +/* Check whether we have saved a new patch already: */ +if (*new_rev && rev < mc->hdr.patch_id) { +if (!__apply_microcode_amd (mc)) { + *new_rev = mc->hdr.patch_id;return ; } } - eq = (struct equiv_cpu_entry *)(desc->data +CONTAINER_HDR_SZ ); - - eq_id =find_equiv_id (eq, cpuid_1_eax); -if (!eq_id) +__load_ucode_amd (cpuid_1_eax, &cp); +if (!(cp.data && cp.size))return ; -if (eq_id == desc->eq_id) { -u32 rev, dummy; - -microcode_rdmsr (MSR_AMD64_PATCH_LEVEL , rev, dummy); -if (mc && rev < mc->hdr.patch_id) { -if (!__apply_microcode_amd (mc)) - ucode_new_rev = mc->hdr.patch_id; - } - - }else { - -/* -* AP has a different equivalence ID than BSP, looks like -* mixed-steppings silicon so go through the ucode blob anew. -*/ - goto reget; - } +apply_microcode_early_amd (cpuid_1_eax, cp.data, cp.size, false); } static enum ucode_state
Signature · 1 line
2.11.0
Re: [PATCH 13/13] x86/microcode/AMD: Remove AP scanning optimization
Quoted 12 lines · Borislav Petkov
From: Borislav Petkov <[email protected]>
The idea was to not scan the microcode blob on each AP (Application Processor) during boot and thus save us some milliseconds. However, on architectures where the microcode engine is shared between threads, this doesn't work. Here's why:
The microcode on CPU0, i.e., the first thread, gets updated. The second thread, i.e., CPU1, i.e., the first AP walks into
load_ucode_amd_ap(), sees that there's no container cached and goes and scans for the proper blob.
Quoted 8 lines
It finds it and as a last step of
apply_microcode_early_amd(), it tries to apply the patch but that core has already the updated microcode revision which it has received through CPU0's update. So it returns false and we dodesc->size= -1 to prevent other APs from scanning.However, the next AP, CPU2, has a different microcode engine which hasn't been updated yet. The
desc->size== -1 test prevents it from scanning the blob anew and we fail to update it.
Well, that could be solved by a proper state member in the global container descriptor. But your solution is better in the end.
Quoted 9 lines
The fix is much more straight-forward than it looks: the BSP (BootStrapping Processor), i.e., CPU0, caches the microcode patch in
amd_ucode_patch. We use that on the AP and try to apply it. In the 99.9999% of cases where we have homogeneous cores - *not* mixed-steppings - the application will be successful and we're good to go.In the remaining small set of systems, we will simply rescan the blob and find (or not, if none present) the proper patch and apply it then.
Makes sense, but how does such a system handle the suspend/resume case when the micro code is in the initrd? Are you caching the per cpu patches somewhere?