[PATCH 0/3] cxl: Prep for QoS class support

[PATCH 0/3] cxl: Prep for QoS class support

[Dave Jiang]

With the "cxl: Add support for QTG ID retrieval for CXL subsystem" series
getting larger and larger and span multiple maintainers, I'm going to try
to break them up into smaller parts. These 3 patches are just preparation
for QTG retrieval. See this series [1] for full series. Full git tree
available at [2]. 

[1]: https://lore.kernel.org/linux-cxl/168193556660.1178687.15477509915255912089.stgit@djiang5-mobl3/T/#t
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/log/?h=cxl-qtg

---

Dave Jiang (3):
      cxl: Export QTG ids from CFMWS to sysfs as qos_class attribute
      cxl: Add checksum verification to CDAT from CXL
      cxl: Add support for reading CXL switch CDAT table


 Documentation/ABI/testing/sysfs-bus-cxl | 15 ++++++++++
 drivers/cxl/acpi.c                      |  3 ++
 drivers/cxl/core/pci.c                  | 39 +++++++++++++++++++++----
 drivers/cxl/core/port.c                 | 11 +++++++
 drivers/cxl/cxl.h                       |  3 ++
 drivers/cxl/port.c                      |  3 ++
 6 files changed, 69 insertions(+), 5 deletions(-)

--

[PATCH 1/3] cxl: Export QTG ids from CFMWS to sysfs as qos_class attribute

[Dave Jiang]

Export the QoS Throttling Group ID from the CXL Fixed Memory Window
Structure (CFMWS) under the root decoder sysfs attributes as qos_class.

CXL rev3.0 9.17.1.3 CXL Fixed Memory Window Structure (CFMWS)

cxl cli will use this id to match with the _DSM retrieved id for a
hot-plugged CXL memory device DPA memory range to make sure that the
DPA range is under the right CFMWS window.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>

---
v5:
- Documentation rewrite. (Dan)
- Rename attrib to qos_class
- Move qtg_id to root decoder
v4:
- Change kernel version for documentation to v6.5
v2:
- Add explanation commit header (Jonathan)
---
 Documentation/ABI/testing/sysfs-bus-cxl |   15 +++++++++++++++
 drivers/cxl/acpi.c                      |    3 +++
 drivers/cxl/core/port.c                 |   11 +++++++++++
 drivers/cxl/cxl.h                       |    3 +++
 4 files changed, 32 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
index 3acf2f17a73f..2f24e42ef36d 100644
--- a/Documentation/ABI/testing/sysfs-bus-cxl
+++ b/Documentation/ABI/testing/sysfs-bus-cxl
@@ -310,6 +310,21 @@ Description:
 		provided it is currently idle / not bound to a driver.
 
 
+What:		/sys/bus/cxl/devices/decoderX.Y/qos_class
+Date:		May, 2023
+KernelVersion:	v6.5
+Contact:	linux-cxl@vger.kernel.org
+Description:
+		(RO) For CXL host platforms that support "QoS Telemmetry" this
+		root-decoder-only attribute conveys a platform specific cookie
+		that identifies a QoS performance class for the CXL Window.
+		This class-id can be compared against a similar "qos_class"
+		published for each memory-type that an endpoint supports. While
+		it is not required that endpoints map their local memory-class
+		to a matching platform class, mismatches are not recommended and
+		there are platform specific side-effects that may result.
+
+
 What:		/sys/bus/cxl/devices/regionZ/uuid
 Date:		May, 2022
 KernelVersion:	v6.0
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 7e1765b09e04..e063df2bf876 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -289,6 +289,9 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
 			}
 		}
 	}
