[PATCH] x86/boot: Load microcode much earlier on boot

Andrew Cooper posted 1 patch 2 weeks, 3 days ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20241119215708.2890691-1-andrew.cooper3@citrix.com
xen/arch/x86/setup.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
[PATCH] x86/boot: Load microcode much earlier on boot
Posted by Andrew Cooper 2 weeks, 3 days ago
Following commit cd7cc5320bb2 ("x86/boot: add start and size fields to struct
boot_module"), bootstrap_map*() works as soon as boot_info is populated.

Resolve the todo, and move microcode loading to be the eariest action after
establishing a console.

A sample boot now looks like:

  (XEN) Xen version 4.20-unstable (andrew@eng.citrite.net) (gcc (Debian 12.2.0-14) 12.2.0) debug=y Tue Nov 19 21:44:46 GMT 2024
  (XEN) Latest ChangeSet: Wed Dec 6 21:54:55 2023 git:1ab612848a23
  (XEN) build-id: 52fe616d1b3a2a2cb44775815507d02cca73315d
  (XEN) CPU Vendor: AMD, Family 25 (0x19), Model 1 (0x1), Stepping 1 (raw 00a00f11)
  (XEN) BSP microcode revision: 0x0a001137
  (XEN) microcode: CPU0 updated from revision 0xa001137 to 0xa0011d7, date = 2024-09-06
  (XEN) Bootloader: GRUB 2.06

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Daniel P. Smith <dpsmith@apertussolutions.com>
---
 xen/arch/x86/setup.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 1239e91b83b0..d8661d7ca699 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1160,6 +1160,13 @@ void asmlinkage __init noreturn __start_xen(void)
     xhci_dbc_uart_init();
     console_init_preirq();
 
+    /*
+     * Try to load microcode as early as possible, although wait until after
+     * configuring the console(s).
+     */
+    early_cpu_init(true);
+    early_microcode_init(bi);
+
     if ( pvh_boot )
         pvh_print_info();
 
@@ -1312,9 +1319,6 @@ void asmlinkage __init noreturn __start_xen(void)
     else
         panic("Bootloader provided no memory information\n");
 
-    /* This must come before e820 code because it sets paddr_bits. */
-    early_cpu_init(true);
-
     /* Choose shadow stack early, to set infrastructure up appropriately. */
     if ( !boot_cpu_has(X86_FEATURE_CET_SS) )
         opt_xen_shstk = 0;
@@ -1416,12 +1420,6 @@ void asmlinkage __init noreturn __start_xen(void)
         if ( bi->mods[i].start & (PAGE_SIZE - 1) )
             panic("Bootloader didn't honor module alignment request\n");
 
-    /*
-     * TODO: load ucode earlier once multiboot modules become accessible
-     * at an earlier stage.
-     */
-    early_microcode_init(bi);
-
     if ( xen_phys_start )
     {
         struct boot_module *xen = &bi->mods[bi->nr_modules];
-- 
2.39.5


Re: [PATCH] x86/boot: Load microcode much earlier on boot
Posted by Jan Beulich 2 weeks, 2 days ago
On 19.11.2024 22:57, Andrew Cooper wrote:
> Following commit cd7cc5320bb2 ("x86/boot: add start and size fields to struct
> boot_module"), bootstrap_map*() works as soon as boot_info is populated.

I'm struggling with following where this connection is coming from. Before
Daniel's work (e.g. in 4.19) we have

    if ( pvh_boot )
    {
        ASSERT(mbi_p == 0);
        pvh_init(&mbi, &mod);
    }
    else
    {
        mbi = __va(mbi_p);
        mod = __va(mbi->mods_addr);
    }

Leaving aside the issue with the PVH side, what would have stood in the way
of accessing any of the modules (e.g. ucode) right afterwards?

The question is also relevant when considering the potential need for
backporting this functionality (even if not directly this change, as the
dependency on Daniel's work would then need stripping off): There might
conceivably be a point where for security needs way may need to have this
as early as you now put it.

> Resolve the todo, and move microcode loading to be the eariest action after
> establishing a console.

So yes, this is merely strengthening a dependency we already have:
bootstrap_map_addr() arranging to avoid allocating any page table memory.
Yet don't we rather need to move away from this? We don't know what's in
the excess space we map, and hence we better wouldn't have any (cachable)
mappings thereof.

Jan
Re: [PATCH] x86/boot: Load microcode much earlier on boot
Posted by Andrew Cooper 2 weeks, 1 day ago
On 20/11/2024 10:44 am, Jan Beulich wrote:
> On 19.11.2024 22:57, Andrew Cooper wrote:
>> Resolve the todo, and move microcode loading to be the eariest action after
>> establishing a console.
> So yes, this is merely strengthening a dependency we already have:
> bootstrap_map_addr() arranging to avoid allocating any page table memory.
> Yet don't we rather need to move away from this? We don't know what's in
> the excess space we map, and hence we better wouldn't have any (cachable)
> mappings thereof.

I don't see it this way.

Yes it is somewhat dodgy that bootstrap_map*() blindly uses 2M pages.

It's a problem with cacheability (in theory at least; TLBs tend to
splinter mappings on MTRR mismatch).  It's a hard problem with the RMP
table (AMD SEV-SNP), where any access into the page yields #PF[Rsvd] if
there's any overlap with the RMP (locked in place by firmware and not
always 2M aligned).

But, any fix to this needs to fix *all* users of bootstrap_map*(),
including the move_memory() loop.

So I don't see any interaction with how early or late the early-map
infrastructure is used.

~Andrew

P.S. I have a few ideas on how to address this.

With only 2 extra pagetables in .init.bss, we can support any arbitrary
single mapping with 4k alignment.

move_memory() is the only user that makes multiple mappings, and it's
broken anyway.  I'm now certain that it does get memmove() wrong if the
move has a partial overlap.

However, move_memory() always has a good destination pointer in the
directmap, so I've got a crazy idea to fix the memmove() problem by also
hooking the bootmap into idle_pg_table[511] creating a high alias of the
mapping, and the source pointer can choose whichever of the aliases are
the correct side of the directmap.

Re: [PATCH] x86/boot: Load microcode much earlier on boot
Posted by Andrew Cooper 2 weeks, 2 days ago
On 20/11/2024 10:44 am, Jan Beulich wrote:
> On 19.11.2024 22:57, Andrew Cooper wrote:
>> Following commit cd7cc5320bb2 ("x86/boot: add start and size fields to struct
>> boot_module"), bootstrap_map*() works as soon as boot_info is populated.
> I'm struggling with following where this connection is coming from.

Specifically, the final hunk:

> @@ -1416,12 +1420,6 @@ void asmlinkage __init noreturn
> __start_xen(void) if ( bi->mods[i].start & (PAGE_SIZE - 1) )
> panic("Bootloader didn't honor module alignment request\n"); - /* - *
> TODO: load ucode earlier once multiboot modules become accessible - *
> at an earlier stage. - */ - early_microcode_init(bi); - if (
> xen_phys_start ) { struct boot_module *xen = &bi->mods[bi->nr_modules];

The context with panic() used to read:

    panic("Bootloader didn't honor module alignment request\n");
    bi->mods[i].mod->mod_end -= bi->mods[i].mod->mod_start;
    bi->mods[i].mod->mod_start >>= PAGE_SHIFT;

and calling bootstrap_map() prior to that mapped junk because the start
is wrong by PAGE_SHIFT.

This is why the TODO was inserted the last time around when we couldn't
move loading as early as wanted.


I suppose in principle this could be backported if we took
bootstrap_map_addr() (which is a self-contained patch), and adjusted
early_microcode_init() to avoid using bootstrap_map(), but it feels
rather fragile, and still depends on most of the ucode fixes.

~Andrew

Re: [PATCH] x86/boot: Load microcode much earlier on boot
Posted by Jan Beulich 2 weeks, 1 day ago
On 20.11.2024 12:24, Andrew Cooper wrote:
> On 20/11/2024 10:44 am, Jan Beulich wrote:
>> On 19.11.2024 22:57, Andrew Cooper wrote:
>>> Following commit cd7cc5320bb2 ("x86/boot: add start and size fields to struct
>>> boot_module"), bootstrap_map*() works as soon as boot_info is populated.
>> I'm struggling with following where this connection is coming from.
> 
> Specifically, the final hunk:
> 
>> @@ -1416,12 +1420,6 @@ void asmlinkage __init noreturn
>> __start_xen(void) if ( bi->mods[i].start & (PAGE_SIZE - 1) )
>> panic("Bootloader didn't honor module alignment request\n"); - /* - *
>> TODO: load ucode earlier once multiboot modules become accessible - *
>> at an earlier stage. - */ - early_microcode_init(bi); - if (
>> xen_phys_start ) { struct boot_module *xen = &bi->mods[bi->nr_modules];
> 
> The context with panic() used to read:
> 
>     panic("Bootloader didn't honor module alignment request\n");
>     bi->mods[i].mod->mod_end -= bi->mods[i].mod->mod_start;
>     bi->mods[i].mod->mod_start >>= PAGE_SHIFT;
> 
> and calling bootstrap_map() prior to that mapped junk because the start
> is wrong by PAGE_SHIFT.
> 
> This is why the TODO was inserted the last time around when we couldn't
> move loading as early as wanted.

Hmm, I see. It wasn't really that they were inaccessible, it merely was
that adjustments of internally maintained data would have been needed.
Which could have been as easy as instantiating a local mod variable,
fill it with suitably adjusted data, and pass an address thereof to
bootstrap_map().

In any event:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

I'm surprised though that you didn't comment at all on the other aspect
I raised.

Jan