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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Fouad Hilly <fouad.hilly@xxxxxxxxx>
> 
> 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.