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

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

On Mon, Jun 12, 2023 at 07:54:00PM +0100, Andrew Cooper wrote:
> On 05/06/2023 6:08 pm, Alejandro Vallejo wrote:
> > diff --git a/xen/arch/x86/cpu/microcode/core.c 
> > b/xen/arch/x86/cpu/microcode/core.c
> > index 4f60d96d98..a4c123118b 100644
> > --- a/xen/arch/x86/cpu/microcode/core.c
> > +++ b/xen/arch/x86/cpu/microcode/core.c
> > @@ -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 };
> 
> I don't see how this (the logic in this_cpu_can_install_update()) can
> work, as ...
> 
> >          break;
> >      }
> >  
> > @@ -900,6 +923,10 @@ int __init early_microcode_init(unsigned long 
> > *module_map,
> >      if ( ucode_mod.mod_end || ucode_blob.size )
> >          rc = early_microcode_update_cpu();
> >  
> > +    /*
> > +     * We just updated microcode so we must reload the boot_cpu_data bits
> > +     * we read before because they might be stale after the updata.
> > +     */
> >      early_read_cpuid_7d0();
> >  
> >      /*
> 
> ... MSR_ARCH_CAPS is read out-of-context down here.
Seeing how the minimal CPU state is read in early_cpu_init() I'll stash the
read to MSR_ARCH_CAPS there too. Then it's a matter of reloading
potentially changing leafs/MSRs after the update, which is a lot clearer
rather than adding reads/writes ad-hoc elsewhere.

Alejandro