Message-ID: <ZZgXBjyh4WbVULJe@rric.localdomain>
Date: Fri, 5 Jan 2024 15:49:42 +0100
From: Robert Richter <rrichter@....com>
To: Ira Weiny <ira.weiny@...el.com>
Cc: Dan Williams <dan.j.williams@...el.com>,
	Davidlohr Bueso <dave@...olabs.net>,
	Jonathan Cameron <jonathan.cameron@...wei.com>,
	Dave Jiang <dave.jiang@...el.com>,
	Alison Schofield <alison.schofield@...el.com>,
	Vishal Verma <vishal.l.verma@...el.com>, linux-cxl@...r.kernel.org,
	linux-kernel@...r.kernel.org, Fan Ni <nifan.cxl@...il.com>,
	Lukas Wunner <lukas@...ner.de>
Subject: Re: [PATCH] cxl/pci: Get rid of pointer arithmetic reading CDAT table

On 28.11.23 12:06:27, Ira Weiny wrote:
> Robert Richter wrote:
> > On 17.11.23 21:09:18, Robert Richter wrote:
> > > I will send an on-top patch for 6.8 that reworks that code area to
> > > remove the pointer arithmetic.
> > 
> > Here it is:
> > 
> > From 13787f72c20b8c54754ae86015d982307eae0397 Mon Sep 17 00:00:00 2001
> > From: Robert Richter <rrichter@....com>
> > 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 (see fix of devm_kfree() in
> > read_cdat_data()).
> > 
> > Rework code to avoid calculations with sizeof(__le32). Introduce
> > struct cdat_doe 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.
> 
> Thanks for this.

Thanks for your comments.

> 
> > 
> > Cc: Lukas Wunner <lukas@...ner.de>
> > Cc: Dave Jiang <dave.jiang@...el.com>
> > Cc: Fan Ni <nifan.cxl@...il.com>
> > Signed-off-by: Robert Richter <rrichter@....com>
> > ---
> >  drivers/cxl/core/pci.c | 80 ++++++++++++++++++++----------------------
> >  drivers/cxl/cxlpci.h   | 19 ++++++++++
> >  2 files changed, 57 insertions(+), 42 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> > index 5aaa0b36c42a..f900740c6dea 100644
> > --- a/drivers/cxl/core/pci.c
> > +++ b/drivers/cxl/core/pci.c
> > @@ -517,14 +517,14 @@ EXPORT_SYMBOL_NS_GPL(cxl_hdm_decode_init, CXL);
> >  	 FIELD_PREP(CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE, (entry_handle)))
> >  
> >  static int cxl_cdat_get_length(struct device *dev,
> > -			       struct pci_doe_mb *cdat_doe,
> > +			       struct pci_doe_mb *doe_mb,
> 
> NIT: Why change the variable name here?

There was a conflict with cdat_doe, so I renamed this. Now, that
cdat_doe is not introduced any longer there is not really a need. On
the other side 'doe_mb' is much more describing the actual meaning, so
I decided to change the name anyway in the next respin but do this
with a separate patch. This makes esp. this patch much more
readable. And of course. Jon would complain about it. :-)

