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.hoem...@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
-----------------------------------------------------------------------------
_______________________________________________
https://lists.nongnu.org/mailman/listinfo/dmidecode-devel
