| Message ID | 20241017170325.3842-18-dpsmith@apertussolutions.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Boot modules for Hyperlaunch | expand |
On 17/10/2024 6:02 pm, Daniel P. Smith wrote: > diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c > index 8564e4d2c94c..22fea80bc97e 100644 > --- a/xen/arch/x86/cpu/microcode/core.c > +++ b/xen/arch/x86/cpu/microcode/core.c > @@ -178,16 +177,16 @@ static void __init microcode_scan_module( > /* > * Try all modules and see whichever could be the microcode blob. > */ > - for ( i = 1 /* Ignore dom0 kernel */; i < mbi->mods_count; i++ ) > + for ( i = 1 /* Ignore dom0 kernel */; i < bi->nr_modules; i++ ) > { > if ( !test_bit(i, module_map) ) > continue; > Somewhere in this series, it would be nice to purge the these "module 0 is dom0" special cases. I'm not sure where best in the series to do it, and it may not be here. Later, in "x86/boot: remove module_map usage from microcode loading" you convert this test_bit() into a type != UNKNOWN check, but the pattern is used elsewhere too. How about a for_each_unknown_module(bi, bm) helper? (which at this point can even use module_map in scope). If you introduce it at this point, then I think the churn later in the series reduces marginally, and I think it removes all the "careful not to look at dom0" special cases. ~Andrew
On 10/17/24 21:14, Andrew Cooper wrote: > On 17/10/2024 6:02 pm, Daniel P. Smith wrote: >> diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c >> index 8564e4d2c94c..22fea80bc97e 100644 >> --- a/xen/arch/x86/cpu/microcode/core.c >> +++ b/xen/arch/x86/cpu/microcode/core.c >> @@ -178,16 +177,16 @@ static void __init microcode_scan_module( >> /* >> * Try all modules and see whichever could be the microcode blob. >> */ >> - for ( i = 1 /* Ignore dom0 kernel */; i < mbi->mods_count; i++ ) >> + for ( i = 1 /* Ignore dom0 kernel */; i < bi->nr_modules; i++ ) >> { >> if ( !test_bit(i, module_map) ) >> continue; >> > > Somewhere in this series, it would be nice to purge the these "module 0 > is dom0" special cases. I'm not sure where best in the series to do it, > and it may not be here. > > Later, in "x86/boot: remove module_map usage from microcode loading" you > convert this test_bit() into a type != UNKNOWN check, but the pattern is > used elsewhere too. > > How about a for_each_unknown_module(bi, bm) helper? (which at this > point can even use module_map in scope). So I do have the first_boot_module_index() iterator which I am using to effectively open-code your suggested for_each_unknown_module() in one or two places. I can introduce that helper when I first do the open coding, though I would like to make it a little more generic. I would prefer to do it as for_each_bootmodule(bi, bm, type). There is a scenario/enhancement that I would like to get to that may require doing an iteration on a type other than BOOTMOD_UNKNOWN. Would you be okay with leaving the module_map usage at this point and changing over to the for_each_bootmodule() when it is dropped? As I see it, otherwise I would have to make the helper initially work with module_map only to turn around and drop it when module_map goes away. At least to me, seems like unnecessary churn unless you see a way without causing the churn in the helper. > If you introduce it at this point, then I think the churn later in the > series reduces marginally, and I think it removes all the "careful not > to look at dom0" special cases. In the end, yes I want to get a way from that, once domain builder is introduced, it will be possible to provide the boot modules in any order. v/r, dps
On 19/10/2024 4:49 pm, Daniel P. Smith wrote: > On 10/17/24 21:14, Andrew Cooper wrote: >> On 17/10/2024 6:02 pm, Daniel P. Smith wrote: >>> diff --git a/xen/arch/x86/cpu/microcode/core.c >>> b/xen/arch/x86/cpu/microcode/core.c >>> index 8564e4d2c94c..22fea80bc97e 100644 >>> --- a/xen/arch/x86/cpu/microcode/core.c >>> +++ b/xen/arch/x86/cpu/microcode/core.c >>> @@ -178,16 +177,16 @@ static void __init microcode_scan_module( >>> /* >>> * Try all modules and see whichever could be the microcode blob. >>> */ >>> - for ( i = 1 /* Ignore dom0 kernel */; i < mbi->mods_count; i++ ) >>> + for ( i = 1 /* Ignore dom0 kernel */; i < bi->nr_modules; i++ ) >>> { >>> if ( !test_bit(i, module_map) ) >>> continue; >>> >> >> Somewhere in this series, it would be nice to purge the these "module 0 >> is dom0" special cases. I'm not sure where best in the series to do it, >> and it may not be here. >> >> Later, in "x86/boot: remove module_map usage from microcode loading" you >> convert this test_bit() into a type != UNKNOWN check, but the pattern is >> used elsewhere too. >> >> How about a for_each_unknown_module(bi, bm) helper? (which at this >> point can even use module_map in scope). > > So I do have the first_boot_module_index() iterator which I am using > to effectively open-code your suggested for_each_unknown_module() in > one or two places. I can introduce that helper when I first do the > open coding, > though I would like to make it a little more generic. I would prefer > to do it as for_each_bootmodule(bi, bm, type). There is a > scenario/enhancement that I would like to get to that may require > doing an iteration on a type other than BOOTMOD_UNKNOWN. > > Would you be okay with leaving the module_map usage at this point and > changing over to the for_each_bootmodule() when it is dropped? As I > see it, otherwise I would have to make the helper initially work with > module_map only to turn around and drop it when module_map goes away. > At least to me, seems like unnecessary churn unless you see a way > without causing the churn in the helper. I'll leave it to your judgement about when is best to introduce the helper. You know the series better than I do. ~Andrew
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c index 8564e4d2c94c..22fea80bc97e 100644 --- a/xen/arch/x86/cpu/microcode/core.c +++ b/xen/arch/x86/cpu/microcode/core.c @@ -35,6 +35,7 @@ #include <xen/watchdog.h> #include <asm/apic.h> +#include <asm/bootinfo.h> #include <asm/cpu-policy.h> #include <asm/nmi.h> #include <asm/processor.h> @@ -153,10 +154,8 @@ static int __init cf_check parse_ucode(const char *s) custom_param("ucode", parse_ucode); static void __init microcode_scan_module( - unsigned long *module_map, - const multiboot_info_t *mbi) + unsigned long *module_map, const struct boot_info *bi) { - module_t *mod = (module_t *)__va(mbi->mods_addr); uint64_t *_blob_start; unsigned long _blob_size; struct cpio_data cd; @@ -178,16 +177,16 @@ static void __init microcode_scan_module( /* * Try all modules and see whichever could be the microcode blob. */ - for ( i = 1 /* Ignore dom0 kernel */; i < mbi->mods_count; i++ ) + for ( i = 1 /* Ignore dom0 kernel */; i < bi->nr_modules; i++ ) { if ( !test_bit(i, module_map) ) continue; - _blob_start = bootstrap_map(&mod[i]); - _blob_size = mod[i].mod_end; + _blob_start = bootstrap_map_bm(&bi->mods[i]); + _blob_size = bi->mods[i].size; if ( !_blob_start ) { - printk("Could not map multiboot module #%d (size: %ld)\n", + printk("Could not map boot module #%d (size: %ld)\n", i, _blob_size); continue; } @@ -205,20 +204,18 @@ static void __init microcode_scan_module( } static void __init microcode_grab_module( - unsigned long *module_map, - const multiboot_info_t *mbi) + unsigned long *module_map, struct boot_info *bi) { - module_t *mod = (module_t *)__va(mbi->mods_addr); - if ( ucode_mod_idx < 0 ) - ucode_mod_idx += mbi->mods_count; - if ( ucode_mod_idx <= 0 || ucode_mod_idx >= mbi->mods_count || + ucode_mod_idx += bi->nr_modules; + if ( ucode_mod_idx <= 0 || ucode_mod_idx >= bi->nr_modules || !__test_and_clear_bit(ucode_mod_idx, module_map) ) goto scan; - ucode_mod = mod[ucode_mod_idx]; + bi->mods[ucode_mod_idx].type = BOOTMOD_MICROCODE; + ucode_mod = *bi->mods[ucode_mod_idx].mod; scan: if ( ucode_scan ) - microcode_scan_module(module_map, mbi); + microcode_scan_module(module_map, bi); } static struct microcode_ops __ro_after_init ucode_ops; @@ -822,8 +819,8 @@ static int __init early_update_cache(const void *data, size_t len) return rc; } -int __init microcode_init_cache(unsigned long *module_map, - const struct multiboot_info *mbi) +int __init microcode_init_cache( + unsigned long *module_map, const struct boot_info *bi) { int rc = 0; @@ -832,7 +829,7 @@ int __init microcode_init_cache(unsigned long *module_map, if ( ucode_scan ) /* Need to rescan the modules because they might have been relocated */ - microcode_scan_module(module_map, mbi); + microcode_scan_module(module_map, bi); if ( ucode_mod.mod_end ) rc = early_update_cache(bootstrap_map(&ucode_mod), @@ -879,7 +876,7 @@ static int __init early_microcode_update_cpu(void) } int __init early_microcode_init(unsigned long *module_map, - const struct multiboot_info *mbi) + struct boot_info *bi) { const struct cpuinfo_x86 *c = &boot_cpu_data; int rc = 0; @@ -922,7 +919,7 @@ int __init early_microcode_init(unsigned long *module_map, return -ENODEV; } - microcode_grab_module(module_map, mbi); + microcode_grab_module(module_map, bi); if ( ucode_mod.mod_end || ucode_blob.size ) rc = early_microcode_update_cpu(); diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h index 6903ab00ec90..19a0ed16ab27 100644 --- a/xen/arch/x86/include/asm/bootinfo.h +++ b/xen/arch/x86/include/asm/bootinfo.h @@ -20,6 +20,7 @@ enum bootmod_type { BOOTMOD_XEN, BOOTMOD_KERNEL, BOOTMOD_RAMDISK, + BOOTMOD_MICROCODE, }; struct boot_module { diff --git a/xen/arch/x86/include/asm/microcode.h b/xen/arch/x86/include/asm/microcode.h index 57c08205d475..495c8f7a7cc5 100644 --- a/xen/arch/x86/include/asm/microcode.h +++ b/xen/arch/x86/include/asm/microcode.h @@ -4,6 +4,8 @@ #include <xen/types.h> #include <xen/percpu.h> +#include <asm/bootinfo.h> + #include <public/xen.h> struct multiboot_info; @@ -22,12 +24,12 @@ struct cpu_signature { DECLARE_PER_CPU(struct cpu_signature, cpu_sig); void microcode_set_module(unsigned int idx); -int microcode_update(XEN_GUEST_HANDLE(const_void) buf, - unsigned long len, unsigned int flags); -int early_microcode_init(unsigned long *module_map, - const struct multiboot_info *mbi); -int microcode_init_cache(unsigned long *module_map, - const struct multiboot_info *mbi); +int microcode_update( + XEN_GUEST_HANDLE(const_void) buf, unsigned long len, unsigned int flags); +int early_microcode_init( + unsigned long *module_map, struct boot_info *bi); +int microcode_init_cache( + unsigned long *module_map, const struct boot_info *bi); int microcode_update_one(void); #endif /* ASM_X86__MICROCODE_H */ diff --git a/xen/arch/x86/include/asm/setup.h b/xen/arch/x86/include/asm/setup.h index 3d189521189d..729f68ca23b8 100644 --- a/xen/arch/x86/include/asm/setup.h +++ b/xen/arch/x86/include/asm/setup.h @@ -2,6 +2,7 @@ #define __X86_SETUP_H_ #include <xen/multiboot.h> +#include <asm/bootinfo.h> #include <asm/numa.h> extern const char __2M_text_start[], __2M_text_end[]; @@ -37,6 +38,7 @@ extern struct boot_info xen_boot_info; unsigned long initial_images_nrpages(nodeid_t node); void discard_initial_images(void); void *bootstrap_map(const module_t *mod); +void *bootstrap_map_bm(const struct boot_module *bm); int remove_xen_ranges(struct rangeset *r); diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 852454c161ee..04de06ba1400 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -482,6 +482,14 @@ void *__init bootstrap_map(const module_t *mod) pfn_to_paddr(mod->mod_start) + mod->mod_end); } +void *__init bootstrap_map_bm(const struct boot_module *bm) +{ + if ( !bm ) + return bootstrap_map_addr(0, 0); + + return bootstrap_map_addr(bm->start, bm->start + bm->size); +} + static void __init move_memory( uint64_t dst, uint64_t src, unsigned int size) { @@ -1377,7 +1385,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) * TODO: load ucode earlier once multiboot modules become accessible * at an earlier stage. */ - early_microcode_init(module_map, mbi); + early_microcode_init(module_map, bi); if ( xen_phys_start ) { @@ -1929,7 +1937,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) timer_init(); - microcode_init_cache(module_map, mbi); /* Needs xmalloc() */ + microcode_init_cache(module_map, bi); /* Needs xmalloc() */ tsx_init(); /* Needs microcode. May change HLE/RTM feature bits. */
Convert the microcode loading functions to take struct boot_info, and then using struct boot_module to map and check for microcode. To keep the changes focused, continue using the struct mod to hold the reference to the microcode that is used by the late microcode logic. To support loading the microcode boot module in to the bootstrap map, introduce bootstrap_map_bm() for mapping struct boot_module. Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> --- Changes since v5: - moved bootstrap_map_bm definition to this commit --- xen/arch/x86/cpu/microcode/core.c | 37 +++++++++++++--------------- xen/arch/x86/include/asm/bootinfo.h | 1 + xen/arch/x86/include/asm/microcode.h | 14 ++++++----- xen/arch/x86/include/asm/setup.h | 2 ++ xen/arch/x86/setup.c | 12 +++++++-- 5 files changed, 38 insertions(+), 28 deletions(-)