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

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: Xen-devel <xen-devel@lists.xenproject.org>,
	"Wei Liu" <wl@xen.org>, "Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH 5/7] x86/ucode/intel: Clean up microcode_update_match()
Date: Thu, 26 Mar 2020 14:36:23 +0000	[thread overview]
Message-ID: <9e1238ea-3ad2-a529-e88e-51c6f607f45d@citrix.com> (raw)
In-Reply-To: <d37aeda3-8eea-db72-51ae-f154dd3aa944@suse.com>

On 25/03/2020 13:51, Jan Beulich wrote:
> 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@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Wei Liu <wl@xen.org>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>>  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()?

Oops - completely missed that.  Will fix.

> If so with this taken care of
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks,

~Andrew