Messages in this thread Patch in this message | | | Date | Tue, 16 Aug 2022 09:00:14 +0000 | From | Ashok Raj <> | Subject | Re: [PATCH] x86/microcode/AMD: Attempt applying on every logical thread |
| |
Hi Boris
Trying to understand if I'm missing something here.
On Sun, Aug 14, 2022 at 02:00:26PM +0200, Borislav Petkov wrote: > From: Borislav Petkov <bp@suse.de> > > Currently, the patch application logic checks whether patch application > is needed. Therefore, on SMT designs where the microcode engine is > shared between the two threads, the application happens only on one of > them.
A re-application means, you want to apply even if the cpu_rev <= patch.rev
if cpu_rev is > patch_rev, clearly its ahead?. say BIOS has a newer version than in the initrd image, do we want to replace the BIOS version since we do no revid checks here.
> > However, there are microcode patches which do per-thread modification, > see Link tag below. > > Therefore, drop the revision check and try applying on each thread. This > is what the BIOS does too so this method is very much tested. > > Reported-by: Ștefan Talpalaru <stefantalpalaru@yahoo.com> > Tested-by: Ștefan Talpalaru <stefantalpalaru@yahoo.com> > Signed-off-by: Borislav Petkov <bp@suse.de> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=216211 > --- > arch/x86/kernel/cpu/microcode/amd.c | 39 +++++++---------------------- > 1 file changed, 9 insertions(+), 30 deletions(-) > > diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c > index 8b2fcdfa6d31..a575dbb4d80c 100644 > --- a/arch/x86/kernel/cpu/microcode/amd.c > +++ b/arch/x86/kernel/cpu/microcode/amd.c > @@ -420,8 +420,8 @@ apply_microcode_early_amd(u32 cpuid_1_eax, void *ucode, size_t size, bool save_p > struct cont_desc desc = { 0 }; > u8 (*patch)[PATCH_MAX_SIZE]; > struct microcode_amd *mc; > - u32 rev, dummy, *new_rev; > bool ret = false; > + u32 *new_rev; > > #ifdef CONFIG_X86_32 > new_rev = (u32 *)__pa_nodebug(&ucode_new_rev); > @@ -439,10 +439,6 @@ apply_microcode_early_amd(u32 cpuid_1_eax, void *ucode, size_t size, bool save_p > if (!mc) > return ret; > > - native_rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy); > - if (rev >= mc->hdr.patch_id) > - return ret; > -
Instead of just removing the entire rev check, you want to reapply even if the rev == patch_rev?
Worried this would allow you to go backwards as well.
if(rev > mc->hdr.patch_id) return ret;
> if (!__apply_microcode_amd(mc)) { > *new_rev = mc->hdr.patch_id; > ret = true; > @@ -516,7 +512,7 @@ void load_ucode_amd_ap(unsigned int cpuid_1_eax) > { > struct microcode_amd *mc; > struct cpio_data cp; > - u32 *new_rev, rev, dummy; > + u32 *new_rev; > > if (IS_ENABLED(CONFIG_X86_32)) { > mc = (struct microcode_amd *)__pa_nodebug(amd_ucode_patch); > @@ -526,10 +522,8 @@ void load_ucode_amd_ap(unsigned int cpuid_1_eax) > new_rev = &ucode_new_rev; > } > > - native_rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy); > - > /* Check whether we have saved a new patch already: */ > - if (*new_rev && rev < mc->hdr.patch_id) { > + if (*new_rev) {
Here cpu_rev < mc->rev, is there a reason to remove this check?
if cpu_rev > mc->rev, the following would go backwards in rev
> if (!__apply_microcode_amd(mc)) { > *new_rev = mc->hdr.patch_id; > return; > @@ -571,23 +565,17 @@ int __init save_microcode_in_initrd_amd(unsigned int cpuid_1_eax) > > void reload_ucode_amd(void) > { > - struct microcode_amd *mc; > - u32 rev, dummy __always_unused; > - > - mc = (struct microcode_amd *)amd_ucode_patch; > + struct microcode_amd *mc = (struct microcode_amd *)amd_ucode_patch; > > - rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy); > - > - if (rev < mc->hdr.patch_id) { > - if (!__apply_microcode_amd(mc)) { > - ucode_new_rev = mc->hdr.patch_id; > - pr_info("reload patch_level=0x%08x\n", ucode_new_rev); > - } > + if (!__apply_microcode_amd(mc)) { > + ucode_new_rev = mc->hdr.patch_id; > + pr_info("reload patch_level=0x%08x\n", ucode_new_rev); > } > } > static u16 __find_equiv_id(unsigned int cpu) > { > struct ucode_cpu_info *uci = ucode_cpu_info + cpu; > + > return find_equiv_id(&equiv_table, uci->cpu_sig.sig); > } > > @@ -678,7 +666,7 @@ static enum ucode_state apply_microcode_amd(int cpu) > struct ucode_cpu_info *uci; > struct ucode_patch *p; > enum ucode_state ret; > - u32 rev, dummy __always_unused; > + u32 rev; > > BUG_ON(raw_smp_processor_id() != cpu); > > @@ -691,14 +679,6 @@ static enum ucode_state apply_microcode_amd(int cpu) > mc_amd = p->data; > uci->mc = p->data; > > - rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy); > - > - /* need to apply patch? */ > - if (rev >= mc_amd->hdr.patch_id) { > - ret = UCODE_OK; > - goto out; > - } > - > if (__apply_microcode_amd(mc_amd)) { > pr_err("CPU%d: update failed for patch_level=0x%08x\n", > cpu, mc_amd->hdr.patch_id); > @@ -710,7 +690,6 @@ static enum ucode_state apply_microcode_amd(int cpu) > > pr_info("CPU%d: new patch_level=0x%08x\n", cpu, rev); > > -out: > uci->cpu_sig.rev = rev; > c->microcode = rev; > > -- > 2.35.1 >
| |