| Message ID | 20241112211915.1473121-4-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/ucode: Simplify/fix loading paths further | expand |
On 12.11.2024 22:19, Andrew Cooper wrote: > @@ -199,8 +198,8 @@ static bool microcode_fits_cpu(const struct microcode_patch *patch) > return equiv.id == patch->processor_rev_id; > } > > -static enum microcode_match_result cf_check compare_patch( > - const struct microcode_patch *new, const struct microcode_patch *old) > +static int cf_check compare_patch( > + const struct microcode_patch *old, const struct microcode_patch *new) > { Let's hope we won't screw up a backport because of this swapping. I'd like to ask to at least consider renaming at least the functions, perhaps also the hook pointer, perhaps simply by switching from singular to plural. This would then also avoid reviewers like me to go hunt for all uses of the function/hook, in an attempt to make sure none was left out when converting. > @@ -54,11 +47,17 @@ struct microcode_ops { > unsigned int flags); > > /* > - * Given two patches, are they both applicable to the current CPU, and is > - * new a higher revision than old? > + * Given a current patch, and a proposed new patch, order them based on revision. > + * > + * This operation is not necessarily symmetrical. In some cases, a debug > + * "new" patch will always considered to be newer, on the expectation that > + * whomever is using debug patches knows exactly what they're doing. > */ > - enum microcode_match_result (*compare_patch)( > - const struct microcode_patch *new, const struct microcode_patch *old); > +#define OLD_UCODE -1 Nit: I'm pretty sure Misra wants parentheses here. Preferably with both (mechanical) adjustments: Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On 14/11/2024 11:41 am, Jan Beulich wrote: > On 12.11.2024 22:19, Andrew Cooper wrote: >> @@ -199,8 +198,8 @@ static bool microcode_fits_cpu(const struct microcode_patch *patch) >> return equiv.id == patch->processor_rev_id; >> } >> >> -static enum microcode_match_result cf_check compare_patch( >> - const struct microcode_patch *new, const struct microcode_patch *old) >> +static int cf_check compare_patch( >> + const struct microcode_patch *old, const struct microcode_patch *new) >> { > Let's hope we won't screw up a backport because of this swapping. I wasn't going to start thinking about backports until the code gets into a better state. But if backports do happen, it will be all-or-nothing. This code is far too tangled. That said, in this specific case, the only thing that would go wrong is with Intel debug patches. Even I've only had a handful of those in the past 8 years. > I'd like to ask to at least consider renaming at least the functions, > perhaps also the hook pointer, perhaps simply by switching from singular > to plural. This would then also avoid reviewers like me to go hunt for > all uses of the function/hook, in an attempt to make sure none was left > out when converting. In the other series I've paused for a while, I have renamed some hooks (along with related cleanup), but I'm undecided on this one. One option is cmp(), or perhaps compare(). But, it occurs to me, another option would be is_newer(). We always care about the operation one way around. > >> @@ -54,11 +47,17 @@ struct microcode_ops { >> unsigned int flags); >> >> /* >> - * Given two patches, are they both applicable to the current CPU, and is >> - * new a higher revision than old? >> + * Given a current patch, and a proposed new patch, order them based on revision. >> + * >> + * This operation is not necessarily symmetrical. In some cases, a debug >> + * "new" patch will always considered to be newer, on the expectation that >> + * whomever is using debug patches knows exactly what they're doing. >> */ >> - enum microcode_match_result (*compare_patch)( >> - const struct microcode_patch *new, const struct microcode_patch *old); >> +#define OLD_UCODE -1 > Nit: I'm pretty sure Misra wants parentheses here. Oh yes, so it does. Rule 20.7 apparently. Fine. > Preferably with both (mechanical) adjustments: > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks. ~Andrew
diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c index 3861fec6565a..366c8c59e93a 100644 --- a/xen/arch/x86/cpu/microcode/amd.c +++ b/xen/arch/x86/cpu/microcode/amd.c @@ -170,8 +170,7 @@ static bool check_final_patch_levels(const struct cpu_signature *sig) return false; } -static enum microcode_match_result compare_revisions( - uint32_t old_rev, uint32_t new_rev) +static int compare_revisions(uint32_t old_rev, uint32_t new_rev) { if ( new_rev > old_rev ) return NEW_UCODE; @@ -199,8 +198,8 @@ static bool microcode_fits_cpu(const struct microcode_patch *patch) return equiv.id == patch->processor_rev_id; } -static enum microcode_match_result cf_check compare_patch( - const struct microcode_patch *new, const struct microcode_patch *old) +static int cf_check compare_patch( + const struct microcode_patch *old, const struct microcode_patch *new) { /* Both patches to compare are supposed to be applicable to local CPU. */ ASSERT(microcode_fits_cpu(new)); @@ -212,11 +211,10 @@ static enum microcode_match_result cf_check compare_patch( static int cf_check apply_microcode(const struct microcode_patch *patch, unsigned int flags) { - int hw_err; + int hw_err, result; unsigned int cpu = smp_processor_id(); struct cpu_signature *sig = &per_cpu(cpu_sig, cpu); uint32_t rev, old_rev = sig->rev; - enum microcode_match_result result; bool ucode_force = flags & XENPF_UCODE_FORCE; if ( !microcode_fits_cpu(patch) ) diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c index 0cc5daa251e2..05d0d68d8158 100644 --- a/xen/arch/x86/cpu/microcode/core.c +++ b/xen/arch/x86/cpu/microcode/core.c @@ -470,8 +470,7 @@ struct ucode_buf { static long cf_check microcode_update_helper(void *data) { struct microcode_patch *patch = NULL; - enum microcode_match_result result; - int ret; + int ret, result; struct ucode_buf *buffer = data; unsigned int cpu, updated; struct patch_with_flags patch_with_flags; @@ -527,7 +526,7 @@ static long cf_check microcode_update_helper(void *data) spin_lock(µcode_mutex); if ( microcode_cache ) { - result = alternative_call(ucode_ops.compare_patch, patch, microcode_cache); + result = alternative_call(ucode_ops.compare_patch, microcode_cache, patch); if ( result != NEW_UCODE && !(ucode_force && (result == OLD_UCODE || result == SAME_UCODE)) ) diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c index 3f37792ab4b5..9616a5e9db4b 100644 --- a/xen/arch/x86/cpu/microcode/intel.c +++ b/xen/arch/x86/cpu/microcode/intel.c @@ -229,8 +229,7 @@ static int microcode_sanity_check(const struct microcode_patch *patch) * Production microcode has a positive revision. Pre-production microcode has * a negative revision. */ -static enum microcode_match_result compare_revisions( - int32_t old_rev, int32_t new_rev) +static int compare_revisions(int32_t old_rev, int32_t new_rev) { if ( new_rev > old_rev ) return NEW_UCODE; @@ -270,8 +269,8 @@ static bool microcode_fits_cpu(const struct microcode_patch *mc) return false; } -static enum microcode_match_result cf_check compare_patch( - const struct microcode_patch *new, const struct microcode_patch *old) +static int cf_check compare_patch( + const struct microcode_patch *old, const struct microcode_patch *new) { /* * Both patches to compare are supposed to be applicable to local CPU. @@ -290,7 +289,7 @@ static int cf_check apply_microcode(const struct microcode_patch *patch, unsigned int cpu = smp_processor_id(); struct cpu_signature *sig = &this_cpu(cpu_sig); uint32_t rev, old_rev = sig->rev; - enum microcode_match_result result; + int result; bool ucode_force = flags & XENPF_UCODE_FORCE; if ( !microcode_fits_cpu(patch) ) diff --git a/xen/arch/x86/cpu/microcode/private.h b/xen/arch/x86/cpu/microcode/private.h index c9dd8ba066f9..957d4d4293d0 100644 --- a/xen/arch/x86/cpu/microcode/private.h +++ b/xen/arch/x86/cpu/microcode/private.h @@ -5,13 +5,6 @@ #include <asm/microcode.h> -enum microcode_match_result { - OLD_UCODE, /* signature matched, but revision id is older */ - SAME_UCODE, /* signature matched, but revision id is the same */ - NEW_UCODE, /* signature matched, but revision id is newer */ - MIS_UCODE, /* signature mismatched */ -}; - /* Opaque. Internals are vendor-specific. */ struct microcode_patch; @@ -54,11 +47,17 @@ struct microcode_ops { unsigned int flags); /* - * Given two patches, are they both applicable to the current CPU, and is - * new a higher revision than old? + * Given a current patch, and a proposed new patch, order them based on revision. + * + * This operation is not necessarily symmetrical. In some cases, a debug + * "new" patch will always considered to be newer, on the expectation that + * whomever is using debug patches knows exactly what they're doing. */ - enum microcode_match_result (*compare_patch)( - const struct microcode_patch *new, const struct microcode_patch *old); +#define OLD_UCODE -1 +#define SAME_UCODE 0 +#define NEW_UCODE 1 + int (*compare_patch)(const struct microcode_patch *old, + const struct microcode_patch *new); /* * For Linux inird microcode compatibliity.
All uses of MIS_UCODE, have been removed, leaving only a simple ordering relation, and microcode_match_result being a stale name. Drop the enum entirely, and use a simple int -1/0/1 scheme like other standard ordering primitives in C. Swap the order or parameters to compare_patch(), to reduce cognitive complexity; all other logic operates the other way around. No functional change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> I don't particular like keeping "result" as a variable name, but nothing better comes to mind. --- xen/arch/x86/cpu/microcode/amd.c | 10 ++++------ xen/arch/x86/cpu/microcode/core.c | 5 ++--- xen/arch/x86/cpu/microcode/intel.c | 9 ++++----- xen/arch/x86/cpu/microcode/private.h | 21 ++++++++++----------- 4 files changed, 20 insertions(+), 25 deletions(-)