[v3,16/16] cxl/pci: Rightsize CDAT response allocation

Jonathan notes that cxl_cdat_get_length() and cxl_cdat_read_table()
allocate 32 dwords for the DOE response even though it may be smaller.

In the case of cxl_cdat_get_length(), only the second dword of the
response is of interest (it contains the length).  So reduce the
allocation to 2 dwords and let DOE discard the remainder.

In the case of cxl_cdat_read_table(), a correctly sized allocation for
the full CDAT already exists.  Let DOE write each table entry directly
into that allocation.  There's a snag in that the table entry is
preceded by a Table Access Response Header (1 dword).  Save the last
dword of the previous table entry, let DOE overwrite it with the
header of the next entry and restore it afterwards.

The resulting CDAT is preceded by 4 unavoidable useless bytes.  Increase
the allocation size accordingly and skip these bytes when exposing CDAT
in sysfs.

The buffer overflow check in cxl_cdat_read_table() becomes unnecessary
because the remaining bytes in the allocation are tracked in "length",
which is passed to DOE and limits how many bytes it writes to the
allocation.  Additionally, cxl_cdat_read_table() bails out if the DOE
response is truncated due to insufficient space.

Tested-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
---
 Changes v2 -> v3:
 * Newly added patch in v3 on popular request (Jonathan)

 drivers/cxl/core/pci.c | 34 ++++++++++++++++++----------------
 drivers/cxl/cxl.h      |  3 ++-
 drivers/cxl/port.c     |  2 +-
 3 files changed, 21 insertions(+), 18 deletions(-)

On Fri, 10 Feb 2023 21:25:16 +0100
Lukas Wunner <lukas@wunner.de> wrote:

> Jonathan notes that cxl_cdat_get_length() and cxl_cdat_read_table()
> allocate 32 dwords for the DOE response even though it may be smaller.
> 
> In the case of cxl_cdat_get_length(), only the second dword of the
> response is of interest (it contains the length).  So reduce the
> allocation to 2 dwords and let DOE discard the remainder.
> 
> In the case of cxl_cdat_read_table(), a correctly sized allocation for
> the full CDAT already exists.  Let DOE write each table entry directly
> into that allocation.  There's a snag in that the table entry is
> preceded by a Table Access Response Header (1 dword).  Save the last
> dword of the previous table entry, let DOE overwrite it with the
> header of the next entry and restore it afterwards.

Marginally nasty, but looks like it works to me and avoid excessive allocations.

> 
> The resulting CDAT is preceded by 4 unavoidable useless bytes.  Increase
> the allocation size accordingly and skip these bytes when exposing CDAT
> in sysfs.
> 
> The buffer overflow check in cxl_cdat_read_table() becomes unnecessary
> because the remaining bytes in the allocation are tracked in "length",
> which is passed to DOE and limits how many bytes it writes to the
> allocation.  Additionally, cxl_cdat_read_table() bails out if the DOE
> response is truncated due to insufficient space.
> 
> Tested-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Follow up comment on earlier one inline.

