Re: [PATCH 1/5] x86: Remove x86 low level version check of microcode

[Fouad Hilly <fouad.hilly@xxxxxxxxx>]

On Mon, Apr 8, 2024 at 10:05 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 05.04.2024 14:11, Fouad Hilly wrote:
> > Remove microcode version check at Intel and AMD Level.
> > Microcode version check will be at higher and common level.
>
> "will be" reads as if you're removing logic here, to introduce some 
> replacement
> later. If so, that's going to be a transient regression, which needs avoiding.
> Indeed ...
>
Higher level at core.c already does version checks, by removing the
check from low level, higher level "will be" the place.
I will update the description.

> > --- a/xen/arch/x86/cpu/microcode/amd.c
> > +++ b/xen/arch/x86/cpu/microcode/amd.c
> > @@ -383,12 +383,8 @@ static struct microcode_patch *cf_check 
> > cpu_request_microcode(
> >                  goto skip;
> >              }
> >
> > -            /*
> > -             * If the new ucode covers current CPU, compare ucodes and 
> > store the
> > -             * one with higher revision.
> > -             */
> > -            if ( (microcode_fits(mc->patch) != MIS_UCODE) &&
> > -                 (!saved || (compare_header(mc->patch, saved) == 
> > NEW_UCODE)) )
> > +            /* If the provided ucode covers current CPU, then store its 
> > revision. */
> > +            if ( (microcode_fits(mc->patch) != MIS_UCODE) && !saved )
> >              {
> >                  saved = mc->patch;
> >                  saved_size = mc->len;
>
> ... this looks like a logic change to me, with there not being anything
> similar in common code afaics. Am I overlooking anything?
>
The code still checks if it is the current CPU; however, I removed the
check for "NEW_CODE" as a prerequisite for storing the firmware
revision.
If there is any error at this stage (CPU specific) an error will be
propagated to a higher level and dealt with.

> > --- a/xen/arch/x86/cpu/microcode/intel.c
> > +++ b/xen/arch/x86/cpu/microcode/intel.c
> > @@ -294,8 +294,7 @@ static int cf_check apply_microcode(const struct 
> > microcode_patch *patch)
> >
> >      result = microcode_update_match(patch);
> >
> > -    if ( result != NEW_UCODE &&
> > -         !(opt_ucode_allow_same && result == SAME_UCODE) )
> > +    if ( result != NEW_UCODE && result != SAME_UCODE )
> >          return -EINVAL;
>
> I'm afraid I can't relate this adjustment with title and description of
> the patch.
>
I will update the patch description

> Jan

Thanks,

Fouad