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

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

On Tue, Jun 13, 2023 at 08:57:06AM +0200, Jan Beulich wrote:
> 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.
Well, before this patch collect_cpu_info() and apply_microcode() were both
set or cleared together , whereas after this patch that's no longer the
case (hence the change). I can submit it standalone patch earlier in v3,
seeing as it does no harm. The commit message could also do with better
wording.

> 
> > @@ -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
I'm moving things around a little in v3. We'll have accessors for both AMD
and Intel that provide the right thing, encapsulating vendor-specifics
(including family checks) inside ${VENDOR}.c

Alejandro