Re: [RFC PATCH 2/2] cxl/mem: Add CDAT table reading from DOE

Re: [RFC PATCH 2/2] cxl/mem: Add CDAT table reading from DOE

[Zayd Qumsieh]

Hello all,

Due to issues with vger.kernel.org marking HTML emails as spam, I'll
be resending the email in plain text:



Hello all, First off, thanks for your work on implementing PCI Data
Object Exchange in QEMU and Linux.

1. Are these patches still being worked on? If not, I’ll try to get
them rebased and finished.

2. Are there any notes not mentioned in the emails you feel are
important to know about?

3. I'm particularly interested in the testing framework - the emails
mention that a lot of testing has been done through QEMU but how can I
carry out these tests on my own? What tools did you use?

Thanks,

Zayd

Re: [RFC PATCH 2/2] cxl/mem: Add CDAT table reading from DOE

[Jonathan Cameron]

On Wed, 12 Jan 2022 14:16:18 -0800
Zayd Qumsieh <zaydq@rivosinc.com> wrote:

> Hello all,
> 
> Due to issues with vger.kernel.org marking HTML emails as spam, I'll
> be resending the email in plain text:
> 
> 
> 
> Hello all, First off, thanks for your work on implementing PCI Data
> Object Exchange in QEMU and Linux.
> 
> 1. Are these patches still being worked on? If not, I’ll try to get
> them rebased and finished.
> 
> 2. Are there any notes not mentioned in the emails you feel are
> important to know about?
> 
> 3. I'm particularly interested in the testing framework - the emails
> mention that a lot of testing has been done through QEMU but how can I
> carry out these tests on my own? What tools did you use?
> 
> Thanks,
> 
> Zayd

https://lore.kernel.org/all/20211105235056.3711389-1-ira.weiny@intel.com/
Is the latest version of this.  For basic testing Ben's qemu branch should
work for you. 
https://gitlab.com/bwidawsk/qemu/-/commits/cxl-2.0v4/

Note there are some big questions over the approach used for integrating
this with the PCI subsystem (auxiliary bus vs library) which I'm not sure
we've reached a conclusion on yet.

One minor thing on the wish list is lspci support to at least identify the
presence of a DOE.  I have patches for that and will send out fairly soon
once I've cleared a backlog of other stuff.

My main focus right now is on resolving some of the opens around the
QEMU support for CXL emul in general, but I haven't touched the DOE stuff
beyond rebasing.  *fingers crossed* I should have an updated qemu tree
out sometime in next week or so.

Thanks,

Jonathan

Re: [RFC PATCH 2/2] cxl/mem: Add CDAT table reading from DOE

[Zayd Qumsieh]

On Thu, Jan 13, 2022 at 1:25 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Wed, 12 Jan 2022 14:16:18 -0800
> Zayd Qumsieh <zaydq@rivosinc.com> wrote:
>
> > Hello all,
> >
> > Due to issues with vger.kernel.org marking HTML emails as spam, I'll
> > be resending the email in plain text:
> >
> >
> >
> > Hello all, First off, thanks for your work on implementing PCI Data
> > Object Exchange in QEMU and Linux.
> >
> > 1. Are these patches still being worked on? If not, I’ll try to get
> > them rebased and finished.
> >
> > 2. Are there any notes not mentioned in the emails you feel are
> > important to know about?
> >
> > 3. I'm particularly interested in the testing framework - the emails
> > mention that a lot of testing has been done through QEMU but how can I
> > carry out these tests on my own? What tools did you use?
> >
> > Thanks,
> >
> > Zayd
>
> https://lore.kernel.org/all/20211105235056.3711389-1-ira.weiny@intel.com/
> Is the latest version of this.  For basic testing Ben's qemu branch should
> work for you.
> https://gitlab.com/bwidawsk/qemu/-/commits/cxl-2.0v4/
>
> Note there are some big questions over the approach used for integrating
> this with the PCI subsystem (auxiliary bus vs library) which I'm not sure
> we've reached a conclusion on yet.
>
> One minor thing on the wish list is lspci support to at least identify the
> presence of a DOE.  I have patches for that and will send out fairly soon
> once I've cleared a backlog of other stuff.
>
> My main focus right now is on resolving some of the opens around the
> QEMU support for CXL emul in general, but I haven't touched the DOE stuff
> beyond rebasing.  *fingers crossed* I should have an updated qemu tree
> out sometime in next week or so.
>
> Thanks,
>
> Jonathan

Hey,

I did some experimenting with Ben's QEMU branch and integrated your
patch locally. I'll be doing some testing of my own. If I run into
anything noteworthy I'll let you know. Again, thank you for all your
help! :)

Thanks,
Zayd

[RFC PATCH 0/2] PCI Data Object Exchange support + CXL CDAT

[Jonathan Cameron]

Series first introduces generic support for DOE mailboxes as defined
in the ECN to the PCI 5.0 specification available from the PCI SIG [0]

A user is then introduced in the form of the table access protocol defined
in the CXL 2.0 specification [1] used to access the
Coherent Device Attribtue Table (CDAT) defined in [2]

Various open questions in the individual patches.

All testing conducted against QEMU emulation of a CXL type 3 device
in conjunction with DOE mailbox patches [3, 4]

[0] https://pcisig.com/specifications
[1] https://www.computeexpresslink.org/download-the-specification
[2] https://uefi.org/node/4093
[3] https://lore.kernel.org/qemu-devel/20210202005948.241655-1-ben.widawsky@intel.com/
[4] https://lore.kernel.org/qemu-devel/1612900760-7361-1-git-send-email-cbrowy@avery-design.com/

Jonathan Cameron (2):
  PCI/doe: Initial support PCI Data Object Exchange
  cxl/mem: Add CDAT table reading from DOE

 drivers/cxl/cdat.h            |  79 ++++++++++
 drivers/cxl/cxl.h             |  13 ++
 drivers/cxl/mem.c             | 253 +++++++++++++++++++++++++++++-
 drivers/pci/pcie/Kconfig      |   8 +
 drivers/pci/pcie/Makefile     |   1 +
 drivers/pci/pcie/doe.c        | 284 ++++++++++++++++++++++++++++++++++
 include/linux/pcie-doe.h      |  35 +++++
 include/uapi/linux/pci_regs.h |  29 +++-
 8 files changed, 700 insertions(+), 2 deletions(-)
 create mode 100644 drivers/cxl/cdat.h
 create mode 100644 drivers/pci/pcie/doe.c
 create mode 100644 include/linux/pcie-doe.h

-- 
2.19.1

[RFC PATCH 2/2] cxl/mem: Add CDAT table reading from DOE

[Jonathan Cameron]

This patch simply provides some debug print outs of the entries
at probe time + a sysfs binary attribute to allow dumping of the
whole table.

Binary dumping is modelled on /sys/firmware/ACPI/tables/

The ability to dump this table will be very useful for emulation of
real devices once they become available as QEMU CXL type 3 device
emulation will be able to load this file in.

Open questions:
* No support here for table updates. Worth including these from the
  start, or leave that complexity for later?
* Worth logging the reported info for debug, or is the binary attribute
  sufficient?  Larger open question of whether to expose this info to
  userspace or not left for another day!
* Where to put the CDAT file?  Is it worth a subdirectory?
* What is maximum size of the SSLBIS entry - I haven't quite managed
  to figure that out and this is the record with largest size.
  We could support dynamic allocation of the record size, but it
  would add complexity that seems unnecessary.
  It would not be compliant with the specification for a type 3 memory
  device to report this record anyway so I'm not that worried about this
  for now.  It will become relevant once we have support for reading
  CDAT from CXL switches.
* cdat.h is formatted in a similar style to pci_regs.h on basis that
  it may well be helpful to share this header with userspace tools.
* Move the generic parts of this out to driver/cxl/cdat.c or leave that
  until we have other CXL drivers wishing to use this?

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/cxl/cdat.h |  79 ++++++++++++++
 drivers/cxl/cxl.h  |  13 +++
 drivers/cxl/mem.c  | 253 ++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 344 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/cdat.h b/drivers/cxl/cdat.h
new file mode 100644
index 000000000000..e67b18e02c35
--- /dev/null
+++ b/drivers/cxl/cdat.h
@@ -0,0 +1,79 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Coherent Device Attribute table (CDAT)
+ *
+ * Specification available from UEFI.org
+ *
+ * Whilst CDAT is defined as a single table, the access via DOE maiboxes is
+ * done one entry at a time, where the first entry is the header.
+ */
+
+#define CXL_DOE_TABLE_ACCESS_REQ_CODE		0x000000ff
+#define   CXL_DOE_TABLE_ACCESS_REQ_CODE_READ	0
+#define CXL_DOE_TABLE_ACCESS_TABLE_TYPE		0x0000ff00
+#define   CXL_DOE_TABLE_ACCESS_TABLE_TYPE_CDATA	0
+#define CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE	0xffff0000
+
+
+/*
+ * CDAT entries are little endian and are read from PCI config space which
+ * is also little endian.
+ * As such, on a big endian system these will have been reversed.
+ * This prevents us from making easy use of packed structures.
+ * Style form pci_regs.h
+ */
+
+#define CDAT_HEADER_LENGTH_DW 3
+#define CDAT_HEADER_DW0_LENGTH		0xFFFFFFFF
+#define CDAT_HEADER_DW1_REVISION	0x000000FF
+#define CDAT_HEADER_DW1_CHECKSUM	0x0000FF00
+#define CDAT_HEADER_DW2_SEQUENCE	0xFFFFFFFF
+
+/* All structures have a common first DW */
+#define CDAT_STRUCTURE_DW0_TYPE		0x000000FF
+#define   CDAT_STRUCTURE_DW0_TYPE_DSMAS 0
+#define   CDAT_STRUCTURE_DW0_TYPE_DSLBIS 1
+#define   CDAT_STRUCTURE_DW0_TYPE_DSMSCIS 2
+#define   CDAT_STRUCTURE_DW0_TYPE_DSIS 3
+#define   CDAT_STRUCTURE_DW0_TYPE_DSEMTS 4
+#define   CDAT_STRUCTURE_DW0_TYPE_SSLBIS 5
+
+#define CDAT_STRUCTURE_DW0_LENGTH	0xFFFF0000
+
+/* Device Scoped Memory Affinity Structure */
+#define CDAT_DSMAS_DW1_DSMAD_HANDLE	0x000000ff
+#define CDAT_DSMAS_DW1_FLAGS		0x0000ff00
+#define CDAT_DSMAS_DPA_OFFSET(entry) ((u64)((entry)[3]) << 32 | (entry)[2])
+#define CDAT_DSMAS_DPA_LEN(entry) ((u64)((entry)[5]) << 32 | (entry)[4])
+
+/* Device Scoped Latency and Bandwidth Information Structure */
+#define CDAT_DSLBIS_DW1_HANDLE		0x000000ff
+#define CDAT_DSLBIS_DW1_FLAGS		0x0000ff00
+#define CDAT_DSLBIS_DW1_DATA_TYPE	0x00ff0000
+#define CDAT_DSLBIS_BASE_UNIT(entry) ((u64)((entry)[3]) << 32 | (entry)[2])
+#define CDAT_DSLBIS_DW4_ENTRY_0		0x0000ffff
+#define CDAT_DSLBIS_DW4_ENTRY_1		0xffff0000
+#define CDAT_DSLBIS_DW5_ENTRY_2		0x0000ffff
+
+/* Device Scoped Memory Side Cache Information Structure */
+#define CDAT_DSMSCIS_DW1_HANDLE		0x000000ff
+#define CDAT_DSMSCIS_MEMORY_SIDE_CACHE_SIZE(entry) \
+	((u64)((entry)[3]) << 32 | (entry)[2])
+#define CDAT_DSMSCIS_DW4_MEMORY_SIDE_CACHE_ATTRS 0xffffffff
+
+/* Device Scoped Initiator Structure */
+#define CDAT_DSIS_DW1_FLAGS		0x000000ff
+#define CDAT_DSIS_DW1_HANDLE		0x0000ff00
+
+/* Device Scoped EFI Memory Type Structure */
+#define CDAT_DSEMTS_DW1_HANDLE		0x000000ff
+#define CDAT_DSEMTS_DW1_EFI_MEMORY_TYPE_ATTR	0x0000ff00
+#define CDAT_DSEMTS_DPA_OFFSET(entry)	((u64)((entry)[3]) << 32 | (entry)[2])
+#define CDAT_DSEMTS_DPA_LENGTH(entry)	((u64)((entry)[5]) << 32 | (entry)[4])
+
+/* Switch Scoped Latency and Bandwidth Information Structure */
+#define CDAT_SSLBIS_DW1_DATA_TYPE	0x000000ff
+#define CDAT_SSLBIS_BASE_UNIT(entry)	((u64)((entry)[3]) << 32 | (entry)[2])
+#define CDAT_SSLBIS_ENTRY_PORT_X(entry, i) ((entry)[4 + (i) * 2] & 0x0000ffff)
+#define CDAT_SSLBIS_ENTRY_PORT_Y(entry, i) (((entry)[4 + (i) * 2] & 0xffff0000) >> 16)
+#define CDAT_SSLBIS_ENTRY_LAT_OR_BW(entry, i) ((entry)[4 + (i) * 2 + 1] & 0x0000ffff)
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 6f14838c2d25..2f5a69201fc3 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -7,6 +7,7 @@
 #include <linux/bitfield.h>
 #include <linux/bitops.h>
 #include <linux/io.h>
+#include <linux/pcie-doe.h>
 
 /* CXL 2.0 8.2.8.1 Device Capabilities Array Register */
 #define CXLDEV_CAP_ARRAY_OFFSET 0x0
@@ -57,10 +58,21 @@
 	(FIELD_GET(CXLMDEV_RESET_NEEDED_MASK, status) !=                       \
 	 CXLMDEV_RESET_NEEDED_NOT)
 
