Re: [PATCH] Update to support DSP0270 v1.3.0

[Jean Delvare]

Hi John,

On Tue,  9 May 2023 23:03:19 +0800, John Chung wrote:
> Added version 2 of USB and PCI/PCIe device descriptors.
> 
>   * USB Network Interface v2
>   * PCI/PCIe Network Interface v2
> 
> Signed-off-by: John Chung <john.chung@arm.com>

Sorry for the late answer and thanks for your contribution. Review
inline.

It would be really helpful for me to have a sample to test this piece
of code, unfortunately I do not currently have any sample in my DMI
table dump collection implementing either of these new device types. If
you have access to such machines, would you possibly capture a table
dump for me? This can be achieved by running "dmidecode --dump-bin".
You can send the generated binary file directly to me by email.

> ---
>  dmidecode.c | 151 ++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 124 insertions(+), 27 deletions(-)
> 
> diff --git a/dmidecode.c b/dmidecode.c
> index 1d5411a..17981a4 100644
> --- a/dmidecode.c
> +++ b/dmidecode.c
> @@ -57,7 +57,7 @@
>   *    Family "2.0", Level 00, Revision 00.43, January 26, 2015
>   *    
> https://trustedcomputinggroup.org/pc-client-platform-tpm-profile-ptp-specification/
>   *  - "RedFish Host Interface Specification" (DMTF DSP0270)
> - *    https://www.dmtf.org/sites/default/files/DSP0270_1.0.1.pdf
> + *    https://www.dmtf.org/sites/default/files/DSP0270_1.3.0.pdf

This link doesn't work for me, the correct link seems to be:

https://www.dmtf.org/sites/default/files/standards/documents/DSP0270_1.3.0.pdf

