On 26/10/2023 8:45 am, Jan Beulich wrote:
> On 25.10.2023 20:06, Andrew Cooper wrote:
>> --- a/xen/arch/x86/cpu/microcode/core.c
>> +++ b/xen/arch/x86/cpu/microcode/core.c
>> @@ -847,25 +847,19 @@ int __init early_microcode_init(unsigned long
>> *module_map,
>> {
>> const struct cpuinfo_x86 *c = &boot_cpu_data;
>> int rc = 0;
>> - bool can_load = false;
>>
>> switch ( c->x86_vendor )
>> {
>> case X86_VENDOR_AMD:
>> - if ( c->x86 >= 0x10 )
>> - {
>> - ucode_ops = amd_ucode_ops;
>> - can_load = true;
>> - }
>> + ucode_probe_amd(&ucode_ops);
>> break;
>>
>> case X86_VENDOR_INTEL:
>> - ucode_ops = intel_ucode_ops;
>> - can_load = intel_can_load_microcode();
>> + ucode_probe_intel(&ucode_ops);
>> break;
>> }
>>
>> - if ( !ucode_ops.apply_microcode )
>> + if ( !ucode_ops.collect_cpu_info )
>> {
>> printk(XENLOG_INFO "Microcode loading not available\n");
>> return -ENODEV;
>> @@ -882,10 +876,10 @@ int __init early_microcode_init(unsigned long
>> *module_map,
>> *
>> * Take the hint in either case and ignore the microcode interface.
>> */
>> - if ( this_cpu(cpu_sig).rev == ~0 || !can_load )
>> + if ( !ucode_ops.apply_microcode || this_cpu(cpu_sig).rev == ~0 )
> Here ucode_ops.apply_microcode simply replaces can_load, as expected.
>
>> {
>> printk(XENLOG_INFO "Microcode loading disabled due to: %s\n",
>> - can_load ? "rev = ~0" : "HW toggle");
>> + ucode_ops.apply_microcode ? "HW toggle" : "rev = ~0");
> Here, otoh, you invert sense, which I don't think is right. With 2nd
> and 3rd operands swapped back
> Reviewed-by: Jan Beulich <jbeul...@suse.com>
Oh, right. This did get adjusted several times. I'll fix.
It's a weird construct anyway, and it gets cleaned up differently by
some of my later work.
>
>> @@ -398,9 +398,17 @@ bool __init intel_can_load_microcode(void)
>> return !(mcu_ctrl & MCU_CONTROL_DIS_MCU_LOAD);
>> }
>>
>> -const struct microcode_ops __initconst_cf_clobber intel_ucode_ops = {
>> +static const struct microcode_ops __initconst_cf_clobber intel_ucode_ops = {
>> .cpu_request_microcode = cpu_request_microcode,
>> .collect_cpu_info = collect_cpu_info,
>> .apply_microcode = apply_microcode,
>> .compare_patch = compare_patch,
>> };
>> +
>> +void __init ucode_probe_intel(struct microcode_ops *ops)
>> +{
>> + *ops = intel_ucode_ops;
>> +
>> + if ( !can_load_microcode() )
>> + ops->apply_microcode = NULL;
>> +}
> I was wondering why you didn't use the return value to supply the pointer
> to the caller, but this override explains it.
Yes. The other option was to export (in private.h at least) the root
ucode_ops, which I decided against in the end.
I also toyed with the idea of having the probe functions return int, so
we could get EOPNOTSUPP or so in the compiled-out case, but that's
almost completely redundant with clobbering the hook, and it's added
complexity for somethign that only randconfig is going to care about.
The only thing I'm not entirely happy about is the volume of the
ifdefary for these ucode_probe() functions in the following patch, but
it's the least invasive option overall IMO.
~Andrew
