| Message ID | 20240416091546.11622-2-fouad.hilly@cloud.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/xen-ucode: Introduce --force option | expand |
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 <fouad.hilly@cloud.com> > --- As a general note, your v2/v3/etc changelog needs to go under this --- line. ~Andrew
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@cloud.com> > --- ... 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
On Mon, Apr 22, 2024 at 3:09 PM Jan Beulich <jbeulich@suse.com> 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 <fouad.hilly@cloud.com> > > --- > > ... 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
On Thu, Apr 18, 2024 at 11:05 AM Andrew Cooper <andrew.cooper3@citrix.com> 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 <fouad.hilly@cloud.com> > > --- > > > As a general note, your v2/v3/etc changelog needs to go under this --- line. Noted. > > ~Andrew 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;
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 <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(-)