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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.04.2024 11:15, Fouad Hilly wrote:
> Update microcode version check at Intel and AMD Level by:
> Preventing the low level code from sending errors if the microcode
> version provided is not a newer version.

And why is this change (a) wanted and (b) correct?

> Other errors will be sent like before.
> When the provided microcode version is the same as the current one, code
> to point to microcode provided.

I'm afraid I can't interpret this sentence.

> Microcode version check happens at higher and common level in core.c.
> Keep all the required code at low level that checks for signature and CPU 
> compatibility
> 
> [v2]
> Update message description to better describe the changes

This belongs ...

> Signed-off-by: Fouad Hilly <fouad.hilly@xxxxxxxxx>
> ---

... below the separator.

> --- 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;
> --- 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;

Unlike the other two adjustments this one results in still permitting
only same-or-newer. How does this fit with the AMD change above and
the other Intel change ...

> @@ -354,12 +353,8 @@ static struct microcode_patch *cf_check 
> cpu_request_microcode(
>          if ( error )
>              break;
>  
> -        /*
> -         * If the new update covers current CPU, compare updates and store 
> the
> -         * one with higher revision.
> -         */
> -        if ( (microcode_update_match(mc) != MIS_UCODE) &&
> -             (!saved || compare_revisions(saved->rev, mc->rev) == NEW_UCODE) 
> )
> +        /* If the provided ucode covers current CPU, then store its 
> revision. */
> +        if ( (microcode_update_match(mc) != MIS_UCODE) && !saved )
>              saved = mc;

... here?

Jan