Discussion:
[dmidecode] [PATCH] update dmidecode to parse Modern Management Controller blocks
Neil Horman
2018-08-02 14:00:27 UTC
Permalink
Starting with version 0x300 the SMBIOS specification defined in more
detail the contents of the management controller type. DMTF further
reserved values to define the Redfish host interface specification.
Update dmidecode to properly parse and present that information

Signed-off-by: Neil Horman <***@tuxdriver.com>
CC: ***@suse.de
CC: ***@redhat.com
CC: ***@redhat.com
---
dmidecode.c | 332 +++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 316 insertions(+), 16 deletions(-)

diff --git a/dmidecode.c b/dmidecode.c
index fa6ecf1..375cd27 100644
--- a/dmidecode.c
+++ b/dmidecode.c
@@ -63,6 +63,7 @@
#include <strings.h>
#include <stdlib.h>
#include <unistd.h>
+#include <arpa/inet.h>

#ifdef __FreeBSD__
#include <errno.h>
@@ -3447,6 +3448,301 @@ static void dmi_tpm_characteristics(u64 code, const char *prefix)
prefix, characteristics[i - 2]);
}

+/*
+ * Type 42 data offsets
+ */
+
+/*
+ * Top Level Offsets
+ */
+#define IFC_TYPE 4
+#define IFC_SDATA_LEN 5
+#define IFC_PROTO_RECORD_BASE 7
+
+/*
+ * Interface specific data offsets
+ */
+#define IFC_DEVICE_TYPE 6
+
+/*
+ * USB Interface descriptor offsets
+ */
+#define USB_DESCRIPTOR 7
+#define USB_VENDOR 0
+#define USB_PRODUCT 2
+#define USB_SERIAL_LENGTH 4
+#define USB_SERIAL_DESCRIPTOR 5
+
+/*
+ * PCI Interface descriptor offsets
+ */
+#define PCI_DESCRIPTOR 7
+#define PCI_VENDOR 0
+#define PCI_DEVICE 2
+#define PCI_SUBVENDOR 4
+#define PCI_SUBDEVICE 6
+
+/*
+ * Protocol Records Offsets
+ */
+#define IFC_PROTO_COUNT 0
+
+/*
+ * Pre Protocol Record Offsets
+ */
+#define PROTO_REC_ID 0
+#define PROTO_REC_LEN 1
+#define PROTO_REC_DATA 2
+
+/*
+ * Record data offsets
+ */
+#define REC_UUID 0
+#define REC_HOST_IP_ASGN_TYPE 16
+#define REC_HOST_IP_ADDR_FMT 17
+#define REC_HOST_ADDR 18
+#define REC_HOST_MASK 34
+#define REC_RFSH_DISC_TYPE 50
+#define REC_RFSH_ADDR_FMT 51
+#define REC_RFSH_ADDR 52
+#define REC_RFSH_MASK 68
+#define REC_RFSH_PORT 84
+#define REC_RFSH_VLAN 86
+#define REC_RFSH_HOST_LEN 90
+#define REC_RFSH_HOSTNAME 91
+
+/*
+ * TYPE 42 Data Field Values
+ */
+
+/*
+ * Top level values
+ */
+#define MCH_NET_HOST_IFC 0x40
+
+/*
+ * Interface specific data values
+ */
+#define IFC_DEVICE_UNKNOWN 0x0
+#define IFC_DEVICE_USB 0x2
+#define IFC_DEVICE_PCI 0x3
+#define IFC_DEVICE_OEM_BASE 0x80
+
+/*
+ * Protocol record data values
+ */
+#define REDFISH_OVER_IP 0x4
+
+/*
+ * Protocol record data values
+ */
+#define IP_UNKNOWN 0
+#define IP_STATIC 0x1
+#define IP_DHCP 0x2
+#define IP_AUTOCONF 0x3
+#define IP_HOSTSEL 0x4
+
+#define ADDR_UNKNOWN 0
+#define ADDR_IPV4 0x1
+#define ADDR_IPV6 0x2
+
+static void decode_management_controller_structure(const struct dmi_header *h, const char *prefix)
+{
+ u8 *data = h->data;
+ u8 type = data[IFC_TYPE];
+ u8 len = data[IFC_SDATA_LEN];
+ u8 count;
+ const char *devname[] = {
+ "Unknown",
+ "Unknown",
+ "USB",
+ "PCI[e]",
+ "OEM",
+ };
+
+ if (h->length < 0x9) {
+ printf("%s Invalid structure\n", prefix);
+ return;
+ }
+
+ printf("%sHost Interface Type: ", prefix);
+ if (type != MCH_NET_HOST_IFC)
+ printf("Unknown\n");
+ else
+ printf("Network\n");
+
+
+ if (len != 0) {
+ type = data[IFC_DEVICE_TYPE];
+ if (type >= IFC_DEVICE_OEM_BASE)
+ type = IFC_DEVICE_UNKNOWN;
+
+ printf("%sDevice Type: %s\n", prefix, devname[type]);
+ if (type == IFC_DEVICE_USB) {
+ /* USB */
+ u8 *usbdata = &data[USB_DESCRIPTOR];
+ printf("%s\tidVendor: 0x%02x\n", prefix, (unsigned short)usbdata[USB_VENDOR]);
+ printf("%s\tidProduct: 0x%02x\n", prefix, (unsigned short)usbdata[USB_PRODUCT]);
+ printf("%s\tSerialNumber:\n", prefix);
+ printf("%s\t\tbDescriptor: 0x%01x\n", prefix, usbdata[USB_SERIAL_DESCRIPTOR]);
+ /* Note bString is not printable here, so skip it */
+ } else if (type == IFC_DEVICE_PCI) {
+ /* PCI */
+ u8 *pcidata = &data[PCI_DESCRIPTOR];
+ printf("%s\tVendorID: %02x\n", prefix, (unsigned short)pcidata[PCI_VENDOR]);
+ printf("%s\tDeviceID: %02x\n", prefix, (unsigned short)pcidata[PCI_DEVICE]);
+ printf("%s\tSubVendorID: %02x\n", prefix, (unsigned short)pcidata[PCI_SUBVENDOR]);
+ printf("%s\tSubDeviceID: %02x\n", prefix, (unsigned short)pcidata[PCI_SUBDEVICE]);
+ }
+ /* Don't mess with unknown types for now */
+ }
+
+ /*
+ * Move to the Protocol Count Area from Table 1
+ * Data[6] points to the start of the interface specific
+ * data, and Data[5] is the length of that region
+ */
+ data = &data[IFC_PROTO_RECORD_BASE+data[5]];
+
+ /* Get the protocol records count */
+ count = (u8)data[IFC_PROTO_COUNT];
+ if (count) {
+ int i, j, k;
+ u8 rid;
+ u8 rlen;
+ u8 *rec = &data[1]; /*first record starts after count value */
+ u8 *rdata;
+ u8 buf[64];
+ u8 uuid[37];
+ u8 assignval;
+ u8 addrtype;
+ u8 hlen;
+ char hname[257];
+
+ const char *assigntype[] = {
+ "Unknown",
+ "Static",
+ "DHCP",
+ "AutoConf",
+ "Host Selected",
+ };
+
+ const char *addressformat[] = {
+ "Unknown",
+ "Ipv4",
+ "Ipv6",
+ };
+
+ printf("%sProtocol Records (%02d):\n", prefix, count);
+ for (i=0; i < count; i++) {
+ rid = (u8)rec[PROTO_REC_ID];
+ rlen = (u8)rec[PROTO_REC_LEN];
+ rdata = &rec[PROTO_REC_DATA];
+
+ if (rid != REDFISH_OVER_IP) {
+ printf("%s\tProtocol ID: %02x (Unknown)\n", prefix, rid);
+ goto next;
+ }
+
+ printf("%s\tProtocol ID: Redfish over IP\n", prefix);
+ memcpy(buf, &rdata[REC_UUID], 16);
+
+ /* Convert UUID to readable characters */
+ for (j=0, k=0; j < 33; j++, k++) {
+ if ((j == 8) || (j == 12) || (j == 16) || (j == 20)) {
+ uuid[k] = '-';
+ k++;
+ }
+
+ if (j & 0x1)
+ uuid[k] = (buf[j/2] >> 4) + 0x30;
+ else
+ uuid[k] = (buf[j/2] & 0x0f) + 0x30;
+
+ if (uuid[j] >= 0x3a)
+ uuid[j] += 7;
+ }
+ uuid[32] = '\0';
+ printf("%s\t\tService UUID: %s\n", prefix, uuid);
+
+ assignval = (u8)rdata[REC_HOST_IP_ASGN_TYPE];
+ if (assignval > IP_HOSTSEL)
+ assignval = IP_UNKNOWN;
+ printf("%s\t\tHost IP Assignment Type: %s\n", prefix, assigntype[assignval]);
+
+ addrtype = (u8)rdata[REC_HOST_IP_ADDR_FMT];
+ if (addrtype > ADDR_IPV6)
+ addrtype = ADDR_UNKNOWN;
+ printf("%s\t\tHost IP Address Format: %s\n", prefix, addressformat[addrtype]);
+
+ /* We only use the Host IP Address and Mask if the assignment type is static */
+ if ((assignval == IP_STATIC) || (assignval == IP_AUTOCONF)) {
+ /* Prints the Host IP Address */
+ printf("%s\t\t%s Address: %s\n", prefix,
+ (addrtype == 0x1 ? "Ipv4" : "Ipv6"),
+ (addrtype == 0x1 ?
+ inet_ntop(AF_INET, (char *)&rdata[REC_HOST_ADDR], (char *)buf, 64) :
+ inet_ntop(AF_INET6, (char *)&rdata[REC_HOST_ADDR], (char *)buf, 64)));
+
+ /* Prints the Host IP Mask */
+ printf("%s\t\t%s Mask: %s\n", prefix,
+ (addrtype == ADDR_IPV4 ? "Ipv4" : "Ipv6"),
+ (addrtype == ADDR_IPV4 ?
+ inet_ntop(AF_INET, (char *)&rdata[REC_HOST_MASK], (char *)buf, 64) :
+ inet_ntop(AF_INET6, (char *)&rdata[REC_HOST_MASK], (char *)buf, 64)));
+ }
+
+ /* Get the Redfish Service IP Discovery Type */
+ assignval = (u8)rdata[REC_RFSH_DISC_TYPE];
+ if (assignval > IP_HOSTSEL)
+ assignval = 0;
+
+ /* Redfish Service IP Discovery type Mirrors Host IP Assignment type */
+ printf("%s\t\tRedfish Service IP Discovery Type: %s\n", prefix, assigntype[assignval]);
+
+ /* Get the Redfish Service IP Address Format */
+ addrtype = (u8)rdata[REC_RFSH_ADDR_FMT];
+ if (addrtype > ADDR_IPV6)
+ addrtype = ADDR_UNKNOWN;
+
+ printf("%s\t\tRedfish Service IP Address Format: %s\n", prefix, addressformat[addrtype]);
+ if ((assignval == IP_STATIC) || (assignval == IP_AUTOCONF)) {
+ u16 port;
+ u32 vlan;
+ /* Prints the Redfish Service Address */
+ printf("%s\t\t%s Redfish Service Address: %s\n", prefix,
+ (addrtype == ADDR_IPV4 ? "Ipv4" : "Ipv6"),
+ (addrtype == ADDR_IPV4 ?
+ inet_ntop(AF_INET, (char *)&rdata[REC_RFSH_ADDR], (char *)buf, 64) :
+ inet_ntop(AF_INET6, (char *)&rdata[REC_RFSH_ADDR], (char *)buf, 64)));
+
+ /* Prints the Redfish Service Mask */
+ printf("%s\t\t%s Redfish Service Mask: %s\n", prefix,
+ (addrtype == ADDR_IPV4 ? "Ipv4" : "Ipv6"),
+ (addrtype == ADDR_IPV4 ?
+ inet_ntop(AF_INET, (char *)&rdata[REC_RFSH_MASK], (char *)buf, 64) :
+ inet_ntop(AF_INET6, (char *)&rdata[REC_RFSH_MASK], (char *)buf, 64)));
+
+ port = (u8)rdata[REC_RFSH_PORT];
+ vlan = (u16)rdata[REC_RFSH_VLAN];
+ printf("%s\t\tRedfish Service Port: %d\n", prefix, port);
+ printf("%s\t\tRedfish Service Vlan: %d\n", prefix, vlan);
+
+ }
+
+ hlen = (u8)rdata[REC_RFSH_HOST_LEN];
+ memcpy(hname, &rdata[REC_RFSH_HOSTNAME], hlen);
+ hname[hlen] = '\0';
+ printf("%s\t\tRedfish Service Hostname: %s\n", prefix, hname);
+next:
+ rec = rec + rlen +1;
+ }
+ }
+
+ return;
+
+}
+
/*
* Main
*/
@@ -4582,22 +4878,26 @@ static void dmi_decode(const struct dmi_header *h, u16 ver)

