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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.06.2023 19:08, Alejandro Vallejo wrote:
> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -749,11 +749,12 @@ __initcall(microcode_init);
>  /* Load a cached update to current cpu */
>  int microcode_update_one(void)
>  {
> +    if ( ucode_ops.collect_cpu_info )
> +        alternative_vcall(ucode_ops.collect_cpu_info);
> +
>      if ( !ucode_ops.apply_microcode )
>          return -EOPNOTSUPP;
>  
> -    alternative_vcall(ucode_ops.collect_cpu_info);
> -
>      return microcode_update_cpu(NULL);
>  }

This adjustment (and related logic below) doesn't really fit the purpose
that the title states. I wonder if the two things wouldn't better be
split.

> @@ -849,12 +850,25 @@ static void __init early_read_cpuid_7d0(void)
>              = cpuid_count_edx(7, 0);
>  }
>  
> +static bool __init this_cpu_can_install_update(void)
> +{
> +    uint64_t mcu_ctrl;
> +
> +    if ( !cpu_has_mcu_ctrl )
> +        return true;
> +
> +    rdmsrl(MSR_MCU_CONTROL, mcu_ctrl);
> +    return !(mcu_ctrl & MCU_CONTROL_DIS_MCU_LOAD);
> +}

As Andrew says, in principle AMD could have a means as well. Surely that
would be a different one, so I wonder if this shouldn't be a new hook.

> @@ -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 };

Similarly I'm not happy to see a direct reference to some vendor specific
field. I think it wants to be the hook to return such an override struct.

Jan