Commit e0c818e0 authored by Robert Richter's avatar Robert Richter Committed by Dan Williams
Browse files

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 c65efe36 ("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 <lukas@wunner.de>
Cc: Fan Ni <nifan.cxl@gmail.com>
Reviewed-by: default avatarDave Jiang <dave.jiang@intel.com>
Signed-off-by: default avatarRobert Richter <rrichter@amd.com>
Reviewed-by: default avatarJonathan Cameron <Jonathan.Cameron@huawei.com>
Link: https://lore.kernel.org/r/20240216155844.406996-3-rrichter@amd.com


Signed-off-by: default avatarDan Williams <dan.j.williams@intel.com>
parent ec8ffff3

drivers/cxl/core/pci.c

+41 −36
Original line number Diff line number Diff line
@@ -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);

drivers/cxl/cxlpci.h

+24 −0
Original line number Diff line number Diff line
@@ -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