| Message ID | 20241006214956.24339-18-dpsmith@apertussolutions.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Boot modules for Hyperlaunch | expand |
On 2024-10-06 17:49, Daniel P. Smith wrote: > 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. > > Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> > --- > 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/setup.c | 4 +-- > 4 files changed, 28 insertions(+), 28 deletions(-) > > 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 > @@ -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; Why the dereference: *bi->mods[ucode_mod_idx].mod; ? I don't think it should be there. > 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) Why the indent style change? I prefer the original indent and it doesn't seem like the line length matters. And it looks like you restore the indent later. > { > int rc = 0; > > 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); More indent churn. The diff would be smaller without it. > int microcode_update_one(void); > > #endif /* ASM_X86__MICROCODE_H */
On 2024-10-07 17:22, Jason Andryuk wrote: > On 2024-10-06 17:49, Daniel P. Smith wrote: >> 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. >> >> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> >> --- >> 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/setup.c | 4 +-- >> 4 files changed, 28 insertions(+), 28 deletions(-) >> >> 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 > >> @@ -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; > > Why the dereference: *bi->mods[ucode_mod_idx].mod; ? I don't think it > should be there. Oh, the next patch shows ucode_mod is not a pointer, so dereferencing is correct. Sorry for the noise. Regards, Jason
On 10/7/24 17:22, Jason Andryuk wrote: >> @@ -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) > > Why the indent style change? I prefer the original indent and it > doesn't seem like the line length matters. And it looks like you > restore the indent later. This is one I missed unrolling earlier version. Originally was applying the style requested but then realized it was unnecessary churn as by the end of the series the only param would be *bi and was waiting until the last change to move to requested format.
On 10/8/24 08:50, Jason Andryuk wrote: > On 2024-10-07 17:22, Jason Andryuk wrote: >> On 2024-10-06 17:49, Daniel P. Smith wrote: >>> @@ -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; >> >> Why the dereference: *bi->mods[ucode_mod_idx].mod; ? I don't think it >> should be there. > > Oh, the next patch shows ucode_mod is not a pointer, so dereferencing is > correct. Sorry for the noise. No worries. v/r, dps
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 7833b065eff1..1ec29a423061 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/setup.c b/xen/arch/x86/setup.c index 235b4e41f653..48c509b62a4c 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -1382,7 +1382,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 ) { @@ -1939,7 +1939,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. Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> --- 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/setup.c | 4 +-- 4 files changed, 28 insertions(+), 28 deletions(-)