case 42: /* 7.43 Management Controller Host Interface */
printf("Management Controller Host Interface\n");
- if (h->length < 0x05) break;
- printf("\tInterface Type: %s\n",
- dmi_management_controller_host_type(data[0x04]));
- /*
- * There you have a type-dependent, variable-length
- * part in the middle of the structure, with no
- * length specifier, so no easy way to decode the
- * common, final part of the structure. What a pity.
- */
- if (h->length < 0x09) break;
- if (data[0x04] == 0xF0) /* OEM */
- {
- printf("\tVendor ID: 0x%02X%02X%02X%02X\n",
- data[0x05], data[0x06], data[0x07],
- data[0x08]);
- }
+
+ if (ver < 0x300) {
+ if (h->length < 0x05) break;
+ printf("\tInterface Type: %s\n",
+ dmi_management_controller_host_type(data[0x04]));
+ /*
+ * There you have a type-dependent, variable-length
+ * part in the middle of the structure, with no
+ * length specifier, so no easy way to decode the
+ * common, final part of the structure. What a pity.
+ */
+ if (h->length < 0x09) break;
+ if (data[0x04] == 0xF0) /* OEM */
+ {
+ printf("\tVendor ID: 0x%02X%02X%02X%02X\n",
+ data[0x05], data[0x06], data[0x07],
+ data[0x08]);
+ }
+ } else
+ decode_management_controller_structure(h, "\t\t");
break;

case 43: /* 7.44 TPM Device */
--
2.17.1


_______________________________________________
https://lists.nongnu.org/mailman/listinfo/dmidecode-devel
Elliott, Robert (Persistent Memory)
2018-08-02 14:17:42 UTC
Permalink
-----Original Message-----
From: dmidecode-devel <dmidecode-devel-
Sent: Thursday, August 02, 2018 9:00 AM
Subject: [dmidecode] [PATCH] update dmidecode to parse Modern
Management Controller blocks
Starting with version 0x300 the SMBIOS specification defined in more
detail the contents of the management controller type. DMTF further
reserved values to define the Redfish host interface specification.
Update dmidecode to properly parse and present that information
...
+ if (type == IFC_DEVICE_USB) {
+ /* USB */
+ u8 *usbdata = &data[USB_DESCRIPTOR];
+ printf("%s\tidVendor: 0x%02x\n", prefix,
(unsigned short)usbdata[USB_VENDOR]);
+ printf("%s\tidProduct: 0x%02x\n", prefix,
(unsigned short)usbdata[USB_PRODUCT]);
+ printf("%s\tSerialNumber:\n", prefix);
+ printf("%s\t\tbDescriptor: 0x%01x\n", prefix,
usbdata[USB_SERIAL_DESCRIPTOR]);
+ /* Note bString is not printable here, so skip it
*/
+ } else if (type == IFC_DEVICE_PCI) {
+ /* PCI */
+ u8 *pcidata = &data[PCI_DESCRIPTOR];
+ printf("%s\tVendorID: %02x\n", prefix, (unsigned
short)pcidata[PCI_VENDOR]);
+ printf("%s\tDeviceID: %02x\n", prefix, (unsigned
short)pcidata[PCI_DEVICE]);
+ printf("%s\tSubVendorID: %02x\n", prefix,
(unsigned short)pcidata[PCI_SUBVENDOR]);
+ printf("%s\tSubDeviceID: %02x\n", prefix,
(unsigned short)pcidata[PCI_SUBDEVICE]);
I'd add 0x prefixes for the PCI fields.

The USB and PCI ID fields are 16-bit values, so %04x is a better fit.

"0x%01x" is probably not right for bDescriptor (0 pad for
1 character?); if that's a 1-byte field, it should be %02x.

Also be careful of endianness; these are all little-endian values. Does
dmidecode support being run on a big-endian machine? If so, it should
still print the same characters. For example, Intel's PCI vendor ID
should always print as "0x8086", not '0x8680" on some machines.


---
Robert Elliott, HPE Persistent Memory



_______________________________________________
https://lists.nongnu.org/mailman/listinfo/dmidecode-devel
Neil Horman
2018-08-02 14:51:32 UTC
Permalink
Post by Elliott, Robert (Persistent Memory)
-----Original Message-----
From: dmidecode-devel <dmidecode-devel-
Sent: Thursday, August 02, 2018 9:00 AM
Subject: [dmidecode] [PATCH] update dmidecode to parse Modern
Management Controller blocks
Starting with version 0x300 the SMBIOS specification defined in more
detail the contents of the management controller type. DMTF further
reserved values to define the Redfish host interface specification.
Update dmidecode to properly parse and present that information
...
+ if (type == IFC_DEVICE_USB) {
+ /* USB */
+ u8 *usbdata = &data[USB_DESCRIPTOR];
+ printf("%s\tidVendor: 0x%02x\n", prefix,
(unsigned short)usbdata[USB_VENDOR]);
+ printf("%s\tidProduct: 0x%02x\n", prefix,
(unsigned short)usbdata[USB_PRODUCT]);
+ printf("%s\tSerialNumber:\n", prefix);
+ printf("%s\t\tbDescriptor: 0x%01x\n", prefix,
usbdata[USB_SERIAL_DESCRIPTOR]);
+ /* Note bString is not printable here, so skip it
*/
+ } else if (type == IFC_DEVICE_PCI) {
+ /* PCI */
+ u8 *pcidata = &data[PCI_DESCRIPTOR];
+ printf("%s\tVendorID: %02x\n", prefix, (unsigned
short)pcidata[PCI_VENDOR]);
+ printf("%s\tDeviceID: %02x\n", prefix, (unsigned
short)pcidata[PCI_DEVICE]);
+ printf("%s\tSubVendorID: %02x\n", prefix,
(unsigned short)pcidata[PCI_SUBVENDOR]);
+ printf("%s\tSubDeviceID: %02x\n", prefix,
(unsigned short)pcidata[PCI_SUBDEVICE]);
I'd add 0x prefixes for the PCI fields.
I can do that.
Post by Elliott, Robert (Persistent Memory)
The USB and PCI ID fields are 16-bit values, so %04x is a better fit.
Yup, my bad, for some reason I was thinking 2 bytes, not 2 characters.
Post by Elliott, Robert (Persistent Memory)
"0x%01x" is probably not right for bDescriptor (0 pad for
1 character?); if that's a 1-byte field, it should be %02x.
Thats always a 1 byte field, I'll make the format %02x
Post by Elliott, Robert (Persistent Memory)
Also be careful of endianness; these are all little-endian values. Does
dmidecode support being run on a big-endian machine? If so, it should
still print the same characters. For example, Intel's PCI vendor ID
should always print as "0x8086", not '0x8680" on some machines.
Thats an interesting question. Currently as far as I know smbios is only
supported on intel, which is a little endian system. The application is written
to expect that, so were good. That said, dmidecode can also read from a dump
file, so its possible that someone might take a dump on an intel system, and go
read it on an old power system.

I would say, it wouldn't hurt to update dmidecode to be endian aware, but such
an update is likely outside the scope of this patch.

I'll post a v2 shortly.
Best
Neil
Post by Elliott, Robert (Persistent Memory)
---
Robert Elliott, HPE Persistent Memory
_______________________________________________
https://lists.nongnu.org/mailman/listinfo/dmidecode-devel
Jean Delvare
2018-08-04 20:59:39 UTC
Permalink
Hi Neil,
Post by Neil Horman
Post by Elliott, Robert (Persistent Memory)
Also be careful of endianness; these are all little-endian values. Does
dmidecode support being run on a big-endian machine? If so, it should
still print the same characters. For example, Intel's PCI vendor ID
should always print as "0x8086", not '0x8680" on some machines.
Thats an interesting question. Currently as far as I know smbios is only
supported on intel, which is a little endian system. The application is written
to expect that, so were good.
Not true. First of all, all of Intel isn't fully little-endian. IA-64 is
(well, was...) bi-endian. Secondly, SMBIOS is supported on ARM these
days, which is also bi-endian. But... I think that both default to
little-endian indeed, and people stick to that?

The original SMBIOS specification (when I started working on dmidecode
2.0 back in 2003) did not specify the byte order in the SMBIOS entry
point and DMI table. I assumed native byte order of the host, and
that's the reason why we have these macros defined in types.h. (In fact
we have these macros exactly because the specification was not clear
and I'd rather update the macros once than change it everywhere
throughout the source code.)

But now that you are raising the point again, I took a look at the most
recent specification and found the following note (apparently added in
2.8.0, but I did not pay attention at that time):

NOTE The Entry Point Structure and all SMBIOS structures assume a little-endian ordering convention, unless
explicitly specified otherwise, i.e., multi-byte numbers (WORD, DWORD, etc.) are stored with the low-order
byte at the lowest address and the high-order byte at the highest address.

Which means I got it wrong and types.h needs to be revisited. This also
suggests that nobody ever tried dmidecode on a big-endian system, or
they would have noticed.
Post by Neil Horman
That said, dmidecode can also read from a dump
file, so its possible that someone might take a dump on an intel system, and go
read it on an old power system.
This is a good point. The endianness of the original system is not
recorded in the dump, so if the data endianness would depend on the
original host, we would be in trouble. But as it seems now clear that
all DMI data must be little-endian, we should be on the safe side. It's
currently broken, but easy to fix.

Reading a dump from a powerpc system might be challenge because
typically distributions don't package dmidecode on architectures which
do not implement SMBIOS. But I could try building it from sources on
the machine directly.
Post by Neil Horman
I would say, it wouldn't hurt to update dmidecode to be endian aware, but such
an update is likely outside the scope of this patch.
Actually we need to update it to be endian agnostic ;-) but yes, that
would be done in a separate patch.

