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

[Jerry Hoemann]

On Thu, Apr 27, 2023 at 04:03:21PM +0200, Jean Delvare wrote:
> 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.

from doc. will change.

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

done.

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

done

> 
> > +                    *  0x0A  | Max Wattage| WORD  | Rated max wattage of 
> > the processor if !0
> 
> "if !0" can be omitted as well, it's an implementation detail.

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

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

done
> 
> > +                    *  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?

changed to %u.

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

done.

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

fixed.

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

fixed.

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

I added a helper function to address the above concerns.

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


This should have been a DWORD, not a QWORD.  I'll remove.

> 
> > +                   break;
> > +
> >             case 199:
> >                     /*
> >                      * Vendor Specific: CPU Microcode Patch
> 
> Thanks,
> -- 
> Jean Delvare
> SUSE L3 Support

-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------