| 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
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(-)