Mailing List Archive: [PATCH 1/5] x86: Remove x86 low level version check of microcode

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

Apr 5, 2024, 5:11 AM

Post #1 of 3 (14 views)

Remove microcode version check at Intel and AMD Level.
Microcode version check will be at higher and common level.

Signed-off-by: Fouad Hilly <fouad.hilly@cloud.com>
---
xen/arch/x86/cpu/microcode/amd.c | 8 ++------
xen/arch/x86/cpu/microcode/intel.c | 11 +++--------
2 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index 75fc84e445ce..4f805f662701 100644
--- 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;
diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index 060c529a6e5d..e65c02a57987 100644
--- 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;

wbinvd();
@@ -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;

buf += blob_size;
--
2.42.0

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

jbeulich at suse

Apr 8, 2024, 2:05 AM

Post #2 of 3 (10 views)

Permalink

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

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

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

Jan

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

fouad.hilly at cloud

Apr 8, 2024, 5:13 AM

Post #3 of 3 (10 views)

Permalink

On Mon, Apr 8, 2024 at 10:05?AM Jan Beulich <jbeulich@suse.com> 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