+
+	cxlrd->qos_class = cfmws->qtg_id;
+
 	rc = cxl_decoder_add(cxld, target_map);
 err_xormap:
 	if (rc)
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 4d1f9c5b5029..a0130aeb8d42 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -276,6 +276,15 @@ static ssize_t interleave_ways_show(struct device *dev,
 
 static DEVICE_ATTR_RO(interleave_ways);
 
+static ssize_t qos_class_show(struct device *dev,
+			      struct device_attribute *attr, char *buf)
+{
+	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev);
+
+	return sysfs_emit(buf, "%d\n", cxlrd->qos_class);
+}
+static DEVICE_ATTR_RO(qos_class);
+
 static struct attribute *cxl_decoder_base_attrs[] = {
 	&dev_attr_start.attr,
 	&dev_attr_size.attr,
@@ -295,6 +304,7 @@ static struct attribute *cxl_decoder_root_attrs[] = {
 	&dev_attr_cap_type2.attr,
 	&dev_attr_cap_type3.attr,
 	&dev_attr_target_list.attr,
+	&dev_attr_qos_class.attr,
 	SET_CXL_REGION_ATTR(create_pmem_region)
 	SET_CXL_REGION_ATTR(create_ram_region)
 	SET_CXL_REGION_ATTR(delete_region)
@@ -1625,6 +1635,7 @@ struct cxl_root_decoder *cxl_root_decoder_alloc(struct cxl_port *port,
 	}
 
 	atomic_set(&cxlrd->region_id, rc);
+	cxlrd->qos_class = CXL_QOS_CLASS_INVALID;
 	return cxlrd;
 }
 EXPORT_SYMBOL_NS_GPL(cxl_root_decoder_alloc, CXL);
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 044a92d9813e..4577d808ac6d 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -300,6 +300,7 @@ enum cxl_decoder_type {
  */
 #define CXL_DECODER_MAX_INTERLEAVE 16
 
+#define CXL_QOS_CLASS_INVALID -1
 
 /**
  * struct cxl_decoder - Common CXL HDM Decoder Attributes
@@ -411,6 +412,7 @@ typedef struct cxl_dport *(*cxl_calc_hb_fn)(struct cxl_root_decoder *cxlrd,
  * @calc_hb: which host bridge covers the n'th position by granularity
  * @platform_data: platform specific configuration data
  * @range_lock: sync region autodiscovery by address range
+ * @qos_class: QoS performance class cookie
  * @cxlsd: base cxl switch decoder
  */
 struct cxl_root_decoder {
@@ -419,6 +421,7 @@ struct cxl_root_decoder {
 	cxl_calc_hb_fn calc_hb;
 	void *platform_data;
 	struct mutex range_lock;
+	int qos_class;
 	struct cxl_switch_decoder cxlsd;
 };
 

Re: [PATCH 1/3] cxl: Export QTG ids from CFMWS to sysfs as qos_class attribute

[Davidlohr Bueso]

On Fri, 05 May 2023, Dave Jiang wrote:

>Export the QoS Throttling Group ID from the CXL Fixed Memory Window
>Structure (CFMWS) under the root decoder sysfs attributes as qos_class.
>
>CXL rev3.0 9.17.1.3 CXL Fixed Memory Window Structure (CFMWS)
>
>cxl cli will use this id to match with the _DSM retrieved id for a
>hot-plugged CXL memory device DPA memory range to make sure that the
>DPA range is under the right CFMWS window.
>
>Reviewed-by: Ira Weiny <ira.weiny@intel.com>
>Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>

[PATCH 2/3] cxl: Add checksum verification to CDAT from CXL

[Dave Jiang]

A CDAT table is available from a CXL device. The table is read by the
driver and cached in software. With the CXL subsystem needing to parse the
CDAT table, the checksum should be verified. Add checksum verification
after the CDAT table is read from device.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>

---
v5:
- Return on CDAT errors. (Dan)
v3:
- Just return the final sum. (Alison)
v2:
- Drop ACPI checksum export and just use local verification. (Dan)
---
 drivers/cxl/core/pci.c |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index bdbd907884ce..046e55a9b419 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -528,6 +528,16 @@ static int cxl_cdat_read_table(struct device *dev,
 	return 0;
 }
 
+static unsigned char cdat_checksum(void *buf, size_t size)
+{
+	unsigned char sum, *data = buf;
+	size_t i;
+
+	for (sum = 0, i = 0; i < size; i++)
+		sum += data[i];
+	return sum;
+}
+
 /**
  * read_cdat_data - Read the CDAT data on this port
  * @port: Port to read data from
@@ -571,9 +581,17 @@ void read_cdat_data(struct cxl_port *port)
 		/* Don't leave table data allocated on error */
 		devm_kfree(dev, cdat_table);
 		dev_err(dev, "CDAT data read error\n");
+		return;
 	}
 
 	port->cdat.table = cdat_table + sizeof(__le32);
+	if (cdat_checksum(port->cdat.table, cdat_length)) {
+		/* Don't leave table data allocated on error */
+		devm_kfree(dev, cdat_table);
+		dev_err(dev, "CDAT data checksum error\n");
+		return;
+	}
+
 	port->cdat.length = cdat_length;
 }
 EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL);

