[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Y7iHfaOD67UO+lbV@a4bf019067fa.jf.intel.com>
Date: Fri, 6 Jan 2023 12:41:33 -0800
From: Ashok Raj <ashok.raj@...el.com>
To: Borislav Petkov <bp@...en8.de>
CC: Thomas Gleixner <tglx@...utronix.de>, X86-kernel <x86@...nel.org>,
"LKML Mailing List" <linux-kernel@...r.kernel.org>,
Dave Hansen <dave.hansen@...el.com>,
Tony Luck <tony.luck@...el.com>,
Alison Schofield <alison.schofield@...el.com>,
Reinette Chatre <reinette.chatre@...el.com>,
Tom Lendacky <thomas.lendacky@....com>,
Ashok Raj <ashok.raj@...el.com>
Subject: Re: [PATCH v3 2/6] x86/microcode/core: Take a snapshot before and
after applying microcode
On Wed, Jan 04, 2023 at 07:56:52PM +0100, Borislav Petkov wrote:
> On Tue, Jan 03, 2023 at 10:02:08AM -0800, Ashok Raj wrote:
> > Fixes: 1008c52c09dc ("x86/CPU: Add a microcode loader callback")
>
> Why a Fixes tag? Do you have a failure scenario for current kernels?
When we reload the same microcode there is no change in CPUID bits, but if
the kernel has turned off a feature, in my case it was SGX. So bsp copy has
SGX off, but new get_cpu_cap() seems to think SGX is there since this is
unfiltered by the kernel.
This results in a miscompare. I have noticed even when i load a brand new
patch but has no change in CPUID, report there might have been a change in
CPUID.
>
> If so, then it would need stable backporting.
>
> If so, it would need the previous patch too.
I think so, but your call.
>
> > diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> > index 387578049de0..ac2e67156b9b 100644
> > --- a/arch/x86/include/asm/processor.h
> > +++ b/arch/x86/include/asm/processor.h
> > @@ -697,6 +697,7 @@ bool xen_set_default_idle(void);
> > #endif
> >
> > void __noreturn stop_this_cpu(void *dummy);
> > +void microcode_store_cpu_caps(struct cpuinfo_x86 *info);
>
> s/microcode_store_cpu_caps/store_cpu_caps/g
Yes, i'll change.
>
> > void microcode_check(struct cpuinfo_x86 *info);
> >
> > enum l1tf_mitigations {
> > diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> > index b9c7529c920e..7c86c6fd07ae 100644
> > --- a/arch/x86/kernel/cpu/common.c
> > +++ b/arch/x86/kernel/cpu/common.c
> > @@ -2297,28 +2297,43 @@ void cpu_init_secondary(void)
> > #endif
> >
> > #ifdef CONFIG_MICROCODE_LATE_LOADING
> > +
> > +void microcode_store_cpu_caps(struct cpuinfo_x86 *info)
> > +{
> > + /* Reload CPUID max function as it might've changed. */
>
> Might've changed how?
This comment existed in the previous microcode_check(). I just moved it
around during the refactor.
I suppose new microcode can bring new features and this max function
enumeration can also change?
>
> > + info->cpuid_level = cpuid_eax(0);
> > +
> > + /*
> > + * Copy all capability leafs to pick up the synthetic ones so that
> > + * memcmp() below doesn't fail on that...
>
> split that comment and put the second part...
>
> > + */
> > + memcpy(info->x86_capability, &boot_cpu_data.x86_capability,
> > + sizeof(info->x86_capability));
> > +
>
> ... here:
Will do.
>
> /*
> * ... the ones coming from CPUID will get overwritten here:
> */
>
> > + get_cpu_cap(info);
> > +}
> > +
> > /*
> > * The microcode loader calls this upon late microcode load to recheck features,
> > * only when microcode has been updated. Caller holds microcode_mutex and CPU
> > * hotplug lock.
> > */
> > -void microcode_check(struct cpuinfo_x86 *info)
> > +void microcode_check(struct cpuinfo_x86 *orig)
> ^^^^^
>
> Yeah, what dhansen said.
Yes, I'm changing that as well.
Powered by blists - more mailing lists