Re: [PATCH 1/1] dmioem: Decode HPE OEM Record 197

[Jean Delvare]

Hi Jerry,

On Thu, 20 Apr 2023 11:34:11 -0600, Jerry Hoemann wrote:
> Decode HPE OEM Record 197: Processor Information
> 
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> ---
>  dmioem.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
> 
> diff --git a/dmioem.c b/dmioem.c
> index 1ce26b9..636831d 100644
> --- a/dmioem.c
> +++ b/dmioem.c
> @@ -789,6 +789,74 @@ static int dmi_decode_hp(const struct dmi_header *h)
>                       pr_attr("Virtual Serial Port", "%s", feat & (1 << 4) ? 
> "Enabled" : "Disabled");
>                       break;
>  
> +             case 197:
> +                     /*
> +                      * Vendor Specific: HPE Processor Specific Information
> +                      *
> +                      * Processor Information structure (Type 197) for each 
> possibly installed
> +                      * physical processor to go along with each standard 
> Processor Info
> +                      * Record (Type 4).  The Type 197 record will be 
> ignored for Processor
> +                      * slots that are empty (specified in the Type 4 
> records).
> +                      *
> +                      * Processor Wattage value will be filled in with 
> information gotten from
> +                      * the CPUID instruction or possibly estimated based on 
> CPU Family/Type.
> +                      *
> +                      * Designator bytes will be 0FFh if the location of the 
> processor does not
> +                      * use it.  If a system has processor slots, but no 
> sockets, then the value
> +                      * in the Socket Designator will be 0FFh. A system 
> would have one or the
> +                      * other, or both.
> +                      *
> +                      * Offset |  Name      | Width | Description
> +                      * -------+------------+-------+-------------
> +                      *  0x00  | Type       | BYTE  | 0xC5, Processor 
> location information

This record contains a lot more than "location" information.

> +                      *  0x01  | Length     | BYTE  | Length of structure
> +                      *  0x02  | Handle     | WORD  | Unique handle
> +                      *  0x04  | Assoc Dev  | WORD  | Handle of Associated 
> Type 4 Record
> +                      *  0x06  | APIC ID    | BYTE  | Processor local APIC 
> ID.

No trailing dot, for consistency.

> +                      *  0x07  | OEM Status | BYTE  | Bits: 0: BSP, 1: 
> x2APIC, 2: Therm Margining
> +                      *  0x08  | Phys Slot  | BYTE  | Matches silk screen. 
> FF -> not used
> +                      *  0x09  | Phys Socket| BYTE  | Matches silk screen. 
> FF -> not used

"FF -> not used" can be omitted, this special case is already explained
above and clearly visible in the code itself.

> +                      *  0x0A  | Max Wattage| WORD  | Rated max wattage of 
> the processor if !0

"if !0" can be omitted as well, it's an implementation detail.

> +                      *  0x0C  | x2APIC ID  | DWORD | Processor x2APIC (if 
> OEM Status -> x2APIC)
> +                      *  0x10  | Proc UUID  | QWORD | Processor Unique 
> Identifier
> +                      *  0x18  | Conn Speed | WORD  | Interconnect speed in 
> MT/s if !0

Ditto.

> +                      *  0x1A  | QDF/S-SPEC |6-BYTES| Processor QDF/S-SPEC 
> Numbers (Intel only)

6 BYTES (no dash).

> +                      *  0x20  | Reserved   | QWORD | Gen11 Reserved
> +                      */
> +                     pr_handle_name("%s Processor Specific Information", 
> company);
> +                     if (h->length < 0x0A) break;
> +                     if (!(opt.flags & FLAG_QUIET))
> +                             pr_attr("Associated Handle", "0x%04X", 
> WORD(data + 0x04));
> +                     pr_attr("APIC ID", "0x%02x", data[0x06]);

The kernel exposes "apicid" as a decimal number in /proc/cpuinfo, so
maybe we should do the same for consistency?

