Re: [PATCH v2 4/4] x86/microcode: Prevent attempting updates if DIS_MCU_LOAD is set

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 05/06/2023 6:08 pm, Alejandro Vallejo wrote:
> diff --git a/xen/arch/x86/cpu/microcode/core.c 
> b/xen/arch/x86/cpu/microcode/core.c
> index 4f60d96d98..a4c123118b 100644
> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -871,6 +885,15 @@ int __init early_microcode_init(unsigned long 
> *module_map,
>           * present.
>           */
>          ucode_ops = intel_ucode_ops;
> +
> +        /*
> +         * In the case where microcode updates are blocked by the
> +         * DIS_MCU_LOAD bit we can still read the microcode version even if
> +         * we can't change it.
> +         */
> +        if ( !this_cpu_can_install_update() )
> +            ucode_ops = (struct microcode_ops){ .collect_cpu_info =
> +                                    intel_ucode_ops.collect_cpu_info };

I don't see how this (the logic in this_cpu_can_install_update()) can
work, as ...

>          break;
>      }
>  
> @@ -900,6 +923,10 @@ int __init early_microcode_init(unsigned long 
> *module_map,
>      if ( ucode_mod.mod_end || ucode_blob.size )
>          rc = early_microcode_update_cpu();
>  
> +    /*
> +     * We just updated microcode so we must reload the boot_cpu_data bits
> +     * we read before because they might be stale after the updata.
> +     */
>      early_read_cpuid_7d0();
>  
>      /*

... MSR_ARCH_CAPS is read out-of-context down here.

In hindsight, I think swapping patch 2 and 3 might be wise.  The rev ==
~0 case doesn't need any of the cpu_has_* shuffling, and it already
starts to build up the if/else chain of cases where we decide to clobber
the apply_microcode() hook.

The call to this_cpu_can_install_update() should be lower down.  In
principle it's not Intel-specific.

~Andrew