On 30.04.2024 14:47, 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 is not a newer version. this is required to allow developers to do > ucode loading testing, including the introduction of Intel "min rev" field, > which requires an override mechanism for newer version checks.
Won't "min rev" checking, for being Intel-only, require quite the opposite, i.e. leaving more of the checking to vendor specific code? > Even though > the check for newer is removed at this level, it still exists at higher > common level, where by default only newer version will be processed. > The option to override version check, will be added as part of this patch > series. Please avoid wording like "this patch", "this commit", and all the more "this patch series". Especially this last one will become completely meaningless once part of a commit message in the tree. > --- a/xen/arch/x86/cpu/microcode/amd.c > +++ b/xen/arch/x86/cpu/microcode/amd.c > @@ -384,11 +384,10 @@ static struct microcode_patch *cf_check > cpu_request_microcode( > } > > /* > - * If the new ucode covers current CPU, compare ucodes and store > the > - * one with higher revision. > + * If the microcode covers current CPU, then store its > + * revision. > */ Nit: This can become a single line comment now, can't it? (Again then in Intel code.) > --- 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 == MIS_UCODE ) > return -EINVAL; I continue to be in trouble with this change, despite the v3 adjustment you make: If this is needed here, why would a similar change not be needed for AMD? Plus the original question remains: Is this actually valid to be changed right here? IOW despite the somewhat improved patch description I'm still having difficulty identifying which vendor-independent check this is redundant with. As opposed to the AMD change further up and ... > @@ -355,11 +354,10 @@ static struct microcode_patch *cf_check > cpu_request_microcode( > break; > > /* > - * If the new update covers current CPU, compare updates and store > the > - * one with higher revision. > + * If the microcode covers current CPU, then store its > + * revision. > */ > - if ( (microcode_update_match(mc) != MIS_UCODE) && > - (!saved || compare_revisions(saved->rev, mc->rev) == NEW_UCODE) > ) > + if ( (microcode_update_match(mc) != MIS_UCODE) && !saved ) > saved = mc; ... this one, where I can see that they are about caching of ucode blobs, which looks to be dealt with by cache maintenance logic in microcode_update_helper() and microcode_update_cache(). Jan
