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:10, Jan Beulich wrote:
> 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).

As expected with patch 5 ops.apply can be NULL without indicating
"fully unavailable". collect_cpu_info being NULL could be taken as
indicator of "fully unavailable", though.

Jan