On Wed, Jul 05, 2023 at 12:51:47PM +0200, Jan Beulich wrote:
> > --- 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
We only know about the ucode revision after the collect_cpu_info() call,
and we can only make that call after the vendor-specific section that sets
the function pointers up (and calls intel_can_load_microcode()).
One could imagine turning can_load into a function pointer so that its
execution is deferred until after the revision check (and skipped
altogether if `rev==~0`).
Alejandro
