Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Message-ID: <74320528-097b-4395-9e61-928a4eb9771a@intel.com>
Date:   Fri, 17 Nov 2023 13:25:06 -0700
User-Agent: Betterbird (Linux)
Subject: Re: [PATCH] cxl/pci: Get rid of pointer arithmetic reading CDAT table
To:     Robert Richter <rrichter@amd.com>,
        Dan Williams <dan.j.williams@intel.com>
CC:     Ira Weiny <ira.weiny@intel.com>,
        Davidlohr Bueso <dave@stgolabs.net>,
        Jonathan Cameron <jonathan.cameron@huawei.com>,
        Alison Schofield <alison.schofield@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        <linux-cxl@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        Fan Ni <nifan.cxl@gmail.com>, Lukas Wunner <lukas@wunner.de>
References: <20231116-fix-cdat-devm-free-v1-1-b148b40707d7@intel.com>
 <ZVfIaNhiSc-yQZo5@rric.localdomain> <ZVfJ6Fxidvw_gz7r@rric.localdomain>
Content-Language: en-US
From:   Dave Jiang <dave.jiang@intel.com>
In-Reply-To: <ZVfJ6Fxidvw_gz7r@rric.localdomain>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?bDNjUUtvSXpxTjR5Wnp2VXNCaHRvN0taTWZCczFUaFFQTWhCdFVsYzNyRFZC?=
 =?utf-8?B?ZEJoZVkyY3BsQ21melIwRnFLWmtXaThJbG1ubUJ4TFBEQUNoOHIrZFZ1UmEr?=
 =?utf-8?B?djRrU2xFaTBDZGQ4bFpDL0pnRmpoTGQ2T2JhSW1oUnhCK1dDd2pISjNmSDYy?=
 =?utf-8?B?eE40dmRGdUdkLzVkbzdQa0JyWVU1Wk9zN3lUZUZGeUgxOGNua3NIaXpjcmZZ?=
 =?utf-8?B?djN0Rm5JN0d4VXhpeTRBREtqaGkzTE9SaGZBT2xSNDZFVXl5SUhLeUtFQVpq?=
 =?utf-8?B?WGwza2lxMXFmcnJITnBFMHdBb0M4eXJNU3YzMUY4ajVvbnRVcGxYUElvMUcr?=
 =?utf-8?B?N00vSkQyemUzYy93azdNaWd6WHk3V0pvVGUyaGtlWWVBQmhYdTNvQmpVNWkv?=
 =?utf-8?B?MXRUbUsySnRQTUUvOWZDZzF5SytvTHNhK3RubHRBb3l2UElvWFVqelhKUyti?=
 =?utf-8?B?SXZBUFByUmcra1E5V25WQmk5WGlWcm9kNzM4TFBTWEF2VnYzNXVBSXRCSHp2?=
 =?utf-8?B?cjlZRS9Gb09Sc0lReDFBZStFUkE3aEJBekI1SmRGUmNGYnE0V0JiTkkzYmFs?=
 =?utf-8?B?d0tjUkM4V2VEZnh3eWpsaXNoenhCaHZJNWlCbS9lMERuUmVKQVlaL2N0K0hq?=
 =?utf-8?B?ZzZnMEppVjNiclJNVUdXc29STzhua1lyYjBBMUY3WHpGWmlUNGhjMjV4WjhH?=
 =?utf-8?B?VCs5QzYwK1VNZjhxclBnSEpjWlVoV05YWlpiRkgrL01PK0FpYnVnNHdDN290?=
 =?utf-8?B?bU1jOTVtSUg1cFh5OXZaUnIySEg1V0NCRmpIYzBLNTlDdEdDVUlrNmI2SnBB?=
 =?utf-8?B?YWVEWkd1N3ZiWktuQnNvOEduYjZVbkNORm9jckgrWjNMbXZsRmFLRFZHQVFr?=
 =?utf-8?B?WmI4MWI5aHVlMmZyRnBqbU5lR0JRaEZxMzB1RElDWml4TGg4L3BJeDFLTjAv?=
 =?utf-8?B?b0JUbTVZa2RWdEVLM09hZElicG4xSFFHKzllLzdHME5zNWJFR3g4UXcwc3Z0?=
 =?utf-8?B?TjFDVk10QWJuQ1FmTTMrZW5qcnhVMUlFdGhPbWNZT2NUakdaTlMzNWtpbDVt?=
 =?utf-8?B?TkFMSGhjZzdabmJYSG5GVmNkOC9jRnhaZVBkUklNa1RTZnI0emdzSHdiSnAv?=
 =?utf-8?B?RTV5V1JkeVJCOWw3Q1FqSnBGTWs2MFBMNDhGRnlEbGNvejQwcklMZFB1R21n?=
 =?utf-8?B?MXR0SW1MWER3MzJmaXJ5NXZEK2dVQVlSeVVhdGp2UlF6dHYzb1RabjNyZldi?=
 =?utf-8?B?bTlnV3kvRlhwcTl1T1I5em5NbXpBeFdTVVZ2NldMSkwxS0U5alRHSWRzYW5U?=
 =?utf-8?B?b0g3aTVCcWhJNUtnZGNTeEdoTm1ScnVvdytrYUYydzJQUEVaZG14SjhzVDB4?=
 =?utf-8?B?WE5ISmc1VnNjejlWRTc2VStoTGRpZzVoWG4wM256U3pabXUyUzc3NFlzVVRn?=
 =?utf-8?B?QSthWjk2TzVwQ2FDQlZ6dVB1czNVWVRSamZRNUpoTG5ITTM5SE5QV0czUXo2?=
 =?utf-8?B?amxxZGJxdmpUUVJlbXJKY1d0blNtNDhzdkkvVStuZzVwSzZQLzVpZng2UDE2?=
 =?utf-8?B?d0dlY2VaU3JIL21GTXE4Z1YwNnM5UTRPb2JnNU1aSFArV1cvV1hwaDRJWXFD?=
 =?utf-8?B?cWl5ZFlnRmN2Ri9QTE5HanAwcDBOR3R1RFlZRnNrS00zOUxweXg5eEIzU3Ji?=
 =?utf-8?B?MDg5dmZRdFIybFQwV1RhVTZScndWRDRyRzY2Z1BCQkRjSVhyTGZCbjhkVnVx?=
 =?utf-8?B?WURYNXBONm9xWFdaOFZLRzdmTklEY1VlbTdzYS9sWkVxMFdHQ1hvMFFLbHlC?=
 =?utf-8?B?Vk5aYTYxMHBGajlMSEF3bUdqcExESStqUHF3OVRyUExvRFZKMkxyeTlpTUd3?=
 =?utf-8?B?Um1HMUJJZUV4dXNOVGs1UWkwMWN1OGEvVEVDMk5LNm9Kb0dVcmxJUFBnbGw1?=
 =?utf-8?B?Q3lCS2NKUXdWL2NKZnNCRUhPZFZBaGJTd2Y5VnFnUmhHMkhhZENHVy9qbVhN?=
 =?utf-8?B?TGRiUUR3QzRFR1JCNEd1dFpzcDVWRFlzcnJPNXZ6QWludFlQbjhiOHhtcVhk?=
 =?utf-8?B?VFBUYWJUUnFJc2MvazVsL1VrWjJGSEM1c2pCalJjcEQxUmFiYmVVaWJqaVJy?=
 =?utf-8?Q?48b+9g0VqzB3n0FpROfVqS1AS?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 69f375af-2ce3-46ae-6b6f-08dbe7ab4b79
X-MS-Exchange-CrossTenant-AuthSource: PH7PR11MB5984.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Nov 2023 20:25:09.6274
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 8fJHu8LbJg4XLfwManfomSz8sVFAUH+UP2u+IsIWQveet3GrRwJ6xTFMQ5MPO9XIQfzX94q1ryEvuxHufEOD5Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR11MB5564
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lipwig.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (lipwig.vger.email [0.0.0.0]); Fri, 17 Nov 2023 12:25:34 -0800 (PST)


On 11/17/23 13:15, 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@amd.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.
> 
> Cc: Lukas Wunner <lukas@wunner.de>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Fan Ni <nifan.cxl@gmail.com>
> Signed-off-by: Robert Richter <rrichter@amd.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.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,
>  			       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);
> +
>  		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;
> +	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;
> +	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];
> +		struct cdat_header header[0];
> +		struct cdat_entry_header entry[0];
> +	};
> +} __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,