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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

>>> 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.

Shame I only came to that realisation after having reworked the series
to take it out...

I'm constructing companion document to
https://xenbits.xen.org/docs/sphinx-unstable/admin-guide/microcode-loading.html
which will live in hypervisor-guide section, and cover a whole range of
topics like this.

~Andrew