| Message ID | 20161216090228.ri4alokiaagqtibt@pd.tnic (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 16/12/16 10:02, Borislav Petkov wrote: > On Fri, Dec 16, 2016 at 08:28:46AM +0100, Juergen Gross wrote: >> Not trying to load ucode in _any_ guest is an optimization only. > > Does the hunk below work too? Without testing, but I doubt it is working. As pv guests aren't coming through check_loader_disabled_bsp() at all I can't see why your patch would work for dom0. Additionally I don't think you want to call native_cpuid() if have_cpuid_p() returns false. So I think you want a generic "platform_allows_ucode_load()" function checking for support of cpuid and virtualization. This function should be called both in check_loader_disabled_bsp() and check_loader_disabled_ap() to bail out early. Juergen > > I don't want to do hypervisor-specific solutions. > > --- > diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c > index 6996413c78c3..54219f619205 100644 > --- a/arch/x86/kernel/cpu/microcode/core.c > +++ b/arch/x86/kernel/cpu/microcode/core.c > @@ -76,6 +76,7 @@ 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); > @@ -91,6 +92,17 @@ static bool __init check_loader_disabled_bsp(void) > if (cmdline_find_option_bool(cmdline, option)) > *res = true; > > + if (!have_cpuid_p()) > + *res = true; > + > + a = 1; > + c = 0; > + native_cpuid(&a, &b, &c, &d); > + > + /* CPUID(1).ECX[31]: reserved for hypervisor use */ > + if (c & BIT(31)) > + *res = true; > + > return *res; > } > > @@ -121,9 +133,6 @@ void __init load_ucode_bsp(void) > if (check_loader_disabled_bsp()) > return; > > - if (!have_cpuid_p()) > - return; > - > vendor = x86_cpuid_vendor(); > family = x86_cpuid_family(); > > @@ -157,9 +166,6 @@ void load_ucode_ap(void) > if (check_loader_disabled_ap()) > return; > > - if (!have_cpuid_p()) > - return; > - > vendor = x86_cpuid_vendor(); > family = x86_cpuid_family(); > >
On Fri, Dec 16, 2016 at 10:20:42AM +0100, Juergen Gross wrote: > Without testing, but I doubt it is working. As pv guests aren't coming > through check_loader_disabled_bsp() at all I can't see why your patch > would work for dom0. Do they go through check_loader_disabled_ap() ? > Additionally I don't think you want to call native_cpuid() if > have_cpuid_p() returns false. Good point, fixed. > So I think you want a generic "platform_allows_ucode_load()" > function checking for support of cpuid and virtualization. This > function should be called both in check_loader_disabled_bsp() and > check_loader_disabled_ap() to bail out early. See question above. If they go through check_loader_disabled_ap(), then I'm inclined to set dis_ucode_ldr to true at build time and let check_loader_disabled_bsp() set it to false on baremetal or if any of the other checks pass. If the pv guests run into check_loader_disabled_ap, then they'll see dis_ucode_ldr true and return. Ok?
On 16/12/16 10:43, Borislav Petkov wrote: > On Fri, Dec 16, 2016 at 10:20:42AM +0100, Juergen Gross wrote: >> Without testing, but I doubt it is working. As pv guests aren't coming >> through check_loader_disabled_bsp() at all I can't see why your patch >> would work for dom0. > > Do they go through check_loader_disabled_ap() ? Yes. > >> Additionally I don't think you want to call native_cpuid() if >> have_cpuid_p() returns false. > > Good point, fixed. > >> So I think you want a generic "platform_allows_ucode_load()" >> function checking for support of cpuid and virtualization. This >> function should be called both in check_loader_disabled_bsp() and >> check_loader_disabled_ap() to bail out early. > > See question above. If they go through check_loader_disabled_ap(), > then I'm inclined to set dis_ucode_ldr to true at build time and let > check_loader_disabled_bsp() set it to false on baremetal or if any of > the other checks pass. > > If the pv guests run into check_loader_disabled_ap, then they'll see > dis_ucode_ldr true and return. > > Ok? Should work. I'm happy to test any patch. :-) Juergen
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c index 6996413c78c3..54219f619205 100644 --- a/arch/x86/kernel/cpu/microcode/core.c +++ b/arch/x86/kernel/cpu/microcode/core.c @@ -76,6 +76,7 @@ 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); @@ -91,6 +92,17 @@ static bool __init check_loader_disabled_bsp(void) if (cmdline_find_option_bool(cmdline, option)) *res = true; + if (!have_cpuid_p()) + *res = true; + + a = 1; + c = 0; + native_cpuid(&a, &b, &c, &d); + + /* CPUID(1).ECX[31]: reserved for hypervisor use */ + if (c & BIT(31)) + *res = true; + return *res; } @@ -121,9 +133,6 @@ void __init load_ucode_bsp(void) if (check_loader_disabled_bsp()) return; - if (!have_cpuid_p()) - return; - vendor = x86_cpuid_vendor(); family = x86_cpuid_family(); @@ -157,9 +166,6 @@ void load_ucode_ap(void) if (check_loader_disabled_ap()) return; - if (!have_cpuid_p()) - return; - vendor = x86_cpuid_vendor(); family = x86_cpuid_family();
On Fri, Dec 16, 2016 at 08:28:46AM +0100, Juergen Gross wrote: > Not trying to load ucode in _any_ guest is an optimization only. Does the hunk below work too? I don't want to do hypervisor-specific solutions. ---