| Message ID | 20240405121128.260493-2-fouad.hilly@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/xen-ucode: Introduce --force option | expand |
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
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
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;
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(-)