Thanks,
--
Jean Delvare
SUSE L3 Support

_______________________________________________
https://lists.nongnu.org/mailman/listinfo/dmidecode-devel
Neil Horman
2018-08-05 16:49:29 UTC
Permalink
Post by Jean Delvare
Hi Neil,
Post by Neil Horman
Post by Elliott, Robert (Persistent Memory)
Also be careful of endianness; these are all little-endian values. Does
dmidecode support being run on a big-endian machine? If so, it should
still print the same characters. For example, Intel's PCI vendor ID
should always print as "0x8086", not '0x8680" on some machines.
Thats an interesting question. Currently as far as I know smbios is only
supported on intel, which is a little endian system. The application is written
to expect that, so were good.
Not true. First of all, all of Intel isn't fully little-endian. IA-64 is
(well, was...) bi-endian. Secondly, SMBIOS is supported on ARM these
days, which is also bi-endian. But... I think that both default to
little-endian indeed, and people stick to that?
Hm, not sure on that. ARM can still be a bit of a wild west show. Its
stabilizing with larger distribution support, but smaller stuff could still go
either way I think.
Post by Jean Delvare
The original SMBIOS specification (when I started working on dmidecode
2.0 back in 2003) did not specify the byte order in the SMBIOS entry
point and DMI table. I assumed native byte order of the host, and
that's the reason why we have these macros defined in types.h. (In fact
we have these macros exactly because the specification was not clear
and I'd rather update the macros once than change it everywhere
throughout the source code.)
Well, sure, but the macros only work if you know the endianess at the source.
Post by Jean Delvare
But now that you are raising the point again, I took a look at the most
recent specification and found the following note (apparently added in
NOTE The Entry Point Structure and all SMBIOS structures assume a little-endian ordering convention, unless
explicitly specified otherwise, i.e., multi-byte numbers (WORD, DWORD, etc.) are stored with the low-order
byte at the lowest address and the high-order byte at the highest address.
Ok, cool, so this suggests that we should be using the ntoh* macros
consistently.
Post by Jean Delvare
Which means I got it wrong and types.h needs to be revisited. This also
suggests that nobody ever tried dmidecode on a big-endian system, or
they would have noticed.
The only system I know of that would have been consistently big endian I think
would have been old pre power-9 systems, which IIRC never supported smbios.
Post by Jean Delvare
Post by Neil Horman
That said, dmidecode can also read from a dump
file, so its possible that someone might take a dump on an intel system, and go
read it on an old power system.
This is a good point. The endianness of the original system is not
recorded in the dump, so if the data endianness would depend on the
original host, we would be in trouble. But as it seems now clear that
all DMI data must be little-endian, we should be on the safe side. It's
currently broken, but easy to fix.
Agreed, if we just assume consistent little endian from the source (be it
in-memory or file), we can move forward.
Post by Jean Delvare
Reading a dump from a powerpc system might be challenge because
typically distributions don't package dmidecode on architectures which
do not implement SMBIOS. But I could try building it from sources on
the machine directly.
Post by Neil Horman
I would say, it wouldn't hurt to update dmidecode to be endian aware, but such
an update is likely outside the scope of this patch.
Actually we need to update it to be endian agnostic ;-) but yes, that
would be done in a separate patch.
Ok, so are you comfortable with this patch as it is? I'm happy to start a
review of the code to consistently use the endian conversion macros if you like.

Neil
Post by Jean Delvare
Thanks,
--
Jean Delvare
SUSE L3 Support
_______________________________________________
https://lists.nongnu.org/mailman/listinfo/dmidecode-devel
Jean Delvare
2018-08-06 13:43:34 UTC
Permalink
Post by Neil Horman
Post by Jean Delvare
But now that you are raising the point again, I took a look at the most
recent specification and found the following note (apparently added in
NOTE The Entry Point Structure and all SMBIOS structures assume a little-endian ordering convention, unless
explicitly specified otherwise, i.e., multi-byte numbers (WORD, DWORD, etc.) are stored with the low-order
byte at the lowest address and the high-order byte at the highest address.
Ok, cool, so this suggests that we should be using the ntoh* macros
consistently.
I'm not sure how ntoh* would help here. We now know that the DMI table
is using little-endian. Network byte order, on the other hand, is big-
endian. Did you mean le*toh functions?

As I understand it, that's pretty much what I open-coded with my macros
in types.h? I'm not sure what would be the benefit of switching to
le*toh functions instead. Using WORD() and DWORD() has the advantage
that these are the terms used in the SMBIOS specification, so it makes
it easier to match the code with the specification, in my opinion. Do
you think differently?
Post by Neil Horman
Post by Jean Delvare
Which means I got it wrong and types.h needs to be revisited. This also
suggests that nobody ever tried dmidecode on a big-endian system, or
they would have noticed.
The only system I know of that would have been consistently big endian I think
would have been old pre power-9 systems, which IIRC never supported smbios.
Correct.
Post by Neil Horman
Post by Jean Delvare
Post by Neil Horman
That said, dmidecode can also read from a dump
file, so its possible that someone might take a dump on an intel system, and go
read it on an old power system.
This is a good point. The endianness of the original system is not
recorded in the dump, so if the data endianness would depend on the
original host, we would be in trouble. But as it seems now clear that
all DMI data must be little-endian, we should be on the safe side. It's
currently broken, but easy to fix.
Agreed, if we just assume consistent little endian from the source (be it
in-memory or file), we can move forward.
I think that's where we are header, yes. I made some preliminary tests
on a POWER7 system. I have a patch almost ready, I'll post it for
review soon.
Post by Neil Horman
Post by Jean Delvare
Reading a dump from a powerpc system might be challenge because
typically distributions don't package dmidecode on architectures which
do not implement SMBIOS. But I could try building it from sources on
the machine directly.
Post by Neil Horman
I would say, it wouldn't hurt to update dmidecode to be endian aware, but such
an update is likely outside the scope of this patch.
Actually we need to update it to be endian agnostic ;-) but yes, that
would be done in a separate patch.
Ok, so are you comfortable with this patch as it is? I'm happy to start a
review of the code to consistently use the endian conversion macros if you like.
I did not read your patch yet, will do when I finally have time. You
should be using WORD() etc like the rest of the code is doing, for
consistency.

Thanks,
--
Jean Delvare
SUSE L3 Support