>   */
>  
>  #include <fcntl.h>
> @@ -3813,7 +3813,7 @@ static const char *dmi_protocol_record_type(u8 type)
>  }
>  
>  /*
> - * DSP0270: 8.6: Protocol IP Assignment types
> + * DSP0270: 8.4.2: Protocol IP Assignment types
>   */
>  static const char *dmi_protocol_assignment_type(u8 type)
>  {
> @@ -3831,7 +3831,7 @@ static const char *dmi_protocol_assignment_type(u8 type)
>  }
>  
>  /*
> - * DSP0270: 8.6: Protocol IP Address type
> + * DSP0270: 8.4.3: Protocol IP Address type
>   */
>  static const char *dmi_address_type(u8 type)
>  {
> @@ -3847,7 +3847,7 @@ static const char *dmi_address_type(u8 type)
>  }
>  
>  /*
> - *  DSP0270: 8.6 Protocol Address decode
> + *  DSP0270: 8.4.3 Protocol Address decode
>   */
>  static const char *dmi_address_decode(u8 *data, char *storage, u8 addrtype)
>  {
> @@ -3859,7 +3859,7 @@ static const char *dmi_address_decode(u8 *data, char 
> *storage, u8 addrtype)
>  }
>  
>  /*
> - * DSP0270: 8.5: Parse the protocol record format
> + * DSP0270: 8.4: Parse the protocol record format
>   */
>  static void dmi_parse_protocol_record(u8 *rec)
>  {
> @@ -3874,11 +3874,11 @@ static void dmi_parse_protocol_record(u8 *rec)
>       const char *hname;
>       char attr[38];
>  
> -     /* DSP0270: 8.5: Protocol Identifier */
> +     /* DSP0270: 8.4: Protocol Identifier */
>       rid = rec[0x0];
> -     /* DSP0270: 8.5: Protocol Record Length */
> +     /* DSP0270: 8.4: Protocol Record Length */
>       rlen = rec[0x1];
> -     /* DSP0270: 8.5: Protocol Record Data */
> +     /* DSP0270: 8.4: Protocol Record Data */
>       rdata = &rec[0x2];
>  
>       pr_attr("Protocol ID", "%02x (%s)", rid,
> @@ -3887,7 +3887,7 @@ static void dmi_parse_protocol_record(u8 *rec)
>       /*
>        * Don't decode anything other than Redfish for now
>        * Note 0x4 is Redfish over IP in 7.43.2
> -      * and DSP0270: 8.5
> +      * and DSP0270: 8.4
>        */
>       if (rid != 0x4)
>               return;
> @@ -3901,7 +3901,7 @@ static void dmi_parse_protocol_record(u8 *rec)
>               return;
>  
>       /*
> -      * DSP0270: 8.6: Redfish Over IP Service UUID
> +      * DSP0270: 8.4.1: Redfish Over IP Service UUID
>        * Note: ver is hardcoded to 0x311 here just for
>        * convenience.  It could get passed from the SMBIOS
>        * header, but that's a lot of passing of pointers just
> @@ -3914,7 +3914,7 @@ static void dmi_parse_protocol_record(u8 *rec)
>       dmi_system_uuid(pr_subattr, "Service UUID", &rdata[0], 0x311);
>  
>       /*
> -      * DSP0270: 8.6: Redfish Over IP Host IP Assignment Type
> +      * DSP0270: 8.4.1: Redfish Over IP Host IP Assignment Type
>        * Note, using decimal indices here, as the DSP0270
>        * uses decimal, so as to make it more comparable
>        */
> @@ -3922,34 +3922,34 @@ static void dmi_parse_protocol_record(u8 *rec)
>       pr_subattr("Host IP Assignment Type", "%s",
>               dmi_protocol_assignment_type(assign_val));
>  
> -     /* DSP0270: 8.6: Redfish Over IP Host Address format */
> +     /* DSP0270: 8.4.1: Redfish Over IP Host Address format */
>       addrtype = rdata[17];
>       addrstr = dmi_address_type(addrtype);
>       pr_subattr("Host IP Address Format", "%s",
>               addrstr);
>  
> -     /* DSP0270: 8.6 IP Assignment types */
> +     /* DSP0270: 8.4.1 IP Assignment types */
>       /* We only use the Host IP Address and Mask if the assignment type is 
> static */
>       if (assign_val == 0x1 || assign_val == 0x3)
>       {
> -             /* DSP0270: 8.6: the Host IPv[4|6] Address */
> +             /* DSP0270: 8.4.1: the Host IPv[4|6] Address */
>               sprintf(attr, "%s Address", addrstr);
>               pr_subattr(attr, "%s",
>                       dmi_address_decode(&rdata[18], buf, addrtype));
>  
> -             /* DSP0270: 8.6: Prints the Host IPv[4|6] Mask */
> +             /* DSP0270: 8.4.1: Prints the Host IPv[4|6] Mask */
>               sprintf(attr, "%s Mask", addrstr);
>               pr_subattr(attr, "%s",
>                       dmi_address_decode(&rdata[34], buf, addrtype));
>       }
>  
> -     /* DSP0270: 8.6: Get the Redfish Service IP Discovery Type */
> +     /* DSP0270: 8.4.1: Get the Redfish Service IP Discovery Type */
>       assign_val = rdata[50];
>       /* Redfish Service IP Discovery type mirrors Host IP Assignment type */
>       pr_subattr("Redfish Service IP Discovery Type", "%s",
>               dmi_protocol_assignment_type(assign_val));
>  
> -     /* DSP0270: 8.6: Get the Redfish Service IP Address Format */
> +     /* DSP0270: 8.4.1: Get the Redfish Service IP Address Format */
>       addrtype = rdata[51];
>       addrstr = dmi_address_type(addrtype);
>       pr_subattr("Redfish Service IP Address Format", "%s",
> @@ -3960,30 +3960,30 @@ static void dmi_parse_protocol_record(u8 *rec)
>               u16 port;
>               u32 vlan;
>  
> -             /* DSP0270: 8.6: Prints the Redfish IPv[4|6] Service Address */
> +             /* DSP0270: 8.4.1: Prints the Redfish IPv[4|6] Service Address 
> */
>               sprintf(attr, "%s Redfish Service Address", addrstr);
>               pr_subattr(attr, "%s",
>                       dmi_address_decode(&rdata[52], buf,
>                       addrtype));
>  
> -             /* DSP0270: 8.6: Prints the Redfish IPv[4|6] Service Mask */
> +             /* DSP0270: 8.4.1: Prints the Redfish IPv[4|6] Service Mask */
>               sprintf(attr, "%s Redfish Service Mask", addrstr);
>               pr_subattr(attr, "%s",
>                       dmi_address_decode(&rdata[68], buf,
>                       addrtype));
>  
> -             /* DSP0270: 8.6: Redfish vlan and port info */
> +             /* DSP0270: 8.4.1: Redfish vlan and port info */
>               port = WORD(&rdata[84]);
>               vlan = DWORD(&rdata[86]);
>               pr_subattr("Redfish Service Port", "%hu", port);
>               pr_subattr("Redfish Service Vlan", "%u", vlan);
>       }
>  
> -     /* DSP0270: 8.6: Redfish host length and name */
> +     /* DSP0270: 8.4.1: Redfish host length and name */
>       hlen = rdata[90];
>  
>       /*
> -      * DSP0270: 8.6: The length of the host string + 91 (the minimum
> +      * DSP0270: 8.4.1: The length of the host string + 91 (the minimum
>        * size of a protocol record) cannot exceed the record length
>        * (rec[0x1])
>        */
> @@ -4004,15 +4004,39 @@ static const char *dmi_parse_device_type(u8 type)
>       const char *devname[] = {
>               "USB",          /* 0x2 */
>               "PCI/PCIe",     /* 0x3 */
> +             "USB v2",       /* 0x4 */
> +             "PCI/PCIe v2",  /* 0x5 */
>       };
>  
> -     if (type >= 0x2 && type <= 0x3)
> +     if (type >= 0x2 && type <= 0x5)
>               return devname[type - 0x2];
>       if (type >= 0x80)
>               return "OEM";
>       return out_of_spec;
>  }
>  
> +/*
> + * DSP0270: 8.3.7: Device Characteristics
> + */
> +static void dmi_device_characteristics(u16 code)
> +{
> +     const char *characteristics[] = {
> +             "Credential bootstrapping via IPMI is supported", /* 0 */
> +             /* Reserved */
> +     };
> +
> +     if ((code & 0x1) == 0)
> +             pr_list_item("None");
> +     else
> +     {
> +             int i;
> +
> +             for (i = 0; i < 1; i++)
> +                     if (code & (1 << i))
> +                             pr_list_item("%s", characteristics[i]);
> +     }
> +}
> +
>  static void dmi_parse_controller_structure(const struct dmi_header *h)
>  {
>       int i;
> @@ -4055,7 +4079,7 @@ static void dmi_parse_controller_structure(const struct 
> dmi_header *h)
>  
>       if (len != 0)
>       {
> -             /* DSP0270: 8.3 Table 2: Device Type */
> +             /* DSP0270: 8.3.1 Table 3: Device Type values */
>               type = data[0x6];
>  
>               pr_attr("Device Type", "%s",
> @@ -4092,7 +4116,80 @@ static void dmi_parse_controller_structure(const 
> struct dmi_header *h)
>                       pr_attr("SubDeviceID", "0x%04x",
>                               WORD(&pcidata[0x6]));
>               }
> -             else if (type == 0x4 && len >= 5)
> +             else if (type == 0x4 && len >= 0x0d)
> +             {
> +                     /* USB Device Type v2 - need at least 12 bytes */
> +                     u8 *usbdata_v2 = &data[7];

I would omit the _v2, it only makes the variable name longer without
adding much value.

> +                     /* USB Device Descriptor v2: idVendor */
> +                     pr_attr("idVendor", "0x%04x",
> +                             WORD(&usbdata_v2[0x1]));
> +                     /* USB Device Descriptor v2: idProduct */
> +                     pr_attr("idProduct", "0x%04x",
> +                             WORD(&usbdata_v2[0x3]));
> +
> +                     /*
> +                      * USB Serial number is here, but its useless, don't
> +                      * bother decoding it
> +                      */
> +
> +                     /* USB Device Descriptor v2: MAC Address */
> +                     pr_attr("MAC Address", "%02x:%02x:%02x:%02x:%02x:%02x",
> +                             usbdata_v2[0x6], usbdata_v2[0x7], 
> usbdata_v2[0x8],
> +                             usbdata_v2[0x9], usbdata_v2[0xa], 
> usbdata_v2[0xb]);
> +
> +                     /* DSP0270 v1.3.0 support */
> +                     if (usbdata_v2[0x0] == 0x11)

You want to test for >= 0x11, as we expect future iterations of the
specification to be compatible. If new fields get added after the
existing ones, you still want to decode everything you already know
about.

I would also test len rather than usbdata_v2[0x0], even though they
should be the same value. len has been checked against buffer overrun
earlier while usbdata_v2[0x0] has not.

To be honest I don't understand why the specification authors added
this redundant length field at the beginning of the type-specific data,
as far as I can see it only adds confusion and wastes one byte. The
nested structures implemented by this specification were already quite
confusing before, that really does not help :-(

I also don't understand why they did not simply add fields to existing
types 0x2 (USB) and 0x3 (PCI) instead of creating separate types. Oh
well...

> +                     {
> +                             /* USB Device Descriptor v2: Device 
> Characteristics */
> +                             pr_list_start("Device Characteristics", NULL);
> +                             dmi_device_characteristics(WORD(usbdata_v2 + 
> 0xc));

For consistency, this should be written WORD(&usbdata_v2[0xc]), even
though this is the only function in dmidecode where this notation is
used.

> +                             pr_list_end();
> +                             /* USB Device Descriptor v2: Credential 
> Bootstrapping Handle */
> +                             pr_attr("Credential Bootstrapping Handle", 
> "0x%04x",
> +                                             WORD(&usbdata_v2[0xe]));

Decoding this value only makes sense if credential bootstrapping via
IPMI is actually supported. According to the specification, if it isn't
supported, the value will be 0xffff and shouldn't be displayed.

> +                     }
> +             }
> +             else if (type == 0x5 && len >= 0x14)
> +             {
> +                     /* PCI Device Type v2 - Need at least 19 bytes */
> +                     u8 *pcidata_v2 = &data[0x7];

Again I'd omit _v2.

> +                     /* PCI Device Descriptor v2: VendorID */
> +                     pr_attr("VendorID", "0x%04x",
> +                             WORD(&pcidata_v2[0x1]));
> +                     /* PCI Device Descriptor v2: DeviceID */
> +                     pr_attr("DeviceID", "0x%04x",
> +                             WORD(&pcidata_v2[0x3]));
> +                     /* PCI Device Descriptor v2: PCI SubvendorID */
> +                     pr_attr("SubVendorID", "0x%04x",
> +                             WORD(&pcidata_v2[0x5]));
> +                     /* PCI Device Descriptor v2: PCI SubdeviceID */
> +                     pr_attr("SubDeviceID", "0x%04x",
> +                             WORD(&pcidata_v2[0x7]));
> +                     /* PCI Device Descriptor v2: MAC Address */
> +                     pr_attr("MAC Address", "%02x:%02x:%02x:%02x:%02x:%02x",
> +                             pcidata_v2[0x9], pcidata_v2[0xa], 
> pcidata_v2[0xb],
> +                             pcidata_v2[0xc], pcidata_v2[0xd], 
> pcidata_v2[0xe]);
> +                     /* PCI Device Descriptor v2: Segment Group Number */
> +                     pr_attr("Segment Group Number", "0x%04x",
> +                             WORD(&pcidata_v2[0xf]));
> +                     /* PCI Device Descriptor v2: Bus Number */
> +                     pr_attr("BusNumber", "0x%02x", &pcidata_v2[0x11]);
> +                     /* PCI Device Descriptor v2: Device/Function Number */
> +                     pr_attr("Dev/Func Number", "0x%02x", &pcidata_v2[0x12]);

I would prefer if you combine these last 3 fields to display the PCI
device bus address using the canonical format as printed by lspci for
example. You can use dmi_slot_segment_bus_func() for that purpose (or
use it as inspiration for your own code).

> +
> +                     /* DSP0270 v1.3.0 support */
> +                     if (pcidata_v2[0x0] == 0x18)

Same comments as for the USB v2 code above.

> +                     {
> +                             /* PCI Device Descriptor v2: Device 
> Characteristics */
> +                             pr_list_start("Device Characteristics", NULL);
> +                             dmi_device_characteristics(WORD(pcidata_v2 + 
> 0x13));

WORD(&pcidata_v2[0x13]) for consistency.

> +                             pr_list_end();
> +                             /* PCI Device Descriptor v2: Credential 
> Bootstrapping Handle */
> +                             pr_attr("Credential Bootstrapping Handle", 
> "0x%04x",
> +                                             WORD(&pcidata_v2[0x15]));

Same comments as for the USB v2 code above.

> +                     }
> +             }
> +             else if (type >= 0x80 && len >= 5)

That was a plain bug in the original code, which will cause newer
implementations to be improperly decoded by older versions of dmidecode
:-(

I think this would deserve a dedicated commit with a Fixes: tag, for
easier backporting.

>               {
>                       /* OEM Device Type - Need at least 4 bytes */
>                       u8 *oemdata = &data[0x7];
> @@ -4105,7 +4202,7 @@ static void dmi_parse_controller_structure(const struct 
> dmi_header *h)
>       }
>  
>       /*
> -      * DSP0270: 8.2 and 8.5: Protocol record count and protocol records
> +      * DSP0270: 8.2 and 8.4: Protocol record count and protocol records
>        * Move to the Protocol Count.
>        */
>       data = &data[total_read];
> @@ -4148,7 +4245,7 @@ static void dmi_parse_controller_structure(const struct 
> dmi_header *h)
>                       dmi_parse_protocol_record(rec);
>  
>                       /*
> -                      * DSP0270: 8.6
> +                      * DSP0270: 8.4.1
>                        * Each record is rec[1] bytes long, starting at the
>                        * data byte immediately following the length field.
>                        * That means we need to add the byte for the rec id,


-- 
Jean Delvare
SUSE L3 Support