Re: [dmidecode] [PATCH 1/1] dmioem: HPE OEM Record 199

[Jerry Hoemann]

On Mon, Mar 01, 2021 at 07:56:31PM +0100, Jean Delvare wrote:
> Hi Jerry,
> 
> On Thu, 25 Feb 2021 12:39:55 -0700, Jerry Hoemann wrote:
> > Decode HPE OEM Record 199: CPU Microcode Patch.
> > 
> > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> > ---
> >  dmioem.c | 30 ++++++++++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> 
> Sweet, thanks for your contribution.
> 
> > diff --git a/dmioem.c b/dmioem.c
> > index 8fe84e0..58ad400 100644
> > --- a/dmioem.c
> > +++ b/dmioem.c
> > @@ -310,6 +310,36 @@ static int dmi_decode_hp(const struct dmi_header *h)
> >  
> >     switch (h->type)
> >     {
> > +           case 199:
> > +                   /*
> > +                    * Vendor Specific: CPU Microcode Patch
> > +                    *
> > +                    * Type 199:
> > +                    * Offset |  Name      | Width | Description
> > +                    * -------------------------------------
> > +                    *  0x00  | Type       | BYTE  | 0xC7, CPU Microcode 
> > Patch
> > +                    *  0x01  | Length     | BYTE  | Length of structure
> > +                    *  0x02  | Handle     | WORD  | Unique handle
> > +                    *  0x04  | Patch Info | Varies| { <DWORD: ID, DWORD 
> > Date, DWORD CPUID> ...}
> > +                    */
> > +                   pr_handle_name("%s ProLiant CPU Microcode Patch Support 
> > Info", company);
> > +
> > +                   for (ptr = 0x4; ptr + 12 <= h->length; ptr += 12) {
> > +                           u32 date;
> > +                           u32 cpuid;
> > +
> > +                           pr_attr("Patch", "0x%08X",  DWORD(data + ptr));
> > +                           date = DWORD(data + ptr + sizeof(u32));
> 
> You can hard-code these sizeofs. The offsets are per the specification,
> they do not depend on the compiler's view on type sizes.

will do.

> 
> > +                           pr_attr("Date", "%2x/%02x/%4x",
> > +                                   (date >> 24) & 0xff, (date >> 16) & 
> > 0xff, date & 0xffff);
> 
> I hadn't seen BCD in use for quite some time ;-)


It's an under appreciated art.  ;)



> 
> I don't think you want to use %2x and %4x. Numbers shorter than 2
> (resp. 4) digits would have spaces inserted before them, so you could
> have a date printed as "12/25/ 800" (if someone would be brave enough
> to release a new CPU firmware the day Charlemagne was crowned emperor).
> 
> Also I think I would always print the month with 2 digits, as this
> seems to be the most popular way of formatting dates in the BIOS world
> (315 to 1 on my samples). So I would go for "%02x/%02x/%x"
> 
> Alternatively, we could use the standard, unambiguous ISO 8601 date
> format. While not as popular in the BIOS world, I can still see a few
> occurrences (13) in my samples.


my quick read of the Wikipedia's description of the ISO 8601 format
would be: YYYY[-]MM[-]DD,  which I like as it sorts lexicographically.

I'd like to use the optional hyphens as IMO it makes it a bit easier
to read.  Also keeping the hyphens helps to distinguish the display
from the raw data which is in a different order.

So format would be "04x-02x-02x".

> 
> 
> > +                           cpuid = DWORD(data + ptr + 2 * sizeof(u32));
> > +                           pr_attr("CPUID", "%02X %02X %02X %02X",
> > +                                           cpuid & 0xff, (cpuid >> 8) & 
> > 0xff,
> > +                                           (cpuid >> 16) & 0xff, (cpuid >> 
> > 24) & 0xff);
> 
> This doesn't seem to be the most popular way to display an x86 CPUID
> value. I know of two ways to present that information:
> 
> 1* Optional type/cpu family/model/stepping, as 3 or 4 decimal numbers.
> 
> 2* A single, raw hexadecimal number.
> 
> Splitting into 4 hexadecimal bytes doesn't really make sense, as this
> doesn't match the different fields within the 32-bit number.


This field contains the output of the CPUID instruction and the printing is
consistent with how dmidecode displays the type 4 "ID" field.

I'm not opposed to changing how we display this, but we should probably
also change the display of the type 4 ID field as well to keep consistent.
That would make it easier to determine which patch would be applied to
the current hardware by matching up the output to:  "dmidecode -t 4 -t 199"



> 
> > +                   }
> > +
> > +                   break;
> > +
> >             case 203:
> >                     /*
> >                      * Vendor Specific: HP Device Correlation Record
> 
> On a broader scope, I think we can improve the readability by making it
> clearer which lines belong to the same group. There aren't many
> examples of that in dmidecode yet, but maybe we can do something
> similar to how type 40 is handled:
> 
> Handle 0x0013, DMI type 40, 18 bytes
> Additional Information 1
>         Referenced Handle: 0x0004
>         Referenced Offset: 0x05
>         String: PCIExpressx16
>         Value: 0xaa
> Additional Information 2
>         Referenced Handle: 0x0000
>         Referenced Offset: 0x05
>         String: Compiler Version: VC 9.0
>         Value: 0x05dc
> 
> Alternatively, it would be possible to add another level of indentation
> by using pr_subattr(), as is done in complex type 42. Please check if
> either option would work for you.


I think having pr_attr("Patch", ...) and date and cpuid as pr_subattr looks 
better.

An example still using the 4 byte display of cpu id and printing patch id as 
decimal:


Handle 0x0001, DMI type 199, 52 bytes
HPE ProLiant CPU Microcode Patch Support Info
        Patch: 33554537
                Date: 2019-12-20
                CPUID: 54 06 05 00
        Patch: 50331663
                Date: 2018-10-08
                CPUID: 55 06 05 00
        Patch: 67120896
                Date: 2020-01-14
                CPUID: 56 06 05 00
        Patch: 83898112
                Date: 2020-01-14
                CPUID: 57 06 05 00


Is the above more like what you were thinking?


-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------