Re: [PATCH v6 1/3] x86/microcode: Ignore microcode loading interface for revision = -1

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

On Tue, Jul 25, 2023 at 08:40:31AM +0200, Jan Beulich wrote:
> On 24.07.2023 18:52, Alejandro Vallejo wrote:
> > Some hypervisors report ~0 as the microcode revision to mean "don't issue
> > microcode updates". Ignore the microcode loading interface in that case.
> > 
> > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
> > Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Hmm.
> 
> > --- a/xen/arch/x86/cpu/microcode/core.c
> > +++ b/xen/arch/x86/cpu/microcode/core.c
> > @@ -867,10 +867,23 @@ int __init early_microcode_init(unsigned long 
> > *module_map,
> >          return -ENODEV;
> >      }
> >  
> > -    microcode_grab_module(module_map, mbi);
> > -
> >      ucode_ops.collect_cpu_info();
> >  
> > +    /*
> > +     * Some hypervisors deliberately report a microcode revision of -1 to
> > +     * mean that they will not accept microcode updates. We take the hint
> > +     * and ignore the microcode interface in that case.
> > +     */
> > +    if ( this_cpu(cpu_sig).rev == ~0 )
> > +    {
> > +        printk(XENLOG_INFO "Microcode loading disabled due to: %s",
> 
> While we have tentatively agreed to adjust what _will_ be emitted by
> default (subject to suitable justification in that change's
> description), such a patch is yet to be sent.
Ugh, that was indeed mistagged. Sorry about that. I touched several parts
of this patch shortly before sending it and it crept in by mistake.

> As it stands this message
> will be invisible by default.
Arguably, that's not necessarily a bad thing. The fact that microcode
cannot be updated is expected behaviour and it makes little sense to warn
about it. If only because they should already be aware of it through their
agreement with their provider.

The case I can think of where a warning would be sensible is where the
system has a microcode blob more recent than the currently installed
revision. This is something the admin may want to be aware of in order to
pester their provider for updates. In the common case the machine won't
even need such an update, so sending unconditional warnings per boot seems
unwarranted.

> 
> > +                           "HW toggle");
> 
> With the comment talking about hypervisors, what is this string supposed
> to tell an observer of the message in a log file?
> 
> Jan
Nothing good. As you noticed later, that's the wrong substring off the last
patch and should've been "rev = ~0"

Thanks,
Alejandro