> 
> >  			       size_t *length)
> >  {
> >  	__le32 request = CDAT_DOE_REQ(0);
> >  	__le32 response[2];
> >  	int rc;
> >  
> > -	rc = pci_doe(cdat_doe, PCI_DVSEC_VENDOR_ID_CXL,
> > +	rc = pci_doe(doe_mb, PCI_DVSEC_VENDOR_ID_CXL,
> >  		     CXL_DOE_PROTOCOL_TABLE_ACCESS,
> >  		     &request, sizeof(request),
> >  		     &response, sizeof(response));
> > @@ -542,56 +542,54 @@ static int cxl_cdat_get_length(struct device *dev,
> >  }
> >  
> >  static int cxl_cdat_read_table(struct device *dev,
> > -			       struct pci_doe_mb *cdat_doe,
> > -			       void *cdat_table, size_t *cdat_length)
> > +			       struct pci_doe_mb *doe_mb,
> > +			       struct cdat_doe *doe, size_t *length)
> >  {
> > -	size_t length = *cdat_length + sizeof(__le32);
> > -	__le32 *data = cdat_table;
> > +	size_t received, remaining = *length;
> >  	int entry_handle = 0;
> >  	__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(cdat_doe, PCI_DVSEC_VENDOR_ID_CXL,
> > +		rc = pci_doe(doe_mb, PCI_DVSEC_VENDOR_ID_CXL,
> >  			     CXL_DOE_PROTOCOL_TABLE_ACCESS,
> >  			     &request, sizeof(request),
> > -			     data, length);
> > +			     doe, sizeof(*doe) + 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 (rc < sizeof(*doe))
> > +			return -EIO;
> > +
> > +		received = rc - sizeof(*doe);
> 
> This is not ideal.  See comments on struct cdat_doe below.

Thanks for your kernel doc pointer, I will rework the flex array
handling.

Here, there is not other way to get the header size of the struct
(without the flexible arrays). Looking esp. at the implementation of
struct_size() using sizeof() for actual variables is allowed and safe
to use.

> 
> > +
> >  		if ((entry_handle == 0 &&
> > -		     rc != sizeof(__le32) + sizeof(struct cdat_header)) ||
> > +		     received != sizeof(doe->header[0])) ||
> >  		    (entry_handle > 0 &&
> > -		     (rc < sizeof(__le32) + sizeof(*entry) ||
> > -		      rc != sizeof(__le32) + le16_to_cpu(entry->length))))
> > +		     (received < sizeof(doe->entry[0]) ||
> > +		      received != le16_to_cpu(doe->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(doe->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;
> > +		doe->doe_header = saved_dw;
> > +		remaining -= received;
> > +		doe = (void *)doe + received;
> > +		saved_dw = doe->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;
> >  }
> > @@ -616,11 +614,11 @@ void read_cdat_data(struct cxl_port *port)
> >  {
> >  	struct device *uport = port->uport_dev;
> >  	struct device *dev = &port->dev;
> > -	struct pci_doe_mb *cdat_doe;
> > +	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 *doe;
> 
> Since we are trying to make this cleaner I would prefer a struct and
> variable name of cdat_doe_rsp.
> 
> So...
> 	struct cdat_doe_rsp *cdat_doe_rsp;

Looks reasonable, though I will use the shorter 'rsp' for the
variable.

> 
> > +	size_t length;
> >  	int rc;
> >  
> >  	if (is_cxl_memdev(uport)) {
> > @@ -637,40 +635,38 @@ void read_cdat_data(struct cxl_port *port)
> >  	if (!pdev)
> >  		return;
> >  
> > -	cdat_doe = pci_find_doe_mailbox(pdev, PCI_DVSEC_VENDOR_ID_CXL,
> > -					CXL_DOE_PROTOCOL_TABLE_ACCESS);
> > -	if (!cdat_doe) {
> > +	doe_mb = pci_find_doe_mailbox(pdev, PCI_DVSEC_VENDOR_ID_CXL,
> > +				      CXL_DOE_PROTOCOL_TABLE_ACCESS);
> > +	if (!doe_mb) {
> >  		dev_dbg(dev, "No CDAT mailbox\n");
> >  		return;
> >  	}
> >  
> >  	port->cdat_available = true;
> >  
> > -	if (cxl_cdat_get_length(dev, cdat_doe, &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;
> > +	doe = devm_kzalloc(dev, sizeof(*doe) + length, GFP_KERNEL);
> > +	if (!doe)
> > +		goto err;
> >  
> > -	rc = cxl_cdat_read_table(dev, cdat_doe, cdat_buf, &cdat_length);
> > +	rc = cxl_cdat_read_table(dev, doe_mb, doe, &length);
> >  	if (rc)
> >  		goto err;
> >  
> > -	cdat_table = cdat_buf + sizeof(__le32);
> > -	if (cdat_checksum(cdat_table, cdat_length))
> > +	if (cdat_checksum(doe->table, length))
> >  		goto err;
> >  
> > -	port->cdat.table = cdat_table;
> > -	port->cdat.length = cdat_length;
> > -	return;
> > +	port->cdat.table = doe->table;
> 
> As an aside: the type of port->cdat may need to change at some point too.
> 
> > +	port->cdat.length = length;
> >  
> > +	return;
> >  err:
> >  	/* Don't leave table data allocated on error */
> > -	devm_kfree(dev, cdat_buf);
> > +	devm_kfree(dev, doe);
> >  	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 0fa4799ea316..d12ed9d8dec1 100644
> > --- a/drivers/cxl/cxlpci.h
> > +++ b/drivers/cxl/cxlpci.h
> > @@ -85,6 +85,25 @@ struct cdat_entry_header {
> >  	__le16 length;
> >  } __packed;
> >  
> > +/*
> > + * Response contains the CDAT only response header of the DOE. The
> > + * response payload is a CDAT structure (either CDAT header or entry),
> > + * it may also mark the beginning of the CDAT table.
> > + *
> > + * Spec refs:
> > + *
> > + * CXL 3.1 Table 8-14: Read Entry Response
> > + * CDAT Specification 1.03: 2 CDAT Data Structures
> > + */
> > +struct cdat_doe {
> > +	__le32 doe_header;
> > +	union {
> > +		u8 table[0];
> 
> At a minimum we need to use flexible arrays here.  See:
> 
> .../Documentation/process/deprecated.rst
> 	'Zero-length and one-element arrays'
> 
> > +		struct cdat_header header[0];
> > +		struct cdat_entry_header entry[0];
> 
> So this would need to be:
> 
> 		DECLARE_FLEX_ARRAY(u8, table);
> 		DECLARE_FLEX_ARRAY(struct cdat_header, header);
> 		DECLARE_FLEX_ARRAY(struct cdat_entry_header, entry);

Changed that.

> 
> Also I think it would be best to use sizeof_field().
> 
> That said I got crossed up when this structure was used to represent not
> only the individual DOE responses but also the cdat table as a whole.
> 
> I think it would be best to call this cdat_doe_rsp and somehow make a
> distinction in read_cdat_data() when assigning to the entire port cdat
> table.

I will name the table buffer's variable 'buf' and add a comment.

Thanks,

-Robert


> 
> Ira
> 
> > +	};
> > +} __packed;
> > +
> >  int devm_cxl_port_enumerate_dports(struct cxl_port *port);
> >  struct cxl_dev_state;
> >  int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
> > -- 
> > 2.39.2
> > 
> > 
> 
> 

Powered by blists - more mailing lists