_______________________________________________
https://lists.nongnu.org/mailman/listinfo/dmidecode-devel
Neil Horman
2018-08-06 20:07:56 UTC
Permalink
Post by Jean Delvare
Post by Neil Horman
Post by Jean Delvare
But now that you are raising the point again, I took a look at the most
recent specification and found the following note (apparently added in
NOTE The Entry Point Structure and all SMBIOS structures assume a little-endian ordering convention, unless
explicitly specified otherwise, i.e., multi-byte numbers (WORD, DWORD, etc.) are stored with the low-order
byte at the lowest address and the high-order byte at the highest address.
Ok, cool, so this suggests that we should be using the ntoh* macros
consistently.
I'm not sure how ntoh* would help here. We now know that the DMI table
is using little-endian. Network byte order, on the other hand, is big-
endian. Did you mean le*toh functions?
Yes, sorry, I'm specifically referring to the endian conversion functions in
endian.h (leXtoh and htoleX). They are not strictly speaking posix compliant,
but are fairly universally available.
Post by Jean Delvare
As I understand it, that's pretty much what I open-coded with my macros
in types.h? I'm not sure what would be the benefit of switching to
le*toh functions instead. Using WORD() and DWORD() has the advantage
that these are the terms used in the SMBIOS specification, so it makes
it easier to match the code with the specification, in my opinion. Do
you think differently?
I think you missunderstood what I was suggesting.

the WORD and DWORD macros are exacty what map to leXtoh in types.h.
What I'm saying is, instead of maintaining a reinvented wheel, why not convert
types.h such that they only contain these defintions:

#define WORD(x) le16toh(x)
#define DWORD(x) le32toh(x)
#deine QWORD(x) le64toh(x)

That seems far more concise than maintaining a reimplementation of those macros,
and still maintains correlation to the spec.
Post by Jean Delvare
Post by Neil Horman
Post by Jean Delvare
Which means I got it wrong and types.h needs to be revisited. This also
suggests that nobody ever tried dmidecode on a big-endian system, or
they would have noticed.
The only system I know of that would have been consistently big endian I think
would have been old pre power-9 systems, which IIRC never supported smbios.
Correct.
Post by Neil Horman
Post by Jean Delvare
Post by Neil Horman
That said, dmidecode can also read from a dump
file, so its possible that someone might take a dump on an intel system, and go
read it on an old power system.
This is a good point. The endianness of the original system is not
recorded in the dump, so if the data endianness would depend on the
original host, we would be in trouble. But as it seems now clear that
all DMI data must be little-endian, we should be on the safe side. It's
currently broken, but easy to fix.
Agreed, if we just assume consistent little endian from the source (be it
in-memory or file), we can move forward.
I think that's where we are header, yes. I made some preliminary tests
on a POWER7 system. I have a patch almost ready, I'll post it for
review soon.
Post by Neil Horman
Post by Jean Delvare
Reading a dump from a powerpc system might be challenge because
typically distributions don't package dmidecode on architectures which
do not implement SMBIOS. But I could try building it from sources on
the machine directly.
Post by Neil Horman
I would say, it wouldn't hurt to update dmidecode to be endian aware, but such
an update is likely outside the scope of this patch.
Actually we need to update it to be endian agnostic ;-) but yes, that
would be done in a separate patch.
Ok, so are you comfortable with this patch as it is? I'm happy to start a
review of the code to consistently use the endian conversion macros if you like.
I did not read your patch yet, will do when I finally have time. You
should be using WORD() etc like the rest of the code is doing, for
consistency.
I'll send a v3 then, the system I was validating this on just died on me. As
soon as I get it back up, I'll test a version with the WORD/DWORD macros used.

Neil
Post by Jean Delvare
Thanks,
--
Jean Delvare
SUSE L3 Support
_______________________________________________
https://lists.nongnu.org/mailman/listinfo/dmidecode-devel
Jean Delvare
2018-08-07 07:29:44 UTC
Permalink
Hi Neil,
Post by Neil Horman
Post by Jean Delvare
As I understand it, that's pretty much what I open-coded with my macros
in types.h? I'm not sure what would be the benefit of switching to
le*toh functions instead. Using WORD() and DWORD() has the advantage
that these are the terms used in the SMBIOS specification, so it makes
it easier to match the code with the specification, in my opinion. Do
you think differently?
I think you missunderstood what I was suggesting.
the WORD and DWORD macros are exacty what map to leXtoh in types.h.
What I'm saying is, instead of maintaining a reinvented wheel, why not convert
#define WORD(x) le16toh(x)
#define DWORD(x) le32toh(x)
#deine QWORD(x) le64toh(x)
That seems far more concise than maintaining a reimplementation of those macros,
and still maintains correlation to the spec.
That's tempting, however it does not seem like these macros are dealing
with unaligned access. If we are running on a little-endian machine
which does not support unaligned memory access (hello IA-64!), we end
up with the following definitions:

# define le16toh(x) (x)
# define le32toh(x) (x)
# define le64toh(x) (x)

which will surely crash. So we still need our own macros for the
ALIGNMENT_WORKAROUND case. And then I don't think there is much benefit
in using le*toh macros in one case, and custom code in the other, as
this is the same amount of code we have now?

Also, endian.h doesn't seem to be available on Solaris? I no longer
have access to such system for proper testing, but at least online man
pages don't have any reference to it.
Post by Neil Horman
Post by Jean Delvare
(...)
I did not read your patch yet, will do when I finally have time. You
should be using WORD() etc like the rest of the code is doing, for
consistency.
I'll send a v3 then, the system I was validating this on just died on me. As
soon as I get it back up, I'll test a version with the WORD/DWORD macros used.
May I suggest that you dump the SMBIOS table of said system to a file,
so you can test your code on another host using --from-dump, thus no
longer depending on the availability of said system? It would be great
if you are also able to share that dump with me, so that I can add it
to my test suite.

Thanks,
--
Jean Delvare
SUSE L3 Support

_______________________________________________
https://lists.nongnu.org/mailman/listinfo/dmidecode-devel
Neil Horman
2018-08-07 11:33:54 UTC
Permalink
Post by Jean Delvare
Hi Neil,
Post by Neil Horman
Post by Jean Delvare
As I understand it, that's pretty much what I open-coded with my macros
in types.h? I'm not sure what would be the benefit of switching to
le*toh functions instead. Using WORD() and DWORD() has the advantage
that these are the terms used in the SMBIOS specification, so it makes
it easier to match the code with the specification, in my opinion. Do
you think differently?
I think you missunderstood what I was suggesting.
the WORD and DWORD macros are exacty what map to leXtoh in types.h.
What I'm saying is, instead of maintaining a reinvented wheel, why not convert
#define WORD(x) le16toh(x)
#define DWORD(x) le32toh(x)
#deine QWORD(x) le64toh(x)
That seems far more concise than maintaining a reimplementation of those macros,
and still maintains correlation to the spec.
That's tempting, however it does not seem like these macros are dealing
with unaligned access. If we are running on a little-endian machine
which does not support unaligned memory access (hello IA-64!), we end
# define le16toh(x) (x)
# define le32toh(x) (x)
# define le64toh(x) (x)
which will surely crash.
I don't think so, at least not on linux, where the kernel installs a handler to
fix up unaligned accesses. Either way, it should be pretty easily fixable I
think by defining the macros as:

#define WORD(x) le16toh(get_unaligned(x))
...
That may not be exactly right (might need to serialize the macro use), but its
close.

Either way, I guess it doesn't really matter, this was more about not
maintaining duplicate code for endian macros, but since its already written and
likely won't change again for another decade, theres less value in this.
Post by Jean Delvare
So we still need our own macros for the
ALIGNMENT_WORKAROUND case. And then I don't think there is much benefit
in using le*toh macros in one case, and custom code in the other, as
this is the same amount of code we have now?
My suggestion was less about the amount of code, and more about maintaining
multiple implementations....i.e. why maintain your own definition when the OS
offers it to you.
Post by Jean Delvare
Also, endian.h doesn't seem to be available on Solaris? I no longer
have access to such system for proper testing, but at least online man
pages don't have any reference to it.
I don't either, but you appear to be right, solaris reuires that you cook your
own versions of these macros with something like this:
#elif defined(__sun)

#include <sys/byteorder.h>
#ifdef _LITTLE_ENDIAN
#define __BYTE_ORDER __LITTLE_ENDIAN
#define le16toh(x) get_unaligned(x)
#else
#define le16toh(x) bswap_16(get_unaligned(x))
#define __BYTE_ORDER __BIG_ENDIAN
#endif


Which is starting to get onerous in its own right....
Post by Jean Delvare
Post by Neil Horman
Post by Jean Delvare
(...)
I did not read your patch yet, will do when I finally have time. You
should be using WORD() etc like the rest of the code is doing, for
consistency.
I'll send a v3 then, the system I was validating this on just died on me. As
soon as I get it back up, I'll test a version with the WORD/DWORD macros used.
May I suggest that you dump the SMBIOS table of said system to a file,
so you can test your code on another host using --from-dump, thus no
longer depending on the availability of said system? It would be great
if you are also able to share that dump with me, so that I can add it
to my test suite.
Sure, as soon as I get it back up and running. Also note, I'm starting to think
it might have a problem in the offset of its protocol record area (I'll need
another system to compare it too, which I'll have later this week I think).
Either way, it might be useful to have an erroneous dump for testing purposes

Neil
Post by Jean Delvare
Thanks,
--
Jean Delvare
SUSE L3 Support
_______________________________________________
https://lists.nongnu.org/mailman/listinfo/dmidecode-devel
Jean Delvare
2018-08-07 13:14:22 UTC
Permalink
Post by Neil Horman
Post by Jean Delvare
That's tempting, however it does not seem like these macros are dealing
with unaligned access. If we are running on a little-endian machine
which does not support unaligned memory access (hello IA-64!), we end
# define le16toh(x) (x)
# define le32toh(x) (x)
# define le64toh(x) (x)
which will surely crash.
I don't think so, at least not on linux, where the kernel installs a handler to
fix up unaligned accesses.
Is it a "recent" addition? I am certain that it used to crash, back in
2003 - I did not implement ALIGNMENT_WORKAROUND just for fun. The git
log also has evidence that gcc used to complain about unaligned
structure member copies. If you can point me to the kernel piece of
code which deals with that, I'm interested. That being said, I am
afraid that it must be slower to do things wrong and let the kernel
cope with the mess, than do the right thing directly?

I guess I could just connect to an IA-64 system at work and try, but if
it doesn't work and I crash the machine, my colleagues might
complain ;-)
Post by Neil Horman
Either way, it should be pretty easily fixable I
#define WORD(x) le16toh(get_unaligned(x))
get_unaligned() doesn't seem to be a standard user-space function.
Post by Neil Horman
...
That may not be exactly right (might need to serialize the macro use), but its
close.
Doesn't sound that much more simple than what I did ;-)
Post by Neil Horman
Either way, I guess it doesn't really matter, this was more about not
maintaining duplicate code for endian macros, but since its already written and
likely won't change again for another decade, theres less value in this.
Well I'm always interested in ways to clean up my code, so it remains
an interesting discussion.
Post by Neil Horman
(...)
Sure, as soon as I get it back up and running. Also note, I'm starting to think
it might have a problem in the offset of its protocol record area (I'll need
Oh you think? ;-D
Post by Neil Horman
another system to compare it too, which I'll have later this week I think).
Either way, it might be useful to have an erroneous dump for testing purposes
Having several samples to test a new features is always good.

And yes, I have a number of known bad DMI tables in my test suite, to
exercise the error paths. But I'm not sure that's what you have... See
my review of your patch.
--
Jean Delvare
SUSE L3 Support

_______________________________________________
https://lists.nongnu.org/mailman/listinfo/dmidecode-devel
Neil Horman
2018-08-02 14:55:15 UTC
Permalink
Starting with version 0x300 the SMBIOS specification defined in more
detail the contents of the management controller type. DMTF further
reserved values to define the Redfish host interface specification.
Update dmidecode to properly parse and present that information

Signed-off-by: Neil Horman <***@tuxdriver.com>
CC: ***@suse.de
CC: ***@redhat.com
CC: ***@redhat.com
CC: ***@hpe.com

---
Change Notes:
V1->V2) Updated string formatting to print matching number of bytes
for unsigned shorts (***@hpe.com)

