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
