Re: [PATCH] x86/microcode: Prevent attempting updates known to fail

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 31/05/2023 6:51 pm, Alejandro Vallejo wrote:
> diff --git a/xen/arch/x86/cpu/microcode/core.c 
> b/xen/arch/x86/cpu/microcode/core.c
> index cd456c476f..e507945932 100644
> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -697,6 +697,17 @@ static long cf_check microcode_update_helper(void *data)
>      return ret;
>  }
>  
> +static bool 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);
> +}
> +
>  int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len)
>  {
>      int ret;
> @@ -708,6 +719,22 @@ int microcode_update(XEN_GUEST_HANDLE(const_void) buf, 
> unsigned long len)
>      if ( !ucode_ops.apply_microcode )
>          return -EINVAL;
>  
> +    if ( !this_cpu_can_install_update() )
> +    {
> +        /*
> +         * This CPU can't install microcode, so it makes no sense to try to
> +         * go on. We're implicitly trusting firmware sanity in that all
> +         * CPUs are expected to have a homogeneous setting. If, for some
> +         * reason, another CPU happens to be locked down when this one
> +         * isn't then unpleasantness will follow. In particular, some CPUs
> +         * will be updated while others will not. A very stern message will
> +         * be displayed in xl-dmesg that case, strongly advising to reboot 
> the
> +         * machine.
> +         */
> +        printk("WARNING: microcode not installed due to DIS_MCU_LOAD=1");
> +        return -EACCES;
> +    }

I had something else in mind here.  Right now, this will read
MSR_MCU_CONTROL and emit a printk() on every microcode load, which will
be every AP, and every time the user uses the xen-ucode tool.

Instead, I recommend the following:

1) One patch moving the early-cpuid/msr read from tsx_init() into
early_microcode_init(), adjusting the comment as it goes.  No point
duplicating that logic, and we need it earlier on boot now.
2) This patch, adjusting early_microcode_init() only.  Have a printk()
saying "microcode loading disabled by firmware" and avoid filling in
ucode_ops.  Every other part of ucode handling understands "loading not
available".

In terms of the commit message, you should call out the usecase
explicitly.  This feature is intended for baremetal clouds where the
platform owner doesn't trust the tenant to choose the microcode version
in use.

Also, I'm really not sure what your 3rd paragraph is trying to say.

~Andrew