Re: [PATCH 1/3] x86/ucode: Break out compare_revisions() from existing infrastructure

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.10.2020 18:25, Andrew Cooper wrote:
> Drop some unnecesserily verbose pr_debug()'s on the AMD side.

To be honest I'm not entirely convinced of this part of the change:
For one, pr_debug() expands to nothing by default. And then you
delete 2/3 of all pr_debug() instances, putting under question why
there's a pr_debug() in the first place.

> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Only after having looked at the subsequent patches to understand
how this is going to be useful:
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
However, ...

> --- a/xen/arch/x86/cpu/microcode/amd.c
> +++ b/xen/arch/x86/cpu/microcode/amd.c
> @@ -168,6 +168,15 @@ 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)

... this (and the respective Intel code) is another good example
where, by our present guide lines, fixed width types aren't
appropriate to use. "unsigned int" (and in the later patch plain
"int" or "signed int") will fulfill the purpose, and hence ought
to be preferred.

Jan