Adjusted string format for bDescriptor (***@hpe.com)

Prefaced PCI id's with 0x (***@hpe.com)
---
dmidecode.c | 332 +++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 316 insertions(+), 16 deletions(-)

diff --git a/dmidecode.c b/dmidecode.c
index fa6ecf1..ea7619f 100644
--- a/dmidecode.c
+++ b/dmidecode.c
@@ -63,6 +63,7 @@
#include <strings.h>
#include <stdlib.h>
#include <unistd.h>
+#include <arpa/inet.h>

#ifdef __FreeBSD__
#include <errno.h>
@@ -3447,6 +3448,301 @@ static void dmi_tpm_characteristics(u64 code, const char *prefix)
prefix, characteristics[i - 2]);
}

+/*
+ * Type 42 data offsets
+ */
+
+/*
+ * Top Level Offsets
+ */
+#define IFC_TYPE 4
+#define IFC_SDATA_LEN 5
+#define IFC_PROTO_RECORD_BASE 7
+
+/*
+ * Interface specific data offsets
+ */
+#define IFC_DEVICE_TYPE 6
+
+/*
+ * USB Interface descriptor offsets
+ */
+#define USB_DESCRIPTOR 7
+#define USB_VENDOR 0
+#define USB_PRODUCT 2
+#define USB_SERIAL_LENGTH 4
+#define USB_SERIAL_DESCRIPTOR 5
+
+/*
+ * PCI Interface descriptor offsets
+ */
+#define PCI_DESCRIPTOR 7
+#define PCI_VENDOR 0
+#define PCI_DEVICE 2
+#define PCI_SUBVENDOR 4
+#define PCI_SUBDEVICE 6
+
+/*
+ * Protocol Records Offsets
+ */
+#define IFC_PROTO_COUNT 0
+
+/*
+ * Pre Protocol Record Offsets
+ */
+#define PROTO_REC_ID 0
+#define PROTO_REC_LEN 1
+#define PROTO_REC_DATA 2
+
+/*
+ * Record data offsets
+ */
+#define REC_UUID 0
+#define REC_HOST_IP_ASGN_TYPE 16
+#define REC_HOST_IP_ADDR_FMT 17
+#define REC_HOST_ADDR 18
+#define REC_HOST_MASK 34
+#define REC_RFSH_DISC_TYPE 50
+#define REC_RFSH_ADDR_FMT 51
+#define REC_RFSH_ADDR 52
+#define REC_RFSH_MASK 68
+#define REC_RFSH_PORT 84
+#define REC_RFSH_VLAN 86
+#define REC_RFSH_HOST_LEN 90
+#define REC_RFSH_HOSTNAME 91
+
+/*
+ * TYPE 42 Data Field Values
+ */
+
+/*
+ * Top level values
+ */
+#define MCH_NET_HOST_IFC 0x40
+
+/*
+ * Interface specific data values
+ */
+#define IFC_DEVICE_UNKNOWN 0x0
+#define IFC_DEVICE_USB 0x2
+#define IFC_DEVICE_PCI 0x3
+#define IFC_DEVICE_OEM_BASE 0x80
+
+/*
+ * Protocol record data values
+ */
+#define REDFISH_OVER_IP 0x4
+
+/*
+ * Protocol record data values
+ */
+#define IP_UNKNOWN 0
+#define IP_STATIC 0x1
+#define IP_DHCP 0x2
+#define IP_AUTOCONF 0x3
+#define IP_HOSTSEL 0x4
+
+#define ADDR_UNKNOWN 0
+#define ADDR_IPV4 0x1
+#define ADDR_IPV6 0x2
+
+static void decode_management_controller_structure(const struct dmi_header *h, const char *prefix)
+{
+ u8 *data = h->data;
+ u8 type = data[IFC_TYPE];
+ u8 len = data[IFC_SDATA_LEN];
+ u8 count;
+ const char *devname[] = {
+ "Unknown",
+ "Unknown",
+ "USB",
+ "PCI[e]",
+ "OEM",
+ };
+
+ if (h->length < 0x9) {
+ printf("%s Invalid structure\n", prefix);
+ return;
+ }
+
+ printf("%sHost Interface Type: ", prefix);
+ if (type != MCH_NET_HOST_IFC)
+ printf("Unknown\n");
+ else
+ printf("Network\n");
+
+
+ if (len != 0) {
+ type = data[IFC_DEVICE_TYPE];
+ if (type >= IFC_DEVICE_OEM_BASE)
+ type = IFC_DEVICE_UNKNOWN;
+
+ printf("%sDevice Type: %s\n", prefix, devname[type]);
+ if (type == IFC_DEVICE_USB) {
+ /* USB */
+ u8 *usbdata = &data[USB_DESCRIPTOR];
+ printf("%s\tidVendor: 0x%04x\n", prefix, (unsigned short)usbdata[USB_VENDOR]);
+ printf("%s\tidProduct: 0x%04x\n", prefix, (unsigned short)usbdata[USB_PRODUCT]);
+ printf("%s\tSerialNumber:\n", prefix);
+ printf("%s\t\tbDescriptor: 0x%02x\n", prefix, usbdata[USB_SERIAL_DESCRIPTOR]);
+ /* Note bString is not printable here, so skip it */
+ } else if (type == IFC_DEVICE_PCI) {
+ /* PCI */
+ u8 *pcidata = &data[PCI_DESCRIPTOR];
+ printf("%s\tVendorID: 0x%04x\n", prefix, (unsigned short)pcidata[PCI_VENDOR]);
+ printf("%s\tDeviceID: 0x%04x\n", prefix, (unsigned short)pcidata[PCI_DEVICE]);
+ printf("%s\tSubVendorID: 0x%04x\n", prefix, (unsigned short)pcidata[PCI_SUBVENDOR]);
+ printf("%s\tSubDeviceID: 0x%04x\n", prefix, (unsigned short)pcidata[PCI_SUBDEVICE]);
+ }
+ /* Don't mess with unknown types for now */
+ }
+
+ /*
+ * Move to the Protocol Count Area from Table 1
+ * Data[6] points to the start of the interface specific
+ * data, and Data[5] is the length of that region
+ */
+ data = &data[IFC_PROTO_RECORD_BASE+data[5]];
+
+ /* Get the protocol records count */
+ count = (u8)data[IFC_PROTO_COUNT];
+ if (count) {
+ int i, j, k;
+ u8 rid;
+ u8 rlen;
+ u8 *rec = &data[1]; /*first record starts after count value */
+ u8 *rdata;
+ u8 buf[64];
+ u8 uuid[37];
+ u8 assignval;
+ u8 addrtype;
+ u8 hlen;
+ char hname[257];
+
+ const char *assigntype[] = {
+ "Unknown",
+ "Static",
+ "DHCP",
+ "AutoConf",
+ "Host Selected",
+ };
+
+ const char *addressformat[] = {
+ "Unknown",
+ "Ipv4",
+ "Ipv6",
+ };
+
+ printf("%sProtocol Records (%02d):\n", prefix, count);
+ for (i=0; i < count; i++) {
+ rid = (u8)rec[PROTO_REC_ID];
+ rlen = (u8)rec[PROTO_REC_LEN];
+ rdata = &rec[PROTO_REC_DATA];
+
+ if (rid != REDFISH_OVER_IP) {
+ printf("%s\tProtocol ID: %02x (Unknown)\n", prefix, rid);
+ goto next;
+ }
+
+ printf("%s\tProtocol ID: Redfish over IP\n", prefix);
+ memcpy(buf, &rdata[REC_UUID], 16);
+
+ /* Convert UUID to readable characters */
+ for (j=0, k=0; j < 33; j++, k++) {
+ if ((j == 8) || (j == 12) || (j == 16) || (j == 20)) {
+ uuid[k] = '-';
+ k++;
+ }
+
+ if (j & 0x1)
+ uuid[k] = (buf[j/2] >> 4) + 0x30;
+ else
+ uuid[k] = (buf[j/2] & 0x0f) + 0x30;
+
+ if (uuid[j] >= 0x3a)
+ uuid[j] += 7;
+ }
+ uuid[32] = '\0';
+ printf("%s\t\tService UUID: %s\n", prefix, uuid);
+
+ assignval = (u8)rdata[REC_HOST_IP_ASGN_TYPE];
+ if (assignval > IP_HOSTSEL)
+ assignval = IP_UNKNOWN;
+ printf("%s\t\tHost IP Assignment Type: %s\n", prefix, assigntype[assignval]);
+
+ addrtype = (u8)rdata[REC_HOST_IP_ADDR_FMT];
+ if (addrtype > ADDR_IPV6)
+ addrtype = ADDR_UNKNOWN;
+ printf("%s\t\tHost IP Address Format: %s\n", prefix, addressformat[addrtype]);
+
+ /* We only use the Host IP Address and Mask if the assignment type is static */
+ if ((assignval == IP_STATIC) || (assignval == IP_AUTOCONF)) {
+ /* Prints the Host IP Address */
+ printf("%s\t\t%s Address: %s\n", prefix,
+ (addrtype == 0x1 ? "Ipv4" : "Ipv6"),
+ (addrtype == 0x1 ?
+ inet_ntop(AF_INET, (char *)&rdata[REC_HOST_ADDR], (char *)buf, 64) :
+ inet_ntop(AF_INET6, (char *)&rdata[REC_HOST_ADDR], (char *)buf, 64)));
+
+ /* Prints the Host IP Mask */
+ printf("%s\t\t%s Mask: %s\n", prefix,
+ (addrtype == ADDR_IPV4 ? "Ipv4" : "Ipv6"),
+ (addrtype == ADDR_IPV4 ?
+ inet_ntop(AF_INET, (char *)&rdata[REC_HOST_MASK], (char *)buf, 64) :
+ inet_ntop(AF_INET6, (char *)&rdata[REC_HOST_MASK], (char *)buf, 64)));
+ }
+
+ /* Get the Redfish Service IP Discovery Type */
+ assignval = (u8)rdata[REC_RFSH_DISC_TYPE];
+ if (assignval > IP_HOSTSEL)
+ assignval = 0;
+
+ /* Redfish Service IP Discovery type Mirrors Host IP Assignment type */
+ printf("%s\t\tRedfish Service IP Discovery Type: %s\n", prefix, assigntype[assignval]);
+
+ /* Get the Redfish Service IP Address Format */
+ addrtype = (u8)rdata[REC_RFSH_ADDR_FMT];
+ if (addrtype > ADDR_IPV6)
+ addrtype = ADDR_UNKNOWN;
+
+ printf("%s\t\tRedfish Service IP Address Format: %s\n", prefix, addressformat[addrtype]);
+ if ((assignval == IP_STATIC) || (assignval == IP_AUTOCONF)) {
+ u16 port;
+ u32 vlan;
+ /* Prints the Redfish Service Address */
+ printf("%s\t\t%s Redfish Service Address: %s\n", prefix,
+ (addrtype == ADDR_IPV4 ? "Ipv4" : "Ipv6"),
+ (addrtype == ADDR_IPV4 ?
+ inet_ntop(AF_INET, (char *)&rdata[REC_RFSH_ADDR], (char *)buf, 64) :
+ inet_ntop(AF_INET6, (char *)&rdata[REC_RFSH_ADDR], (char *)buf, 64)));
+
+ /* Prints the Redfish Service Mask */
+ printf("%s\t\t%s Redfish Service Mask: %s\n", prefix,
+ (addrtype == ADDR_IPV4 ? "Ipv4" : "Ipv6"),
+ (addrtype == ADDR_IPV4 ?
+ inet_ntop(AF_INET, (char *)&rdata[REC_RFSH_MASK], (char *)buf, 64) :
+ inet_ntop(AF_INET6, (char *)&rdata[REC_RFSH_MASK], (char *)buf, 64)));
+
+ port = (u8)rdata[REC_RFSH_PORT];
+ vlan = (u16)rdata[REC_RFSH_VLAN];
+ printf("%s\t\tRedfish Service Port: %d\n", prefix, port);
+ printf("%s\t\tRedfish Service Vlan: %d\n", prefix, vlan);
+
+ }
+
+ hlen = (u8)rdata[REC_RFSH_HOST_LEN];
+ memcpy(hname, &rdata[REC_RFSH_HOSTNAME], hlen);
+ hname[hlen] = '\0';
+ printf("%s\t\tRedfish Service Hostname: %s\n", prefix, hname);
+next:
+ rec = rec + rlen +1;
+ }
+ }
+
+ return;
+
+}
+
/*
* Main
*/
@@ -4582,22 +4878,26 @@ static void dmi_decode(const struct dmi_header *h, u16 ver)

