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

[Jean Delvare]

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.

> +                             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 ;-)

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.


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

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

-- 
Jean Delvare
SUSE L3 Support