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

[Jean Delvare]

On Mon, 1 Mar 2021 23:11:23 -0700, Jerry Hoemann wrote:
> On Mon, Mar 01, 2021 at 07:56:31PM +0100, Jean Delvare wrote:
> > 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 didn't even know hyphens were optional, I always put them.

And yes, the fact that it sorts easily (even more so for the next 7979
years) is very convenient.

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

Agreed.

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

"%x-%02x-%02x", methinks.

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

Not really. That field in type 4 is displayed twice. As a raw "ID"
first, before taking the architecture into account. And then once again
in an arch-specific way. For i686 and later, this is decoded as
"Signature" with format strings "Type %u, Family %u, Model %u, Stepping
%u" (for Intel) or "Family %u, Model %u, Stepping %u" (for AMD).

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

Reusing the same strings I mentioned above for consistency would be
perfect, agreed.

> > (...)
> > 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?

That was one of the options, yes, even though I did not mention it
explicitly. If you like it this way, let's go with it :-)

-- 
Jean Delvare
SUSE L3 Support