case 42: /* 7.43 Management Controller Host Interface */
printf("Management Controller Host Interface\n");
- if (h->length < 0x05) break;
- printf("\tInterface Type: %s\n",
- dmi_management_controller_host_type(data[0x04]));
- /*
- * There you have a type-dependent, variable-length
- * part in the middle of the structure, with no
- * length specifier, so no easy way to decode the
- * common, final part of the structure. What a pity.
- */
- if (h->length < 0x09) break;
- if (data[0x04] == 0xF0) /* OEM */
- {
- printf("\tVendor ID: 0x%02X%02X%02X%02X\n",
- data[0x05], data[0x06], data[0x07],
- data[0x08]);
- }
+
+ if (ver < 0x300) {
+ if (h->length < 0x05) break;
+ printf("\tInterface Type: %s\n",
+ dmi_management_controller_host_type(data[0x04]));
+ /*
+ * There you have a type-dependent, variable-length
+ * part in the middle of the structure, with no
+ * length specifier, so no easy way to decode the
+ * common, final part of the structure. What a pity.
+ */
+ if (h->length < 0x09) break;
+ if (data[0x04] == 0xF0) /* OEM */
+ {
+ printf("\tVendor ID: 0x%02X%02X%02X%02X\n",
+ data[0x05], data[0x06], data[0x07],
+ data[0x08]);
+ }
+ } else
+ decode_management_controller_structure(h, "\t\t");
break;

case 43: /* 7.44 TPM Device */
--
2.17.1


_______________________________________________
https://lists.nongnu.org/mailman/listinfo/dmidecode-devel
Jean Delvare
2018-08-07 12:51:51 UTC
Permalink
Hi Neil,
Post by Neil Horman
Starting with version 0x300 the SMBIOS specification defined in more
detail the contents of the management controller type. DMTF further
reserved values to define the Redfish host interface specification.
Update dmidecode to properly parse and present that information
Please provide links to the documents you used to write this patch. I
don't see anything relevant in the SMBIOS specification itself. Every
document needed to understand the code should be mentioned in the
header comment (search for "Additional references:"). Without that
information, I can only review the code from a technical perspective,
but I can't say anything about its functional correctness.

Note: for the time being, the only machine I have access to which
implements type 42 had the type set to 0xF0 (OEM) and an apparently
invalid vendor ID (0xFF0102FF). Your patch changes the output of
dmidecode from useless information to different but equally useless
(and most certainly wrong) information:

Handle 0x0014, DMI type 42, 12 bytes
Management Controller Host Interface
- Interface Type: OEM
- Vendor ID: 0xFF0102FF
+ Host Interface Type: Unknown
+ Device Type: Unknown
+ Protocol Records (16):
+ Protocol ID: 17 (Unknown)
+ Protocol ID: 00 (Unknown)
+ Protocol ID: 00 (Unknown)
+ Protocol ID: 00 (Unknown)
+ Protocol ID: 00 (Unknown)
+ Protocol ID: 82 (Unknown)
+ Protocol ID: 00 (Unknown)
+ Protocol ID: 00 (Unknown)
+ Protocol ID: 00 (Unknown)
+ Protocol ID: 00 (Unknown)
+ Protocol ID: 00 (Unknown)
+ Protocol ID: 01 (Unknown)
+ Protocol ID: 00 (Unknown)
+ Protocol ID: 00 (Unknown)
+ Protocol ID: 00 (Unknown)
+ Protocol ID: 38 (Unknown)
Post by Neil Horman
---
V1->V2) Updated string formatting to print matching number of bytes
Note for next time: please start a new thread for each new iteration of
a patch. Otherwise the comments about different versions of the patch
can interleave and this create confusion.

Robert, thanks for having reviewed v1 of this patch, this is
appreciated.
Post by Neil Horman
---
dmidecode.c | 332 +++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 316 insertions(+), 16 deletions(-)
diff --git a/dmidecode.c b/dmidecode.c
index fa6ecf1..ea7619f 100644
--- a/dmidecode.c
+++ b/dmidecode.c
@@ -63,6 +63,7 @@
#include <strings.h>
#include <stdlib.h>
#include <unistd.h>
+#include <arpa/inet.h>
#ifdef __FreeBSD__
#include <errno.h>
@@ -3447,6 +3448,301 @@ static void dmi_tpm_characteristics(u64 code, const char *prefix)
prefix, characteristics[i - 2]);
}
+/*
+ * Type 42 data offsets
+ */
This should all go in the following section of the source file:

/*
* 7.43 Management Controller Host Interface (Type 42)
*/

