Re: [Xen-devel] [PATCH 5/7] x86/ucode/intel: Clean up microcode_update_match()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.03.2020 11:17, Andrew Cooper wrote:
> Implement a new get_ext_sigtable() helper to abstract the logic for
> identifying whether an extended signature table exists.  As part of this,
> rename microcode_intel.bits to data and change its type so it can be usefully
> used in combination with the datasize header field.
> 
> Also, replace the sigmatch() macro with a static inline with a more useful
> API, and an explanation of why it is safe to drop one of the previous
> conditionals.
> 
> No practical change in behaviour.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
>  xen/arch/x86/cpu/microcode/intel.c | 75 
> +++++++++++++++++++++++++-------------
>  1 file changed, 49 insertions(+), 26 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu/microcode/intel.c 
> b/xen/arch/x86/cpu/microcode/intel.c
> index dfe44679be..bc3bbf139e 100644
> --- a/xen/arch/x86/cpu/microcode/intel.c
> +++ b/xen/arch/x86/cpu/microcode/intel.c
> @@ -61,7 +61,7 @@ struct microcode_header_intel {
>  
>  struct microcode_intel {
>      struct microcode_header_intel hdr;
> -    unsigned int bits[0];
> +    uint8_t data[];
>  };
>  
>  /* microcode format is extended from prescott processors */
> @@ -98,8 +98,41 @@ static uint32_t get_totalsize(const struct 
> microcode_header_intel *hdr)
>      return hdr->_totalsize ?: PPRO_UCODE_DATASIZE + MC_HEADER_SIZE;
>  }
>  
> -#define sigmatch(s1, s2, p1, p2) \
> -        (((s1) == (s2)) && (((p1) & (p2)) || (((p1) == 0) && ((p2) == 0))))
> +/*
> + * A piece of microcode has an extended signature table if there is space
> + * between the end of data[] and the total size.  (This logic also works
> + * appropriately for Pentium Pro/II microcode, which has 0 for both size
> + * fields, and no extended signature table.)
> + */
> +static const struct extended_sigtable *get_ext_sigtable(
> +    const struct microcode_intel *mc)
> +{
> +    if ( mc->hdr._totalsize > (MC_HEADER_SIZE + mc->hdr._datasize) )
> +        return (void *)&mc->data[mc->hdr._datasize];
> +
> +    return NULL;
> +}
> +
> +/*
> + * A piece of microcode is applicable for a CPU if:
> + *  1) the signatures (CPUID.1.EAX - Family/Model/Stepping) match, and
> + *  2) The Platform Flags bitmap intersect.
> + *
> + * A CPU will have a single Platform Flag bit, while the microcode may be
> + * common to multiple platforms and have multiple bits set.
> + *
> + * Note: The Pentium Pro/II microcode didn't use platform flags, and should
> + * treat 0 as a match.  However, Xen being 64bit means that the cpu signature
> + * won't match, allowing us to simplify the logic.
> + */
> +static bool signature_maches(const struct cpu_signature *cpu_sig,
> +                             unsigned int ucode_sig, unsigned int ucode_pf)

I guess you've lost a 't' here and really mean signature_matches()?
If so with this taken care of
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan