Re: [PATCH v3 5/5] x86/microcode: Disable microcode update handler if DIS_MCU_UPDATE is set

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

On Tue, Jun 20, 2023 at 11:51:00AM +0200, Jan Beulich wrote:
> On 15.06.2023 17:48, Alejandro Vallejo wrote:
> > --- a/xen/arch/x86/cpu/common.c
> > +++ b/xen/arch/x86/cpu/common.c
> > @@ -352,6 +352,11 @@ void __init early_cpu_init(void)
> >                         &c->x86_capability[FEATURESET_7c0],
> >                         &c->x86_capability[FEATURESET_7d0]);
> >  
> > +           if (test_bit(X86_FEATURE_ARCH_CAPS, c->x86_capability))
> > +                   rdmsr(MSR_ARCH_CAPABILITIES,
> > +                         c->x86_capability[FEATURESET_m10Al],
> > +                         c->x86_capability[FEATURESET_m10Ah]);
> 
> Is this still going to be needed if early_microcode_init() uniformly
> does this, for things to be correct for tsx_init() (as pointed out
> in the other patch)?
Yes. This is needed so MSR_ARCH_CAPS are available in order to check
DIS_MCU_LOAD before the microcode update itself. early_cpu_init() does the
read before the microcode update, while early_microcode_init() refreshes
the fields that might have been modified after the update AND are needed
before the general CPU detection logic.

> 
> > --- a/xen/arch/x86/cpu/microcode/intel.c
> > +++ b/xen/arch/x86/cpu/microcode/intel.c
> > @@ -387,8 +387,22 @@ static struct microcode_patch *cf_check 
> > cpu_request_microcode(
> >  
> >  void __init intel_get_ucode_ops(struct microcode_ops *ops)
> >  {
> > +    uint64_t mcu_ctrl;
> > +
> >      ops->cpu_request_microcode = cpu_request_microcode;
> >      ops->collect_cpu_info      = collect_cpu_info;
> >      ops->apply_microcode       = apply_microcode;
> >      ops->compare_patch         = compare_patch;
> > +
> > +    if ( cpu_has_mcu_ctrl )
> > +    {
> > +        rdmsrl(MSR_MCU_CONTROL, mcu_ctrl);
> > +        /*
> > +         * If DIS_MCU_LOAD is set applying microcode updates won't work. We
> > +         * can still query the current version and things like that, so
> > +         * we'll leave the other handlers in place.
> > +         */
> > +        if ( mcu_ctrl & MCU_CONTROL_DIS_MCU_LOAD )
> > +            ops->apply_microcode = NULL;
> > +    }
> 
> While this won't address Andrew's request (afaict), but only the
> cf_clobber requirement, I think you want to drop removing of the struct
> instances from patch 2, return pointers from the new per-vendor
> functions, and simply introduce another static instance of struct
> microcode_ops here, with the one hook simply left unset. This lives in
> init data, so the size increase is of no major concern.
> 
> Jan
As mentioned in other email. This is my bad for not having understood the
ultimate readon for cf_clobber. I'll just leave the structs as is.

Alejandro