From e0c818e00443ce4a704519d85a21e3c14179e3a6 Mon Sep 17 00:00:00 2001 From: Robert Richter Date: Fri, 16 Feb 2024 16:58:43 +0100 Subject: [PATCH] cxl/pci: Get rid of pointer arithmetic reading CDAT table Reading the CDAT table using DOE requires a Table Access Response Header in addition to the CDAT entry. In current implementation this has caused offsets with sizeof(__le32) to the actual buffers. This led to hardly readable code and even bugs. E.g., see fix of devm_kfree() in read_cdat_data(): commit c65efe3685f5 ("cxl/cdat: Free correct buffer on checksum error") Rework code to avoid calculations with sizeof(__le32). Introduce struct cdat_doe_rsp for this which contains the Table Access Response Header and a variable payload size for various data structures afterwards to access the CDAT table and its CDAT Data Structures without recalculating buffer offsets. Cc: Lukas Wunner Cc: Fan Ni Reviewed-by: Dave Jiang Signed-off-by: Robert Richter Reviewed-by: Jonathan Cameron Link: https://lore.kernel.org/r/20240216155844.406996-3-rrichter@amd.com Signed-off-by: Dan Williams --- drivers/cxl/core/pci.c | 77 ++++++++++++++++++++++-------------------- drivers/cxl/cxlpci.h | 24 +++++++++++++ 2 files changed, 65 insertions(+), 36 deletions(-) diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c index c8214990c230..184deff887d0 100644 --- a/drivers/cxl/core/pci.c +++ b/drivers/cxl/core/pci.c @@ -544,55 +544,57 @@ static int cxl_cdat_get_length(struct device *dev, static int cxl_cdat_read_table(struct device *dev, struct pci_doe_mb *doe_mb, - void *cdat_table, size_t *cdat_length) + struct cdat_doe_rsp *rsp, size_t *length) { - size_t length = *cdat_length + sizeof(__le32); - __le32 *data = cdat_table; - int entry_handle = 0; + size_t received, remaining = *length; + unsigned int entry_handle = 0; + union cdat_data *data; __le32 saved_dw = 0; do { __le32 request = CDAT_DOE_REQ(entry_handle); - struct cdat_entry_header *entry; - size_t entry_dw; int rc; rc = pci_doe(doe_mb, PCI_DVSEC_VENDOR_ID_CXL, CXL_DOE_PROTOCOL_TABLE_ACCESS, &request, sizeof(request), - data, length); + rsp, sizeof(*rsp) + remaining); if (rc < 0) { dev_err(dev, "DOE failed: %d", rc); return rc; } - /* 1 DW Table Access Response Header + CDAT entry */ - entry = (struct cdat_entry_header *)(data + 1); - if ((entry_handle == 0 && - rc != sizeof(__le32) + sizeof(struct cdat_header)) || - (entry_handle > 0 && - (rc < sizeof(__le32) + sizeof(*entry) || - rc != sizeof(__le32) + le16_to_cpu(entry->length)))) + if (rc < sizeof(*rsp)) return -EIO; + data = (union cdat_data *)rsp->data; + received = rc - sizeof(*rsp); + + if (entry_handle == 0) { + if (received != sizeof(data->header)) + return -EIO; + } else { + if (received < sizeof(data->entry) || + received != le16_to_cpu(data->entry.length)) + return -EIO; + } + /* Get the CXL table access header entry handle */ entry_handle = FIELD_GET(CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE, - le32_to_cpu(data[0])); - entry_dw = rc / sizeof(__le32); - /* Skip Header */ - entry_dw -= 1; + le32_to_cpu(rsp->doe_header)); + /* * Table Access Response Header overwrote the last DW of * previous entry, so restore that DW */ - *data = saved_dw; - length -= entry_dw * sizeof(__le32); - data += entry_dw; - saved_dw = *data; + rsp->doe_header = saved_dw; + remaining -= received; + rsp = (void *)rsp + received; + saved_dw = rsp->doe_header; } while (entry_handle != CXL_DOE_TABLE_ACCESS_LAST_ENTRY); /* Length in CDAT header may exceed concatenation of CDAT entries */ - *cdat_length -= length - sizeof(__le32); + *length -= remaining; return 0; } @@ -620,8 +622,8 @@ void read_cdat_data(struct cxl_port *port) struct pci_doe_mb *doe_mb; struct pci_dev *pdev = NULL; struct cxl_memdev *cxlmd; - size_t cdat_length; - void *cdat_table, *cdat_buf; + struct cdat_doe_rsp *buf; + size_t length; int rc; if (is_cxl_memdev(uport)) { @@ -647,30 +649,33 @@ void read_cdat_data(struct cxl_port *port) port->cdat_available = true; - if (cxl_cdat_get_length(dev, doe_mb, &cdat_length)) { + if (cxl_cdat_get_length(dev, doe_mb, &length)) { dev_dbg(dev, "No CDAT length\n"); return; } - cdat_buf = devm_kzalloc(dev, cdat_length + sizeof(__le32), GFP_KERNEL); - if (!cdat_buf) - return; + /* + * The begin of the CDAT buffer needs space for additional 4 + * bytes for the DOE header. Table data starts afterwards. + */ + buf = devm_kzalloc(dev, sizeof(*buf) + length, GFP_KERNEL); + if (!buf) + goto err; - rc = cxl_cdat_read_table(dev, doe_mb, cdat_buf, &cdat_length); + rc = cxl_cdat_read_table(dev, doe_mb, buf, &length); if (rc) goto err; - cdat_table = cdat_buf + sizeof(__le32); - if (cdat_checksum(cdat_table, cdat_length)) + if (cdat_checksum(buf->data, length)) goto err; - port->cdat.table = cdat_table; - port->cdat.length = cdat_length; - return; + port->cdat.table = buf->data; + port->cdat.length = length; + return; err: /* Don't leave table data allocated on error */ - devm_kfree(dev, cdat_buf); + devm_kfree(dev, buf); dev_err(dev, "Failed to read/validate CDAT.\n"); } EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL); diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h index 711b05d9a370..93992a1c8eec 100644 --- a/drivers/cxl/cxlpci.h +++ b/drivers/cxl/cxlpci.h @@ -71,6 +71,15 @@ enum cxl_regloc_type { CXL_REGLOC_RBI_TYPES }; +/* + * Table Access DOE, CDAT Read Entry Response + * + * Spec refs: + * + * CXL 3.1 8.1.11, Table 8-14: Read Entry Response + * CDAT Specification 1.03: 2 CDAT Data Structures + */ + struct cdat_header { __le32 length; u8 revision; @@ -85,6 +94,21 @@ struct cdat_entry_header { __le16 length; } __packed; +/* + * The DOE CDAT read response contains a CDAT read entry (either the + * CDAT header or a structure). + */ +union cdat_data { + struct cdat_header header; + struct cdat_entry_header entry; +} __packed; + +/* There is an additional CDAT response header of 4 bytes. */ +struct cdat_doe_rsp { + __le32 doe_header; + u8 data[]; +} __packed; + /* * CXL v3.0 6.2.3 Table 6-4 * The table indicates that if PCIe Flit Mode is set, then CXL is in 256B flits