Re: [PATCH v5 4/4] x86/microcode: Disable microcode update handler if DIS_MCU_UPDATE is set

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.06.2023 17:26, Alejandro Vallejo wrote:
> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -847,17 +847,21 @@ 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;
> +        }
>          break;
>  
>      case X86_VENDOR_INTEL:
> -        if ( c->x86 >= 6 )
> -            ucode_ops = intel_ucode_ops;
> +        ucode_ops = intel_ucode_ops;
> +        can_load = intel_can_load_microcode();
>          break;
>      }
>  
> @@ -874,7 +878,7 @@ int __init early_microcode_init(unsigned long *module_map,
>       * mean that they will not accept microcode updates. We take the hint
>       * and ignore the microcode interface in that case.
>       */
> -    if ( this_cpu(cpu_sig).rev == ~0 )
> +    if ( this_cpu(cpu_sig).rev == ~0 || !can_load )

While not too bad, the addition brings code and comment slightly out
of sync.

> --- a/xen/arch/x86/cpu/microcode/intel.c
> +++ b/xen/arch/x86/cpu/microcode/intel.c
> @@ -385,6 +385,19 @@ static struct microcode_patch *cf_check 
> cpu_request_microcode(
>      return patch;
>  }
>  
> +bool __init intel_can_load_microcode(void)
> +{
> +    uint64_t mcu_ctrl;
> +
> +    if ( !cpu_has_mcu_ctrl )
> +        return true;
> +
> +    rdmsrl(MSR_MCU_CONTROL, mcu_ctrl);

While one would hope that feature bit and MSR access working come in
matched pairs, I still wonder whether - just to be on the safe side -
the caller wouldn't better avoid calling here when rev == ~0 (and
hence we won't try to load ucode anyway). I would envision can_load's
initializer to become this_cpu(cpu_sig).rev != ~0, with other logic
adjusted as necessary in early_microcode_init().

Jan