+#define CXL_DOE_PROTOCOL_COMPLIANCE 0
+#define CXL_DOE_PROTOCOL_TABLE_ACCESS 2
+
+/* Common to request and response */
+#define CXL_DOE_TABLE_ACCESS_3_CODE GENMASK(7, 0)
+#define   CXL_DOE_TABLE_ACCESS_3_CODE_READ 0
+#define CXL_DOE_TABLE_ACCESS_3_TYPE GENMASK(15, 8)
+#define   CXL_DOE_TABLE_ACCESS_3_TYPE_CDAT 0
+#define CXL_DOE_TABLE_ACCESS_3_ENTRY_HANDLE GENMASK(31, 16)
+
 struct cxl_memdev;
 /**
  * struct cxl_mem - A CXL memory device
  * @pdev: The PCI device associated with this CXL device.
+ * @doe: Data exchange object mailbox used to read tables.
  * @regs: IO mappings to the device's MMIO
  * @status_regs: CXL 2.0 8.2.8.3 Device Status Registers
  * @mbox_regs: CXL 2.0 8.2.8.4 Mailbox Registers
@@ -75,6 +87,7 @@ struct cxl_memdev;
  */
 struct cxl_mem {
 	struct pci_dev *pdev;
+	struct pcie_doe doe;
 	void __iomem *regs;
 	struct cxl_memdev *cxlmd;
 
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 4597b28aeb3f..71de66bc6c54 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -12,6 +12,7 @@
 #include <linux/io-64-nonatomic-lo-hi.h>
 #include "pci.h"
 #include "cxl.h"
+#include "cdat.h"
 
 /**
  * DOC: cxl mem
@@ -91,6 +92,11 @@ struct mbox_cmd {
 #define CXL_MBOX_SUCCESS 0
 };
 
+struct doe_table_attr {
+	struct bin_attribute attr;
+	void *table;
+};
+
 /**
  * struct cxl_memdev - CXL bus object representing a Type-3 Memory Device
  * @dev: driver core device object
@@ -98,6 +104,7 @@ struct mbox_cmd {
  * @cxlm: pointer to the parent device driver data
  * @ops_active: active user of @cxlm in ops handlers
  * @ops_dead: completion when all @cxlm ops users have exited
+ * @table_attr: attribute used to provide dumping of table
  * @id: id number of this memdev instance.
  */
 struct cxl_memdev {
@@ -106,6 +113,7 @@ struct cxl_memdev {
 	struct cxl_mem *cxlm;
 	struct percpu_ref ops_active;
 	struct completion ops_dead;
+	struct doe_table_attr table_attr;
 	int id;
 };
 
@@ -976,13 +984,165 @@ static int cxl_mem_setup_mailbox(struct cxl_mem *cxlm)
 	return 0;
 }
 
+#define CDAT_DOE_REQ(entry_handle)					\
+	[0] = FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_VID,		\
+			 PCI_DVSEC_VENDOR_ID_CXL) |			\
+	      FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_TYPE,		\
+			   CXL_DOE_PROTOCOL_TABLE_ACCESS),		\
+	[1] = FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH, 3),	\
+	[2] = FIELD_PREP(CXL_DOE_TABLE_ACCESS_REQ_CODE,			\
+			 CXL_DOE_TABLE_ACCESS_REQ_CODE_READ) |		\
+	      FIELD_PREP(CXL_DOE_TABLE_ACCESS_TABLE_TYPE,		\
+			 CXL_DOE_TABLE_ACCESS_TABLE_TYPE_CDATA) |	\
+	      FIELD_PREP(CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE, (entry_handle))
+
+static ssize_t cdat_get_length(struct pcie_doe *doe)
+{
+	u32 cdat_request[3] = {
+		CDAT_DOE_REQ(0),
+	};
+	u32 cdat_response[32];
+	ssize_t rc;
+
+	rc = pcie_doe_exchange(doe, cdat_request, sizeof(cdat_request),
+			       cdat_response, sizeof(cdat_response));
+	if (rc)
+		return rc;
+
+	return cdat_response[3];
+}
+
+static int cdat_to_buffer(struct pcie_doe *doe, u32 *buffer, size_t length)
+{
+	int entry_handle = 0;
+	int rc;
+
+	do {
+		u32 cdat_request[3] = {
+			CDAT_DOE_REQ(entry_handle)
+		};
+		u32 cdat_response[32];
+		size_t entry_dw;
+		u32 *entry;
+
+		rc = pcie_doe_exchange(doe, cdat_request, sizeof(cdat_request),
+				       cdat_response, sizeof(cdat_response));
+		if (rc)
+			return rc;
+
+		entry = cdat_response + CDAT_HEADER_LENGTH_DW;
+
+		entry_dw = FIELD_GET(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH, cdat_response[1]);
+		/* Skip Header */
+		entry_dw -= 3;
+		entry_dw = min(length / 4, entry_dw);
+		memcpy(buffer, entry, entry_dw * sizeof(u32));
+		length -= entry_dw * sizeof(u32);
+		buffer += entry_dw;
+		entry_handle = FIELD_GET(CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE, cdat_response[2]);
+	} while (entry_handle != 0xFFFF);
+
+	return 0;
+}
+
+static int cdat_dump(struct pcie_doe *doe)
+{
+	struct pci_dev *dev = doe->pdev;
+	int entry_handle = 0;
+	int rc;
+
+	do {
+		/* Table access is available */
+		u32 cdat_request[3] = {
+			CDAT_DOE_REQ(entry_handle)
+		};
+		u32 cdat_response[32];
+		u32 *entry;
+
+		rc = pcie_doe_exchange(doe, cdat_request, sizeof(cdat_request),
+				       cdat_response, sizeof(cdat_response));
+		if (rc)
+			return rc;
+
+		entry = cdat_response + CDAT_HEADER_LENGTH_DW;
+		if (entry_handle == 0) {
+			pci_info(dev,
+				 "CDAT Header (Length=%u, Revision=%u, Checksum=0x%x, Sequence=%u\n",
+				 entry[0],
+				 FIELD_GET(CDAT_HEADER_DW1_REVISION, entry[1]),
+				 FIELD_GET(CDAT_HEADER_DW1_CHECKSUM, entry[1]),
+				 entry[2]);
+		} else {
+			u8 entry_type = FIELD_GET(CDAT_STRUCTURE_DW0_TYPE, entry[0]);
+
+			switch (entry_type) {
+			case CDAT_STRUCTURE_DW0_TYPE_DSMAS:
+				pci_info(dev,
+					 "CDAT DSMAS (handle=%u flags=0x%x, dpa(0x%llx 0x%llx)\n",
+					 FIELD_GET(CDAT_DSMAS_DW1_DSMAD_HANDLE, entry[1]),
+					 FIELD_GET(CDAT_DSMAS_DW1_FLAGS, entry[1]),
+					 CDAT_DSMAS_DPA_OFFSET(entry),
+					 CDAT_DSMAS_DPA_LEN(entry));
+				break;
+			case CDAT_STRUCTURE_DW0_TYPE_DSLBIS:
+				pci_info(dev,
+					 "CDAT DSLBIS (handle=%u flags=0x%x, ent_base=0x%llx, entry[%u %u %u])\n",
+					 FIELD_GET(CDAT_DSLBIS_DW1_HANDLE, entry[1]),
+					 FIELD_GET(CDAT_DSLBIS_DW1_FLAGS, entry[1]),
+					 CDAT_DSLBIS_BASE_UNIT(entry),
+					 FIELD_GET(CDAT_DSLBIS_DW4_ENTRY_0, entry[4]),
+					 FIELD_GET(CDAT_DSLBIS_DW4_ENTRY_1, entry[4]),
+					 FIELD_GET(CDAT_DSLBIS_DW5_ENTRY_2, entry[5]));
+				break;
+			case CDAT_STRUCTURE_DW0_TYPE_DSMSCIS:
+				pci_info(dev,
+					 "CDAT DSMSCIS (handle=%u sc_size=0x%llx attrs=0x%x)\n",
+					 FIELD_GET(CDAT_DSMSCIS_DW1_HANDLE, entry[1]),
+					 CDAT_DSMSCIS_MEMORY_SIDE_CACHE_SIZE(entry),
+					 FIELD_GET(CDAT_DSMSCIS_DW4_MEMORY_SIDE_CACHE_ATTRS,
+						   entry[4]));
+				break;
+			case CDAT_STRUCTURE_DW0_TYPE_DSIS:
+				pci_info(dev,
+					 "CDAT DSIS (handle=%u flags=0x%x)\n",
+					 FIELD_GET(CDAT_DSIS_DW1_HANDLE, entry[1]),
+					 FIELD_GET(CDAT_DSIS_DW1_FLAGS, entry[1]));
+				break;
+			case CDAT_STRUCTURE_DW0_TYPE_DSEMTS:
+				pci_info(dev,
+					 "CDAT DSEMTS (handle=%u EFI=0x%x dpa(0x%llx 0x%llx)\n",
+					 FIELD_GET(CDAT_DSEMTS_DW1_HANDLE, entry[1]),
+					 FIELD_GET(CDAT_DSEMTS_DW1_EFI_MEMORY_TYPE_ATTR,
+						   entry[1]),
+					 CDAT_DSEMTS_DPA_OFFSET(entry),
+					 CDAT_DSEMTS_DPA_LENGTH(entry));
+				break;
+			case CDAT_STRUCTURE_DW0_TYPE_SSLBIS:
+				pci_info(dev,
+					 "CDAT SSLBIS (type%u ent_base=%llu...)\n",
+					 FIELD_GET(CDAT_SSLBIS_DW1_DATA_TYPE,
+						   entry[1]),
+					 CDAT_SSLBIS_BASE_UNIT(entry));
+				break;
+			}
+		}
+		entry_handle = FIELD_GET(CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE,
+					 cdat_response[2]);
+	} while (entry_handle != 0xFFFF);
+
+	return 0;
+}
+
 static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev, u32 reg_lo,
 				      u32 reg_hi)
 {
 	struct device *dev = &pdev->dev;
 	struct cxl_mem *cxlm;
 	void __iomem *regs;
+	bool doe_use_irq;
+	int pos = 0;
 	u64 offset;
+	int irqs;
 	u8 bar;
 	int rc;
 
@@ -1021,6 +1181,44 @@ static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev, u32 reg_lo,
 		return NULL;
 	}
 
+	/*
+	 * An implementation of a cxl type3 device may support an unknown
+	 * number of interrupts. Assume that number is not that large and
+	 * request them all.
+	 */
+	irqs = pci_msix_vec_count(pdev);
+	rc = pci_alloc_irq_vectors(pdev, irqs, irqs, PCI_IRQ_MSIX);
+	if (rc != irqs) {
+		/* No interrupt available - carry on */
+		dev_dbg(dev, "No interrupts available for DOE\n");
+		doe_use_irq = false;
+	} else {
+		/*
+		 * Enabling bus mastering could be done within the DOE
+		 * initialization, but as it potentially has other impacts
+		 * keep it within the driver.
+		 */
+		pci_set_master(pdev);
+		doe_use_irq = true;
+	}
+
+	/*
+	 * Find a DOE mailbox that supports CDAT.
+	 * Supporting other DOE protocols will require more complexity.
+	 */
+	do {
+		pos = pci_find_next_ext_capability(pdev, pos, PCI_EXT_CAP_ID_DOE);
+		if (!pos)
+			return NULL;
+
+		pcie_doe_init(&cxlm->doe, pdev, pos, doe_use_irq);
+	} while (pcie_doe_protocol_check(&cxlm->doe, PCI_DVSEC_VENDOR_ID_CXL,
+					 CXL_DOE_PROTOCOL_TABLE_ACCESS));
+
+	rc = cdat_dump(&cxlm->doe);
+	if (rc)
+		return NULL;
+
 	dev_dbg(dev, "Mapped CXL Memory Device resource\n");
 	return cxlm;
 }
@@ -1178,6 +1376,55 @@ static void cxlmdev_ops_active_release(struct percpu_ref *ref)
 	complete(&cxlmd->ops_dead);
 }
 