> ---
>  Changes v2 -> v3:
>  * Newly added patch in v3 on popular request (Jonathan)
> 
>  drivers/cxl/core/pci.c | 34 ++++++++++++++++++----------------
>  drivers/cxl/cxl.h      |  3 ++-
>  drivers/cxl/port.c     |  2 +-
>  3 files changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 1b954783b516..70097cc75302 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -471,7 +471,7 @@ static int cxl_cdat_get_length(struct device *dev,
>  			       size_t *length)
>  {
>  	__le32 request = CDAT_DOE_REQ(0);
> -	__le32 response[32];
> +	__le32 response[2];
>  	int rc;
>  
>  	rc = pci_doe(cdat_doe, PCI_DVSEC_VENDOR_ID_CXL,
> @@ -495,28 +495,28 @@ static int cxl_cdat_read_table(struct device *dev,
>  			       struct pci_doe_mb *cdat_doe,
>  			       struct cxl_cdat *cdat)
>  {
> -	size_t length = cdat->length;
> -	u32 *data = cdat->table;
> +	size_t length = cdat->length + sizeof(u32);
> +	__le32 *data = cdat->table;

Ah. Makes my earlier comment on this type irrelevant.

>  	int entry_handle = 0;
> +	__le32 saved_dw = 0;
>  
>  	do {
>  		__le32 request = CDAT_DOE_REQ(entry_handle);
>  		struct cdat_entry_header *entry;
> -		__le32 response[32];
>  		size_t entry_dw;
>  		int rc;
>  
>  		rc = pci_doe(cdat_doe, PCI_DVSEC_VENDOR_ID_CXL,
>  			     CXL_DOE_PROTOCOL_TABLE_ACCESS,
>  			     &request, sizeof(request),
> -			     &response, sizeof(response));
> +			     data, length);
>  		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 *)(response + 1);
> +		entry = (struct cdat_entry_header *)(data + 1);
>  		if ((entry_handle == 0 &&
>  		     rc != sizeof(u32) + sizeof(struct cdat_header)) ||
>  		    (entry_handle > 0 &&
> @@ -526,21 +526,22 @@ static int cxl_cdat_read_table(struct device *dev,
>  
>  		/* Get the CXL table access header entry handle */
>  		entry_handle = FIELD_GET(CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE,
> -					 le32_to_cpu(response[0]));
> +					 le32_to_cpu(data[0]));
>  		entry_dw = rc / sizeof(u32);
>  		/* Skip Header */
>  		entry_dw -= 1;
> -		entry_dw = min(length / sizeof(u32), entry_dw);
> -		/* Prevent length < 1 DW from causing a buffer overflow */
> -		if (entry_dw) {
> -			memcpy(data, entry, entry_dw * sizeof(u32));
> -			length -= entry_dw * sizeof(u32);
> -			data += entry_dw;
> -		}
> +		/*
> +		 * Table Access Response Header overwrote the last DW of
> +		 * previous entry, so restore that DW
> +		 */
> +		*data = saved_dw;
> +		length -= entry_dw * sizeof(u32);
> +		data += entry_dw;
> +		saved_dw = *data;
>  	} while (entry_handle != CXL_DOE_TABLE_ACCESS_LAST_ENTRY);
>  
>  	/* Length in CDAT header may exceed concatenation of CDAT entries */
> -	cdat->length -= length;
> +	cdat->length -= length - sizeof(u32);
>  
>  	return 0;
>  }
> @@ -576,7 +577,8 @@ void read_cdat_data(struct cxl_port *port)
>  		return;
>  	}
>  
> -	port->cdat.table = devm_kzalloc(dev, cdat_length, GFP_KERNEL);
> +	port->cdat.table = devm_kzalloc(dev, cdat_length + sizeof(u32),
> +					GFP_KERNEL);
>  	if (!port->cdat.table)
>  		return;
>  
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 1b1cf459ac77..78f5cae5134c 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -494,7 +494,8 @@ struct cxl_pmem_region {
>   * @component_reg_phys: component register capability base address (optional)
>   * @dead: last ep has been removed, force port re-creation
>   * @depth: How deep this port is relative to the root. depth 0 is the root.
> - * @cdat: Cached CDAT data
> + * @cdat: Cached CDAT data (@table is preceded by 4 null bytes, these are not
> + *	  included in @length)
>   * @cdat_available: Should a CDAT attribute be available in sysfs
>   */
>  struct cxl_port {
> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> index 5453771bf330..0705343ac5ca 100644
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
> @@ -95,7 +95,7 @@ static ssize_t CDAT_read(struct file *filp, struct kobject *kobj,
>  		return 0;
>  
>  	return memory_read_from_buffer(buf, count, &offset,
> -				       port->cdat.table,
> +				       port->cdat.table + sizeof(u32),
>  				       port->cdat.length);
>  }
>

Lukas Wunner wrote:
> Jonathan notes that cxl_cdat_get_length() and cxl_cdat_read_table()
> allocate 32 dwords for the DOE response even though it may be smaller.
> 
> In the case of cxl_cdat_get_length(), only the second dword of the
> response is of interest (it contains the length).  So reduce the
> allocation to 2 dwords and let DOE discard the remainder.
> 
> In the case of cxl_cdat_read_table(), a correctly sized allocation for
> the full CDAT already exists.  Let DOE write each table entry directly
> into that allocation.  There's a snag in that the table entry is
> preceded by a Table Access Response Header (1 dword).

Where is this 'Table Access Response Header' defined?

Ira

> Save the last
> dword of the previous table entry, let DOE overwrite it with the
> header of the next entry and restore it afterwards.
> 
> The resulting CDAT is preceded by 4 unavoidable useless bytes.  Increase
> the allocation size accordingly and skip these bytes when exposing CDAT
> in sysfs.
> 
> The buffer overflow check in cxl_cdat_read_table() becomes unnecessary
> because the remaining bytes in the allocation are tracked in "length",
> which is passed to DOE and limits how many bytes it writes to the
> allocation.  Additionally, cxl_cdat_read_table() bails out if the DOE
> response is truncated due to insufficient space.
> 
> Tested-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> ---
>  Changes v2 -> v3:
>  * Newly added patch in v3 on popular request (Jonathan)
> 
>  drivers/cxl/core/pci.c | 34 ++++++++++++++++++----------------
>  drivers/cxl/cxl.h      |  3 ++-
>  drivers/cxl/port.c     |  2 +-
>  3 files changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 1b954783b516..70097cc75302 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -471,7 +471,7 @@ static int cxl_cdat_get_length(struct device *dev,
>  			       size_t *length)
>  {
>  	__le32 request = CDAT_DOE_REQ(0);
> -	__le32 response[32];
> +	__le32 response[2];
>  	int rc;
>  
>  	rc = pci_doe(cdat_doe, PCI_DVSEC_VENDOR_ID_CXL,
> @@ -495,28 +495,28 @@ static int cxl_cdat_read_table(struct device *dev,
>  			       struct pci_doe_mb *cdat_doe,
>  			       struct cxl_cdat *cdat)
>  {
> -	size_t length = cdat->length;
> -	u32 *data = cdat->table;
> +	size_t length = cdat->length + sizeof(u32);
> +	__le32 *data = cdat->table;
>  	int entry_handle = 0;
> +	__le32 saved_dw = 0;
>  
>  	do {
>  		__le32 request = CDAT_DOE_REQ(entry_handle);
>  		struct cdat_entry_header *entry;
> -		__le32 response[32];
>  		size_t entry_dw;
>  		int rc;
>  
>  		rc = pci_doe(cdat_doe, PCI_DVSEC_VENDOR_ID_CXL,
>  			     CXL_DOE_PROTOCOL_TABLE_ACCESS,
>  			     &request, sizeof(request),
> -			     &response, sizeof(response));
> +			     data, length);
>  		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 *)(response + 1);
> +		entry = (struct cdat_entry_header *)(data + 1);
>  		if ((entry_handle == 0 &&
>  		     rc != sizeof(u32) + sizeof(struct cdat_header)) ||
>  		    (entry_handle > 0 &&
> @@ -526,21 +526,22 @@ static int cxl_cdat_read_table(struct device *dev,
>  
>  		/* Get the CXL table access header entry handle */
>  		entry_handle = FIELD_GET(CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE,
> -					 le32_to_cpu(response[0]));
> +					 le32_to_cpu(data[0]));
>  		entry_dw = rc / sizeof(u32);
>  		/* Skip Header */
>  		entry_dw -= 1;
> -		entry_dw = min(length / sizeof(u32), entry_dw);
> -		/* Prevent length < 1 DW from causing a buffer overflow */
> -		if (entry_dw) {
> -			memcpy(data, entry, entry_dw * sizeof(u32));
> -			length -= entry_dw * sizeof(u32);
> -			data += entry_dw;
> -		}
> +		/*
> +		 * Table Access Response Header overwrote the last DW of
> +		 * previous entry, so restore that DW
> +		 */
> +		*data = saved_dw;
> +		length -= entry_dw * sizeof(u32);
> +		data += entry_dw;
> +		saved_dw = *data;
>  	} while (entry_handle != CXL_DOE_TABLE_ACCESS_LAST_ENTRY);
>  
>  	/* Length in CDAT header may exceed concatenation of CDAT entries */
> -	cdat->length -= length;
> +	cdat->length -= length - sizeof(u32);
>  
>  	return 0;
>  }
> @@ -576,7 +577,8 @@ void read_cdat_data(struct cxl_port *port)
>  		return;
>  	}
>  
> -	port->cdat.table = devm_kzalloc(dev, cdat_length, GFP_KERNEL);
> +	port->cdat.table = devm_kzalloc(dev, cdat_length + sizeof(u32),
> +					GFP_KERNEL);
>  	if (!port->cdat.table)
>  		return;
>  
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 1b1cf459ac77..78f5cae5134c 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -494,7 +494,8 @@ struct cxl_pmem_region {
>   * @component_reg_phys: component register capability base address (optional)
>   * @dead: last ep has been removed, force port re-creation
>   * @depth: How deep this port is relative to the root. depth 0 is the root.
> - * @cdat: Cached CDAT data
> + * @cdat: Cached CDAT data (@table is preceded by 4 null bytes, these are not
> + *	  included in @length)
>   * @cdat_available: Should a CDAT attribute be available in sysfs
>   */
>  struct cxl_port {
> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> index 5453771bf330..0705343ac5ca 100644
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
> @@ -95,7 +95,7 @@ static ssize_t CDAT_read(struct file *filp, struct kobject *kobj,
>  		return 0;
>  
>  	return memory_read_from_buffer(buf, count, &offset,
> -				       port->cdat.table,
> +				       port->cdat.table + sizeof(u32),
>  				       port->cdat.length);
>  }
>  
> -- 
> 2.39.1
>

On Wed, Feb 15, 2023 at 04:56:48PM -0800, Ira Weiny wrote:
> Lukas Wunner wrote:
> > Jonathan notes that cxl_cdat_get_length() and cxl_cdat_read_table()
> > allocate 32 dwords for the DOE response even though it may be smaller.
> > 
> > In the case of cxl_cdat_get_length(), only the second dword of the
> > response is of interest (it contains the length).  So reduce the
> > allocation to 2 dwords and let DOE discard the remainder.
> > 
> > In the case of cxl_cdat_read_table(), a correctly sized allocation for
> > the full CDAT already exists.  Let DOE write each table entry directly
> > into that allocation.  There's a snag in that the table entry is
> > preceded by a Table Access Response Header (1 dword).
> 
> Where is this 'Table Access Response Header' defined?

CXL r3.0 table 8-14 (sec 8.1.11.1, page 399).

I'll amend the commit message with a reference to the spec.

Thanks,

Lukas

On 11/2/23 07:25, Lukas Wunner wrote:
> Jonathan notes that cxl_cdat_get_length() and cxl_cdat_read_table()
> allocate 32 dwords for the DOE response even though it may be smaller.
> 
> In the case of cxl_cdat_get_length(), only the second dword of the
> response is of interest (it contains the length).  So reduce the
> allocation to 2 dwords and let DOE discard the remainder.
> 
> In the case of cxl_cdat_read_table(), a correctly sized allocation for
> the full CDAT already exists.  Let DOE write each table entry directly
> into that allocation.  There's a snag in that the table entry is
> preceded by a Table Access Response Header (1 dword).  Save the last
> dword of the previous table entry, let DOE overwrite it with the
> header of the next entry and restore it afterwards.
> 
> The resulting CDAT is preceded by 4 unavoidable useless bytes.  Increase
> the allocation size accordingly and skip these bytes when exposing CDAT
> in sysfs.
> 
> The buffer overflow check in cxl_cdat_read_table() becomes unnecessary
> because the remaining bytes in the allocation are tracked in "length",
> which is passed to DOE and limits how many bytes it writes to the
> allocation.  Additionally, cxl_cdat_read_table() bails out if the DOE
> response is truncated due to insufficient space.
> 
> Tested-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> ---
>   Changes v2 -> v3:
>   * Newly added patch in v3 on popular request (Jonathan)
> 
>   drivers/cxl/core/pci.c | 34 ++++++++++++++++++----------------


Almost no change from this patchset applied cleanly to 
drivers/cxl/core/pci.c, what is the patchset based on? Thanks,

On Tue, Feb 28, 2023 at 12:45:33PM +1100, Alexey Kardashevskiy wrote:
> Almost no change from this patchset applied cleanly to
> drivers/cxl/core/pci.c, what is the patchset based on? Thanks,

This series is based on pci/next, i.e. v6.2-rc1 plus PCI changes for v6.3:

https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=next

It should apply cleanly to cxl/next as well, save for a trivial merge
conflict in patch [13/16] due to a context change.  Here's a branch
containing v3 of this series based on cxl/next:

https://github.com/l1k/linux/commits/doe_rework_v3_cxl_next

That said, you're probably better off just using the development branch
for CMA/SPDM, it incorporates all the changes requested during review
of v3 and will form the basis for v4 (slated for submission in a few
days):

https://github.com/l1k/linux/commits/doe

Thanks,

Lukas