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 16:19:20 +0100	[thread overview]
Message-ID: <d4ac2045-7567-c0af-0c88-1e87e3df04bb@suse.com> (raw)
In-Reply-To: <d4b56282-1df4-0c18-9ca7-5277a3829fb3@citrix.com>

On 26.03.2020 16:09, Andrew Cooper wrote:
> On 26/03/2020 14:56, Jan Beulich wrote:
>> 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.
> 
> The net result of of patch 5 is three adjacent helpers, which are the
> only code which use the fields themselves.
> 
> I can drop if you really insist.  We're only talking about 400 lines or
> code, or thereabouts.

Well, I find it odd this way, but no, if you're convinced it's better,
I'm not going to insist.

Jan