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.hoem...@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
-----------------------------------------------------------------------------
