| Message ID | 20241119215708.2890691-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/boot: Load microcode much earlier on boot | expand |
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
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
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
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.
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];
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(-)