The file is pretty large by now, if we don't group the type-specific
code, it will get messy in no time.
Post by Neil Horman
+
+/*
+ * Top Level Offsets
+ */
+#define IFC_TYPE 4
+#define IFC_SDATA_LEN 5
+#define IFC_PROTO_RECORD_BASE 7
+
+/*
+ * Interface specific data offsets
+ */
+#define IFC_DEVICE_TYPE 6
+
+/*
+ * USB Interface descriptor offsets
+ */
+#define USB_DESCRIPTOR 7
+#define USB_VENDOR 0
+#define USB_PRODUCT 2
+#define USB_SERIAL_LENGTH 4
+#define USB_SERIAL_DESCRIPTOR 5
+
+/*
+ * PCI Interface descriptor offsets
+ */
+#define PCI_DESCRIPTOR 7
+#define PCI_VENDOR 0
+#define PCI_DEVICE 2
+#define PCI_SUBVENDOR 4
+#define PCI_SUBDEVICE 6
+
+/*
+ * Protocol Records Offsets
+ */
+#define IFC_PROTO_COUNT 0
+
+/*
+ * Pre Protocol Record Offsets
+ */
+#define PROTO_REC_ID 0
+#define PROTO_REC_LEN 1
+#define PROTO_REC_DATA 2
+
+/*
+ * Record data offsets
+ */
+#define REC_UUID 0
+#define REC_HOST_IP_ASGN_TYPE 16
+#define REC_HOST_IP_ADDR_FMT 17
+#define REC_HOST_ADDR 18
+#define REC_HOST_MASK 34
+#define REC_RFSH_DISC_TYPE 50
+#define REC_RFSH_ADDR_FMT 51
+#define REC_RFSH_ADDR 52
+#define REC_RFSH_MASK 68
+#define REC_RFSH_PORT 84
+#define REC_RFSH_VLAN 86
+#define REC_RFSH_HOST_LEN 90
+#define REC_RFSH_HOSTNAME 91
+
+/*
+ * TYPE 42 Data Field Values
+ */
+
+/*
+ * Top level values
+ */
What does "top level" means in this context? No idea.
Post by Neil Horman
+#define MCH_NET_HOST_IFC 0x40
+
+/*
+ * Interface specific data values
+ */
+#define IFC_DEVICE_UNKNOWN 0x0
+#define IFC_DEVICE_USB 0x2
+#define IFC_DEVICE_PCI 0x3
+#define IFC_DEVICE_OEM_BASE 0x80
+
+/*
+ * Protocol record data values
+ */
+#define REDFISH_OVER_IP 0x4
+
+/*
+ * Protocol record data values
+ */
+#define IP_UNKNOWN 0
+#define IP_STATIC 0x1
+#define IP_DHCP 0x2
+#define IP_AUTOCONF 0x3
+#define IP_HOSTSEL 0x4
+
+#define ADDR_UNKNOWN 0
+#define ADDR_IPV4 0x1
+#define ADDR_IPV6 0x2
I'm quite skeptical about that long list of defines. You will note that
I made the choice to not introduce such defines for any other DMI
structure type. The rationale is that they are only used locally, and
in general only once, and I find it in fact easier to match the
constant directly to the specification, than to have to look up a
separate define. Also note that the code consistently uses hexadecimal
for all field offsets, because that's what the specification uses. You
have a mix of decimal and hexadecimal.

Also all your defines are specific to type 42 but nothing in their name
says so. Imagine how many collisions we would get if we would have
defines for all other types using the same (lack of) pattern... Which
was one of the reasons why I did not go that way in the first place.

In my experience, if the use of a constant directly in the code is not
clear enough, it can be solved by adding a comment, instead of a
define. As a matter of fact, you have many such comments in your code
already (which is very good), making the defines redundant
(IFC_DEVICE_USB and IFC_DEVICE_PCI for example, but also the IP
addresses and masks). Good variable names and printf strings also tend
to clarify what data we are dealing with.
Post by Neil Horman
+static void decode_management_controller_structure(const struct dmi_header *h, const char *prefix)
For consistency, the name of this function should be prefixed with
"dmi_". "decode" and "structure" on the other hand are pretty much
implied and no other type-specific function name has them.

