Re: [PATCH v2 1/5] x86: Update x86 low level version check of microcode
On Thu, Apr 18, 2024 at 11:05 AM Andrew Cooper wrote: > > On 16/04/2024 10:15 am, 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. 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. > > 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 > > > > Signed-off-by: Fouad Hilly > > --- > > > As a general note, your v2/v3/etc changelog needs to go under this --- line. Noted. > > ~Andrew Thanks, Fouad
Re: [PATCH v2 1/5] x86: Update x86 low level version check of microcode
On Mon, Apr 22, 2024 at 3:09 PM Jan Beulich wrote:
>
> 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?
I will improve the message description to cover more details and reasoning.
>
> > 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.
"provided" is the firmware presented\provided to the code for firmware
flashing. As above, I will provide more comprehensive description.
>
> > 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
> > ---
>
> ... 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 ...
To be fixed in V3
>
> > @@ -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?
I assume this refers to the previous comment? Which will be fixed in V3
>
> Jan
Thanks,
Fouad
Re: [PATCH v2 1/5] x86: Update x86 low level version check of microcode
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
> ---
... 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
Re: [PATCH v2 1/5] x86: Update x86 low level version check of microcode
On 16/04/2024 10:15 am, 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. 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. > 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 > > Signed-off-by: Fouad Hilly > --- As a general note, your v2/v3/etc changelog needs to go under this --- line. ~Andrew