+static ssize_t cdat_table_show(struct file *filp, struct kobject *kobj,
+			       struct bin_attribute *bin_attr, char *buf,
+			       loff_t offset, size_t count)
+{
+	struct doe_table_attr *table_attr =
+		container_of(bin_attr, struct doe_table_attr, attr);
+
+	return memory_read_from_buffer(buf, count, &offset, table_attr->table,
+				       bin_attr->size);
+}
+
+static void cxl_remove_table_sysfs(void *_cxlmd)
+{
+	struct cxl_memdev *cxlmd = _cxlmd;
+	struct device *dev = &cxlmd->dev;
+
+	sysfs_remove_bin_file(&dev->kobj, &cxlmd->table_attr.attr);
+}
+
+static int cxl_add_table_sysfs(struct cxl_memdev *cxlmd)
+{
+	struct cxl_mem *cxlm = cxlmd->cxlm;
+	struct device *dev = &cxlmd->dev;
+	ssize_t cdat_length;
+	int rc;
+
+	cdat_length = cdat_get_length(&cxlm->doe);
+	if (cdat_length < 0)
+		return cdat_length;
+
+	sysfs_attr_init(&cxlmd->table_attr.attr.attr);
+	/* Updates of CDAT are not yet handled so length is fixed. */
+	cxlmd->table_attr.attr.size = cdat_length;
+	cxlmd->table_attr.attr.read = cdat_table_show;
+	cxlmd->table_attr.attr.attr.name = "CDAT";
+	cxlmd->table_attr.attr.attr.mode = 0400;
+	cxlmd->table_attr.table = devm_kzalloc(dev->parent, cdat_length, GFP_KERNEL);
+
+	rc = cdat_to_buffer(&cxlm->doe, cxlmd->table_attr.table, cdat_length);
+	if (rc)
+		return rc;
+
+	rc = sysfs_create_bin_file(&dev->kobj, &cxlmd->table_attr.attr);
+	if (rc)
+		return rc;
+
+	return devm_add_action_or_reset(dev->parent, cxl_remove_table_sysfs, cxlmd);
+}
+
 static int cxl_mem_add_memdev(struct cxl_mem *cxlm)
 {
 	struct pci_dev *pdev = cxlm->pdev;
@@ -1221,7 +1468,11 @@ static int cxl_mem_add_memdev(struct cxl_mem *cxlm)
 	if (rc)
 		goto err_add;
 
-	return devm_add_action_or_reset(dev->parent, cxlmdev_unregister, cxlmd);
+	rc = devm_add_action_or_reset(dev->parent, cxlmdev_unregister, cxlmd);
+	if (rc)
+		return rc;
+
+	return cxl_add_table_sysfs(cxlmd);
 
 err_add:
 	ida_free(&cxl_memdev_ida, cxlmd->id);
-- 
2.19.1

Re: [RFC PATCH 2/2] cxl/mem: Add CDAT table reading from DOE

[Jonathan Cameron]

On Thu, 11 Mar 2021 02:03:06 +0800
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> This patch simply provides some debug print outs of the entries
> at probe time + a sysfs binary attribute to allow dumping of the
> whole table.
> 
> Binary dumping is modelled on /sys/firmware/ACPI/tables/
> 
> The ability to dump this table will be very useful for emulation of
> real devices once they become available as QEMU CXL type 3 device
> emulation will be able to load this file in.
> 
> Open questions:
> * No support here for table updates. Worth including these from the
>   start, or leave that complexity for later?
> * Worth logging the reported info for debug, or is the binary attribute
>   sufficient?  Larger open question of whether to expose this info to
>   userspace or not left for another day!
> * Where to put the CDAT file?  Is it worth a subdirectory?
> * What is maximum size of the SSLBIS entry - I haven't quite managed
>   to figure that out and this is the record with largest size.
>   We could support dynamic allocation of the record size, but it
>   would add complexity that seems unnecessary.
>   It would not be compliant with the specification for a type 3 memory
>   device to report this record anyway so I'm not that worried about this
>   for now.  It will become relevant once we have support for reading
>   CDAT from CXL switches.
> * cdat.h is formatted in a similar style to pci_regs.h on basis that
>   it may well be helpful to share this header with userspace tools.
> * Move the generic parts of this out to driver/cxl/cdat.c or leave that
>   until we have other CXL drivers wishing to use this?

Naturally I remembered another open question within 10 seconds of sending :(

  * Do we want to add any sort of header to the RAW dump of CDAT to aid
    tooling?  Whilst it looks a little like an ACPI table it doesn't have
    a signature.

My gut feeling is no, because the CDAT specification doesn't define one but
I can see that it might be very convenient to have something that identified
the data once it was put in a file.

@Chris, what were you thinking for the QEMU load of raw CDAT data for use
in emulation?

> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/cxl/cdat.h |  79 ++++++++++++++
>  drivers/cxl/cxl.h  |  13 +++
>  drivers/cxl/mem.c  | 253 ++++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 344 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/cdat.h b/drivers/cxl/cdat.h
> new file mode 100644
> index 000000000000..e67b18e02c35
> --- /dev/null
> +++ b/drivers/cxl/cdat.h
> @@ -0,0 +1,79 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Coherent Device Attribute table (CDAT)
> + *
> + * Specification available from UEFI.org
> + *
> + * Whilst CDAT is defined as a single table, the access via DOE maiboxes is
> + * done one entry at a time, where the first entry is the header.
> + */
> +
> +#define CXL_DOE_TABLE_ACCESS_REQ_CODE		0x000000ff
> +#define   CXL_DOE_TABLE_ACCESS_REQ_CODE_READ	0
> +#define CXL_DOE_TABLE_ACCESS_TABLE_TYPE		0x0000ff00
> +#define   CXL_DOE_TABLE_ACCESS_TABLE_TYPE_CDATA	0
> +#define CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE	0xffff0000
> +
> +
> +/*
> + * CDAT entries are little endian and are read from PCI config space which
> + * is also little endian.
> + * As such, on a big endian system these will have been reversed.
> + * This prevents us from making easy use of packed structures.
> + * Style form pci_regs.h
> + */
> +
> +#define CDAT_HEADER_LENGTH_DW 3
> +#define CDAT_HEADER_DW0_LENGTH		0xFFFFFFFF
> +#define CDAT_HEADER_DW1_REVISION	0x000000FF
> +#define CDAT_HEADER_DW1_CHECKSUM	0x0000FF00
> +#define CDAT_HEADER_DW2_SEQUENCE	0xFFFFFFFF
> +
> +/* All structures have a common first DW */
> +#define CDAT_STRUCTURE_DW0_TYPE		0x000000FF
> +#define   CDAT_STRUCTURE_DW0_TYPE_DSMAS 0
> +#define   CDAT_STRUCTURE_DW0_TYPE_DSLBIS 1
> +#define   CDAT_STRUCTURE_DW0_TYPE_DSMSCIS 2
> +#define   CDAT_STRUCTURE_DW0_TYPE_DSIS 3
> +#define   CDAT_STRUCTURE_DW0_TYPE_DSEMTS 4
> +#define   CDAT_STRUCTURE_DW0_TYPE_SSLBIS 5
> +
> +#define CDAT_STRUCTURE_DW0_LENGTH	0xFFFF0000
> +
> +/* Device Scoped Memory Affinity Structure */
> +#define CDAT_DSMAS_DW1_DSMAD_HANDLE	0x000000ff
> +#define CDAT_DSMAS_DW1_FLAGS		0x0000ff00
> +#define CDAT_DSMAS_DPA_OFFSET(entry) ((u64)((entry)[3]) << 32 | (entry)[2])
> +#define CDAT_DSMAS_DPA_LEN(entry) ((u64)((entry)[5]) << 32 | (entry)[4])
> +
> +/* Device Scoped Latency and Bandwidth Information Structure */
> +#define CDAT_DSLBIS_DW1_HANDLE		0x000000ff
> +#define CDAT_DSLBIS_DW1_FLAGS		0x0000ff00
> +#define CDAT_DSLBIS_DW1_DATA_TYPE	0x00ff0000
> +#define CDAT_DSLBIS_BASE_UNIT(entry) ((u64)((entry)[3]) << 32 | (entry)[2])
> +#define CDAT_DSLBIS_DW4_ENTRY_0		0x0000ffff
> +#define CDAT_DSLBIS_DW4_ENTRY_1		0xffff0000
> +#define CDAT_DSLBIS_DW5_ENTRY_2		0x0000ffff
> +
> +/* Device Scoped Memory Side Cache Information Structure */
> +#define CDAT_DSMSCIS_DW1_HANDLE		0x000000ff
> +#define CDAT_DSMSCIS_MEMORY_SIDE_CACHE_SIZE(entry) \
> +	((u64)((entry)[3]) << 32 | (entry)[2])
> +#define CDAT_DSMSCIS_DW4_MEMORY_SIDE_CACHE_ATTRS 0xffffffff
> +
> +/* Device Scoped Initiator Structure */
> +#define CDAT_DSIS_DW1_FLAGS		0x000000ff
> +#define CDAT_DSIS_DW1_HANDLE		0x0000ff00
> +
> +/* Device Scoped EFI Memory Type Structure */
> +#define CDAT_DSEMTS_DW1_HANDLE		0x000000ff
> +#define CDAT_DSEMTS_DW1_EFI_MEMORY_TYPE_ATTR	0x0000ff00
> +#define CDAT_DSEMTS_DPA_OFFSET(entry)	((u64)((entry)[3]) << 32 | (entry)[2])
> +#define CDAT_DSEMTS_DPA_LENGTH(entry)	((u64)((entry)[5]) << 32 | (entry)[4])
> +
> +/* Switch Scoped Latency and Bandwidth Information Structure */
> +#define CDAT_SSLBIS_DW1_DATA_TYPE	0x000000ff
> +#define CDAT_SSLBIS_BASE_UNIT(entry)	((u64)((entry)[3]) << 32 | (entry)[2])
> +#define CDAT_SSLBIS_ENTRY_PORT_X(entry, i) ((entry)[4 + (i) * 2] & 0x0000ffff)
> +#define CDAT_SSLBIS_ENTRY_PORT_Y(entry, i) (((entry)[4 + (i) * 2] & 0xffff0000) >> 16)
> +#define CDAT_SSLBIS_ENTRY_LAT_OR_BW(entry, i) ((entry)[4 + (i) * 2 + 1] & 0x0000ffff)
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 6f14838c2d25..2f5a69201fc3 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -7,6 +7,7 @@
>  #include <linux/bitfield.h>
>  #include <linux/bitops.h>
>  #include <linux/io.h>
> +#include <linux/pcie-doe.h>
>  
>  /* CXL 2.0 8.2.8.1 Device Capabilities Array Register */
>  #define CXLDEV_CAP_ARRAY_OFFSET 0x0
> @@ -57,10 +58,21 @@
>  	(FIELD_GET(CXLMDEV_RESET_NEEDED_MASK, status) !=                       \
>  	 CXLMDEV_RESET_NEEDED_NOT)
>  
> +#define CXL_DOE_PROTOCOL_COMPLIANCE 0
> +#define CXL_DOE_PROTOCOL_TABLE_ACCESS 2
> +
> +/* Common to request and response */
> +#define CXL_DOE_TABLE_ACCESS_3_CODE GENMASK(7, 0)
> +#define   CXL_DOE_TABLE_ACCESS_3_CODE_READ 0
> +#define CXL_DOE_TABLE_ACCESS_3_TYPE GENMASK(15, 8)
> +#define   CXL_DOE_TABLE_ACCESS_3_TYPE_CDAT 0
> +#define CXL_DOE_TABLE_ACCESS_3_ENTRY_HANDLE GENMASK(31, 16)
> +
>  struct cxl_memdev;
>  /**
>   * struct cxl_mem - A CXL memory device
>   * @pdev: The PCI device associated with this CXL device.
> + * @doe: Data exchange object mailbox used to read tables.
>   * @regs: IO mappings to the device's MMIO
>   * @status_regs: CXL 2.0 8.2.8.3 Device Status Registers
>   * @mbox_regs: CXL 2.0 8.2.8.4 Mailbox Registers
> @@ -75,6 +87,7 @@ struct cxl_memdev;
>   */
>  struct cxl_mem {
>  	struct pci_dev *pdev;
> +	struct pcie_doe doe;
>  	void __iomem *regs;
>  	struct cxl_memdev *cxlmd;
>  
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 4597b28aeb3f..71de66bc6c54 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -12,6 +12,7 @@
>  #include <linux/io-64-nonatomic-lo-hi.h>
>  #include "pci.h"
>  #include "cxl.h"
> +#include "cdat.h"
>  
>  /**
>   * DOC: cxl mem
> @@ -91,6 +92,11 @@ struct mbox_cmd {
>  #define CXL_MBOX_SUCCESS 0
>  };
>  
> +struct doe_table_attr {
> +	struct bin_attribute attr;
> +	void *table;
> +};
> +
>  /**
>   * struct cxl_memdev - CXL bus object representing a Type-3 Memory Device
>   * @dev: driver core device object
> @@ -98,6 +104,7 @@ struct mbox_cmd {
>   * @cxlm: pointer to the parent device driver data
>   * @ops_active: active user of @cxlm in ops handlers
>   * @ops_dead: completion when all @cxlm ops users have exited
> + * @table_attr: attribute used to provide dumping of table
>   * @id: id number of this memdev instance.
>   */
>  struct cxl_memdev {
> @@ -106,6 +113,7 @@ struct cxl_memdev {
>  	struct cxl_mem *cxlm;
>  	struct percpu_ref ops_active;
>  	struct completion ops_dead;
> +	struct doe_table_attr table_attr;
>  	int id;
>  };
>  
> @@ -976,13 +984,165 @@ static int cxl_mem_setup_mailbox(struct cxl_mem *cxlm)
>  	return 0;
>  }
>  
> +#define CDAT_DOE_REQ(entry_handle)					\
> +	[0] = FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_VID,		\
> +			 PCI_DVSEC_VENDOR_ID_CXL) |			\
> +	      FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_TYPE,		\
> +			   CXL_DOE_PROTOCOL_TABLE_ACCESS),		\
> +	[1] = FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH, 3),	\
> +	[2] = FIELD_PREP(CXL_DOE_TABLE_ACCESS_REQ_CODE,			\
> +			 CXL_DOE_TABLE_ACCESS_REQ_CODE_READ) |		\
> +	      FIELD_PREP(CXL_DOE_TABLE_ACCESS_TABLE_TYPE,		\
> +			 CXL_DOE_TABLE_ACCESS_TABLE_TYPE_CDATA) |	\
> +	      FIELD_PREP(CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE, (entry_handle))
> +
> +static ssize_t cdat_get_length(struct pcie_doe *doe)
> +{
> +	u32 cdat_request[3] = {
> +		CDAT_DOE_REQ(0),
> +	};
> +	u32 cdat_response[32];
> +	ssize_t rc;
> +
> +	rc = pcie_doe_exchange(doe, cdat_request, sizeof(cdat_request),
> +			       cdat_response, sizeof(cdat_response));
> +	if (rc)
> +		return rc;
> +
> +	return cdat_response[3];
> +}
> +
> +static int cdat_to_buffer(struct pcie_doe *doe, u32 *buffer, size_t length)
> +{
> +	int entry_handle = 0;
> +	int rc;
> +
> +	do {
> +		u32 cdat_request[3] = {
> +			CDAT_DOE_REQ(entry_handle)
> +		};
> +		u32 cdat_response[32];
> +		size_t entry_dw;
> +		u32 *entry;
> +
> +		rc = pcie_doe_exchange(doe, cdat_request, sizeof(cdat_request),
> +				       cdat_response, sizeof(cdat_response));
> +		if (rc)
> +			return rc;
> +
> +		entry = cdat_response + CDAT_HEADER_LENGTH_DW;
> +
> +		entry_dw = FIELD_GET(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH, cdat_response[1]);
> +		/* Skip Header */
> +		entry_dw -= 3;
> +		entry_dw = min(length / 4, entry_dw);
> +		memcpy(buffer, entry, entry_dw * sizeof(u32));
> +		length -= entry_dw * sizeof(u32);
> +		buffer += entry_dw;
> +		entry_handle = FIELD_GET(CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE, cdat_response[2]);
> +	} while (entry_handle != 0xFFFF);
> +
> +	return 0;
> +}
> +
> +static int cdat_dump(struct pcie_doe *doe)
> +{
> +	struct pci_dev *dev = doe->pdev;
> +	int entry_handle = 0;
> +	int rc;
> +
> +	do {
> +		/* Table access is available */
> +		u32 cdat_request[3] = {
> +			CDAT_DOE_REQ(entry_handle)
> +		};
> +		u32 cdat_response[32];
> +		u32 *entry;
> +
> +		rc = pcie_doe_exchange(doe, cdat_request, sizeof(cdat_request),
> +				       cdat_response, sizeof(cdat_response));
> +		if (rc)
> +			return rc;
> +
> +		entry = cdat_response + CDAT_HEADER_LENGTH_DW;
> +		if (entry_handle == 0) {
> +			pci_info(dev,
> +				 "CDAT Header (Length=%u, Revision=%u, Checksum=0x%x, Sequence=%u\n",
> +				 entry[0],
> +				 FIELD_GET(CDAT_HEADER_DW1_REVISION, entry[1]),
> +				 FIELD_GET(CDAT_HEADER_DW1_CHECKSUM, entry[1]),
> +				 entry[2]);
> +		} else {
> +			u8 entry_type = FIELD_GET(CDAT_STRUCTURE_DW0_TYPE, entry[0]);
> +
> +			switch (entry_type) {
> +			case CDAT_STRUCTURE_DW0_TYPE_DSMAS:
> +				pci_info(dev,
> +					 "CDAT DSMAS (handle=%u flags=0x%x, dpa(0x%llx 0x%llx)\n",
> +					 FIELD_GET(CDAT_DSMAS_DW1_DSMAD_HANDLE, entry[1]),
> +					 FIELD_GET(CDAT_DSMAS_DW1_FLAGS, entry[1]),
> +					 CDAT_DSMAS_DPA_OFFSET(entry),
> +					 CDAT_DSMAS_DPA_LEN(entry));
> +				break;
> +			case CDAT_STRUCTURE_DW0_TYPE_DSLBIS:
> +				pci_info(dev,
> +					 "CDAT DSLBIS (handle=%u flags=0x%x, ent_base=0x%llx, entry[%u %u %u])\n",
> +					 FIELD_GET(CDAT_DSLBIS_DW1_HANDLE, entry[1]),
> +					 FIELD_GET(CDAT_DSLBIS_DW1_FLAGS, entry[1]),
> +					 CDAT_DSLBIS_BASE_UNIT(entry),
> +					 FIELD_GET(CDAT_DSLBIS_DW4_ENTRY_0, entry[4]),
> +					 FIELD_GET(CDAT_DSLBIS_DW4_ENTRY_1, entry[4]),
> +					 FIELD_GET(CDAT_DSLBIS_DW5_ENTRY_2, entry[5]));
> +				break;
> +			case CDAT_STRUCTURE_DW0_TYPE_DSMSCIS:
> +				pci_info(dev,
> +					 "CDAT DSMSCIS (handle=%u sc_size=0x%llx attrs=0x%x)\n",
> +					 FIELD_GET(CDAT_DSMSCIS_DW1_HANDLE, entry[1]),
> +					 CDAT_DSMSCIS_MEMORY_SIDE_CACHE_SIZE(entry),
> +					 FIELD_GET(CDAT_DSMSCIS_DW4_MEMORY_SIDE_CACHE_ATTRS,
> +						   entry[4]));
> +				break;
> +			case CDAT_STRUCTURE_DW0_TYPE_DSIS:
> +				pci_info(dev,
> +					 "CDAT DSIS (handle=%u flags=0x%x)\n",
> +					 FIELD_GET(CDAT_DSIS_DW1_HANDLE, entry[1]),
> +					 FIELD_GET(CDAT_DSIS_DW1_FLAGS, entry[1]));
> +				break;
> +			case CDAT_STRUCTURE_DW0_TYPE_DSEMTS:
> +				pci_info(dev,
> +					 "CDAT DSEMTS (handle=%u EFI=0x%x dpa(0x%llx 0x%llx)\n",
> +					 FIELD_GET(CDAT_DSEMTS_DW1_HANDLE, entry[1]),
> +					 FIELD_GET(CDAT_DSEMTS_DW1_EFI_MEMORY_TYPE_ATTR,
> +						   entry[1]),
> +					 CDAT_DSEMTS_DPA_OFFSET(entry),
> +					 CDAT_DSEMTS_DPA_LENGTH(entry));
> +				break;
> +			case CDAT_STRUCTURE_DW0_TYPE_SSLBIS:
> +				pci_info(dev,
> +					 "CDAT SSLBIS (type%u ent_base=%llu...)\n",
> +					 FIELD_GET(CDAT_SSLBIS_DW1_DATA_TYPE,
> +						   entry[1]),
> +					 CDAT_SSLBIS_BASE_UNIT(entry));
> +				break;
> +			}
> +		}
> +		entry_handle = FIELD_GET(CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE,
> +					 cdat_response[2]);
> +	} while (entry_handle != 0xFFFF);
> +
> +	return 0;
> +}
> +
>  static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev, u32 reg_lo,
>  				      u32 reg_hi)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct cxl_mem *cxlm;
>  	void __iomem *regs;
> +	bool doe_use_irq;
> +	int pos = 0;
>  	u64 offset;
> +	int irqs;
>  	u8 bar;
>  	int rc;
>  
> @@ -1021,6 +1181,44 @@ static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev, u32 reg_lo,
>  		return NULL;
>  	}
>  
> +	/*
> +	 * An implementation of a cxl type3 device may support an unknown
> +	 * number of interrupts. Assume that number is not that large and
> +	 * request them all.
> +	 */
> +	irqs = pci_msix_vec_count(pdev);
> +	rc = pci_alloc_irq_vectors(pdev, irqs, irqs, PCI_IRQ_MSIX);
> +	if (rc != irqs) {
> +		/* No interrupt available - carry on */
> +		dev_dbg(dev, "No interrupts available for DOE\n");
> +		doe_use_irq = false;
> +	} else {
> +		/*
> +		 * Enabling bus mastering could be done within the DOE
> +		 * initialization, but as it potentially has other impacts
> +		 * keep it within the driver.
> +		 */
> +		pci_set_master(pdev);
> +		doe_use_irq = true;
> +	}
> +
> +	/*
> +	 * Find a DOE mailbox that supports CDAT.
> +	 * Supporting other DOE protocols will require more complexity.
> +	 */
> +	do {
> +		pos = pci_find_next_ext_capability(pdev, pos, PCI_EXT_CAP_ID_DOE);
> +		if (!pos)
> +			return NULL;
> +
> +		pcie_doe_init(&cxlm->doe, pdev, pos, doe_use_irq);
> +	} while (pcie_doe_protocol_check(&cxlm->doe, PCI_DVSEC_VENDOR_ID_CXL,
> +					 CXL_DOE_PROTOCOL_TABLE_ACCESS));
> +
> +	rc = cdat_dump(&cxlm->doe);
> +	if (rc)
> +		return NULL;
> +
>  	dev_dbg(dev, "Mapped CXL Memory Device resource\n");
>  	return cxlm;
>  }
> @@ -1178,6 +1376,55 @@ static void cxlmdev_ops_active_release(struct percpu_ref *ref)
>  	complete(&cxlmd->ops_dead);
>  }
>  
> +static ssize_t cdat_table_show(struct file *filp, struct kobject *kobj,
> +			       struct bin_attribute *bin_attr, char *buf,
> +			       loff_t offset, size_t count)
> +{
> +	struct doe_table_attr *table_attr =
> +		container_of(bin_attr, struct doe_table_attr, attr);
> +
> +	return memory_read_from_buffer(buf, count, &offset, table_attr->table,
> +				       bin_attr->size);
> +}
> +
> +static void cxl_remove_table_sysfs(void *_cxlmd)
> +{
> +	struct cxl_memdev *cxlmd = _cxlmd;
> +	struct device *dev = &cxlmd->dev;
> +
> +	sysfs_remove_bin_file(&dev->kobj, &cxlmd->table_attr.attr);
> +}
> +
> +static int cxl_add_table_sysfs(struct cxl_memdev *cxlmd)
> +{
> +	struct cxl_mem *cxlm = cxlmd->cxlm;
> +	struct device *dev = &cxlmd->dev;
> +	ssize_t cdat_length;
> +	int rc;
> +
> +	cdat_length = cdat_get_length(&cxlm->doe);
> +	if (cdat_length < 0)
> +		return cdat_length;
> +
> +	sysfs_attr_init(&cxlmd->table_attr.attr.attr);
> +	/* Updates of CDAT are not yet handled so length is fixed. */
> +	cxlmd->table_attr.attr.size = cdat_length;
> +	cxlmd->table_attr.attr.read = cdat_table_show;
> +	cxlmd->table_attr.attr.attr.name = "CDAT";
> +	cxlmd->table_attr.attr.attr.mode = 0400;
> +	cxlmd->table_attr.table = devm_kzalloc(dev->parent, cdat_length, GFP_KERNEL);
> +
> +	rc = cdat_to_buffer(&cxlm->doe, cxlmd->table_attr.table, cdat_length);
> +	if (rc)
> +		return rc;
> +
> +	rc = sysfs_create_bin_file(&dev->kobj, &cxlmd->table_attr.attr);
> +	if (rc)
> +		return rc;
> +
> +	return devm_add_action_or_reset(dev->parent, cxl_remove_table_sysfs, cxlmd);
> +}
> +
>  static int cxl_mem_add_memdev(struct cxl_mem *cxlm)
>  {
>  	struct pci_dev *pdev = cxlm->pdev;
> @@ -1221,7 +1468,11 @@ static int cxl_mem_add_memdev(struct cxl_mem *cxlm)
>  	if (rc)
>  		goto err_add;
>  
> -	return devm_add_action_or_reset(dev->parent, cxlmdev_unregister, cxlmd);
> +	rc = devm_add_action_or_reset(dev->parent, cxlmdev_unregister, cxlmd);
> +	if (rc)
> +		return rc;
> +
> +	return cxl_add_table_sysfs(cxlmd);
>  
>  err_add:
>  	ida_free(&cxl_memdev_ida, cxlmd->id);

Re: [RFC PATCH 2/2] cxl/mem: Add CDAT table reading from DOE

[Chris Browy]

> On Mar 10, 2021, at 1:14 PM, Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> 
> On Thu, 11 Mar 2021 02:03:06 +0800
> Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> 
>> This patch simply provides some debug print outs of the entries
>> at probe time + a sysfs binary attribute to allow dumping of the
>> whole table.
>> 
>> Binary dumping is modelled on /sys/firmware/ACPI/tables/
>> 
>> The ability to dump this table will be very useful for emulation of
>> real devices once they become available as QEMU CXL type 3 device
>> emulation will be able to load this file in.
>> 
>> Open questions:
>> * No support here for table updates. Worth including these from the
>>  start, or leave that complexity for later?
>> * Worth logging the reported info for debug, or is the binary attribute
>>  sufficient?  Larger open question of whether to expose this info to
>>  userspace or not left for another day!
>> * Where to put the CDAT file?  Is it worth a subdirectory?
>> * What is maximum size of the SSLBIS entry - I haven't quite managed
>>  to figure that out and this is the record with largest size.
>>  We could support dynamic allocation of the record size, but it
>>  would add complexity that seems unnecessary.
>>  It would not be compliant with the specification for a type 3 memory
>>  device to report this record anyway so I'm not that worried about this
>>  for now.  It will become relevant once we have support for reading
>>  CDAT from CXL switches.
>> * cdat.h is formatted in a similar style to pci_regs.h on basis that
>>  it may well be helpful to share this header with userspace tools.
>> * Move the generic parts of this out to driver/cxl/cdat.c or leave that
>>  until we have other CXL drivers wishing to use this?
> 
> Naturally I remembered another open question within 10 seconds of sending :(
> 
>  * Do we want to add any sort of header to the RAW dump of CDAT to aid
>    tooling?  Whilst it looks a little like an ACPI table it doesn't have
>    a signature.
> 
> My gut feeling is no, because the CDAT specification doesn't define one but
> I can see that it might be very convenient to have something that identified
> the data once it was put in a file.
> 
> @Chris, what were you thinking for the QEMU load of raw CDAT data for use
> in emulation?

In the v3 released yesterday you can specify the CDAT file on command line with property, 
cdat=<file.aml>.  But in light of the discussion in yesterday’s CXL SW WG it appears using 
iASL with non-ACPI tables is out of the question.

https://lore.kernel.org/qemu-devel/1615323876-17716-1-git-send-email-cbrowy@avery-design.com/

So we’ll make a change and put up a new patch that reads a pure raw binary file.  The 
cdat=<raw binary file> will be used to provide the binary file.

We could support both with a different property if we wanted 
cdat-iasl=<file.aml> or cdat=<raw binary file>


> 
>> 
>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> ---
>> drivers/cxl/cdat.h |  79 ++++++++++++++
>> drivers/cxl/cxl.h  |  13 +++
>> drivers/cxl/mem.c  | 253 ++++++++++++++++++++++++++++++++++++++++++++-
>> 3 files changed, 344 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/cxl/cdat.h b/drivers/cxl/cdat.h
>> new file mode 100644
>> index 000000000000..e67b18e02c35
>> --- /dev/null
>> +++ b/drivers/cxl/cdat.h
>> @@ -0,0 +1,79 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Coherent Device Attribute table (CDAT)
>> + *
>> + * Specification available from UEFI.org
>> + *
>> + * Whilst CDAT is defined as a single table, the access via DOE maiboxes is
>> + * done one entry at a time, where the first entry is the header.
>> + */
>> +
>> +#define CXL_DOE_TABLE_ACCESS_REQ_CODE		0x000000ff
>> +#define   CXL_DOE_TABLE_ACCESS_REQ_CODE_READ	0
>> +#define CXL_DOE_TABLE_ACCESS_TABLE_TYPE		0x0000ff00
>> +#define   CXL_DOE_TABLE_ACCESS_TABLE_TYPE_CDATA	0
>> +#define CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE	0xffff0000
>> +
>> +
>> +/*
>> + * CDAT entries are little endian and are read from PCI config space which
>> + * is also little endian.
>> + * As such, on a big endian system these will have been reversed.
>> + * This prevents us from making easy use of packed structures.
>> + * Style form pci_regs.h
>> + */
>> +
>> +#define CDAT_HEADER_LENGTH_DW 3
>> +#define CDAT_HEADER_DW0_LENGTH		0xFFFFFFFF
>> +#define CDAT_HEADER_DW1_REVISION	0x000000FF
>> +#define CDAT_HEADER_DW1_CHECKSUM	0x0000FF00
>> +#define CDAT_HEADER_DW2_SEQUENCE	0xFFFFFFFF
>> +
>> +/* All structures have a common first DW */
>> +#define CDAT_STRUCTURE_DW0_TYPE		0x000000FF
>> +#define   CDAT_STRUCTURE_DW0_TYPE_DSMAS 0
>> +#define   CDAT_STRUCTURE_DW0_TYPE_DSLBIS 1
>> +#define   CDAT_STRUCTURE_DW0_TYPE_DSMSCIS 2
>> +#define   CDAT_STRUCTURE_DW0_TYPE_DSIS 3
>> +#define   CDAT_STRUCTURE_DW0_TYPE_DSEMTS 4
>> +#define   CDAT_STRUCTURE_DW0_TYPE_SSLBIS 5
>> +
>> +#define CDAT_STRUCTURE_DW0_LENGTH	0xFFFF0000
>> +
>> +/* Device Scoped Memory Affinity Structure */
>> +#define CDAT_DSMAS_DW1_DSMAD_HANDLE	0x000000ff
>> +#define CDAT_DSMAS_DW1_FLAGS		0x0000ff00
>> +#define CDAT_DSMAS_DPA_OFFSET(entry) ((u64)((entry)[3]) << 32 | (entry)[2])
>> +#define CDAT_DSMAS_DPA_LEN(entry) ((u64)((entry)[5]) << 32 | (entry)[4])
>> +
>> +/* Device Scoped Latency and Bandwidth Information Structure */
>> +#define CDAT_DSLBIS_DW1_HANDLE		0x000000ff
>> +#define CDAT_DSLBIS_DW1_FLAGS		0x0000ff00
>> +#define CDAT_DSLBIS_DW1_DATA_TYPE	0x00ff0000
>> +#define CDAT_DSLBIS_BASE_UNIT(entry) ((u64)((entry)[3]) << 32 | (entry)[2])
>> +#define CDAT_DSLBIS_DW4_ENTRY_0		0x0000ffff
>> +#define CDAT_DSLBIS_DW4_ENTRY_1		0xffff0000
>> +#define CDAT_DSLBIS_DW5_ENTRY_2		0x0000ffff
>> +
>> +/* Device Scoped Memory Side Cache Information Structure */
>> +#define CDAT_DSMSCIS_DW1_HANDLE		0x000000ff
>> +#define CDAT_DSMSCIS_MEMORY_SIDE_CACHE_SIZE(entry) \
>> +	((u64)((entry)[3]) << 32 | (entry)[2])
>> +#define CDAT_DSMSCIS_DW4_MEMORY_SIDE_CACHE_ATTRS 0xffffffff
>> +
>> +/* Device Scoped Initiator Structure */
>> +#define CDAT_DSIS_DW1_FLAGS		0x000000ff
>> +#define CDAT_DSIS_DW1_HANDLE		0x0000ff00
>> +
>> +/* Device Scoped EFI Memory Type Structure */
>> +#define CDAT_DSEMTS_DW1_HANDLE		0x000000ff
>> +#define CDAT_DSEMTS_DW1_EFI_MEMORY_TYPE_ATTR	0x0000ff00
>> +#define CDAT_DSEMTS_DPA_OFFSET(entry)	((u64)((entry)[3]) << 32 | (entry)[2])
>> +#define CDAT_DSEMTS_DPA_LENGTH(entry)	((u64)((entry)[5]) << 32 | (entry)[4])
>> +
>> +/* Switch Scoped Latency and Bandwidth Information Structure */
>> +#define CDAT_SSLBIS_DW1_DATA_TYPE	0x000000ff
>> +#define CDAT_SSLBIS_BASE_UNIT(entry)	((u64)((entry)[3]) << 32 | (entry)[2])
>> +#define CDAT_SSLBIS_ENTRY_PORT_X(entry, i) ((entry)[4 + (i) * 2] & 0x0000ffff)
>> +#define CDAT_SSLBIS_ENTRY_PORT_Y(entry, i) (((entry)[4 + (i) * 2] & 0xffff0000) >> 16)
>> +#define CDAT_SSLBIS_ENTRY_LAT_OR_BW(entry, i) ((entry)[4 + (i) * 2 + 1] & 0x0000ffff)
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index 6f14838c2d25..2f5a69201fc3 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -7,6 +7,7 @@
>> #include <linux/bitfield.h>
>> #include <linux/bitops.h>
>> #include <linux/io.h>
>> +#include <linux/pcie-doe.h>
>> 
>> /* CXL 2.0 8.2.8.1 Device Capabilities Array Register */
>> #define CXLDEV_CAP_ARRAY_OFFSET 0x0
>> @@ -57,10 +58,21 @@
>> 	(FIELD_GET(CXLMDEV_RESET_NEEDED_MASK, status) !=                       \
>> 	 CXLMDEV_RESET_NEEDED_NOT)
>> 
>> +#define CXL_DOE_PROTOCOL_COMPLIANCE 0
>> +#define CXL_DOE_PROTOCOL_TABLE_ACCESS 2
>> +
>> +/* Common to request and response */
>> +#define CXL_DOE_TABLE_ACCESS_3_CODE GENMASK(7, 0)
>> +#define   CXL_DOE_TABLE_ACCESS_3_CODE_READ 0
>> +#define CXL_DOE_TABLE_ACCESS_3_TYPE GENMASK(15, 8)
>> +#define   CXL_DOE_TABLE_ACCESS_3_TYPE_CDAT 0
>> +#define CXL_DOE_TABLE_ACCESS_3_ENTRY_HANDLE GENMASK(31, 16)
>> +
>> struct cxl_memdev;
>> /**
>>  * struct cxl_mem - A CXL memory device
>>  * @pdev: The PCI device associated with this CXL device.
>> + * @doe: Data exchange object mailbox used to read tables.
>>  * @regs: IO mappings to the device's MMIO
>>  * @status_regs: CXL 2.0 8.2.8.3 Device Status Registers
>>  * @mbox_regs: CXL 2.0 8.2.8.4 Mailbox Registers
>> @@ -75,6 +87,7 @@ struct cxl_memdev;
>>  */
>> struct cxl_mem {
>> 	struct pci_dev *pdev;
>> +	struct pcie_doe doe;
>> 	void __iomem *regs;
>> 	struct cxl_memdev *cxlmd;
>> 
>> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
>> index 4597b28aeb3f..71de66bc6c54 100644
>> --- a/drivers/cxl/mem.c
>> +++ b/drivers/cxl/mem.c
>> @@ -12,6 +12,7 @@
>> #include <linux/io-64-nonatomic-lo-hi.h>
>> #include "pci.h"
>> #include "cxl.h"
>> +#include "cdat.h"
>> 
>> /**
>>  * DOC: cxl mem
>> @@ -91,6 +92,11 @@ struct mbox_cmd {
>> #define CXL_MBOX_SUCCESS 0
>> };
>> 
>> +struct doe_table_attr {
>> +	struct bin_attribute attr;
>> +	void *table;
>> +};
>> +
>> /**
>>  * struct cxl_memdev - CXL bus object representing a Type-3 Memory Device
>>  * @dev: driver core device object
>> @@ -98,6 +104,7 @@ struct mbox_cmd {
>>  * @cxlm: pointer to the parent device driver data
>>  * @ops_active: active user of @cxlm in ops handlers
>>  * @ops_dead: completion when all @cxlm ops users have exited
>> + * @table_attr: attribute used to provide dumping of table
>>  * @id: id number of this memdev instance.
>>  */
>> struct cxl_memdev {
>> @@ -106,6 +113,7 @@ struct cxl_memdev {
>> 	struct cxl_mem *cxlm;
>> 	struct percpu_ref ops_active;
>> 	struct completion ops_dead;
>> +	struct doe_table_attr table_attr;
>> 	int id;
>> };
>> 
>> @@ -976,13 +984,165 @@ static int cxl_mem_setup_mailbox(struct cxl_mem *cxlm)
>> 	return 0;
>> }
>> 
>> +#define CDAT_DOE_REQ(entry_handle)					\
>> +	[0] = FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_VID,		\
>> +			 PCI_DVSEC_VENDOR_ID_CXL) |			\
>> +	      FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_TYPE,		\
>> +			   CXL_DOE_PROTOCOL_TABLE_ACCESS),		\
>> +	[1] = FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH, 3),	\
>> +	[2] = FIELD_PREP(CXL_DOE_TABLE_ACCESS_REQ_CODE,			\
>> +			 CXL_DOE_TABLE_ACCESS_REQ_CODE_READ) |		\
>> +	      FIELD_PREP(CXL_DOE_TABLE_ACCESS_TABLE_TYPE,		\
>> +			 CXL_DOE_TABLE_ACCESS_TABLE_TYPE_CDATA) |	\
>> +	      FIELD_PREP(CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE, (entry_handle))
>> +
>> +static ssize_t cdat_get_length(struct pcie_doe *doe)
>> +{
>> +	u32 cdat_request[3] = {
>> +		CDAT_DOE_REQ(0),
>> +	};
>> +	u32 cdat_response[32];
>> +	ssize_t rc;
>> +
>> +	rc = pcie_doe_exchange(doe, cdat_request, sizeof(cdat_request),
>> +			       cdat_response, sizeof(cdat_response));
>> +	if (rc)
>> +		return rc;
>> +
>> +	return cdat_response[3];
>> +}
>> +
>> +static int cdat_to_buffer(struct pcie_doe *doe, u32 *buffer, size_t length)
>> +{
>> +	int entry_handle = 0;
>> +	int rc;
>> +
>> +	do {
>> +		u32 cdat_request[3] = {
>> +			CDAT_DOE_REQ(entry_handle)
>> +		};
>> +		u32 cdat_response[32];
>> +		size_t entry_dw;
>> +		u32 *entry;
>> +
>> +		rc = pcie_doe_exchange(doe, cdat_request, sizeof(cdat_request),
>> +				       cdat_response, sizeof(cdat_response));
>> +		if (rc)
>> +			return rc;
>> +
>> +		entry = cdat_response + CDAT_HEADER_LENGTH_DW;
>> +
>> +		entry_dw = FIELD_GET(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH, cdat_response[1]);
>> +		/* Skip Header */
>> +		entry_dw -= 3;
>> +		entry_dw = min(length / 4, entry_dw);
>> +		memcpy(buffer, entry, entry_dw * sizeof(u32));
>> +		length -= entry_dw * sizeof(u32);
>> +		buffer += entry_dw;
>> +		entry_handle = FIELD_GET(CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE, cdat_response[2]);
>> +	} while (entry_handle != 0xFFFF);
>> +
>> +	return 0;
>> +}
>> +
>> +static int cdat_dump(struct pcie_doe *doe)
>> +{
>> +	struct pci_dev *dev = doe->pdev;
>> +	int entry_handle = 0;
>> +	int rc;
>> +
>> +	do {
>> +		/* Table access is available */
>> +		u32 cdat_request[3] = {
>> +			CDAT_DOE_REQ(entry_handle)
>> +		};
>> +		u32 cdat_response[32];
>> +		u32 *entry;
>> +
>> +		rc = pcie_doe_exchange(doe, cdat_request, sizeof(cdat_request),
>> +				       cdat_response, sizeof(cdat_response));
>> +		if (rc)
>> +			return rc;
>> +
>> +		entry = cdat_response + CDAT_HEADER_LENGTH_DW;
>> +		if (entry_handle == 0) {
>> +			pci_info(dev,
>> +				 "CDAT Header (Length=%u, Revision=%u, Checksum=0x%x, Sequence=%u\n",
>> +				 entry[0],
>> +				 FIELD_GET(CDAT_HEADER_DW1_REVISION, entry[1]),
>> +				 FIELD_GET(CDAT_HEADER_DW1_CHECKSUM, entry[1]),
>> +				 entry[2]);
>> +		} else {
>> +			u8 entry_type = FIELD_GET(CDAT_STRUCTURE_DW0_TYPE, entry[0]);
>> +
>> +			switch (entry_type) {
>> +			case CDAT_STRUCTURE_DW0_TYPE_DSMAS:
>> +				pci_info(dev,
>> +					 "CDAT DSMAS (handle=%u flags=0x%x, dpa(0x%llx 0x%llx)\n",
>> +					 FIELD_GET(CDAT_DSMAS_DW1_DSMAD_HANDLE, entry[1]),
>> +					 FIELD_GET(CDAT_DSMAS_DW1_FLAGS, entry[1]),
>> +					 CDAT_DSMAS_DPA_OFFSET(entry),
>> +					 CDAT_DSMAS_DPA_LEN(entry));
>> +				break;
>> +			case CDAT_STRUCTURE_DW0_TYPE_DSLBIS:
>> +				pci_info(dev,
>> +					 "CDAT DSLBIS (handle=%u flags=0x%x, ent_base=0x%llx, entry[%u %u %u])\n",
>> +					 FIELD_GET(CDAT_DSLBIS_DW1_HANDLE, entry[1]),
>> +					 FIELD_GET(CDAT_DSLBIS_DW1_FLAGS, entry[1]),
>> +					 CDAT_DSLBIS_BASE_UNIT(entry),
>> +					 FIELD_GET(CDAT_DSLBIS_DW4_ENTRY_0, entry[4]),
>> +					 FIELD_GET(CDAT_DSLBIS_DW4_ENTRY_1, entry[4]),
>> +					 FIELD_GET(CDAT_DSLBIS_DW5_ENTRY_2, entry[5]));
>> +				break;
>> +			case CDAT_STRUCTURE_DW0_TYPE_DSMSCIS:
>> +				pci_info(dev,
>> +					 "CDAT DSMSCIS (handle=%u sc_size=0x%llx attrs=0x%x)\n",
>> +					 FIELD_GET(CDAT_DSMSCIS_DW1_HANDLE, entry[1]),
>> +					 CDAT_DSMSCIS_MEMORY_SIDE_CACHE_SIZE(entry),
>> +					 FIELD_GET(CDAT_DSMSCIS_DW4_MEMORY_SIDE_CACHE_ATTRS,
>> +						   entry[4]));
>> +				break;
>> +			case CDAT_STRUCTURE_DW0_TYPE_DSIS:
>> +				pci_info(dev,
>> +					 "CDAT DSIS (handle=%u flags=0x%x)\n",
>> +					 FIELD_GET(CDAT_DSIS_DW1_HANDLE, entry[1]),
>> +					 FIELD_GET(CDAT_DSIS_DW1_FLAGS, entry[1]));
>> +				break;
>> +			case CDAT_STRUCTURE_DW0_TYPE_DSEMTS:
>> +				pci_info(dev,
>> +					 "CDAT DSEMTS (handle=%u EFI=0x%x dpa(0x%llx 0x%llx)\n",
>> +					 FIELD_GET(CDAT_DSEMTS_DW1_HANDLE, entry[1]),
>> +					 FIELD_GET(CDAT_DSEMTS_DW1_EFI_MEMORY_TYPE_ATTR,
>> +						   entry[1]),
>> +					 CDAT_DSEMTS_DPA_OFFSET(entry),
>> +					 CDAT_DSEMTS_DPA_LENGTH(entry));
>> +				break;
>> +			case CDAT_STRUCTURE_DW0_TYPE_SSLBIS:
>> +				pci_info(dev,
>> +					 "CDAT SSLBIS (type%u ent_base=%llu...)\n",
>> +					 FIELD_GET(CDAT_SSLBIS_DW1_DATA_TYPE,
>> +						   entry[1]),
>> +					 CDAT_SSLBIS_BASE_UNIT(entry));
>> +				break;
>> +			}
>> +		}
>> +		entry_handle = FIELD_GET(CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE,
>> +					 cdat_response[2]);
>> +	} while (entry_handle != 0xFFFF);
>> +
>> +	return 0;
>> +}
>> +
>> static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev, u32 reg_lo,
>> 				      u32 reg_hi)
>> {
>> 	struct device *dev = &pdev->dev;
>> 	struct cxl_mem *cxlm;
>> 	void __iomem *regs;
>> +	bool doe_use_irq;
>> +	int pos = 0;
>> 	u64 offset;
>> +	int irqs;
>> 	u8 bar;
>> 	int rc;
>> 
>> @@ -1021,6 +1181,44 @@ static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev, u32 reg_lo,
>> 		return NULL;
>> 	}
>> 
>> +	/*
>> +	 * An implementation of a cxl type3 device may support an unknown
>> +	 * number of interrupts. Assume that number is not that large and
>> +	 * request them all.
>> +	 */
>> +	irqs = pci_msix_vec_count(pdev);
>> +	rc = pci_alloc_irq_vectors(pdev, irqs, irqs, PCI_IRQ_MSIX);
>> +	if (rc != irqs) {
>> +		/* No interrupt available - carry on */
>> +		dev_dbg(dev, "No interrupts available for DOE\n");
>> +		doe_use_irq = false;
>> +	} else {
>> +		/*
>> +		 * Enabling bus mastering could be done within the DOE
>> +		 * initialization, but as it potentially has other impacts
>> +		 * keep it within the driver.
>> +		 */
>> +		pci_set_master(pdev);
>> +		doe_use_irq = true;
>> +	}
>> +
>> +	/*
>> +	 * Find a DOE mailbox that supports CDAT.
>> +	 * Supporting other DOE protocols will require more complexity.
>> +	 */
>> +	do {
>> +		pos = pci_find_next_ext_capability(pdev, pos, PCI_EXT_CAP_ID_DOE);
>> +		if (!pos)
>> +			return NULL;
>> +
>> +		pcie_doe_init(&cxlm->doe, pdev, pos, doe_use_irq);
>> +	} while (pcie_doe_protocol_check(&cxlm->doe, PCI_DVSEC_VENDOR_ID_CXL,
>> +					 CXL_DOE_PROTOCOL_TABLE_ACCESS));
>> +
>> +	rc = cdat_dump(&cxlm->doe);
>> +	if (rc)
>> +		return NULL;
>> +
>> 	dev_dbg(dev, "Mapped CXL Memory Device resource\n");
>> 	return cxlm;
>> }
>> @@ -1178,6 +1376,55 @@ static void cxlmdev_ops_active_release(struct percpu_ref *ref)
>> 	complete(&cxlmd->ops_dead);
>> }
>> 
>> +static ssize_t cdat_table_show(struct file *filp, struct kobject *kobj,
>> +			       struct bin_attribute *bin_attr, char *buf,
>> +			       loff_t offset, size_t count)
>> +{
>> +	struct doe_table_attr *table_attr =
>> +		container_of(bin_attr, struct doe_table_attr, attr);
>> +
>> +	return memory_read_from_buffer(buf, count, &offset, table_attr->table,
>> +				       bin_attr->size);
>> +}
>> +
>> +static void cxl_remove_table_sysfs(void *_cxlmd)
>> +{
>> +	struct cxl_memdev *cxlmd = _cxlmd;
>> +	struct device *dev = &cxlmd->dev;
>> +
>> +	sysfs_remove_bin_file(&dev->kobj, &cxlmd->table_attr.attr);
>> +}
>> +
>> +static int cxl_add_table_sysfs(struct cxl_memdev *cxlmd)
>> +{
>> +	struct cxl_mem *cxlm = cxlmd->cxlm;
>> +	struct device *dev = &cxlmd->dev;
>> +	ssize_t cdat_length;
>> +	int rc;
>> +
>> +	cdat_length = cdat_get_length(&cxlm->doe);
>> +	if (cdat_length < 0)
>> +		return cdat_length;
>> +
>> +	sysfs_attr_init(&cxlmd->table_attr.attr.attr);
>> +	/* Updates of CDAT are not yet handled so length is fixed. */
>> +	cxlmd->table_attr.attr.size = cdat_length;
>> +	cxlmd->table_attr.attr.read = cdat_table_show;
>> +	cxlmd->table_attr.attr.attr.name = "CDAT";
>> +	cxlmd->table_attr.attr.attr.mode = 0400;
>> +	cxlmd->table_attr.table = devm_kzalloc(dev->parent, cdat_length, GFP_KERNEL);
>> +
>> +	rc = cdat_to_buffer(&cxlm->doe, cxlmd->table_attr.table, cdat_length);
>> +	if (rc)
>> +		return rc;
>> +
>> +	rc = sysfs_create_bin_file(&dev->kobj, &cxlmd->table_attr.attr);
>> +	if (rc)
>> +		return rc;
>> +
>> +	return devm_add_action_or_reset(dev->parent, cxl_remove_table_sysfs, cxlmd);
>> +}
>> +
>> static int cxl_mem_add_memdev(struct cxl_mem *cxlm)
>> {
>> 	struct pci_dev *pdev = cxlm->pdev;
>> @@ -1221,7 +1468,11 @@ static int cxl_mem_add_memdev(struct cxl_mem *cxlm)
>> 	if (rc)
>> 		goto err_add;
>> 
>> -	return devm_add_action_or_reset(dev->parent, cxlmdev_unregister, cxlmd);
>> +	rc = devm_add_action_or_reset(dev->parent, cxlmdev_unregister, cxlmd);
>> +	if (rc)
>> +		return rc;
>> +
>> +	return cxl_add_table_sysfs(cxlmd);
>> 
>> err_add:
>> 	ida_free(&cxl_memdev_ida, cxlmd->id);
> 

Re: [RFC PATCH 2/2] cxl/mem: Add CDAT table reading from DOE

[Dan Williams]

On Wed, Mar 10, 2021 at 10:15 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Thu, 11 Mar 2021 02:03:06 +0800
> Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
>
> > This patch simply provides some debug print outs of the entries
> > at probe time + a sysfs binary attribute to allow dumping of the
> > whole table.
> >
> > Binary dumping is modelled on /sys/firmware/ACPI/tables/
> >
> > The ability to dump this table will be very useful for emulation of
> > real devices once they become available as QEMU CXL type 3 device
> > emulation will be able to load this file in.
> >
> > Open questions:
> > * No support here for table updates. Worth including these from the
> >   start, or leave that complexity for later?
> > * Worth logging the reported info for debug, or is the binary attribute
> >   sufficient?  Larger open question of whether to expose this info to
> >   userspace or not left for another day!
> > * Where to put the CDAT file?  Is it worth a subdirectory?
> > * What is maximum size of the SSLBIS entry - I haven't quite managed
> >   to figure that out and this is the record with largest size.
> >   We could support dynamic allocation of the record size, but it
> >   would add complexity that seems unnecessary.
> >   It would not be compliant with the specification for a type 3 memory
> >   device to report this record anyway so I'm not that worried about this
> >   for now.  It will become relevant once we have support for reading
> >   CDAT from CXL switches.
> > * cdat.h is formatted in a similar style to pci_regs.h on basis that
> >   it may well be helpful to share this header with userspace tools.
> > * Move the generic parts of this out to driver/cxl/cdat.c or leave that
> >   until we have other CXL drivers wishing to use this?
>
> Naturally I remembered another open question within 10 seconds of sending :(
>
>   * Do we want to add any sort of header to the RAW dump of CDAT to aid
>     tooling?  Whilst it looks a little like an ACPI table it doesn't have
>     a signature.
>
> My gut feeling is no, because the CDAT specification doesn't define one but
> I can see that it might be very convenient to have something that identified
> the data once it was put in a file.

I'm not yet convinced raw dumping is worth it for the same reason that
command payload logging was eliminated from the v5.12-rc1 submission.
There's not much userspace can do with the information besides debug
the kernel behavior. If the kernel assigns a numa node to target a
given CXL memory range with NUMA apis then HMEM_REPORTING should
enumerate the properties. In other words, don't expand the userspace
ABI problem, funnel users to the canonical source for such data.

Re: [RFC PATCH 2/2] cxl/mem: Add CDAT table reading from DOE

[Jonathan Cameron]

On Mon, 15 Mar 2021 15:00:08 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> On Wed, Mar 10, 2021 at 10:15 AM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Thu, 11 Mar 2021 02:03:06 +0800
> > Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> >  
> > > This patch simply provides some debug print outs of the entries
> > > at probe time + a sysfs binary attribute to allow dumping of the
> > > whole table.
> > >
> > > Binary dumping is modelled on /sys/firmware/ACPI/tables/
> > >
> > > The ability to dump this table will be very useful for emulation of
> > > real devices once they become available as QEMU CXL type 3 device
> > > emulation will be able to load this file in.
> > >
> > > Open questions:
> > > * No support here for table updates. Worth including these from the
> > >   start, or leave that complexity for later?
> > > * Worth logging the reported info for debug, or is the binary attribute
> > >   sufficient?  Larger open question of whether to expose this info to
> > >   userspace or not left for another day!
> > > * Where to put the CDAT file?  Is it worth a subdirectory?
> > > * What is maximum size of the SSLBIS entry - I haven't quite managed
> > >   to figure that out and this is the record with largest size.
> > >   We could support dynamic allocation of the record size, but it
> > >   would add complexity that seems unnecessary.
> > >   It would not be compliant with the specification for a type 3 memory
> > >   device to report this record anyway so I'm not that worried about this
> > >   for now.  It will become relevant once we have support for reading
> > >   CDAT from CXL switches.
> > > * cdat.h is formatted in a similar style to pci_regs.h on basis that
> > >   it may well be helpful to share this header with userspace tools.
> > > * Move the generic parts of this out to driver/cxl/cdat.c or leave that
> > >   until we have other CXL drivers wishing to use this?  
> >
> > Naturally I remembered another open question within 10 seconds of sending :(
> >
> >   * Do we want to add any sort of header to the RAW dump of CDAT to aid
> >     tooling?  Whilst it looks a little like an ACPI table it doesn't have
> >     a signature.
> >
> > My gut feeling is no, because the CDAT specification doesn't define one but
> > I can see that it might be very convenient to have something that identified
> > the data once it was put in a file.  
> 
> I'm not yet convinced raw dumping is worth it for the same reason that
> command payload logging was eliminated from the v5.12-rc1 submission.
> There's not much userspace can do with the information besides debug
> the kernel behavior. If the kernel assigns a numa node to target a
> given CXL memory range with NUMA apis then HMEM_REPORTING should
> enumerate the properties. In other words, don't expand the userspace
> ABI problem, funnel users to the canonical source for such data.

As someone who finds raw dumping of ACPI tables extremely helpful in every
day use for debugging of some of our 'interesting' hardware, I know I'm going
to end up carrying that element locally anyway.  I don't have a particular
problem doing so if we decide to not to upstream it.

Much like the ACPI table dumping, it's not an interface you expect userspace
to ever use and I fully agree that we should expose things properly as you
describe.

Short term my interest here is to get the DOE code upstream as step 1 of
moving to a full solution.  The printing and dumping is really just PoC element
to prove out the interface.  Any issue with putting the prints under _dbg()?

Jonathan

Re: [RFC PATCH 2/2] cxl/mem: Add CDAT table reading from DOE

[Dan Williams]

On Tue, Mar 16, 2021 at 3:38 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Mon, 15 Mar 2021 15:00:08 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > On Wed, Mar 10, 2021 at 10:15 AM Jonathan Cameron
> > <Jonathan.Cameron@huawei.com> wrote:
> > >
> > > On Thu, 11 Mar 2021 02:03:06 +0800
> > > Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> > >
> > > > This patch simply provides some debug print outs of the entries
> > > > at probe time + a sysfs binary attribute to allow dumping of the
> > > > whole table.
> > > >
> > > > Binary dumping is modelled on /sys/firmware/ACPI/tables/
> > > >
> > > > The ability to dump this table will be very useful for emulation of
> > > > real devices once they become available as QEMU CXL type 3 device
> > > > emulation will be able to load this file in.
> > > >
> > > > Open questions:
> > > > * No support here for table updates. Worth including these from the
> > > >   start, or leave that complexity for later?
> > > > * Worth logging the reported info for debug, or is the binary attribute
> > > >   sufficient?  Larger open question of whether to expose this info to
> > > >   userspace or not left for another day!
> > > > * Where to put the CDAT file?  Is it worth a subdirectory?
> > > > * What is maximum size of the SSLBIS entry - I haven't quite managed
> > > >   to figure that out and this is the record with largest size.
> > > >   We could support dynamic allocation of the record size, but it
> > > >   would add complexity that seems unnecessary.
> > > >   It would not be compliant with the specification for a type 3 memory
> > > >   device to report this record anyway so I'm not that worried about this
> > > >   for now.  It will become relevant once we have support for reading
> > > >   CDAT from CXL switches.
> > > > * cdat.h is formatted in a similar style to pci_regs.h on basis that
> > > >   it may well be helpful to share this header with userspace tools.
> > > > * Move the generic parts of this out to driver/cxl/cdat.c or leave that
> > > >   until we have other CXL drivers wishing to use this?
> > >
> > > Naturally I remembered another open question within 10 seconds of sending :(
> > >
> > >   * Do we want to add any sort of header to the RAW dump of CDAT to aid
> > >     tooling?  Whilst it looks a little like an ACPI table it doesn't have
> > >     a signature.
> > >
> > > My gut feeling is no, because the CDAT specification doesn't define one but
> > > I can see that it might be very convenient to have something that identified
> > > the data once it was put in a file.
> >
> > I'm not yet convinced raw dumping is worth it for the same reason that
> > command payload logging was eliminated from the v5.12-rc1 submission.
> > There's not much userspace can do with the information besides debug
> > the kernel behavior. If the kernel assigns a numa node to target a
> > given CXL memory range with NUMA apis then HMEM_REPORTING should
> > enumerate the properties. In other words, don't expand the userspace
> > ABI problem, funnel users to the canonical source for such data.
>
> As someone who finds raw dumping of ACPI tables extremely helpful in every
> day use for debugging of some of our 'interesting' hardware, I know I'm going
> to end up carrying that element locally anyway.  I don't have a particular
> problem doing so if we decide to not to upstream it.
>
> Much like the ACPI table dumping, it's not an interface you expect userspace
> to ever use and I fully agree that we should expose things properly as you
> describe.
>
> Short term my interest here is to get the DOE code upstream as step 1 of
> moving to a full solution.  The printing and dumping is really just PoC element
> to prove out the interface.  Any issue with putting the prints under _dbg()?

debugfs_create_blob()? Although debugfs makes it annoying to support
per device blobs. I could get on board with a root-only sysfs
attribute, but using a static DEVICE_ATTR_ADMIN_RO()... more comments
incoming in a review of the patch.

Re: [RFC PATCH 2/2] cxl/mem: Add CDAT table reading from DOE

[Dan Williams]

On Wed, Mar 10, 2021 at 10:08 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> This patch simply provides some debug print outs of the entries
> at probe time + a sysfs binary attribute to allow dumping of the
> whole table.
>
> Binary dumping is modelled on /sys/firmware/ACPI/tables/
>
> The ability to dump this table will be very useful for emulation of
> real devices once they become available as QEMU CXL type 3 device
> emulation will be able to load this file in.
>

Certainly useful for kernel / QEMU development + debug, not
necessarily as a userspace ABI.

> Open questions:
> * No support here for table updates. Worth including these from the
>   start, or leave that complexity for later?

Depends what the kernel will do with this information. If it's just to
assign a numa node then the interface could be to override / augment
the numa node assignment, not necessarily override the entire table
data.

> * Worth logging the reported info for debug, or is the binary attribute
>   sufficient?  Larger open question of whether to expose this info to
>   userspace or not left for another day!

Of all the options I like logging the least... root-only sysfs at
least doesn't enable a functional user ABI out of the gate.

> * Where to put the CDAT file?  Is it worth a subdirectory?

I think where you have it is fine, hanging directly off the memdev. It
isn't a PCIE property, it's CXL data and the memdev is the CXL
sub-functionality of the PCIE device.

However, It helps udev scripts if userspace can't race device
visibility with attribute visibility. I.e. if it is added to the
static cxl_memdev_attribute_groups rather than a dynamic attribute
creation then there is no gap between when userspace sees the KOBJ_ADD
event and when it can access the attribute.

> * What is maximum size of the SSLBIS entry - I haven't quite managed
>   to figure that out and this is the record with largest size.
>   We could support dynamic allocation of the record size, but it
>   would add complexity that seems unnecessary.
>   It would not be compliant with the specification for a type 3 memory
>   device to report this record anyway so I'm not that worried about this
>   for now.  It will become relevant once we have support for reading
>   CDAT from CXL switches.
> * cdat.h is formatted in a similar style to pci_regs.h on basis that
>   it may well be helpful to share this header with userspace tools.
> * Move the generic parts of this out to driver/cxl/cdat.c or leave that
>   until we have other CXL drivers wishing to use this?
>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/cxl/cdat.h |  79 ++++++++++++++
>  drivers/cxl/cxl.h  |  13 +++
>  drivers/cxl/mem.c  | 253 ++++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 344 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cxl/cdat.h b/drivers/cxl/cdat.h
> new file mode 100644
> index 000000000000..e67b18e02c35
> --- /dev/null
> +++ b/drivers/cxl/cdat.h
> @@ -0,0 +1,79 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Coherent Device Attribute table (CDAT)
> + *
> + * Specification available from UEFI.org
> + *
> + * Whilst CDAT is defined as a single table, the access via DOE maiboxes is
> + * done one entry at a time, where the first entry is the header.
> + */
> +
> +#define CXL_DOE_TABLE_ACCESS_REQ_CODE          0x000000ff
> +#define   CXL_DOE_TABLE_ACCESS_REQ_CODE_READ   0
> +#define CXL_DOE_TABLE_ACCESS_TABLE_TYPE                0x0000ff00
> +#define   CXL_DOE_TABLE_ACCESS_TABLE_TYPE_CDATA        0
> +#define CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE      0xffff0000
> +
> +
> +/*
> + * CDAT entries are little endian and are read from PCI config space which
> + * is also little endian.
> + * As such, on a big endian system these will have been reversed.
> + * This prevents us from making easy use of packed structures.
> + * Style form pci_regs.h
> + */
> +
> +#define CDAT_HEADER_LENGTH_DW 3
> +#define CDAT_HEADER_DW0_LENGTH         0xFFFFFFFF
> +#define CDAT_HEADER_DW1_REVISION       0x000000FF
> +#define CDAT_HEADER_DW1_CHECKSUM       0x0000FF00
> +#define CDAT_HEADER_DW2_SEQUENCE       0xFFFFFFFF
> +
> +/* All structures have a common first DW */
> +#define CDAT_STRUCTURE_DW0_TYPE                0x000000FF
> +#define   CDAT_STRUCTURE_DW0_TYPE_DSMAS 0
> +#define   CDAT_STRUCTURE_DW0_TYPE_DSLBIS 1
> +#define   CDAT_STRUCTURE_DW0_TYPE_DSMSCIS 2
> +#define   CDAT_STRUCTURE_DW0_TYPE_DSIS 3
> +#define   CDAT_STRUCTURE_DW0_TYPE_DSEMTS 4
> +#define   CDAT_STRUCTURE_DW0_TYPE_SSLBIS 5
> +
> +#define CDAT_STRUCTURE_DW0_LENGTH      0xFFFF0000
> +
> +/* Device Scoped Memory Affinity Structure */
> +#define CDAT_DSMAS_DW1_DSMAD_HANDLE    0x000000ff
> +#define CDAT_DSMAS_DW1_FLAGS           0x0000ff00
> +#define CDAT_DSMAS_DPA_OFFSET(entry) ((u64)((entry)[3]) << 32 | (entry)[2])
> +#define CDAT_DSMAS_DPA_LEN(entry) ((u64)((entry)[5]) << 32 | (entry)[4])
> +
> +/* Device Scoped Latency and Bandwidth Information Structure */
> +#define CDAT_DSLBIS_DW1_HANDLE         0x000000ff
> +#define CDAT_DSLBIS_DW1_FLAGS          0x0000ff00
> +#define CDAT_DSLBIS_DW1_DATA_TYPE      0x00ff0000
> +#define CDAT_DSLBIS_BASE_UNIT(entry) ((u64)((entry)[3]) << 32 | (entry)[2])
> +#define CDAT_DSLBIS_DW4_ENTRY_0                0x0000ffff
> +#define CDAT_DSLBIS_DW4_ENTRY_1                0xffff0000
> +#define CDAT_DSLBIS_DW5_ENTRY_2                0x0000ffff
> +
> +/* Device Scoped Memory Side Cache Information Structure */
> +#define CDAT_DSMSCIS_DW1_HANDLE                0x000000ff
> +#define CDAT_DSMSCIS_MEMORY_SIDE_CACHE_SIZE(entry) \
> +       ((u64)((entry)[3]) << 32 | (entry)[2])
> +#define CDAT_DSMSCIS_DW4_MEMORY_SIDE_CACHE_ATTRS 0xffffffff
> +
> +/* Device Scoped Initiator Structure */
> +#define CDAT_DSIS_DW1_FLAGS            0x000000ff
> +#define CDAT_DSIS_DW1_HANDLE           0x0000ff00
> +
> +/* Device Scoped EFI Memory Type Structure */
> +#define CDAT_DSEMTS_DW1_HANDLE         0x000000ff
> +#define CDAT_DSEMTS_DW1_EFI_MEMORY_TYPE_ATTR   0x0000ff00
> +#define CDAT_DSEMTS_DPA_OFFSET(entry)  ((u64)((entry)[3]) << 32 | (entry)[2])
> +#define CDAT_DSEMTS_DPA_LENGTH(entry)  ((u64)((entry)[5]) << 32 | (entry)[4])
> +
> +/* Switch Scoped Latency and Bandwidth Information Structure */
> +#define CDAT_SSLBIS_DW1_DATA_TYPE      0x000000ff
> +#define CDAT_SSLBIS_BASE_UNIT(entry)   ((u64)((entry)[3]) << 32 | (entry)[2])
> +#define CDAT_SSLBIS_ENTRY_PORT_X(entry, i) ((entry)[4 + (i) * 2] & 0x0000ffff)
> +#define CDAT_SSLBIS_ENTRY_PORT_Y(entry, i) (((entry)[4 + (i) * 2] & 0xffff0000) >> 16)
> +#define CDAT_SSLBIS_ENTRY_LAT_OR_BW(entry, i) ((entry)[4 + (i) * 2 + 1] & 0x0000ffff)
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 6f14838c2d25..2f5a69201fc3 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -7,6 +7,7 @@
>  #include <linux/bitfield.h>
>  #include <linux/bitops.h>
>  #include <linux/io.h>
> +#include <linux/pcie-doe.h>
>
>  /* CXL 2.0 8.2.8.1 Device Capabilities Array Register */
>  #define CXLDEV_CAP_ARRAY_OFFSET 0x0
> @@ -57,10 +58,21 @@
>         (FIELD_GET(CXLMDEV_RESET_NEEDED_MASK, status) !=                       \
>          CXLMDEV_RESET_NEEDED_NOT)
>
> +#define CXL_DOE_PROTOCOL_COMPLIANCE 0
> +#define CXL_DOE_PROTOCOL_TABLE_ACCESS 2
> +
> +/* Common to request and response */
> +#define CXL_DOE_TABLE_ACCESS_3_CODE GENMASK(7, 0)
> +#define   CXL_DOE_TABLE_ACCESS_3_CODE_READ 0
> +#define CXL_DOE_TABLE_ACCESS_3_TYPE GENMASK(15, 8)
> +#define   CXL_DOE_TABLE_ACCESS_3_TYPE_CDAT 0
> +#define CXL_DOE_TABLE_ACCESS_3_ENTRY_HANDLE GENMASK(31, 16)
> +
>  struct cxl_memdev;
>  /**
>   * struct cxl_mem - A CXL memory device
>   * @pdev: The PCI device associated with this CXL device.
> + * @doe: Data exchange object mailbox used to read tables.
>   * @regs: IO mappings to the device's MMIO
>   * @status_regs: CXL 2.0 8.2.8.3 Device Status Registers
>   * @mbox_regs: CXL 2.0 8.2.8.4 Mailbox Registers
> @@ -75,6 +87,7 @@ struct cxl_memdev;
>   */
>  struct cxl_mem {
>         struct pci_dev *pdev;
> +       struct pcie_doe doe;

Hmm, I would have expected this to be a pointer. DOE is an optional
component of a hardware device, so it should not be a mandatory
component of a 'struct cxl_mem' instance.

>         void __iomem *regs;
>         struct cxl_memdev *cxlmd;
>
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 4597b28aeb3f..71de66bc6c54 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -12,6 +12,7 @@
>  #include <linux/io-64-nonatomic-lo-hi.h>
>  #include "pci.h"
>  #include "cxl.h"
> +#include "cdat.h"
>
>  /**
>   * DOC: cxl mem
> @@ -91,6 +92,11 @@ struct mbox_cmd {
>  #define CXL_MBOX_SUCCESS 0
>  };
>
> +struct doe_table_attr {
> +       struct bin_attribute attr;
> +       void *table;
> +};
> +
>  /**
>   * struct cxl_memdev - CXL bus object representing a Type-3 Memory Device
>   * @dev: driver core device object
> @@ -98,6 +104,7 @@ struct mbox_cmd {
>   * @cxlm: pointer to the parent device driver data
>   * @ops_active: active user of @cxlm in ops handlers
>   * @ops_dead: completion when all @cxlm ops users have exited
> + * @table_attr: attribute used to provide dumping of table
>   * @id: id number of this memdev instance.
>   */
>  struct cxl_memdev {
> @@ -106,6 +113,7 @@ struct cxl_memdev {
>         struct cxl_mem *cxlm;
>         struct percpu_ref ops_active;
>         struct completion ops_dead;
> +       struct doe_table_attr table_attr;

No, cxl_memdev attributes are statically defined and optionally
visible, see cxl_memdev_attribute_groups.

>         int id;
>  };
>
> @@ -976,13 +984,165 @@ static int cxl_mem_setup_mailbox(struct cxl_mem *cxlm)
>         return 0;
>  }
>
> +#define CDAT_DOE_REQ(entry_handle)                                     \
> +       [0] = FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_VID,              \
> +                        PCI_DVSEC_VENDOR_ID_CXL) |                     \
> +             FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_TYPE,             \
> +                          CXL_DOE_PROTOCOL_TABLE_ACCESS),              \
> +       [1] = FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH, 3),       \
> +       [2] = FIELD_PREP(CXL_DOE_TABLE_ACCESS_REQ_CODE,                 \
> +                        CXL_DOE_TABLE_ACCESS_REQ_CODE_READ) |          \
> +             FIELD_PREP(CXL_DOE_TABLE_ACCESS_TABLE_TYPE,               \
> +                        CXL_DOE_TABLE_ACCESS_TABLE_TYPE_CDATA) |       \
> +             FIELD_PREP(CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE, (entry_handle))


I feel like this is asking for more type safety and an easier to read
definition. I.e. even though specification defines requests in terms
of arrays of dwords, there's nothing stopping the implementation from
defining something like:

struct pcie_doe_object {
    u16 vendor;
    u8 type;
    u8 reserved;
    u32 length;
    u32 payload[];
};

> +
> +static ssize_t cdat_get_length(struct pcie_doe *doe)
> +{
> +       u32 cdat_request[3] = {
> +               CDAT_DOE_REQ(0),
> +       };
> +       u32 cdat_response[32];
> +       ssize_t rc;
> +
> +       rc = pcie_doe_exchange(doe, cdat_request, sizeof(cdat_request),
> +                              cdat_response, sizeof(cdat_response));
> +       if (rc)
> +               return rc;
> +
> +       return cdat_response[3];
> +}
> +
> +static int cdat_to_buffer(struct pcie_doe *doe, u32 *buffer, size_t length)
> +{
> +       int entry_handle = 0;
> +       int rc;
> +
> +       do {
> +               u32 cdat_request[3] = {
> +                       CDAT_DOE_REQ(entry_handle)
> +               };
> +               u32 cdat_response[32];
> +               size_t entry_dw;
> +               u32 *entry;
> +
> +               rc = pcie_doe_exchange(doe, cdat_request, sizeof(cdat_request),
> +                                      cdat_response, sizeof(cdat_response));
> +               if (rc)
> +                       return rc;
> +
> +               entry = cdat_response + CDAT_HEADER_LENGTH_DW;
> +
> +               entry_dw = FIELD_GET(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH, cdat_response[1]);
> +               /* Skip Header */
> +               entry_dw -= 3;
> +               entry_dw = min(length / 4, entry_dw);
> +               memcpy(buffer, entry, entry_dw * sizeof(u32));
> +               length -= entry_dw * sizeof(u32);
> +               buffer += entry_dw;
> +               entry_handle = FIELD_GET(CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE, cdat_response[2]);
> +       } while (entry_handle != 0xFFFF);
> +
> +       return 0;
> +}
> +
> +static int cdat_dump(struct pcie_doe *doe)
> +{
> +       struct pci_dev *dev = doe->pdev;
> +       int entry_handle = 0;
> +       int rc;
> +
> +       do {
> +               /* Table access is available */
> +               u32 cdat_request[3] = {
> +                       CDAT_DOE_REQ(entry_handle)
> +               };
> +               u32 cdat_response[32];
> +               u32 *entry;
> +
> +               rc = pcie_doe_exchange(doe, cdat_request, sizeof(cdat_request),
> +                                      cdat_response, sizeof(cdat_response));
> +               if (rc)
> +                       return rc;
> +
> +               entry = cdat_response + CDAT_HEADER_LENGTH_DW;
> +               if (entry_handle == 0) {
> +                       pci_info(dev,
> +                                "CDAT Header (Length=%u, Revision=%u, Checksum=0x%x, Sequence=%u\n",
> +                                entry[0],
> +                                FIELD_GET(CDAT_HEADER_DW1_REVISION, entry[1]),
> +                                FIELD_GET(CDAT_HEADER_DW1_CHECKSUM, entry[1]),
> +                                entry[2]);
> +               } else {
> +                       u8 entry_type = FIELD_GET(CDAT_STRUCTURE_DW0_TYPE, entry[0]);
> +
> +                       switch (entry_type) {
> +                       case CDAT_STRUCTURE_DW0_TYPE_DSMAS:
> +                               pci_info(dev,
> +                                        "CDAT DSMAS (handle=%u flags=0x%x, dpa(0x%llx 0x%llx)\n",
> +                                        FIELD_GET(CDAT_DSMAS_DW1_DSMAD_HANDLE, entry[1]),
> +                                        FIELD_GET(CDAT_DSMAS_DW1_FLAGS, entry[1]),
> +                                        CDAT_DSMAS_DPA_OFFSET(entry),
> +                                        CDAT_DSMAS_DPA_LEN(entry));
> +                               break;
> +                       case CDAT_STRUCTURE_DW0_TYPE_DSLBIS:
> +                               pci_info(dev,
> +                                        "CDAT DSLBIS (handle=%u flags=0x%x, ent_base=0x%llx, entry[%u %u %u])\n",
> +                                        FIELD_GET(CDAT_DSLBIS_DW1_HANDLE, entry[1]),
> +                                        FIELD_GET(CDAT_DSLBIS_DW1_FLAGS, entry[1]),
> +                                        CDAT_DSLBIS_BASE_UNIT(entry),
> +                                        FIELD_GET(CDAT_DSLBIS_DW4_ENTRY_0, entry[4]),
> +                                        FIELD_GET(CDAT_DSLBIS_DW4_ENTRY_1, entry[4]),
> +                                        FIELD_GET(CDAT_DSLBIS_DW5_ENTRY_2, entry[5]));
> +                               break;
> +                       case CDAT_STRUCTURE_DW0_TYPE_DSMSCIS:
> +                               pci_info(dev,
> +                                        "CDAT DSMSCIS (handle=%u sc_size=0x%llx attrs=0x%x)\n",
> +                                        FIELD_GET(CDAT_DSMSCIS_DW1_HANDLE, entry[1]),
> +                                        CDAT_DSMSCIS_MEMORY_SIDE_CACHE_SIZE(entry),
> +                                        FIELD_GET(CDAT_DSMSCIS_DW4_MEMORY_SIDE_CACHE_ATTRS,
> +                                                  entry[4]));
> +                               break;
> +                       case CDAT_STRUCTURE_DW0_TYPE_DSIS:
> +                               pci_info(dev,
> +                                        "CDAT DSIS (handle=%u flags=0x%x)\n",
> +                                        FIELD_GET(CDAT_DSIS_DW1_HANDLE, entry[1]),
> +                                        FIELD_GET(CDAT_DSIS_DW1_FLAGS, entry[1]));
> +                               break;
> +                       case CDAT_STRUCTURE_DW0_TYPE_DSEMTS:
> +                               pci_info(dev,
> +                                        "CDAT DSEMTS (handle=%u EFI=0x%x dpa(0x%llx 0x%llx)\n",
> +                                        FIELD_GET(CDAT_DSEMTS_DW1_HANDLE, entry[1]),
> +                                        FIELD_GET(CDAT_DSEMTS_DW1_EFI_MEMORY_TYPE_ATTR,
> +                                                  entry[1]),
> +                                        CDAT_DSEMTS_DPA_OFFSET(entry),
> +                                        CDAT_DSEMTS_DPA_LENGTH(entry));
> +                               break;
> +                       case CDAT_STRUCTURE_DW0_TYPE_SSLBIS:
> +                               pci_info(dev,
> +                                        "CDAT SSLBIS (type%u ent_base=%llu...)\n",
> +                                        FIELD_GET(CDAT_SSLBIS_DW1_DATA_TYPE,
> +                                                  entry[1]),
> +                                        CDAT_SSLBIS_BASE_UNIT(entry));
> +                               break;
> +                       }
> +               }
> +               entry_handle = FIELD_GET(CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE,
> +                                        cdat_response[2]);
> +       } while (entry_handle != 0xFFFF);

I'd drop the logging in favor of the bin file.

> +
> +       return 0;
> +}
> +
>  static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev, u32 reg_lo,
>                                       u32 reg_hi)
>  {
>         struct device *dev = &pdev->dev;
>         struct cxl_mem *cxlm;
>         void __iomem *regs;
> +       bool doe_use_irq;
> +       int pos = 0;
>         u64 offset;
> +       int irqs;
>         u8 bar;
>         int rc;
>
> @@ -1021,6 +1181,44 @@ static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev, u32 reg_lo,
>                 return NULL;
>         }
>
> +       /*
> +        * An implementation of a cxl type3 device may support an unknown
> +        * number of interrupts. Assume that number is not that large and
> +        * request them all.
> +        */
> +       irqs = pci_msix_vec_count(pdev);
> +       rc = pci_alloc_irq_vectors(pdev, irqs, irqs, PCI_IRQ_MSIX);
> +       if (rc != irqs) {
> +               /* No interrupt available - carry on */
> +               dev_dbg(dev, "No interrupts available for DOE\n");
> +               doe_use_irq = false;
> +       } else {
> +               /*
> +                * Enabling bus mastering could be done within the DOE
> +                * initialization, but as it potentially has other impacts
> +                * keep it within the driver.
> +                */
> +               pci_set_master(pdev);
> +               doe_use_irq = true;
> +       }
> +
> +       /*
> +        * Find a DOE mailbox that supports CDAT.
> +        * Supporting other DOE protocols will require more complexity.
> +        */
> +       do {
> +               pos = pci_find_next_ext_capability(pdev, pos, PCI_EXT_CAP_ID_DOE);
> +               if (!pos)
> +                       return NULL;
> +
> +               pcie_doe_init(&cxlm->doe, pdev, pos, doe_use_irq);

I don't think the driver needs to say use-irq or not. If
pcie_doe_init() sees ->{msi,msix}_enabled then try to find the doe
irq, if not then don't...

> +       } while (pcie_doe_protocol_check(&cxlm->doe, PCI_DVSEC_VENDOR_ID_CXL,
> +                                        CXL_DOE_PROTOCOL_TABLE_ACCESS));

Feel like this search should be internal to the library the host
driver does not care which DOE instance is chosen it just cares about
CXL_DOE_PROTOCOL_TABLE_ACCESS.

> +
> +       rc = cdat_dump(&cxlm->doe);
> +       if (rc)
> +               return NULL;
> +
>         dev_dbg(dev, "Mapped CXL Memory Device resource\n");
>         return cxlm;
>  }
> @@ -1178,6 +1376,55 @@ static void cxlmdev_ops_active_release(struct percpu_ref *ref)
>         complete(&cxlmd->ops_dead);
>  }
>
> +static ssize_t cdat_table_show(struct file *filp, struct kobject *kobj,
> +                              struct bin_attribute *bin_attr, char *buf,
> +                              loff_t offset, size_t count)
> +{
> +       struct doe_table_attr *table_attr =
> +               container_of(bin_attr, struct doe_table_attr, attr);
> +
> +       return memory_read_from_buffer(buf, count, &offset, table_attr->table,
> +                                      bin_attr->size);
> +}
> +
> +static void cxl_remove_table_sysfs(void *_cxlmd)
> +{
> +       struct cxl_memdev *cxlmd = _cxlmd;
> +       struct device *dev = &cxlmd->dev;
> +
> +       sysfs_remove_bin_file(&dev->kobj, &cxlmd->table_attr.attr);
> +}
> +
> +static int cxl_add_table_sysfs(struct cxl_memdev *cxlmd)
> +{
> +       struct cxl_mem *cxlm = cxlmd->cxlm;
> +       struct device *dev = &cxlmd->dev;
> +       ssize_t cdat_length;
> +       int rc;
> +
> +       cdat_length = cdat_get_length(&cxlm->doe);
> +       if (cdat_length < 0)
> +               return cdat_length;
> +
> +       sysfs_attr_init(&cxlmd->table_attr.attr.attr);
> +       /* Updates of CDAT are not yet handled so length is fixed. */
> +       cxlmd->table_attr.attr.size = cdat_length;
> +       cxlmd->table_attr.attr.read = cdat_table_show;
> +       cxlmd->table_attr.attr.attr.name = "CDAT";
> +       cxlmd->table_attr.attr.attr.mode = 0400;
> +       cxlmd->table_attr.table = devm_kzalloc(dev->parent, cdat_length, GFP_KERNEL);
> +

This all gets fixed up when switching to static DEVICE_ATTR_ADMIN_RO()
with an ->is_visible() implementation that hides the attribute if the
table is not found.