Re: [PATCH] x86/ucode: Further fixes to identify "ucode already up to date"

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/05/2024 12:44 pm, Jan Beulich wrote:
> On 16.05.2024 13:31, Andrew Cooper wrote:
>> When the revision in hardware is newer than anything Xen has to hand,
>> 'microcode_cache' isn't set up.  Then, `xen-ucode` initiates the update
>> because it doesn't know whether the revisions across the system are symmetric
>> or not.  This involves the patch getting all the way into the
>> apply_microcode() hooks before being found to be too old.
>>
>> This is all a giant mess and needs an overhaul, but in the short term simply
>> adjust the apply_microcode() to return -EEXIST.
>>
>> Also, unconditionally print the preexisting microcode revision on boot.  It's
>> relevant information which is otherwise unavailable if Xen doesn't find new
>> microcode to use.
> Since you do this for the BSP only, I'm okay with that. Doing this for all
> CPUs would have added too much verbosity imo, and I would then have asked
> to log the pre-existing revision only when no update would be done by us.
>
>> Fixes: 648db37a155a ("x86/ucode: Distinguish "ucode already up to date"")
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks.

> with one small request related to the remark above:
>
>> --- a/xen/arch/x86/cpu/microcode/core.c
>> +++ b/xen/arch/x86/cpu/microcode/core.c
>> @@ -881,6 +881,8 @@ int __init early_microcode_init(unsigned long 
>> *module_map,
>>  
>>      ucode_ops.collect_cpu_info();
>>  
>> +    printk(XENLOG_INFO "Boot microcode revision: 0x%08x\n", 
>> this_cpu(cpu_sig).rev);
> Can this please be "BSP" or "Boot CPU" instead of just "Boot", to clarify
> which CPU's information this is? I'm pretty sure you too have hit systems
> where firmware doesn't update all cores.

I'll switch to BSP.

I have further plans (4.20 at this point) to reduce logspam.

* The AP boot path should warn if it finds a revision which isn't the
bsp_orig version, cached version, or thread-0 revision.  Most of what is
printed right now is expected and normal in the system.

* Late load should print once, not once per CPU.

~Andrew