There is no strict 80-column deadline in the dmidecode source code,
however please consider splitting long lines when there is no extra
cost in doing so.
Post by Neil Horman
+{
+ u8 *data = h->data;
+ u8 type = data[IFC_TYPE];
+ u8 len = data[IFC_SDATA_LEN];
+ u8 count;
+ const char *devname[] = {
+ "Unknown",
+ "Unknown",
+ "USB",
+ "PCI[e]",
I think I would prefer "PCI/PCIe".
Post by Neil Horman
+ "OEM",
+ };
+
+ if (h->length < 0x9) {
+ printf("%s Invalid structure\n", prefix);
+ return;
+ }
We don't print such a message anywhere else. The standard action if a
structure is not long enough is to just stop decoding it silently.

Also, according to version 3.2.0 of the specification, the minimum
length for this structure is now 0xB.

You also need to ensure that 0x06 + len + 1 does fit in h->length.
Otherwise you may end up reading beyond the end of the structure.
Post by Neil Horman
+
+ printf("%sHost Interface Type: ", prefix);
+ if (type != MCH_NET_HOST_IFC)
+ printf("Unknown\n");
+ else
+ printf("Network\n");
Could this be consolidated into dmi_management_controller_host_type()?
Post by Neil Horman
+
+
Extra blank line.
Post by Neil Horman
+ if (len != 0) {
+ type = data[IFC_DEVICE_TYPE];
This is dependent on the interface type. I don't know about other
types, but at least for the OEM type (0xf0) the above statement is not
valid.
Post by Neil Horman
+ if (type >= IFC_DEVICE_OEM_BASE)
+ type = IFC_DEVICE_UNKNOWN;
This causes OEM device types to be reported as "Unknwown" instead of
"OEM".
Post by Neil Horman
+
+ printf("%sDevice Type: %s\n", prefix, devname[type]);
This is unsafe. type could be well beyond the size of devname[].

The dmidecode.c source file has dozen of examples of how this should be
handled.
Post by Neil Horman
+ if (type == IFC_DEVICE_USB) {
+ /* USB */
+ u8 *usbdata = &data[USB_DESCRIPTOR];
You need to ensure that len is sufficient to cover all the USB-specific
fields.
Post by Neil Horman
+ printf("%s\tidVendor: 0x%04x\n", prefix, (unsigned short)usbdata[USB_VENDOR]);
Please use WORD() as discussed somewhere else in this discussion thread.
Post by Neil Horman
+ printf("%s\tidProduct: 0x%04x\n", prefix, (unsigned short)usbdata[USB_PRODUCT]);
+ printf("%s\tSerialNumber:\n", prefix);
+ printf("%s\t\tbDescriptor: 0x%02x\n", prefix, usbdata[USB_SERIAL_DESCRIPTOR]);
+ /* Note bString is not printable here, so skip it */
The comment is confusing. Why is bString not printable here? You said
too much, or not enough.
Post by Neil Horman
+ } else if (type == IFC_DEVICE_PCI) {
+ /* PCI */
+ u8 *pcidata = &data[PCI_DESCRIPTOR];
You need to ensure that len is sufficient to cover all the PCI-specific
fields.
Post by Neil Horman
+ printf("%s\tVendorID: 0x%04x\n", prefix, (unsigned short)pcidata[PCI_VENDOR]);
+ printf("%s\tDeviceID: 0x%04x\n", prefix, (unsigned short)pcidata[PCI_DEVICE]);
+ printf("%s\tSubVendorID: 0x%04x\n", prefix, (unsigned short)pcidata[PCI_SUBVENDOR]);
+ printf("%s\tSubDeviceID: 0x%04x\n", prefix, (unsigned short)pcidata[PCI_SUBDEVICE]);
+ }
+ /* Don't mess with unknown types for now */
+ }
+
+ /*
+ * Move to the Protocol Count Area from Table 1
What is "Table 1"?
Post by Neil Horman
+ * Data[6] points to the start of the interface specific
+ * data, and Data[5] is the length of that region
+ */
+ data = &data[IFC_PROTO_RECORD_BASE+data[5]];
Notice the inconsistent mix of define and raw number, as well as the
mix of case for "data" ;-) At this point, data[5] is already known as
"len", you should use it for consistency. Also, you mention data[6] but
IFC_PROTO_RECORD_BASE is 7, not 6. This is the kind of nasty side
effects of using defines, which I was mentioning previously.
Post by Neil Horman
+
+ /* Get the protocol records count */
+ count = (u8)data[IFC_PROTO_COUNT];
Looks wrong to me. At this point, data points to h + 7 + len bytes.
This corresponds to the "Protocol Records" field in the specification.
However the statement above assumes that data is pointing to the
"Number of Protocol Records" field.
Post by Neil Horman
+ if (count) {
I believe that this block of code is crying to be a separate function,
that would decode one protocol. Surrounded by a loop to walk over the
list of protocols.
Post by Neil Horman
+ int i, j, k;
+ u8 rid;
+ u8 rlen;
+ u8 *rec = &data[1]; /*first record starts after count value */
Leading space and capital in comment, please.
Post by Neil Horman
+ u8 *rdata;
+ u8 buf[64];
+ u8 uuid[37];
+ u8 assignval;
+ u8 addrtype;
+ u8 hlen;
+ char hname[257];
hlen is 255 maximum. Plus the NUL terminator, 256, not 257. But you
probably don't need that buffer in the first place, see below.

Note: such a long list of local variables is usually a good sign that
some of the code should be moved to helper functions. More about that
below.
Post by Neil Horman
+
+ const char *assigntype[] = {
+ "Unknown",
+ "Static",
+ "DHCP",
+ "AutoConf",
+ "Host Selected",
+ };
+
+ const char *addressformat[] = {
+ "Unknown",
+ "Ipv4",
+ "Ipv6",
+ };
The standard casing is IPv[46] (capital P). There are more occurrences
below.
Post by Neil Horman
+
+ printf("%sProtocol Records (%02d):\n", prefix, count);
+ for (i=0; i < count; i++) {
Spaces around "=". Before you attempt to decode a protocol, you have to
ensure that its record fully fits in h->length.
Post by Neil Horman
+ rid = (u8)rec[PROTO_REC_ID];
+ rlen = (u8)rec[PROTO_REC_LEN];
Both casts seem useless.
Post by Neil Horman
+ rdata = &rec[PROTO_REC_DATA];
+
+ if (rid != REDFISH_OVER_IP) {
+ printf("%s\tProtocol ID: %02x (Unknown)\n", prefix, rid);
+ goto next;
+ }
+
+ printf("%s\tProtocol ID: Redfish over IP\n", prefix);
"Redfish over IP" is the protocol name, not ID. So just "Protocol:"
would do.
Post by Neil Horman
+ memcpy(buf, &rdata[REC_UUID], 16);
+
+ /* Convert UUID to readable characters */
+ for (j=0, k=0; j < 33; j++, k++) {
+ if ((j == 8) || (j == 12) || (j == 16) || (j == 20)) {
+ uuid[k] = '-';
+ k++;
+ }
+
+ if (j & 0x1)
+ uuid[k] = (buf[j/2] >> 4) + 0x30;
+ else
+ uuid[k] = (buf[j/2] & 0x0f) + 0x30;
+
+ if (uuid[j] >= 0x3a)
+ uuid[j] += 7;
+ }
+ uuid[32] = '\0';
+ printf("%s\t\tService UUID: %s\n", prefix, uuid);
Ouch. Please reuse dmi_system_uuid(). If you can't reuse it directly,
adjust it so that you can reuse it. But don't duplicate the effort here.

Also, just for completeness, you declared uuid as a 37 character
buffer, but in the end you NUL-terminate it at offset 32. Makes no
sense to me. "j < 33" in the loop condition looks equally wrong to me.
Post by Neil Horman
+
+ assignval = (u8)rdata[REC_HOST_IP_ASGN_TYPE];
+ if (assignval > IP_HOSTSEL)
+ assignval = IP_UNKNOWN;
+ printf("%s\t\tHost IP Assignment Type: %s\n", prefix, assigntype[assignval]);
+
+ addrtype = (u8)rdata[REC_HOST_IP_ADDR_FMT];
+ if (addrtype > ADDR_IPV6)
+ addrtype = ADDR_UNKNOWN;
+ printf("%s\t\tHost IP Address Format: %s\n", prefix, addressformat[addrtype]);
Again, there is a standardized way to deal with such enumerated lists
in dmidecode, please stick to it. Among the obvious benefit of being
consistent, it also allows differentiating between "Unknown" being set
by the BIOS itself and an actual value being set by the BIOS but not
yet known to dmidecode (which is reported as "<OUT OF SPEC>", and is
often addressed by the following version of the specification.)

And again the casts to u8 are not needed.
Post by Neil Horman
+
+ /* We only use the Host IP Address and Mask if the assignment type is static */
+ if ((assignval == IP_STATIC) || (assignval == IP_AUTOCONF)) {
You don't need all these parentheses.
Post by Neil Horman
+ /* Prints the Host IP Address */
+ printf("%s\t\t%s Address: %s\n", prefix,
+ (addrtype == 0x1 ? "Ipv4" : "Ipv6"),
You assume that the address type must be either IPv4 or IPv6, while you
explicitly test for unknown address type above. This needs proper
"error" handling.
Post by Neil Horman
+ (addrtype == 0x1 ?
+ inet_ntop(AF_INET6, (char *)&rdata[REC_HOST_ADDR], (char *)buf, 64)));
+
+ /* Prints the Host IP Mask */
+ printf("%s\t\t%s Mask: %s\n", prefix,
+ (addrtype == ADDR_IPV4 ? "Ipv4" : "Ipv6"),
+ (addrtype == ADDR_IPV4 ?
+ inet_ntop(AF_INET6, (char *)&rdata[REC_HOST_MASK], (char *)buf, 64)));
+ }
Any idea how portable is inet_ntop()? Note that in theory it can fail,
but you don't check for that. I admit I can't see how that would happen
here, but if it did, printf would cause a segfault.

Cast to char* for the source doesn't seem to be needed (inet_ntop takes
void* for src according to the man page).

Several parentheses are again superfluous in the printfs.

There is redundancy among the inet_ntop calls. Instead of:
addrtype == ADDR_IPV4 ? inet_ntop(AF_INET, ...) : net_ntop(AF_INET6, ...)
you could do:
inet_ntop(addrtype == ADDR_IPV4 ? AF_INET : AF_INET6, ...)

The decoding of these two fields is very similar, please consider
refactoring the code to a separate function. Which you could also reuse
below...
Post by Neil Horman
+
+ /* Get the Redfish Service IP Discovery Type */
+ assignval = (u8)rdata[REC_RFSH_DISC_TYPE];
+ if (assignval > IP_HOSTSEL)
+ assignval = 0;
Not consistent with how you wrote it for REC_HOST_IP_ASGN_TYPE and
REC_HOST_IP_ADDR_FMT above. Anyway, again, I would prefer if you stick
with the dmidecode way to deal with enumerated values.
Post by Neil Horman
+
+ /* Redfish Service IP Discovery type Mirrors Host IP Assignment type */
No need for leading capital for "mirror", methinks.
Post by Neil Horman
+ printf("%s\t\tRedfish Service IP Discovery Type: %s\n", prefix, assigntype[assignval]);
+
+ /* Get the Redfish Service IP Address Format */
+ addrtype = (u8)rdata[REC_RFSH_ADDR_FMT];
+ if (addrtype > ADDR_IPV6)
+ addrtype = ADDR_UNKNOWN;
+
+ printf("%s\t\tRedfish Service IP Address Format: %s\n", prefix, addressformat[addrtype]);
+ if ((assignval == IP_STATIC) || (assignval == IP_AUTOCONF)) {
Double space before "||". You don't need all these parentheses.
Post by Neil Horman
+ u16 port;
+ u32 vlan;
A blank line here would be appreciated.
Post by Neil Horman
+ /* Prints the Redfish Service Address */
+ printf("%s\t\t%s Redfish Service Address: %s\n", prefix,
+ (addrtype == ADDR_IPV4 ? "Ipv4" : "Ipv6"),
+ (addrtype == ADDR_IPV4 ?
+ inet_ntop(AF_INET6, (char *)&rdata[REC_RFSH_ADDR], (char *)buf, 64)));
+
+ /* Prints the Redfish Service Mask */
+ printf("%s\t\t%s Redfish Service Mask: %s\n", prefix,
+ (addrtype == ADDR_IPV4 ? "Ipv4" : "Ipv6"),
+ (addrtype == ADDR_IPV4 ?
+ inet_ntop(AF_INET6, (char *)&rdata[REC_RFSH_MASK], (char *)buf, 64)));
+
+ port = (u8)rdata[REC_RFSH_PORT];
+ vlan = (u16)rdata[REC_RFSH_VLAN];
That cast may crash on IA-64. Use WORD().

Are there holes in the structure? According to your defines, there is
room for 2 bytes for the port and 4 bytes for the vlan. This matches
your variable types, but not the casts above. Please double check.
Post by Neil Horman
+ printf("%s\t\tRedfish Service Port: %d\n", prefix, port);
+ printf("%s\t\tRedfish Service Vlan: %d\n", prefix, vlan);
%u is more appropriate for unsigned values.
Post by Neil Horman
+
Extra blank line.
Post by Neil Horman
+ }
+
+ hlen = (u8)rdata[REC_RFSH_HOST_LEN];
Useless cast.
Post by Neil Horman
+ memcpy(hname, &rdata[REC_RFSH_HOSTNAME], hlen);
+ hname[hlen] = '\0';
+ printf("%s\t\tRedfish Service Hostname: %s\n", prefix, hname);
You can use %s with a precision modifier to avoid the need to copy the
characters to a NUL-terminated buffer. Quoting printf(3):

"If a precision is given, no null byte need be present;"

It's a bit tricky because the precision is not known in advance, but
the following should work:

printf("%s\t\tRedfish Service Hostname: %*s\n", prefix, hlen,
&rdata[REC_RFSH_HOSTNAME]);
Post by Neil Horman
+ rec = rec + rlen +1;
You can use "+=".

And I think you are off by 1. Each record is made of ID + length byte +
data, which is 1 + 1 + rlen, so 2 + rlen.
Post by Neil Horman
+ }
+ }
+
+ return;
+
Extra blank line. And return itself is not needed for a void function.
Post by Neil Horman
+}
+
/*
* Main
*/
@@ -4582,22 +4878,26 @@ static void dmi_decode(const struct dmi_header *h, u16 ver)
case 42: /* 7.43 Management Controller Host Interface */
printf("Management Controller Host Interface\n");
- if (h->length < 0x05) break;
- printf("\tInterface Type: %s\n",
- dmi_management_controller_host_type(data[0x04]));
- /*
- * There you have a type-dependent, variable-length
- * part in the middle of the structure, with no
- * length specifier, so no easy way to decode the
- * common, final part of the structure. What a pity.
- */
- if (h->length < 0x09) break;
- if (data[0x04] == 0xF0) /* OEM */
- {
- printf("\tVendor ID: 0x%02X%02X%02X%02X\n",
- data[0x05], data[0x06], data[0x07],
- data[0x08]);
- }
+
Unneeded blank line.
Post by Neil Horman
+ if (ver < 0x300) {
Why are you splitting on version 3.0.0? As far as I can see, the
definition for type 42 structures changed in SMBIOS specification
3.2.0, so you should test for ver < 0x0302. That explains why the
output is now broken on my system (which has ver == 0x0301).

I'm very happy that the DMTF changed it, by the way. The original
definition was simply unusable.

Brace goes on following line. Almost all your curly brace placements
are infringing the coding style used by dmidecode, even though I did
not mention it every time so that we could focus on the code itself. I
don't like it either, but that's how things are for now, please be
consistent. If I would start the project today, I would make different
coding style choices, there is no question about that.
Post by Neil Horman
+ if (h->length < 0x05) break;
+ printf("\tInterface Type: %s\n",
+ dmi_management_controller_host_type(data[0x04]));
+ /*
+ * There you have a type-dependent, variable-length
+ * part in the middle of the structure, with no
+ * length specifier, so no easy way to decode the
+ * common, final part of the structure. What a pity.
+ */
+ if (h->length < 0x09) break;
+ if (data[0x04] == 0xF0) /* OEM */
+ {
+ printf("\tVendor ID: 0x%02X%02X%02X%02X\n",
+ data[0x05], data[0x06], data[0x07],
+ data[0x08]);
+ }
+ } else
+ decode_management_controller_structure(h, "\t\t");
The double tab makes the output too indented, I think the prefix should
be just "\t".
Post by Neil Horman
break;
case 43: /* 7.44 TPM Device */
One issue with your patch is that the Interface Type Specific Data part
of OEM records is no longer decoded for recent SMBIOS implementations.
This could lead to regressions for some users (although to be honest
type 42 records are not exactly popular, so that's essentially
theoretical...)

By the way, I am considering the possibility to stop decoding type 42
records completely for SMBIOS implementations < 3.2.0 because the
definition was just too crappy. I have yet to see a valid type 42
record other than yours...

Thanks,
--
Jean Delvare
SUSE L3 Support

_______________________________________________
https://lists.nongnu.org/mailman/listinfo/dmidecode-devel
Loading...