> +                     feat = data[0x07];
> +                     pr_attr("OEM Status", "0x%02x", feat);
> +                     pr_subattr("BSP", "%s", feat & 0x01 ? "Yes" : "No");
> +                     pr_subattr("x2APIC", "%s", feat & 0x02 ? "Yes" : "No");
> +                     pr_subattr("Advanced Thermal Margining", "%s", feat & 
> 0x04 ? "Yes" : "No");

The raw feat value isn't particularly valuable, as everything is
printed in a more human-friendly way afterward. The fact that all 3
values are held in the same byte is an implementation detail, the
meaning of these bits is unrelated. So I suggest simplifying the
display by removing the "OEM Status" line and using pr_attr for the
flags themselves.

> +                     if (data[0x08] != 0xFF)
> +                             pr_attr("Physical Slot", "%d", data[0x08]);
> +                     if (data[0x09] != 0xFF)
> +                             pr_attr("Physical Socket", "%d", data[0x09]);
> +                     if (h->length < 0x0C) break;
> +                     if (data[0x0A])
> +                             pr_attr("Maximum Power", "%d Watts", 
> data[0x0A]);

Your documentation indicates this field is a WORD, not a BYTE.

> +                     if (h->length < 0x10) break;
> +                     if (feat & 0x02)
> +                             pr_attr("x2APIC ID", "0x%08x", DWORD(data + 
> 0x0C));
> +                     if (h->length < 0x18) break;
> +                     if (DWORD(data + 0x10) || DWORD(data + 0x14))
> +                             pr_attr("UUID", "0x%08x", QWORD(data + 0x10));

The format length doesn't match the parameter. QWORD is 8 bytes, so 16
nibbles must be displayed. Also I'm not entirely sure it is safe to
pass a QWORD to printf directly, after all it is implemented as a
struct. It would be more portable to do something like:

                                pr_attr("UUID", "0x%08x%08x", DWORD(data + 
0x14), DWORD(data + 0x10));

as done in functions dmi_64bit_memory_error_address() and
dmi_ipmi_base_address() for example.

> +                     if (h->length < 0x1A) break;
> +                     if (WORD(data + 0x18))
> +                             pr_attr("Interconnect Speed", "%d MT/s", 
> WORD(data + 0x18));
> +                     if (h->length < 0x20) break;
> +                     if (WORD(data + 0x1A) && WORD(data + 0x1C) && WORD(data 
> + 0x1E))

This test looks odd. If you want to ensure that at least one byte is
non-zero, you should use || instead of &&. If you want to ensure that
all bytes are set, then you can't test with WORD(), you need to test
each byte individually.

But do you actually need to test all bytes? As far as I can see, what
we have here is either all zeroes or a 6-char string. If data[0x1F] is
not 0 then the string is set?

> +                             pr_attr("QDF/S-SPEC", "%c%c%c%c%c%c", 
> data[0x1A], data[0x1B],
> +                                     data[0x1C], data[0x1D], data[0x1E], 
> data[0x1F]);

This needs some care. The code assumes all bytes are printable ASCII
characters. You should call helper function is_printable() to ensure
this is actually the case.

Also, as far as I can see from my samples, the string is padded with
spaces on the left hand side (1 for production samples, 2 for
engineering samples). These spaces do not look good in the output, so
it would be better to not include them.

I think I would introduce a 7-char buffer to store an actual
NUL-terminated string value generated from the 6 bytes in this field.
Before adding a character to the string, I would ensure it is valid
(ASCII printable), and skip it if it is a space. Then I would pass the
string to pr_attr/printf, using "%s" as the format. What do you think?

> +                     if (h->length < 0x24) break;
> +                     pr_attr("Reserved", "0x%08x", QWORD(data + 0x20));

Same issue as above about non-matching length and passing a struct to
printf(). Additionally, I can't see the point of printing a value when
nobody knows what it represents. So I'd rather just ignore it for the
time being, and decode that field later if/when we finally know what it
is and how to decode it.

> +                     break;
> +
>               case 199:
>                       /*
>                        * Vendor Specific: CPU Microcode Patch

Thanks,
-- 
Jean Delvare
SUSE L3 Support