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>
> @@ -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.
Jan