Re: [PATCH 2/3] cxl: Add checksum verification to CDAT from CXL

[Davidlohr Bueso]

On Fri, 05 May 2023, Dave Jiang wrote:

> /**
>  * read_cdat_data - Read the CDAT data on this port
>  * @port: Port to read data from
>@@ -571,9 +581,17 @@ void read_cdat_data(struct cxl_port *port)
>		/* Don't leave table data allocated on error */
>		devm_kfree(dev, cdat_table);
>		dev_err(dev, "CDAT data read error\n");
>+		return;

Should this be a separate fixlet?

>	}
>
>	port->cdat.table = cdat_table + sizeof(__le32);
>+	if (cdat_checksum(port->cdat.table, cdat_length)) {
>+		/* Don't leave table data allocated on error */
>+		devm_kfree(dev, cdat_table);
>+		dev_err(dev, "CDAT data checksum error\n");
>+		return;
>+	}
>+
>	port->cdat.length = cdat_length;

Upon error, port->cdat should remain consistent (only be set after passing the
checksum. Ie, wouldn't the following be better?

	  cdat_table += sizeof(__le32);
	  if (cdat_checksum()) {
	     ...
	  }
	  port->cdat.table = cdat_table;
	  port->cdat.length = cdat_length;


Thanks,
Davidlohr

Re: [PATCH 2/3] cxl: Add checksum verification to CDAT from CXL

[Dave Jiang]

On 5/5/23 12:15 PM, Davidlohr Bueso wrote:
> On Fri, 05 May 2023, Dave Jiang wrote:
> 
>> /**
>>  * read_cdat_data - Read the CDAT data on this port
>>  * @port: Port to read data from
>> @@ -571,9 +581,17 @@ void read_cdat_data(struct cxl_port *port)
>>         /* Don't leave table data allocated on error */
>>         devm_kfree(dev, cdat_table);
>>         dev_err(dev, "CDAT data read error\n");
>> +        return;
> 
> Should this be a separate fixlet?

Yeah I can split that out.
> 
>>     }
>>
>>     port->cdat.table = cdat_table + sizeof(__le32);
>> +    if (cdat_checksum(port->cdat.table, cdat_length)) {
>> +        /* Don't leave table data allocated on error */
>> +        devm_kfree(dev, cdat_table);
>> +        dev_err(dev, "CDAT data checksum error\n");
>> +        return;
>> +    }
>> +
>>     port->cdat.length = cdat_length;
> 
> Upon error, port->cdat should remain consistent (only be set after 
> passing the
> checksum. Ie, wouldn't the following be better?

yup. I'll update.

> 
>        cdat_table += sizeof(__le32);
>        if (cdat_checksum()) {
>           ...
>        }
>        port->cdat.table = cdat_table;
>        port->cdat.length = cdat_length;
> 
> 
> Thanks,
> Davidlohr

[PATCH 3/3] cxl: Add support for reading CXL switch CDAT table

[Dave Jiang]

Add read_cdat_data() call in cxl_switch_port_probe() to allow
reading of CDAT data for CXL switches. read_cdat_data() needs
to be adjusted for the retrieving of the PCIe device depending
on if the passed in port is endpoint or switch.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>

---
v5:
- Rebase after fix [1]. (Dan)

[1]: http://lore.kernel.org/r/168213190748.708404.16215095414060364800.stgit@dwillia2-xfh.jf.intel.com
v4:
- Remove cxl_test wrapper. (Ira)
---
 drivers/cxl/core/pci.c |   21 ++++++++++++++++-----
 drivers/cxl/port.c     |    3 +++
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 046e55a9b419..9fabfe458499 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -546,18 +546,29 @@ static unsigned char cdat_checksum(void *buf, size_t size)
  */
 void read_cdat_data(struct cxl_port *port)
 {
-	struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport);
-	struct device *host = cxlmd->dev.parent;
+	struct device *uport = port->uport;
 	struct device *dev = &port->dev;
 	struct pci_doe_mb *cdat_doe;
+	struct cxl_memdev *cxlmd;
+	struct pci_dev *pdev;
+	struct device *host;
 	size_t cdat_length;
 	void *cdat_table;
 	int rc;
 
-	if (!dev_is_pci(host))
+	if (is_cxl_memdev(uport)) {
+		cxlmd = to_cxl_memdev(uport);
+		host = cxlmd->dev.parent;
+		if (!dev_is_pci(host))
+			return;
+		pdev = to_pci_dev(host);
+	} else if (dev_is_pci(uport)) {
+		pdev = to_pci_dev(uport);
+	} else {
 		return;
-	cdat_doe = pci_find_doe_mailbox(to_pci_dev(host),
-					PCI_DVSEC_VENDOR_ID_CXL,
+	}
+
+	cdat_doe = pci_find_doe_mailbox(pdev, PCI_DVSEC_VENDOR_ID_CXL,
 					CXL_DOE_PROTOCOL_TABLE_ACCESS);
 	if (!cdat_doe) {
 		dev_dbg(dev, "No CDAT mailbox\n");
diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
index 22a7ab2bae7c..a49f5eb149f1 100644
--- a/drivers/cxl/port.c
+++ b/drivers/cxl/port.c
@@ -62,6 +62,9 @@ static int cxl_switch_port_probe(struct cxl_port *port)
 	struct cxl_hdm *cxlhdm;
 	int rc;
 
+	/* Cache the data early to ensure is_visible() works */
+	read_cdat_data(port);
+
 	rc = devm_cxl_port_enumerate_dports(port);
 	if (rc < 0)
 		return rc;

Re: [PATCH 3/3] cxl: Add support for reading CXL switch CDAT table

[Davidlohr Bueso]

On Fri, 05 May 2023, Dave Jiang wrote:

>Add read_cdat_data() call in cxl_switch_port_probe() to allow
>reading of CDAT data for CXL switches. read_cdat_data() needs
>to be adjusted for the retrieving of the PCIe device depending
>on if the passed in port is endpoint or switch.
>
>Reviewed-by: Ira Weiny <ira.weiny@intel.com>
>Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Some style comments, otherwise feel free to add my:

Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>

>
>---
>v5:
>- Rebase after fix [1]. (Dan)
>
>[1]: http://lore.kernel.org/r/168213190748.708404.16215095414060364800.stgit@dwillia2-xfh.jf.intel.com
>v4:
>- Remove cxl_test wrapper. (Ira)
>---
> drivers/cxl/core/pci.c |   21 ++++++++++++++++-----
> drivers/cxl/port.c     |    3 +++
> 2 files changed, 19 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>index 046e55a9b419..9fabfe458499 100644
>--- a/drivers/cxl/core/pci.c
>+++ b/drivers/cxl/core/pci.c
>@@ -546,18 +546,29 @@ static unsigned char cdat_checksum(void *buf, size_t size)
>  */
> void read_cdat_data(struct cxl_port *port)
> {
>-	struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport);
>-	struct device *host = cxlmd->dev.parent;
>+	struct device *uport = port->uport;
>	struct device *dev = &port->dev;
>	struct pci_doe_mb *cdat_doe;
>+	struct cxl_memdev *cxlmd;
>+	struct pci_dev *pdev;

maybe instead set pdev to nil, then check...

>+	struct device *host;
>	size_t cdat_length;
>	void *cdat_table;
>	int rc;
>
>-	if (!dev_is_pci(host))
>+	if (is_cxl_memdev(uport)) {
>+		cxlmd = to_cxl_memdev(uport);
>+		host = cxlmd->dev.parent;
>+		if (!dev_is_pci(host))
>+			return;
>+		pdev = to_pci_dev(host);
>+	} else if (dev_is_pci(uport)) {
>+		pdev = to_pci_dev(uport);
>+	} else {
>		return;

... here

	if (!pdev)
	   return;

Thanks,
Davidlohr

>-	cdat_doe = pci_find_doe_mailbox(to_pci_dev(host),
>-					PCI_DVSEC_VENDOR_ID_CXL,
>+	}
>+
>+	cdat_doe = pci_find_doe_mailbox(pdev, PCI_DVSEC_VENDOR_ID_CXL,
>					CXL_DOE_PROTOCOL_TABLE_ACCESS);
>	if (!cdat_doe) {
>		dev_dbg(dev, "No CDAT mailbox\n");
>diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
>index 22a7ab2bae7c..a49f5eb149f1 100644
>--- a/drivers/cxl/port.c
>+++ b/drivers/cxl/port.c
>@@ -62,6 +62,9 @@ static int cxl_switch_port_probe(struct cxl_port *port)
>	struct cxl_hdm *cxlhdm;
>	int rc;
>
>+	/* Cache the data early to ensure is_visible() works */
>+	read_cdat_data(port);
>+
>	rc = devm_cxl_port_enumerate_dports(port);
>	if (rc < 0)
>		return rc;
>
>