Re: [Xen-devel] [PATCH 4/7] x86/ucode/intel: Reimplement get_{data, total}size() helpers

From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.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 4/7] x86/ucode/intel: Reimplement get_{data, total}size() helpers
Date: Thu, 26 Mar 2020 15:56:40 +0100	[thread overview]
Message-ID: <83909134-8bd7-b1b2-40f7-040dd00cc517@suse.com> (raw)
In-Reply-To: <eabacc07-ef92-0839-798b-ffee123f4f89@citrix.com>

On 26.03.2020 15:35, Andrew Cooper wrote:
> On 25/03/2020 13:41, Jan Beulich wrote:
>> On 23.03.2020 11:17, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/cpu/microcode/intel.c
>>> +++ b/xen/arch/x86/cpu/microcode/intel.c
>>> @@ -46,9 +46,16 @@ struct microcode_header_intel {
>>>      unsigned int sig;
>>>      unsigned int cksum;
>>>      unsigned int ldrver;
>>> +
>>> +    /*
>>> +     * Microcode for the Pentium Pro and II had all further fields in the
>>> +     * header reserved, had a fixed datasize of 2000 and totalsize of 2048,
>>> +     * and didn't use platform flags despite the availability of the MSR.
>>> +     */
>>> +
>>>      unsigned int pf;
>>> -    unsigned int datasize;
>>> -    unsigned int totalsize;
>>> +    unsigned int _datasize;
>>> +    unsigned int _totalsize;
>> ... the underscores here dropped again. Or else - why did you add
>> them? This (to me at least) doesn't e.g. make any more clear that
>> the fields may be zero on old hardware.
> 
> No, but it is our normal hint that you shouldn't be using the field
> directly, and should be using the accessors instead.

Yet in patch 5 you do. Perhaps for an understandable reason, but
that way you at least partly invalidate what you say above.

>> Furthermore - do we really need this PPro/PentiumII logic seeing
>> that these aren't 64-bit capable CPUs?
> 
> I did actually drop support in one version of my series, but put it back in.
> 
> These old microcode blobs are still around, including in some versions
> of microcode.dat.  By dropping the ability to recognise them as
> legitimate, we'd break the logic to search through a container of
> multiple blobs to find the one which matches.

Oh, good point.

Jan