Re: [PATCH] x86/ucode: Further fixes to identify "ucode already up to date"

["Roger Pau Monné" <roger.pau@citrix.com>]

On Thu, May 16, 2024 at 01:30:21PM +0100, Andrew Cooper wrote:
> On 16/05/2024 12:50 pm, Roger Pau Monné wrote:
> > On Thu, May 16, 2024 at 12:31:03PM +0100, Andrew Cooper wrote:
> >> When the revision in hardware is newer than anything Xen has to hand,
> >> 'microcode_cache' isn't set up.  Then, `xen-ucode` initiates the update
> >> because it doesn't know whether the revisions across the system are symmetric
> >> or not.  This involves the patch getting all the way into the
> >> apply_microcode() hooks before being found to be too old.
> >>
> >> This is all a giant mess and needs an overhaul, but in the short term simply
> >> adjust the apply_microcode() to return -EEXIST.
> >>
> >> Also, unconditionally print the preexisting microcode revision on boot.  It's
> >> relevant information which is otherwise unavailable if Xen doesn't find new
> >> microcode to use.
> >>
> >> Fixes: 648db37a155a ("x86/ucode: Distinguish "ucode already up to date"")
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> ---
> >> CC: Jan Beulich <JBeulich@suse.com>
> >> CC: Roger Pau Monné <roger.pau@citrix.com>
> >> CC: Fouad Hilly <fouad.hilly@cloud.com>
> >>
> >> Sorry Fouad, but this collides with your `--force` series once again.
> >> Hopefully it might make things fractionally easier.
> >>
> >> Background: For 06-55-04 (Skylake server, stepping 4 specifically), there's a
> >> recent production firmware update which has a newer microcode revision than
> >> exists in the Intel public microcode repository.  It's causing a mess in our
> >> automated testing, although it is finding good bugs...
> >> ---
> >>  xen/arch/x86/cpu/microcode/amd.c   | 7 +++++--
> >>  xen/arch/x86/cpu/microcode/core.c  | 2 ++
> >>  xen/arch/x86/cpu/microcode/intel.c | 7 +++++--
> >>  3 files changed, 12 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
> >> index 17e68697d5bf..f76a563c8b84 100644
> >> --- a/xen/arch/x86/cpu/microcode/amd.c
> >> +++ b/xen/arch/x86/cpu/microcode/amd.c
> >> @@ -222,12 +222,15 @@ static int cf_check apply_microcode(const struct microcode_patch *patch)
> >>      uint32_t rev, old_rev = sig->rev;
> >>      enum microcode_match_result result = microcode_fits(patch);
> >>  
> >> +    if ( result == MIS_UCODE )
> >> +        return -EINVAL;
> >> +
> >>      /*
> >>       * Allow application of the same revision to pick up SMT-specific changes
> >>       * even if the revision of the other SMT thread is already up-to-date.
> >>       */
> >> -    if ( result != NEW_UCODE && result != SAME_UCODE )
> >> -        return -EINVAL;
> >> +    if ( result == OLD_UCODE )
> >> +        return -EEXIST;
> > Won't it be simpler to just add this check ahead of the existing one,
> > so that you can leave the code as-is, iow:
> >
> >     if ( result == OLD_UCODE )
> >         return -EEXIST;
> >
> >     /*
> >      * Allow application of the same revision to pick up SMT-specific changes
> >      * even if the revision of the other SMT thread is already up-to-date.
> >      */
> >     if ( result != NEW_UCODE && result != SAME_UCODE )
> >         return -EINVAL;
> >
> > Thanks, Roger.
> 
> Not really, no.  That still leaves this piece of logic which is
> misleading IMO.
> 
> MIS_UCODE is the only -EINVAL worthy case.
> 
> Every other *_UCODE constant needs to be 0 or -EEXIST, depending on
> allow-same/--force.

OK, my main concern was the previous logic wouldn't allow a newly
introduced state to get past the return -EINVAL, while the new logic
could possibly allow it to pass through.

I don't think adding states is that common, and if you prefer it that
way it's fine.

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.