Re: [PATCH v5 19/44] x86/boot: use consumed boot module flag for microcode

["Daniel P. Smith" <dpsmith@apertussolutions.com>]

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