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

[Jean Delvare]

Hi Jerry,

On Mon, 15 Mar 2021 23:00:42 -0600, Jerry Hoemann wrote:
> Decode HPE OEM Record 199: CPU Microcode Patch.
> 
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> ---
>  dmioem.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/dmioem.c b/dmioem.c
> index 3cdfdea..3ed5f61 100644
> --- a/dmioem.c
> +++ b/dmioem.c
> @@ -313,6 +313,37 @@ static int dmi_decode_hp(const struct dmi_header *h)
>  
>       switch (h->type)
>       {
> +             case 199:
> +                     /*
> +                      * Vendor Specific: CPU Microcode Patch
> +                      *
> +                      * 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> ...}
> +                      */
> +                     if (gen < G9) break;

What's the rationale for this? I have dumps from older generations with
Intel CPU where the decoding worked just fine. I'm sad to see this go.

Plus, even if you can't decode the record for whatever reason, you
should at least print the handle name below.

> +                     pr_handle_name("%s ProLiant CPU Microcode Patch Support 
> Info", company);
> +
> +                     for (ptr = 0x4; ptr + 12 <= h->length; ptr += 12) {
> +                             u32 cpuid = DWORD(data + ptr + 2 * 4);
> +                             u32 date;
> +
> +                             // AMD omits BaseFamily. Reconstruction valid 
> on family >= 15.

Please use legacy /* C-style comments */ for consistency.

> +                             if (cpuid_type == cpuid_x86_amd)
> +                                     cpuid = ((cpuid  & 0xfff00) << 8) | 
> 0x0f00 | (cpuid & 0xff);

Doubled space.

> +
> +                             dmi_print_cpuid(pr_attr, "CPU ID", cpuid_type, 
> (u8 *) &cpuid);
> +
> +                             date = DWORD(data + ptr + 4);
> +                             pr_subattr("Date", "%04x-%02x-%02x",
> +                                     date & 0xffff, (date >> 24) & 0xff, 
> (date >> 16) & 0xff);
> +                             pr_subattr("Patch", "0x%X",  DWORD(data + ptr));

Doubled space.

> +                     }
> +                     break;
> +
>               case 203:
>                       /*
>                        * Vendor Specific: HP Device Correlation Record

I really would like to decode as much as possible, even if we can't
output the data in the most user-friendly way. Ideas come to my mind:

* If cpuid_type != cpuid_x86_amd, we can decode unconditionally.

* If cpuid_type == cpuid_x86_amd, can we determine whether we can
  figure out the complete CPUID based on the Family of the installed
  CPU, instead of the platform generation? We could export this
  information from dmi_get_cpuid_type(). Either have two distinct
  cpuid_type values for AMD (cpuid_x86_amd and cpuid_x86_amd_extfam for
  example), or save the combined family value in a global variable (not
  exactly nice, but could work still).

* In fact, if we know the base family of the installed AMD CPU, can't
  we use it to reconstruct the partial CPUIDs of type 199? I assume a
  given motherboard only supports one Family of processors?

What do you think?

-- 
Jean Delvare
SUSE L3 Support