| Message ID | 20241006214956.24339-20-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: > To track if the microcode boot module was loaded, a copy of the boot module is > kept. The size element of this copy is set to zero as the indicator that the > microcode was loaded. A side effect is that the modules have to be rescanned to > find the boot module post-relocation, so the cache copy can be created. > > Use the consumed boot module flag to track the loading of the microcode boot > module. This removes the need to manipulate the boot module size element, no > longer requiring the copy, thus allowing it to be replaced by a reference. As a > result it is no longer necessary to rescan the boot modules after relocation > has occurred. > > Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> > --- > xen/arch/x86/cpu/microcode/core.c | 28 ++++++++++++++-------------- > 1 file changed, 14 insertions(+), 14 deletions(-) > > diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c > index 7bcc17e0ab2f..5b42aad2fdd0 100644 > --- a/xen/arch/x86/cpu/microcode/core.c > +++ b/xen/arch/x86/cpu/microcode/core.c > @@ -826,14 +826,14 @@ int __init microcode_init_cache( > if ( !ucode_ops.apply_microcode ) > return -ENODEV; > > - if ( ucode_scan ) > - /* Need to rescan the modules because they might have been relocated */ > + /* Scan if microcode was not detected earlier */ > + if ( !ucode_mod ) ucode_scan is a user-controlled variable (ucode=scan=$bool), so I think it still needs to be respected. > microcode_scan_module(module_map, bi); > > - if ( ucode_mod.size ) > - rc = early_update_cache(bootstrap_map_bm(&ucode_mod), > - ucode_mod.size); > - else if ( ucode_blob.size ) > + if ( ucode_mod && !(ucode_mod->flags & BOOTMOD_FLAG_X86_CONSUMED) ) > + rc = early_update_cache(bootstrap_map_bm(ucode_mod), > + ucode_mod->size); > + else if ( ucode_mod && ucode_blob.size ) ucode_blob seems independent of ucode_mod, so I don't see why this didn't stay `else if ( ucode_blob.size )` > rc = early_update_cache(ucode_blob.data, ucode_blob.size); > > return rc; > @@ -851,10 +851,10 @@ static int __init early_microcode_update_cpu(void) Regards, Jason
On 10/8/24 11:56, Jason Andryuk wrote: > On 2024-10-06 17:49, Daniel P. Smith wrote: >> To track if the microcode boot module was loaded, a copy of the boot >> module is >> kept. The size element of this copy is set to zero as the indicator >> that the >> microcode was loaded. A side effect is that the modules have to be >> rescanned to >> find the boot module post-relocation, so the cache copy can be created. >> >> Use the consumed boot module flag to track the loading of the >> microcode boot >> module. This removes the need to manipulate the boot module size >> element, no >> longer requiring the copy, thus allowing it to be replaced by a >> reference. As a >> result it is no longer necessary to rescan the boot modules after >> relocation >> has occurred. >> >> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> >> --- >> xen/arch/x86/cpu/microcode/core.c | 28 ++++++++++++++-------------- >> 1 file changed, 14 insertions(+), 14 deletions(-) >> >> diff --git a/xen/arch/x86/cpu/microcode/core.c >> b/xen/arch/x86/cpu/microcode/core.c >> index 7bcc17e0ab2f..5b42aad2fdd0 100644 >> --- a/xen/arch/x86/cpu/microcode/core.c >> +++ b/xen/arch/x86/cpu/microcode/core.c > >> @@ -826,14 +826,14 @@ int __init microcode_init_cache( >> if ( !ucode_ops.apply_microcode ) >> return -ENODEV; >> - if ( ucode_scan ) >> - /* Need to rescan the modules because they might have been >> relocated */ >> + /* Scan if microcode was not detected earlier */ >> + if ( !ucode_mod ) > > ucode_scan is a user-controlled variable (ucode=scan=$bool), so I think > it still needs to be respected. The ucode_scan was introduced due to the complex situation attempting to be addressed. The microcode needs to be loaded earlier before it is possible to safely store a cached copy. Multiboot's module_t had no method of state tracking, identification and consumption. To address this short coming, the early loading made a copy of the module so it could use the mod_end field as a flag without breaking the relocation logic later. And now because it made a copy instead of holding a reference, when the relocation occurs, the mod_start is no longer valid. I am not sure why the scan was a user exposed flag, but with boot_module having identification and state, it is no longer necessary to hold a copy and a reference can now be used. Since it is now a reference, when the relocation occurs, there is no longer a need to rescan because of a relocation. I did leave a rescan if there wasn't microcode detected during the early load. Though, honestly that probably should go since it should be the exact same modules that were scanned during early load. >> microcode_scan_module(module_map, bi); >> - if ( ucode_mod.size ) >> - rc = early_update_cache(bootstrap_map_bm(&ucode_mod), >> - ucode_mod.size); >> - else if ( ucode_blob.size ) >> + if ( ucode_mod && !(ucode_mod->flags & BOOTMOD_FLAG_X86_CONSUMED) ) >> + rc = early_update_cache(bootstrap_map_bm(ucode_mod), >> + ucode_mod->size); >> + else if ( ucode_mod && ucode_blob.size ) > > ucode_blob seems independent of ucode_mod, so I don't see why this > didn't stay `else if ( ucode_blob.size )` From my inspection, looks like that should have been an '||" and not a '&&'. The reason being is that the function will fall back to ucode_mod if ucode_blob is not set. v/r, dps
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c index 7bcc17e0ab2f..5b42aad2fdd0 100644 --- a/xen/arch/x86/cpu/microcode/core.c +++ b/xen/arch/x86/cpu/microcode/core.c @@ -58,7 +58,7 @@ */ #define MICROCODE_UPDATE_TIMEOUT_US 1000000 -static struct boot_module __initdata ucode_mod; +static struct boot_module __initdata *ucode_mod; static signed int __initdata ucode_mod_idx; static bool __initdata ucode_mod_forced; static unsigned int nr_cores; @@ -211,7 +211,7 @@ static void __init microcode_grab_module( !__test_and_clear_bit(ucode_mod_idx, module_map) ) goto scan; bi->mods[ucode_mod_idx].type = BOOTMOD_MICROCODE; - ucode_mod = bi->mods[ucode_mod_idx]; + ucode_mod = &bi->mods[ucode_mod_idx]; scan: if ( ucode_scan ) microcode_scan_module(module_map, bi); @@ -769,10 +769,10 @@ static int __init cf_check microcode_init(void) ucode_blob.size = 0; ucode_blob.data = NULL; } - else if ( ucode_mod.size ) + else if ( ucode_mod && !(ucode_mod->flags & BOOTMOD_FLAG_X86_CONSUMED) ) { bootstrap_map_bm(NULL); - ucode_mod.size = 0; + ucode_mod->flags |= BOOTMOD_FLAG_X86_CONSUMED; } return 0; @@ -826,14 +826,14 @@ int __init microcode_init_cache( if ( !ucode_ops.apply_microcode ) return -ENODEV; - if ( ucode_scan ) - /* Need to rescan the modules because they might have been relocated */ + /* Scan if microcode was not detected earlier */ + if ( !ucode_mod ) microcode_scan_module(module_map, bi); - if ( ucode_mod.size ) - rc = early_update_cache(bootstrap_map_bm(&ucode_mod), - ucode_mod.size); - else if ( ucode_blob.size ) + if ( ucode_mod && !(ucode_mod->flags & BOOTMOD_FLAG_X86_CONSUMED) ) + rc = early_update_cache(bootstrap_map_bm(ucode_mod), + ucode_mod->size); + else if ( ucode_mod && ucode_blob.size ) rc = early_update_cache(ucode_blob.data, ucode_blob.size); return rc; @@ -851,10 +851,10 @@ static int __init early_microcode_update_cpu(void) len = ucode_blob.size; data = ucode_blob.data; } - else if ( ucode_mod.size ) + else if ( ucode_mod && !(ucode_mod->flags & BOOTMOD_FLAG_X86_CONSUMED) ) { - len = ucode_mod.size; - data = bootstrap_map_bm(&ucode_mod); + len = ucode_mod->size; + data = bootstrap_map_bm(ucode_mod); } if ( !data ) @@ -920,7 +920,7 @@ int __init early_microcode_init(unsigned long *module_map, microcode_grab_module(module_map, bi); - if ( ucode_mod.size || ucode_blob.size ) + if ( ucode_mod || ucode_blob.size ) rc = early_microcode_update_cpu(); /*
To track if the microcode boot module was loaded, a copy of the boot module is kept. The size element of this copy is set to zero as the indicator that the microcode was loaded. A side effect is that the modules have to be rescanned to find the boot module post-relocation, so the cache copy can be created. Use the consumed boot module flag to track the loading of the microcode boot module. This removes the need to manipulate the boot module size element, no longer requiring the copy, thus allowing it to be replaced by a reference. As a result it is no longer necessary to rescan the boot modules after relocation has occurred. Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> --- xen/arch/x86/cpu/microcode/core.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)