lkml.org 
[lkml]   [2022]   [Aug]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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
>

\
 
 \ /
  Last update: 2022-08-16 11:54    [W:0.270 / U:0.380 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site