Re: [PATCH v3 1/5] x86/microcode: Allow reading microcode revision even if it can't be updated

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.06.2023 18:06, Andrew Cooper wrote:
> On 19/06/2023 4:58 pm, Jan Beulich wrote:
>> On 19.06.2023 17:49, Andrew Cooper wrote:
>>> On 15/06/2023 4:48 pm, Alejandro Vallejo wrote:
>>>> diff --git a/xen/arch/x86/cpu/microcode/core.c 
>>>> b/xen/arch/x86/cpu/microcode/core.c
>>>> index e65af4b82e..df7e1df870 100644
>>>> --- a/xen/arch/x86/cpu/microcode/core.c
>>>> +++ b/xen/arch/x86/cpu/microcode/core.c
>>>> @@ -750,11 +750,12 @@ __initcall(microcode_init);
>>>> @@ -860,6 +861,9 @@ int __init early_microcode_init(unsigned long 
>>>> *module_map,
>>>>          break;
>>>>      }
>>>>  
>>>> +    if ( ucode_ops.collect_cpu_info )
>>>> +        ucode_ops.collect_cpu_info();
>>>> +
>>> I still think this wants to be the other side of "ucode loading fully
>>> unavailable", just below.
>>>
>>> I'm confident it will result in easier-to-follow logic.
>> Yet wouldn't that be against the purpose of obtaining the ucode
>> revision even if loading isn't possible?
> 
> No.  The logical order of checks is:
> 
> if ( !ops.apply )
>     return; // Fully unavailable
> 
> collect_cpu_info();
> 
> if ( rev == ~0 || !can_load )
>     return; // Loading unavailable
> 
> // search for blob to load
> 
> 
> This form has fewer misleading NULL checks, and doesn't get
> printk(XENLOG_WARNING "Microcode loading not available\n"); after
> successful microcode actions.

But from the earlier version and from what I've seen in patches 1-4
so far, I expect patch 5 will introduce a case with ops.apply being
NULL but ops.collect being non-NULL. Otherwise I don't see why patch
2 does what it does (sacrificing cf_clobber, albeit likely not really
intentionally).

Jan