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

[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 1/2] PCI/doe: Initial support PCI Data Object Exchange

[Jonathan Cameron]

Introduced in an ECN to the PCI 5.0, DOE provides a config space
based mailbox with standard protocol discovery.  Each mailbox
is accessed through a DOE PCIE Extended Capability.

A device may have 1 or more DOE mailboxes, each of which is allowed
to support any number of protocols (some DOE protocols
specifications apply additional restrictions).  A given protocol
may be supported on more than one DOE mailbox on a given function.

The current infrastructure is fairly simplistic and pushes the burden
of handling this many-to-many relantionship to the drivers. In many
cases the arrangement will be static, making this straight forward.

Open questions:
* timeouts: The DOE specification allows for 1 second for some
  operations, but notes that specific protocols may have different
  requirements. Should we introduce the flexiblity now, or leave
  that to be implemented when support for such a protocol is added?
* DOE mailboxes may use MSI / MSIX to signal that the have prepared
  a response. These require normal conditions are setup by the driver.
  Should we move some of this into the DOE support (such as ensuring
  bus mastering is enabled)?

Testing conducted against QEMU using:

https://lore.kernel.org/qemu-devel/1612900760-7361-1-git-send-email-cbrowy@avery-design.com/
+ fix for interrupt flag mentioned in that thread.

Additional testing to be done, particularly around error handling.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 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 +++-
 5 files changed, 356 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
index 45a2ef702b45..f1cada7790fd 100644
--- a/drivers/pci/pcie/Kconfig
+++ b/drivers/pci/pcie/Kconfig
@@ -142,3 +142,11 @@ config PCIE_EDR
 	  the PCI Firmware Specification r3.2.  Enable this if you want to
 	  support hybrid DPC model which uses both firmware and OS to
 	  implement DPC.
+
+config PCIE_DOE
+       bool "PCI Express Data Object Exchange support"
+       help
+         This enables library support PCI Data Object Exchange capability.
+         DOE provides a simple mailbox in PCI express config space that is
+         used by a number of different protocols.
+         It is defined in he Data Object Exchnage ECN to PCI 5.0.
diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
index b2980db88cc0..801fdd5fbfc1 100644
--- a/drivers/pci/pcie/Makefile
+++ b/drivers/pci/pcie/Makefile
@@ -13,3 +13,4 @@ obj-$(CONFIG_PCIE_PME)		+= pme.o
 obj-$(CONFIG_PCIE_DPC)		+= dpc.o
 obj-$(CONFIG_PCIE_PTM)		+= ptm.o
 obj-$(CONFIG_PCIE_EDR)		+= edr.o
+obj-$(CONFIG_PCIE_DOE)		+= doe.o
diff --git a/drivers/pci/pcie/doe.c b/drivers/pci/pcie/doe.c
new file mode 100644
index 000000000000..b091ef379362
--- /dev/null
+++ b/drivers/pci/pcie/doe.c
@@ -0,0 +1,284 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Data Object Exchange was added to the PCI spec as an ECN to 5.0.
+ *
+ * Copyright (C) 2021 Huawei
+ *     Jonathan Cameron <Jonathan.Cameron@huawei.com>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/delay.h>
+#include <linux/jiffies.h>
+#include <linux/mutex.h>
+#include <linux/pci.h>
+#include <linux/pcie-doe.h>
+
+static irqreturn_t doe_irq(int irq, void *data)
+{
+	struct pcie_doe *doe = data;
+	struct pci_dev *pdev = doe->pdev;
+	u32 val;
+
+	pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_STATUS, &val);
+	if (FIELD_GET(PCI_DOE_STATUS_INT_STATUS, val)) {
+		pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_STATUS,
+				       val);
+		complete(&doe->c);
+		return IRQ_HANDLED;
+	}
+	/* Leave the error case to be handled outside irq */
+	if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) {
+		complete(&doe->c);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+static int pcie_doe_abort(struct pcie_doe *doe)
+{
+	struct pci_dev *pdev = doe->pdev;
+	int retry = 0;
+	u32 val;
+
+	pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_CTRL,
+			       PCI_DOE_CTRL_ABORT);
+	/* Abort is allowed to take up to 1 second */
+	do {
+		retry++;
+		pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_STATUS,
+				      &val);
+		if (FIELD_GET(PCI_DOE_STATUS_ERROR, val) &&
+		    !FIELD_GET(PCI_DOE_STATUS_BUSY, val))
+			return 0;
+		usleep_range(1000, 2000);
+	} while (retry < 1000);
+
+	return -EIO;
+}
+
+/**
+ * pcie_doe_init() - Initialise a Data Object Exchange mailbox
+ * @doe: state structure for the DOE mailbox
+ * @pdev: pci device which has this DOE mailbox
+ * @doe_offset: offset in configuration space of the DOE extended capability.
+ * @use_int: whether to use the optional interrupt
+ * Returns: 0 on success, <0 on error
+ *
+ * Caller responsible for calling pci_alloc_irq_vectors() including DOE
+ * interrupt.
+ */
+int pcie_doe_init(struct pcie_doe *doe, struct pci_dev *pdev, int doe_offset,
+		  bool use_int)
+{
+	u32 val;
+	int rc;
+
+	mutex_init(&doe->lock);
+	init_completion(&doe->c);
+	doe->cap_offset = doe_offset;
+	doe->pdev = pdev;
+	/* Reset the mailbox by issuing an abort */
+	rc = pcie_doe_abort(doe);
+	if (rc)
+		return rc;
+
+	pci_read_config_dword(pdev, doe_offset + PCI_DOE_CAP, &val);
+
+	if (use_int && FIELD_GET(PCI_DOE_CAP_INT, val)) {
+		rc = devm_request_irq(&pdev->dev,
+				      pci_irq_vector(pdev,
+						     FIELD_GET(PCI_DOE_CAP_IRQ, val)),
+				      doe_irq, 0, "DOE", doe);
+		if (rc)
+			return rc;
+
+		doe->use_int = use_int;
+		pci_write_config_dword(pdev, doe_offset + PCI_DOE_CTRL,
+				       FIELD_PREP(PCI_DOE_CTRL_INT_EN, 1));
+	}
+
+	return 0;
+}
+
+
+/**
+ * pcie_doe_exchange() - Send a request and receive a response
+ * @doe: DOE mailbox state structure
+ * @request: request data to be sent
+ * @request_sz: size of request in bytes
+ * @response: buffer into which to place the response
+ * @response_sz: size of available response buffer in bytes
+ *
+ * Return: 0 on success, < 0 on error
+ * Excess data will be discarded.
+ */
+int pcie_doe_exchange(struct pcie_doe *doe, u32 *request, size_t request_sz,
+		      u32 *response, size_t response_sz)
+{
+	struct pci_dev *pdev = doe->pdev;
+	int ret = 0;
+	int i;
+	u32 val;
+	int retry = -1;
+	size_t length;
+
+	/* DOE requests must be a whole number of DW */
+	if (request_sz % sizeof(u32))
+		return -EINVAL;
+
+	/* Need at least 2 DW to get the length */
+	if (response_sz < 2 * sizeof(u32))
+		return -EINVAL;
+
+	mutex_lock(&doe->lock);
+	/*
+	 * Check the DOE busy bit is not set.
+	 * If it is set, this could indicate someone other than Linux is
+	 * using the mailbox.
+	 */
+	pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_STATUS, &val);
+	if (FIELD_GET(PCI_DOE_STATUS_BUSY, val)) {
+		ret = -EBUSY;
+		goto unlock;
+	}
+
+	if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) {
+		ret = pcie_doe_abort(doe);
+		if (ret)
+			goto unlock;
+	}
+
+	for (i = 0; i < request_sz / 4; i++)
+		pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_WRITE,
+				       request[i]);
+
+	reinit_completion(&doe->c);
+	pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_CTRL,
+			       PCI_DOE_CTRL_GO);
+
+	if (doe->use_int) {
+		/*
+		 * Timeout of 1 second from 6.xx.1 ECN - Data Object Exchange
+		 * Note a protocol is allowed to specify a different timeout, so
+		 * that may need supporting in future.
+		 */
+		if (!wait_for_completion_timeout(&doe->c,
+						 msecs_to_jiffies(1000))) {
+			ret = -ETIMEDOUT;
+			goto unlock;
+		}
+
+		pci_read_config_dword(pdev,
+				      doe->cap_offset + PCI_DOE_STATUS,
+				      &val);
+		if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) {
+			pcie_doe_abort(doe);
+			ret = -EIO;
+			goto unlock;
+		}
+	} else {
+		do {
+			retry++;
+			pci_read_config_dword(pdev,
+					      doe->cap_offset + PCI_DOE_STATUS,
+					      &val);
+			if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) {
+				pcie_doe_abort(doe);
+				ret = -EIO;
+				goto unlock;
+			}
+
+			if (FIELD_GET(PCI_DOE_STATUS_DATA_OBJECT_READY, val))
+				break;
+			usleep_range(1000, 2000);
+		} while (retry < 1000);
+		if (!FIELD_GET(PCI_DOE_STATUS_DATA_OBJECT_READY, val)) {
+			ret = -ETIMEDOUT;
+			goto unlock;
+		}
+	}
+
+	/* Read the first two dwords to get the length */
+	pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_READ,
+			      &response[0]);
+	pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_READ, 0);
+	pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_READ,
+			      &response[1]);
+	pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_READ, 0);
+	length = FIELD_GET(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH,
+			   response[1]);
+	if (length > SZ_1M)
+		return -EIO;
+
+	for (i = 2; i < min(length, response_sz / 4); i++) {
+		pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_READ,
+				      &response[i]);
+		pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_READ, 0);
+	}
+	/* flush excess length */
+	for (; i < length; i++) {
+		pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_READ,
+				      &val);
+		pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_READ, 0);
+	}
+	/* Final error check to pick up on any since Data Object Ready */
+	pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_STATUS, &val);
+	if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) {
+		pcie_doe_abort(doe);
+		ret = -EIO;
+	}
+unlock:
+	mutex_unlock(&doe->lock);
+
+	return ret;
+}
+
+
+static int pcie_doe_discovery(struct pcie_doe *doe, u8 *index, u16 *vid, u8 *protocol)
+{
+	u32 request[3] = {
+		[0] = FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_VID, 0001) |
+		FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_TYPE, 0),
+		[1] = FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH, 3),
+		[2] = FIELD_PREP(PCI_DOE_DATA_OBJECT_DISC_REQ_3_INDEX, *index)
+	};
+	u32 response[3];
+	int ret;
+
+	ret = pcie_doe_exchange(doe, request, sizeof(request), response, sizeof(response));
+	if (ret)
+		return ret;
+
+	*vid = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_VID, response[2]);
+	*protocol = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_PROTOCOL, response[2]);
+	*index = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_NEXT_INDEX, response[2]);
+
+	return 0;
+}
+
+/**
+ * pcie_doe_protocol_check() - check if this DOE mailbox supports specific protocol
+ * @doe: DOE state structure
+ * @vid: Vendor ID
+ * @protocol: Protocol number as defined by Vendor
+ * Returns: 0 on success, <0 on error
+ */
+int pcie_doe_protocol_check(struct pcie_doe *doe, u16 vid, u8 protocol)
+{
+	u8 index = 0;
+
+	do {
+		u8 this_protocol;
+		u16 this_vid;
+		int ret;
+
+		ret = pcie_doe_discovery(doe, &index, &this_vid, &this_protocol);
+		if (ret)
+			return ret;
+		if (this_vid == vid && this_protocol == protocol)
+			return 0;
+	} while (index);
+
+	return -ENODEV;
+}
diff --git a/include/linux/pcie-doe.h b/include/linux/pcie-doe.h
new file mode 100644
index 000000000000..36eaa8532254
--- /dev/null
+++ b/include/linux/pcie-doe.h
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Data Object Exchange was added to the PCI spec as an ECN to 5.0.
+ *
+ * Copyright (C) 2021 Huawei
+ *     Jonathan Cameron <Jonathan.Cameron@huawei.com>
+ */
+
+#include <linux/completion.h>
+#include <linux/mutex.h>
+
+#ifndef LINUX_PCIE_DOE_H
+#define LINUX_PCIE_DOE_H
+/**
+ * struct pcie_doe - State to support use of DOE mailbox
+ * @lock: Ensure users of the mailbox are serialized
+ * @cap_offset: Config space offset to base of DOE capability.
+ * @pdev: PCI device that hosts this DOE.
+ * @c: Completion used for interrupt handling.
+ * @use_int: Flage to indicate if interrupts rather than polling used.
+ */
+struct pcie_doe {
+	struct mutex lock;
+	int cap_offset;
+	struct pci_dev *pdev;
+	struct completion c;
+	bool use_int;
+};
+
+int pcie_doe_init(struct pcie_doe *doe, struct pci_dev *dev, int doe_offset,
+		  bool use_int);
+int pcie_doe_exchange(struct pcie_doe *doe, u32 *request, size_t request_sz,
+		      u32 *response, size_t response_sz);
+int pcie_doe_protocol_check(struct pcie_doe *doe, u16 vid, u8 protocol);
+#endif
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index e709ae8235e7..4d8a5fee2cdf 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -730,7 +730,8 @@
 #define PCI_EXT_CAP_ID_DVSEC	0x23	/* Designated Vendor-Specific */
 #define PCI_EXT_CAP_ID_DLF	0x25	/* Data Link Feature */
 #define PCI_EXT_CAP_ID_PL_16GT	0x26	/* Physical Layer 16.0 GT/s */
-#define PCI_EXT_CAP_ID_MAX	PCI_EXT_CAP_ID_PL_16GT
+#define PCI_EXT_CAP_ID_DOE	0x2E	/* Data Object Exchange */
+#define PCI_EXT_CAP_ID_MAX	PCI_EXT_CAP_ID_DOE
 
 #define PCI_EXT_CAP_DSN_SIZEOF	12
 #define PCI_EXT_CAP_MCAST_ENDPOINT_SIZEOF 40
@@ -1092,4 +1093,30 @@
 #define  PCI_PL_16GT_LE_CTRL_USP_TX_PRESET_MASK		0x000000F0
 #define  PCI_PL_16GT_LE_CTRL_USP_TX_PRESET_SHIFT	4
 
+/* Data Object Exchange */
+#define PCI_DOE_CAP		0x04	/* DOE Capabilities Register */
+#define  PCI_DOE_CAP_INT			0x00000001  /* Interrupt Support */
+#define  PCI_DOE_CAP_IRQ			0x00000ffe  /* Interrupt Message Number */
+#define PCI_DOE_CTRL		0x08	/* DOE Control Register */
+#define  PCI_DOE_CTRL_ABORT			0x00000001  /* DOE Abort */
+#define  PCI_DOE_CTRL_INT_EN			0x00000002  /* DOE Interrupt Enable */
+#define  PCI_DOE_CTRL_GO			0x80000000  /* DOE Go */
+#define PCI_DOE_STATUS		0x0C	/* DOE Status Register */
+#define  PCI_DOE_STATUS_BUSY			0x00000001  /* DOE Busy */
+#define  PCI_DOE_STATUS_INT_STATUS		0x00000002  /* DOE Interrupt Status */
+#define  PCI_DOE_STATUS_ERROR			0x00000004  /* DOE Error */
+#define  PCI_DOE_STATUS_DATA_OBJECT_READY	0x80000000  /* Data Object Ready */
+#define PCI_DOE_WRITE		0x10	/* DOE Write Data Mailbox Register */
+#define PCI_DOE_READ		0x14	/* DOE Read Data Mailbox Register */
+
+/* DOE Data Object - note not actually registers */
+#define PCI_DOE_DATA_OBJECT_HEADER_1_VID	0x0000FFFF
+#define PCI_DOE_DATA_OBJECT_HEADER_1_TYPE	0x00FF0000
+#define PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH	0x0003FFFF
+
+#define PCI_DOE_DATA_OBJECT_DISC_REQ_3_INDEX	0x000000FF
+#define PCI_DOE_DATA_OBJECT_DISC_RSP_3_VID	0x0000FFFF
+#define PCI_DOE_DATA_OBJECT_DISC_RSP_3_PROTOCOL	0x00FF0000
+#define PCI_DOE_DATA_OBJECT_DISC_RSP_3_NEXT_INDEX 0xFF000000
+
 #endif /* LINUX_PCI_REGS_H */
-- 
2.19.1

Re: [RFC PATCH 1/2] PCI/doe: Initial support PCI Data Object Exchange

[Dan Williams]

Hey Jonathan, happy to see this, some comments below...

On Wed, Mar 10, 2021 at 10:08 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> Introduced in an ECN to the PCI 5.0, DOE provides a config space
> based mailbox with standard protocol discovery.  Each mailbox
> is accessed through a DOE PCIE Extended Capability.
>
> A device may have 1 or more DOE mailboxes, each of which is allowed
> to support any number of protocols (some DOE protocols
> specifications apply additional restrictions).  A given protocol
> may be supported on more than one DOE mailbox on a given function.

Are all those protocol instances shared? I'm trying to mental model
whether, for example, an auxiliary driver instance could be loaded per
DOE mailbox, or if there would need to be coordination of a given
protocol no matter how many DOE mailboxes on that device implemented
that protocol.

>
> The current infrastructure is fairly simplistic and pushes the burden
> of handling this many-to-many relantionship to the drivers. In many

s/relantionship/relationship/

> cases the arrangement will be static, making this straight forward.
>
> Open questions:
> * timeouts: The DOE specification allows for 1 second for some
>   operations, but notes that specific protocols may have different
>   requirements. Should we introduce the flexiblity now, or leave

s/flexiblity/flexibility/

>   that to be implemented when support for such a protocol is added?

If the timeout is property of the protocol then perhaps it should wait
and not be modeled at the transport level, but that's just an initial
reaction. I have not spent quality time with the DOE spec.

> * DOE mailboxes may use MSI / MSIX to signal that the have prepared
>   a response. These require normal conditions are setup by the driver.
>   Should we move some of this into the DOE support (such as ensuring
>   bus mastering is enabled)?

DOE support seems suitable to just be a library and leave the
host-device management to the host driver.

> Testing conducted against QEMU using:
>
> https://lore.kernel.org/qemu-devel/1612900760-7361-1-git-send-email-cbrowy@avery-design.com/
> + fix for interrupt flag mentioned in that thread.
>

I came across this the other day and made me wonder about SPDM
emulation as another test case:

https://cfp.osfc.io/media/osfc2020/submissions/ECQ88N/resources/An_open_source_SPDM_implementation_for_secure_devi_kmIgAQe.pdf


> Additional testing to be done, particularly around error handling.
>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  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 +++-
>  5 files changed, 356 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
> index 45a2ef702b45..f1cada7790fd 100644
> --- a/drivers/pci/pcie/Kconfig
> +++ b/drivers/pci/pcie/Kconfig
> @@ -142,3 +142,11 @@ config PCIE_EDR
>           the PCI Firmware Specification r3.2.  Enable this if you want to
>           support hybrid DPC model which uses both firmware and OS to
>           implement DPC.
> +
> +config PCIE_DOE
> +       bool "PCI Express Data Object Exchange support"

Make this tristate. It's a library that a driver can use and there's
nothing in the implementation that I can see that requires this
support to be built-in.

> +       help
> +         This enables library support PCI Data Object Exchange capability.

I'm not sure this option deserves help text to make it selectable by
the user. It should only be something that a driver selects. I.e.
unlike the other port services (DPC, PME, AER, etc...), nothing
happens by default if the user turns this on.

> +         DOE provides a simple mailbox in PCI express config space that is
> +         used by a number of different protocols.
> +         It is defined in he Data Object Exchnage ECN to PCI 5.0.

If the help text stays, or gets turned into a comment:

s/he Data Object Exchnage/the Data Object Exchange (DOE)/

> diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
> index b2980db88cc0..801fdd5fbfc1 100644
> --- a/drivers/pci/pcie/Makefile
> +++ b/drivers/pci/pcie/Makefile
> @@ -13,3 +13,4 @@ obj-$(CONFIG_PCIE_PME)                += pme.o
>  obj-$(CONFIG_PCIE_DPC)         += dpc.o
>  obj-$(CONFIG_PCIE_PTM)         += ptm.o
>  obj-$(CONFIG_PCIE_EDR)         += edr.o
> +obj-$(CONFIG_PCIE_DOE)         += doe.o
> diff --git a/drivers/pci/pcie/doe.c b/drivers/pci/pcie/doe.c
> new file mode 100644
> index 000000000000..b091ef379362
> --- /dev/null
> +++ b/drivers/pci/pcie/doe.c
> @@ -0,0 +1,284 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Data Object Exchange was added to the PCI spec as an ECN to 5.0.

Perhaps just put the ECN link here?

> + *
> + * Copyright (C) 2021 Huawei
> + *     Jonathan Cameron <Jonathan.Cameron@huawei.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/jiffies.h>
> +#include <linux/mutex.h>
> +#include <linux/pci.h>
> +#include <linux/pcie-doe.h>
> +
> +static irqreturn_t doe_irq(int irq, void *data)
> +{
> +       struct pcie_doe *doe = data;
> +       struct pci_dev *pdev = doe->pdev;
> +       u32 val;
> +
> +       pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_STATUS, &val);
> +       if (FIELD_GET(PCI_DOE_STATUS_INT_STATUS, val)) {
> +               pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_STATUS,
> +                                      val);
> +               complete(&doe->c);
> +               return IRQ_HANDLED;
> +       }
> +       /* Leave the error case to be handled outside irq */
> +       if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) {
> +               complete(&doe->c);
> +               return IRQ_HANDLED;
> +       }

Only one DOE command can be outstanding at a time per PCI device? This
seems insufficient in the multi-mailbox case / feels like there should
be a 'struct pcie_doe_request' object to track what it is to be
completed.

> +
> +       return IRQ_NONE;
> +}
> +
> +static int pcie_doe_abort(struct pcie_doe *doe)
> +{
> +       struct pci_dev *pdev = doe->pdev;
> +       int retry = 0;
> +       u32 val;
> +
> +       pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_CTRL,
> +                              PCI_DOE_CTRL_ABORT);
> +       /* Abort is allowed to take up to 1 second */
> +       do {
> +               retry++;
> +               pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_STATUS,
> +                                     &val);
> +               if (FIELD_GET(PCI_DOE_STATUS_ERROR, val) &&
> +                   !FIELD_GET(PCI_DOE_STATUS_BUSY, val))
> +                       return 0;
> +               usleep_range(1000, 2000);
> +       } while (retry < 1000);
> +
> +       return -EIO;

What's the state of the mailbox after an abort failure?

> +}
> +
> +/**
> + * pcie_doe_init() - Initialise a Data Object Exchange mailbox
> + * @doe: state structure for the DOE mailbox
> + * @pdev: pci device which has this DOE mailbox
> + * @doe_offset: offset in configuration space of the DOE extended capability.
> + * @use_int: whether to use the optional interrupt
> + * Returns: 0 on success, <0 on error
> + *
> + * Caller responsible for calling pci_alloc_irq_vectors() including DOE
> + * interrupt.
> + */
> +int pcie_doe_init(struct pcie_doe *doe, struct pci_dev *pdev, int doe_offset,
> +                 bool use_int)
> +{
> +       u32 val;
> +       int rc;
> +
> +       mutex_init(&doe->lock);
> +       init_completion(&doe->c);
> +       doe->cap_offset = doe_offset;
> +       doe->pdev = pdev;
> +       /* Reset the mailbox by issuing an abort */
> +       rc = pcie_doe_abort(doe);
> +       if (rc)
> +               return rc;
> +
> +       pci_read_config_dword(pdev, doe_offset + PCI_DOE_CAP, &val);
> +
> +       if (use_int && FIELD_GET(PCI_DOE_CAP_INT, val)) {
> +               rc = devm_request_irq(&pdev->dev,

Lets not hide devm semantics from the caller, so at a minimum this
function should be called pcim_pcie_doe_init() to indicate to the
caller that it has placed something into the devm stack. However, this
may not be convenient for the caller. I'd leave it to the user to call
a pcie_doe() unregister routine via devm_add_action_or_reset() if it
wants.

Lastly, I don't expect _init() routines to fail so perhaps split this
into pure "init" and "register" functionality?

> +                                     pci_irq_vector(pdev,
> +                                                    FIELD_GET(PCI_DOE_CAP_IRQ, val)),
> +                                     doe_irq, 0, "DOE", doe);
> +               if (rc)
> +                       return rc;
> +
> +               doe->use_int = use_int;
> +               pci_write_config_dword(pdev, doe_offset + PCI_DOE_CTRL,
> +                                      FIELD_PREP(PCI_DOE_CTRL_INT_EN, 1));
> +       }
> +
> +       return 0;
> +}
> +
> +
> +/**
> + * pcie_doe_exchange() - Send a request and receive a response
> + * @doe: DOE mailbox state structure
> + * @request: request data to be sent
> + * @request_sz: size of request in bytes
> + * @response: buffer into which to place the response
> + * @response_sz: size of available response buffer in bytes
> + *
> + * Return: 0 on success, < 0 on error
> + * Excess data will be discarded.
> + */
> +int pcie_doe_exchange(struct pcie_doe *doe, u32 *request, size_t request_sz,
> +                     u32 *response, size_t response_sz)

Are requests made against a specific protocol?

This interface feels under-decorated for a public API for host-drivers to use.

> +{
> +       struct pci_dev *pdev = doe->pdev;
> +       int ret = 0;
> +       int i;
> +       u32 val;
> +       int retry = -1;
> +       size_t length;
> +
> +       /* DOE requests must be a whole number of DW */
> +       if (request_sz % sizeof(u32))
> +               return -EINVAL;
> +
> +       /* Need at least 2 DW to get the length */
> +       if (response_sz < 2 * sizeof(u32))
> +               return -EINVAL;
> +
> +       mutex_lock(&doe->lock);
> +       /*
> +        * Check the DOE busy bit is not set.
> +        * If it is set, this could indicate someone other than Linux is
> +        * using the mailbox.
> +        */

Ugh, makes me think we need to extend the support for blocking pci
device MMIO while a driver is attached to config-space as well. How
can a communication protocol work if initiators can trample each
other's state?

> +       pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_STATUS, &val);
> +       if (FIELD_GET(PCI_DOE_STATUS_BUSY, val)) {
> +               ret = -EBUSY;
> +               goto unlock;
> +       }
> +
> +       if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) {
> +               ret = pcie_doe_abort(doe);
> +               if (ret)
> +                       goto unlock;
> +       }
> +
> +       for (i = 0; i < request_sz / 4; i++)
> +               pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_WRITE,
> +                                      request[i]);
> +
> +       reinit_completion(&doe->c);
> +       pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_CTRL,
> +                              PCI_DOE_CTRL_GO);
> +
> +       if (doe->use_int) {
> +               /*
> +                * Timeout of 1 second from 6.xx.1 ECN - Data Object Exchange
> +                * Note a protocol is allowed to specify a different timeout, so
> +                * that may need supporting in future.
> +                */
> +               if (!wait_for_completion_timeout(&doe->c,
> +                                                msecs_to_jiffies(1000))) {

s/msecs_to_jiffies(1000)/HZ/

> +                       ret = -ETIMEDOUT;
> +                       goto unlock;
> +               }
> +
> +               pci_read_config_dword(pdev,
> +                                     doe->cap_offset + PCI_DOE_STATUS,
> +                                     &val);
> +               if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) {
> +                       pcie_doe_abort(doe);
> +                       ret = -EIO;
> +                       goto unlock;
> +               }
> +       } else {
> +               do {
> +                       retry++;
> +                       pci_read_config_dword(pdev,
> +                                             doe->cap_offset + PCI_DOE_STATUS,
> +                                             &val);
> +                       if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) {
> +                               pcie_doe_abort(doe);
> +                               ret = -EIO;
> +                               goto unlock;
> +                       }
> +
> +                       if (FIELD_GET(PCI_DOE_STATUS_DATA_OBJECT_READY, val))
> +                               break;
> +                       usleep_range(1000, 2000);
> +               } while (retry < 1000);
> +               if (!FIELD_GET(PCI_DOE_STATUS_DATA_OBJECT_READY, val)) {
> +                       ret = -ETIMEDOUT;
> +                       goto unlock;

Rather than a lock and polling loop I'd organize this as a single
threaded delayed_workqueue that periodically services requests or
immediately runs the workqueue upon receipt of an interrupt. This
provides a software queuing model that can optionally be treated as
async / sync depending on the use case.


> +               }
> +       }
> +
> +       /* Read the first two dwords to get the length */
> +       pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_READ,
> +                             &response[0]);
> +       pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_READ, 0);
> +       pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_READ,
> +                             &response[1]);
> +       pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_READ, 0);
> +       length = FIELD_GET(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH,
> +                          response[1]);
> +       if (length > SZ_1M)
> +               return -EIO;
> +
> +       for (i = 2; i < min(length, response_sz / 4); i++) {
> +               pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_READ,
> +                                     &response[i]);
> +               pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_READ, 0);
> +       }
> +       /* flush excess length */
> +       for (; i < length; i++) {
> +               pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_READ,
> +                                     &val);
> +               pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_READ, 0);
> +       }
> +       /* Final error check to pick up on any since Data Object Ready */
> +       pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_STATUS, &val);
> +       if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) {
> +               pcie_doe_abort(doe);
> +               ret = -EIO;
> +       }
> +unlock:
> +       mutex_unlock(&doe->lock);
> +
> +       return ret;
> +}
> +
> +
> +static int pcie_doe_discovery(struct pcie_doe *doe, u8 *index, u16 *vid, u8 *protocol)
> +{
> +       u32 request[3] = {

Should this be a proper struct with named fields rather than an array?

> +               [0] = FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_VID, 0001) |
> +               FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_TYPE, 0),
> +               [1] = FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH, 3),
> +               [2] = FIELD_PREP(PCI_DOE_DATA_OBJECT_DISC_REQ_3_INDEX, *index)
> +       };
> +       u32 response[3];
> +       int ret;
> +
> +       ret = pcie_doe_exchange(doe, request, sizeof(request), response, sizeof(response));
> +       if (ret)
> +               return ret;
> +
> +       *vid = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_VID, response[2]);
> +       *protocol = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_PROTOCOL, response[2]);
> +       *index = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_NEXT_INDEX, response[2]);
> +
> +       return 0;
> +}
> +
> +/**
> + * pcie_doe_protocol_check() - check if this DOE mailbox supports specific protocol
> + * @doe: DOE state structure
> + * @vid: Vendor ID
> + * @protocol: Protocol number as defined by Vendor
> + * Returns: 0 on success, <0 on error
> + */
> +int pcie_doe_protocol_check(struct pcie_doe *doe, u16 vid, u8 protocol)

Not clear to me that this is a comfortable API for a driver. I would
expect that at registration time all the supported protocols would be
retrieved and cached in the 'struct pcie_doe' context and then the
host driver could query from there without going back to the device
again.

> +{
> +       u8 index = 0;
> +
> +       do {
> +               u8 this_protocol;
> +               u16 this_vid;
> +               int ret;
> +
> +               ret = pcie_doe_discovery(doe, &index, &this_vid, &this_protocol);
> +               if (ret)
> +                       return ret;
> +               if (this_vid == vid && this_protocol == protocol)
> +                       return 0;
> +       } while (index);
> +
> +       return -ENODEV;
> +}
> diff --git a/include/linux/pcie-doe.h b/include/linux/pcie-doe.h
> new file mode 100644
> index 000000000000..36eaa8532254
> --- /dev/null
> +++ b/include/linux/pcie-doe.h
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Data Object Exchange was added to the PCI spec as an ECN to 5.0.
> + *
> + * Copyright (C) 2021 Huawei
> + *     Jonathan Cameron <Jonathan.Cameron@huawei.com>
> + */
> +
> +#include <linux/completion.h>
> +#include <linux/mutex.h>
> +
> +#ifndef LINUX_PCIE_DOE_H
> +#define LINUX_PCIE_DOE_H
> +/**
> + * struct pcie_doe - State to support use of DOE mailbox
> + * @lock: Ensure users of the mailbox are serialized
> + * @cap_offset: Config space offset to base of DOE capability.
> + * @pdev: PCI device that hosts this DOE.
> + * @c: Completion used for interrupt handling.
> + * @use_int: Flage to indicate if interrupts rather than polling used.
> + */
> +struct pcie_doe {
> +       struct mutex lock;
> +       int cap_offset;

s/cap_offset/cap/

...to save some typing and be more idiomatic with other PCIE
capability based drivers.

> +       struct pci_dev *pdev;
> +       struct completion c;
> +       bool use_int;

Typically the polarity of this variable is flipped to whether polled
operation is enabled or not. I.e. s/use_int/poll/.

> +};
> +
> +int pcie_doe_init(struct pcie_doe *doe, struct pci_dev *dev, int doe_offset,
> +                 bool use_int);
> +int pcie_doe_exchange(struct pcie_doe *doe, u32 *request, size_t request_sz,
> +                     u32 *response, size_t response_sz);
> +int pcie_doe_protocol_check(struct pcie_doe *doe, u16 vid, u8 protocol);


> +#endif
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index e709ae8235e7..4d8a5fee2cdf 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -730,7 +730,8 @@
>  #define PCI_EXT_CAP_ID_DVSEC   0x23    /* Designated Vendor-Specific */
>  #define PCI_EXT_CAP_ID_DLF     0x25    /* Data Link Feature */
>  #define PCI_EXT_CAP_ID_PL_16GT 0x26    /* Physical Layer 16.0 GT/s */
> -#define PCI_EXT_CAP_ID_MAX     PCI_EXT_CAP_ID_PL_16GT
> +#define PCI_EXT_CAP_ID_DOE     0x2E    /* Data Object Exchange */
> +#define PCI_EXT_CAP_ID_MAX     PCI_EXT_CAP_ID_DOE
>
>  #define PCI_EXT_CAP_DSN_SIZEOF 12
>  #define PCI_EXT_CAP_MCAST_ENDPOINT_SIZEOF 40
> @@ -1092,4 +1093,30 @@
>  #define  PCI_PL_16GT_LE_CTRL_USP_TX_PRESET_MASK                0x000000F0
>  #define  PCI_PL_16GT_LE_CTRL_USP_TX_PRESET_SHIFT       4
>
> +/* Data Object Exchange */
> +#define PCI_DOE_CAP            0x04    /* DOE Capabilities Register */
> +#define  PCI_DOE_CAP_INT                       0x00000001  /* Interrupt Support */
> +#define  PCI_DOE_CAP_IRQ                       0x00000ffe  /* Interrupt Message Number */
> +#define PCI_DOE_CTRL           0x08    /* DOE Control Register */
> +#define  PCI_DOE_CTRL_ABORT                    0x00000001  /* DOE Abort */
> +#define  PCI_DOE_CTRL_INT_EN                   0x00000002  /* DOE Interrupt Enable */
> +#define  PCI_DOE_CTRL_GO                       0x80000000  /* DOE Go */
> +#define PCI_DOE_STATUS         0x0C    /* DOE Status Register */
> +#define  PCI_DOE_STATUS_BUSY                   0x00000001  /* DOE Busy */
> +#define  PCI_DOE_STATUS_INT_STATUS             0x00000002  /* DOE Interrupt Status */
> +#define  PCI_DOE_STATUS_ERROR                  0x00000004  /* DOE Error */
> +#define  PCI_DOE_STATUS_DATA_OBJECT_READY      0x80000000  /* Data Object Ready */
> +#define PCI_DOE_WRITE          0x10    /* DOE Write Data Mailbox Register */
> +#define PCI_DOE_READ           0x14    /* DOE Read Data Mailbox Register */
> +
> +/* DOE Data Object - note not actually registers */
> +#define PCI_DOE_DATA_OBJECT_HEADER_1_VID       0x0000FFFF
> +#define PCI_DOE_DATA_OBJECT_HEADER_1_TYPE      0x00FF0000
> +#define PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH    0x0003FFFF
> +
> +#define PCI_DOE_DATA_OBJECT_DISC_REQ_3_INDEX   0x000000FF
> +#define PCI_DOE_DATA_OBJECT_DISC_RSP_3_VID     0x0000FFFF
> +#define PCI_DOE_DATA_OBJECT_DISC_RSP_3_PROTOCOL        0x00FF0000
> +#define PCI_DOE_DATA_OBJECT_DISC_RSP_3_NEXT_INDEX 0xFF000000
> +
>  #endif /* LINUX_PCI_REGS_H */
> --
> 2.19.1
>

Re: [RFC PATCH 1/2] PCI/doe: Initial support PCI Data Object Exchange

[Jonathan Cameron]

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

> Hey Jonathan, happy to see this, some comments below...

Hi Dan,

Thanks for taking a look!

> 
> On Wed, Mar 10, 2021 at 10:08 AM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > Introduced in an ECN to the PCI 5.0, DOE provides a config space
> > based mailbox with standard protocol discovery.  Each mailbox
> > is accessed through a DOE PCIE Extended Capability.
> >
> > A device may have 1 or more DOE mailboxes, each of which is allowed
> > to support any number of protocols (some DOE protocols
> > specifications apply additional restrictions).  A given protocol
> > may be supported on more than one DOE mailbox on a given function.  
> 
> Are all those protocol instances shared?
> I'm trying to mental model
> whether, for example, an auxiliary driver instance could be loaded per
> DOE mailbox, or if there would need to be coordination of a given
> protocol no matter how many DOE mailboxes on that device implemented
> that protocol.

Just to check I've understood corectly, you mean multiple instances of same
protocol across different DOE mailboxes on a given device?

At DOE ECN level I don't think it is actually defined if they can
interact or not.  I've trawled though the released protocols that I know of
to see if there is a consensus but not finding much information.

I would argue however that there would be no reason to have the OS make
use of more than one DOE mailbox for the same protocol. Bit fiddly to
handle, but doesn't seem impossible to only register a protocol with first
DOE that supports it.

CMA does talk about use of multiple methods to communicate with the device
and the need for results consistency. However that is referring to out of
band vs DOE rather than multiple DOEs.  Plus it isn't making statements
about protocol coordination just responses to particular queries.

Things might get crazy if you tried to do IDE setup from two different DOE
mailboxes. The IDE ECN refers to "the specific instance of DOE used for..."
implying I think that there might be multiple but software should only
use one of them?

My other gut feeling is that only some of the DOE mailboxes are ever going
to be in the control of Linux. IDE calls out models where firmware or a TEE is
responsible for it for example. I'm not sure how that is going to be communicated
to the OS (can guess of course)

Sub drivers are a plausible model that I'll think about some more - but
for now it feels like too early to go that way..

> 
> >
> > The current infrastructure is fairly simplistic and pushes the burden
> > of handling this many-to-many relantionship to the drivers. In many  
> 
> s/relantionship/relationship/
> 
> > cases the arrangement will be static, making this straight forward.
> >
> > Open questions:
> > * timeouts: The DOE specification allows for 1 second for some
> >   operations, but notes that specific protocols may have different
> >   requirements. Should we introduce the flexiblity now, or leave  
> 
> s/flexiblity/flexibility/

Gah. One day I'll remember to spell check. Sorry about that.

> 
> >   that to be implemented when support for such a protocol is added?  
> 
> If the timeout is property of the protocol then perhaps it should wait
> and not be modeled at the transport level, but that's just an initial
> reaction. I have not spent quality time with the DOE spec.

I'm not sure it's possible to do so without breaking the abstraction of
DOE request / response into a bunch of messy sub steps.  Perhaps there is
a clean way of doing it but I can't immediately think of it.

If a protocol comes along that varies the timeout we can just add
a parameter to say what it is on a call by call basis.

> 
> > * DOE mailboxes may use MSI / MSIX to signal that the have prepared
> >   a response. These require normal conditions are setup by the driver.
> >   Should we move some of this into the DOE support (such as ensuring
> >   bus mastering is enabled)?  
> 
> DOE support seems suitable to just be a library and leave the
> host-device management to the host driver.

Agreed.  Though might be worth some debug checks.

Speaking from experience it's easy to spend half a day wondering why your
interrupts aren't turning up (I was blaming QEMU) because bus mastering
wasn't enabled.

> 
> > Testing conducted against QEMU using:
> >
> > https://lore.kernel.org/qemu-devel/1612900760-7361-1-git-send-email-cbrowy@avery-design.com/
> > + fix for interrupt flag mentioned in that thread.
> >  
> 
> I came across this the other day and made me wonder about SPDM
> emulation as another test case:
> 
> https://cfp.osfc.io/media/osfc2020/submissions/ECQ88N/resources/An_open_source_SPDM_implementation_for_secure_devi_kmIgAQe.pdf

Nice!  Looking at CMA / IDE emulation was on my todo list and that looks like
it might make that job a lot easier.

> 
> 
> > Additional testing to be done, particularly around error handling.
> >
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Anything not commented on should be in v2.

> > ---
> >  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 +++-
> >  5 files changed, 356 insertions(+), 1 deletion(-)
> 
> > diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
> > index b2980db88cc0..801fdd5fbfc1 100644
> > --- a/drivers/pci/pcie/Makefile
> > +++ b/drivers/pci/pcie/Makefile
> > @@ -13,3 +13,4 @@ obj-$(CONFIG_PCIE_PME)                += pme.o
> >  obj-$(CONFIG_PCIE_DPC)         += dpc.o
> >  obj-$(CONFIG_PCIE_PTM)         += ptm.o
> >  obj-$(CONFIG_PCIE_EDR)         += edr.o
> > +obj-$(CONFIG_PCIE_DOE)         += doe.o
> > diff --git a/drivers/pci/pcie/doe.c b/drivers/pci/pcie/doe.c
> > new file mode 100644
> > index 000000000000..b091ef379362
> > --- /dev/null
> > +++ b/drivers/pci/pcie/doe.c
> > @@ -0,0 +1,284 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Data Object Exchange was added to the PCI spec as an ECN to 5.0.  
> 
> Perhaps just put the ECN link here?

It's by number so I've left the title here as well as a link.

> 
> > + *
> > + * Copyright (C) 2021 Huawei
> > + *     Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/delay.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/mutex.h>
> > +#include <linux/pci.h>
> > +#include <linux/pcie-doe.h>
> > +
> > +static irqreturn_t doe_irq(int irq, void *data)
> > +{
> > +       struct pcie_doe *doe = data;
> > +       struct pci_dev *pdev = doe->pdev;
> > +       u32 val;
> > +
> > +       pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_STATUS, &val);
> > +       if (FIELD_GET(PCI_DOE_STATUS_INT_STATUS, val)) {
> > +               pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_STATUS,
> > +                                      val);
> > +               complete(&doe->c);
> > +               return IRQ_HANDLED;
> > +       }
> > +       /* Leave the error case to be handled outside irq */
> > +       if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) {
> > +               complete(&doe->c);
> > +               return IRQ_HANDLED;
> > +       }  
> 
> Only one DOE command can be outstanding at a time per PCI device? 

No, unless I'm missing something, that is one command per DOE mailbox at a time.
The completion is part of the pcie_doe structure, not the pci_dev.
That represents a single DOE mailbox.

There can be multiple commands in flight to multiple DOE mailboxes. Not clear
that there ever will be in real use cases however.

This comes up later wrt to async operation.  The mailbox only
supports one request / response cycle at a time, they cannot be overlapped.

> This
> seems insufficient in the multi-mailbox case / feels like there should
> be a 'struct pcie_doe_request' object to track what it is to be
> completed.

No need for the complexity with one request / response in flight per
mailbox at a time and each mailbox having separate state maintenance.

> 
> > +
> > +       return IRQ_NONE;
> > +}
> > +
> > +static int pcie_doe_abort(struct pcie_doe *doe)
> > +{
> > +       struct pci_dev *pdev = doe->pdev;
> > +       int retry = 0;
> > +       u32 val;
> > +
> > +       pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_CTRL,
> > +                              PCI_DOE_CTRL_ABORT);
> > +       /* Abort is allowed to take up to 1 second */
> > +       do {
> > +               retry++;
> > +               pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_STATUS,
> > +                                     &val);
> > +               if (FIELD_GET(PCI_DOE_STATUS_ERROR, val) &&
> > +                   !FIELD_GET(PCI_DOE_STATUS_BUSY, val))
> > +                       return 0;
> > +               usleep_range(1000, 2000);
> > +       } while (retry < 1000);
> > +
> > +       return -EIO;  
> 
> What's the state of the mailbox after an abort failure?

Good question.  I think the answer to that is dead device, reboot the machine
or at least the device if you can do a hard enough slot reset.

The specification goes with...
"It is strongly recommend that implementations ensure that the functionality
of the DOE Abort bit is resilient, including that DOE Abort functionality is
maintained even in cases where device firmware is malfunctioning"

So cross our fingers everyone obeys that strong recommendation or try to
work out what to do?

> 
> > +}
> > +
> > +/**
> > + * pcie_doe_init() - Initialise a Data Object Exchange mailbox
> > + * @doe: state structure for the DOE mailbox
> > + * @pdev: pci device which has this DOE mailbox
> > + * @doe_offset: offset in configuration space of the DOE extended capability.
> > + * @use_int: whether to use the optional interrupt
> > + * Returns: 0 on success, <0 on error
> > + *
> > + * Caller responsible for calling pci_alloc_irq_vectors() including DOE
> > + * interrupt.
> > + */
> > +int pcie_doe_init(struct pcie_doe *doe, struct pci_dev *pdev, int doe_offset,
> > +                 bool use_int)
> > +{
> > +       u32 val;
> > +       int rc;
> > +
> > +       mutex_init(&doe->lock);
> > +       init_completion(&doe->c);
> > +       doe->cap_offset = doe_offset;
> > +       doe->pdev = pdev;
> > +       /* Reset the mailbox by issuing an abort */
> > +       rc = pcie_doe_abort(doe);
> > +       if (rc)
> > +               return rc;
> > +
> > +       pci_read_config_dword(pdev, doe_offset + PCI_DOE_CAP, &val);
> > +
> > +       if (use_int && FIELD_GET(PCI_DOE_CAP_INT, val)) {
> > +               rc = devm_request_irq(&pdev->dev,  
> 
> Lets not hide devm semantics from the caller, so at a minimum this
> function should be called pcim_pcie_doe_init() to indicate to the
> caller that it has placed something into the devm stack. However, this
> may not be convenient for the caller. I'd leave it to the user to call
> a pcie_doe() unregister routine via devm_add_action_or_reset() if it
> wants.

> 
> Lastly, I don't expect _init() routines to fail so perhaps split this
> into pure "init" and "register" functionality?

I'm a bit doubtful on naming of register() but will go with that for v2.

It's not registering with anything so that feels a bit wrong as a description
for part 2 of setup.  Can leave that bike shedding for now though.

> 
> > +                                     pci_irq_vector(pdev,
> > +                                                    FIELD_GET(PCI_DOE_CAP_IRQ, val)),
> > +                                     doe_irq, 0, "DOE", doe);
> > +               if (rc)
> > +                       return rc;
> > +
> > +               doe->use_int = use_int;
> > +               pci_write_config_dword(pdev, doe_offset + PCI_DOE_CTRL,
> > +                                      FIELD_PREP(PCI_DOE_CTRL_INT_EN, 1));
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +
> > +/**
> > + * pcie_doe_exchange() - Send a request and receive a response
> > + * @doe: DOE mailbox state structure
> > + * @request: request data to be sent
> > + * @request_sz: size of request in bytes
> > + * @response: buffer into which to place the response
> > + * @response_sz: size of available response buffer in bytes
> > + *
> > + * Return: 0 on success, < 0 on error
> > + * Excess data will be discarded.
> > + */
> > +int pcie_doe_exchange(struct pcie_doe *doe, u32 *request, size_t request_sz,
> > +                     u32 *response, size_t response_sz)  
> 
> Are requests made against a specific protocol?

Yes, but the descriptive header is very brea.

> 
> This interface feels under-decorated for a public API for host-drivers to use.

I'll see what I can come up with for v2.
Likely to look something like

int pcie_doe_exchange(struct pci_doe *doe, u16 vid, u8 type,
		      u32 *request_pl, size_t request_pl_sz,
		      u32 *response_pl, size_t response_pl_sz)

and return received length or negative on error.

The disadvantage is that at least some of the specs just have the
header as their first few DW.  So there isn't a clear distinction
between header and payload. May lead to people getting offsets wrong
in a way they wouldn't do if driver was responsible for building the
whole message.

> 
> > +{
> > +       struct pci_dev *pdev = doe->pdev;
> > +       int ret = 0;
> > +       int i;
> > +       u32 val;
> > +       int retry = -1;
> > +       size_t length;
> > +
> > +       /* DOE requests must be a whole number of DW */
> > +       if (request_sz % sizeof(u32))
> > +               return -EINVAL;
> > +
> > +       /* Need at least 2 DW to get the length */
> > +       if (response_sz < 2 * sizeof(u32))
> > +               return -EINVAL;
> > +
> > +       mutex_lock(&doe->lock);
> > +       /*
> > +        * Check the DOE busy bit is not set.
> > +        * If it is set, this could indicate someone other than Linux is
> > +        * using the mailbox.
> > +        */  
> 
> Ugh, makes me think we need to extend the support for blocking pci
> device MMIO while a driver is attached to config-space as well. How
> can a communication protocol work if initiators can trample each
> other's state?

Agreed. It is crazy. At very least we need a means of saying
keep your hands off this DOE to the OS.

We can't do it on a per protocol basis, which was what I was previously
thinking, because we can't call the discovery protocol to see what
a given DOE is for.

> 
> > +       pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_STATUS, &val);
> > +       if (FIELD_GET(PCI_DOE_STATUS_BUSY, val)) {
> > +               ret = -EBUSY;
> > +               goto unlock;
> > +       }
> > +
> > +       if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) {
> > +               ret = pcie_doe_abort(doe);
> > +               if (ret)
> > +                       goto unlock;
> > +       }
> > +
> > +       for (i = 0; i < request_sz / 4; i++)
> > +               pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_WRITE,
> > +                                      request[i]);
> > +
> > +       reinit_completion(&doe->c);
> > +       pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_CTRL,
> > +                              PCI_DOE_CTRL_GO);
> > +
> > +       if (doe->use_int) {
> > +               /*
> > +                * Timeout of 1 second from 6.xx.1 ECN - Data Object Exchange
> > +                * Note a protocol is allowed to specify a different timeout, so
> > +                * that may need supporting in future.
> > +                */
> > +               if (!wait_for_completion_timeout(&doe->c,
> > +                                                msecs_to_jiffies(1000))) {  
> 
> s/msecs_to_jiffies(1000)/HZ/

huh. Missed that :)

> 
> > +                       ret = -ETIMEDOUT;
> > +                       goto unlock;
> > +               }
> > +
> > +               pci_read_config_dword(pdev,
> > +                                     doe->cap_offset + PCI_DOE_STATUS,
> > +                                     &val);
> > +               if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) {
> > +                       pcie_doe_abort(doe);
> > +                       ret = -EIO;
> > +                       goto unlock;
> > +               }
> > +       } else {
> > +               do {
> > +                       retry++;
> > +                       pci_read_config_dword(pdev,
> > +                                             doe->cap_offset + PCI_DOE_STATUS,
> > +                                             &val);
> > +                       if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) {
> > +                               pcie_doe_abort(doe);
> > +                               ret = -EIO;
> > +                               goto unlock;
> > +                       }
> > +
> > +                       if (FIELD_GET(PCI_DOE_STATUS_DATA_OBJECT_READY, val))
> > +                               break;
> > +                       usleep_range(1000, 2000);
> > +               } while (retry < 1000);
> > +               if (!FIELD_GET(PCI_DOE_STATUS_DATA_OBJECT_READY, val)) {
> > +                       ret = -ETIMEDOUT;
> > +                       goto unlock;  
> 
> Rather than a lock and polling loop I'd organize this as a single
> threaded delayed_workqueue that periodically services requests or
> immediately runs the workqueue upon receipt of an interrupt. This
> provides a software queuing model that can optionally be treated as
> async / sync depending on the use case.

Given it's single element in flight I don't think there is any benefit
to enabling async.  The lock has to be held throughout anyway.
It is always possible a particular caller wants to overlap this
transaction with some other actions, but I'd rather put the burden
on that clever caller which can spin this out to a thread of one type
or another.

We can revisit and split this in half if we have a user who benefits
from the complexity.

> 
> 
> > +               }
> > +       }
> > +
> > +       /* Read the first two dwords to get the length */
> > +       pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_READ,
> > +                             &response[0]);
> > +       pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_READ, 0);
> > +       pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_READ,
> > +                             &response[1]);
> > +       pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_READ, 0);
> > +       length = FIELD_GET(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH,
> > +                          response[1]);
> > +       if (length > SZ_1M)

oops. That's exiting with mutex held. Fixed in v2.

> > +               return -EIO;
> > +
> > +       for (i = 2; i < min(length, response_sz / 4); i++) {
> > +               pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_READ,
> > +                                     &response[i]);
> > +               pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_READ, 0);
> > +       }
> > +       /* flush excess length */
> > +       for (; i < length; i++) {
> > +               pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_READ,
> > +                                     &val);
> > +               pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_READ, 0);
> > +       }
> > +       /* Final error check to pick up on any since Data Object Ready */
> > +       pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_STATUS, &val);
> > +       if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) {
> > +               pcie_doe_abort(doe);
> > +               ret = -EIO;
> > +       }
> > +unlock:
> > +       mutex_unlock(&doe->lock);
> > +
> > +       return ret;
> > +}
> > +
> > +
> > +static int pcie_doe_discovery(struct pcie_doe *doe, u8 *index, u16 *vid, u8 *protocol)
> > +{
> > +       u32 request[3] = {  
> 
> Should this be a proper struct with named fields rather than an array?

Well the field names are going to end up as dw0 dw1 etc as there isn't a lot more
meaningful to call them.  We also want to keep them as u32 values throughout to
avoid fiddly packing manipulation on different endian machines.

This becomes rather simpler when it's just the payload due to changes in the
interface in v2.

> 
> > +               [0] = FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_VID, 0001) |
> > +               FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_TYPE, 0),
> > +               [1] = FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH, 3),
> > +               [2] = FIELD_PREP(PCI_DOE_DATA_OBJECT_DISC_REQ_3_INDEX, *index)
> > +       };
> > +       u32 response[3];
> > +       int ret;
> > +
> > +       ret = pcie_doe_exchange(doe, request, sizeof(request), response, sizeof(response));
> > +       if (ret)
> > +               return ret;
> > +
> > +       *vid = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_VID, response[2]);
> > +       *protocol = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_PROTOCOL, response[2]);
> > +       *index = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_NEXT_INDEX, response[2]);
> > +
> > +       return 0;
> > +}
> > +
> > +/**
> > + * pcie_doe_protocol_check() - check if this DOE mailbox supports specific protocol
> > + * @doe: DOE state structure
> > + * @vid: Vendor ID
> > + * @protocol: Protocol number as defined by Vendor
> > + * Returns: 0 on success, <0 on error
> > + */
> > +int pcie_doe_protocol_check(struct pcie_doe *doe, u16 vid, u8 protocol)  
> 
> Not clear to me that this is a comfortable API for a driver. I would
> expect that at registration time all the supported protocols would be
> retrieved and cached in the 'struct pcie_doe' context and then the
> host driver could query from there without going back to the device
> again.

I'm not sure I follow.

Any driver will fall into one of the following categories:
a) Already knows what protocols are available on a
   given DOE instance perhaps because that's a characteristic of the hardware
   supported, in which case it has no reason to check (unless driver writer
   is paranoid)
b) It has no way to know (e.g. class driver), then it makes sense to query
   the DOE instance to find out what protocols are available.

Absolutely we could cache them, but it wouldn't change the interface
presented to the driver. I think doing so at this stage is
premature optimization.

We could present this at a different level and wrap it up as a
find_doe that will return a DOE instance that supports the desired
protocol, but then that puts the burden of reference counting etc
for the different DOE instances on the core - the one thing I think
we want to avoid.

So far we have no evidence any device will actually need that.

Of the existing protocols, only a few are allowed to coexist with
each other and in well defined sets (CMA and IDE for example).

An alternative model we could look at (which is much more complex)
is to have something like the following: 

struct pcie_doe_set - Central location which is responsible for
all DOE mailboxes on a PCI device.

At init that scans all DOE mailboxes and builds a look up table
from [vid, protocol] to [struct pcie_doe]
Note this is 1 to 1, so if a protocol is supported on multiple
mailboxes we use the first one.

pcie_doe_exchange(struct pcie_doe_set, u16 vid, u8 protocol...)
Looks up the relevant DOE instance and does exchange on that.

So far I'm not convinced we should engage in this complexity.
Nothing stops us adding it if and when it becomes apparent we
actually need it.

An intermediate point would be to add basic list and reference
counting infrastructure so that a driver can call

struct pcie_doe *pcie_doe_get(struct pci_dev, u16 vid, u8 protocol)
void pci_doe_put(struct pci_doe *doe);

That means at least a list_head and possibly a lock being added
to pci_dev. Not sure how Bjorn will feel about that.

I might see how bad this looks for v2.

> 
> > +{
> > +       u8 index = 0;
> > +
> > +       do {
> > +               u8 this_protocol;
> > +               u16 this_vid;
> > +               int ret;
> > +
> > +               ret = pcie_doe_discovery(doe, &index, &this_vid, &this_protocol);
> > +               if (ret)
> > +                       return ret;
> > +               if (this_vid == vid && this_protocol == protocol)
> > +                       return 0;
> > +       } while (index);
> > +
> > +       return -ENODEV;
> > +}
> > diff --git a/include/linux/pcie-doe.h b/include/linux/pcie-doe.h
> > new file mode 100644
> > index 000000000000..36eaa8532254
> > --- /dev/null
> > +++ b/include/linux/pcie-doe.h
> > @@ -0,0 +1,35 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Data Object Exchange was added to the PCI spec as an ECN to 5.0.
> > + *
> > + * Copyright (C) 2021 Huawei
> > + *     Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > + */
> > +
> > +#include <linux/completion.h>
> > +#include <linux/mutex.h>
> > +
> > +#ifndef LINUX_PCIE_DOE_H
> > +#define LINUX_PCIE_DOE_H
> > +/**
> > + * struct pcie_doe - State to support use of DOE mailbox
> > + * @lock: Ensure users of the mailbox are serialized
> > + * @cap_offset: Config space offset to base of DOE capability.
> > + * @pdev: PCI device that hosts this DOE.
> > + * @c: Completion used for interrupt handling.
> > + * @use_int: Flage to indicate if interrupts rather than polling used.
> > + */
> > +struct pcie_doe {
> > +       struct mutex lock;
> > +       int cap_offset;  
> 
> s/cap_offset/cap/
> 
> ...to save some typing and be more idiomatic with other PCIE
> capability based drivers.
> 
> > +       struct pci_dev *pdev;
> > +       struct completion c;
> > +       bool use_int;  
> 
> Typically the polarity of this variable is flipped to whether polled
> operation is enabled or not. I.e. s/use_int/poll/.

This went away in v2 as we now store the irq to allow it to be removed
in _unregister.

I did change to poll as a parameter to relevant functions

> 
> > +};
> > +
> > +int pcie_doe_init(struct pcie_doe *doe, struct pci_dev *dev, int doe_offset,
> > +                 bool use_int);
> > +int pcie_doe_exchange(struct pcie_doe *doe, u32 *request, size_t request_sz,
> > +                     u32 *response, size_t response_sz);
> > +int pcie_doe_protocol_check(struct pcie_doe *doe, u16 vid, u8 protocol);  
> 
> 
> > +#endif

I'll work up a v2 with the above changes and have a mess with list based
handling and reference counting for the DOE instances.

Jonathan

Re: [RFC PATCH 1/2] PCI/doe: Initial support PCI Data Object Exchange

[Jonathan Cameron]

On Tue, 16 Mar 2021 16:29:52 +0000
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:


> >   
> > > +               [0] = FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_VID, 0001) |
> > > +               FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_TYPE, 0),
> > > +               [1] = FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH, 3),
> > > +               [2] = FIELD_PREP(PCI_DOE_DATA_OBJECT_DISC_REQ_3_INDEX, *index)
> > > +       };
> > > +       u32 response[3];
> > > +       int ret;
> > > +
> > > +       ret = pcie_doe_exchange(doe, request, sizeof(request), response, sizeof(response));
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       *vid = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_VID, response[2]);
> > > +       *protocol = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_PROTOCOL, response[2]);
> > > +       *index = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_NEXT_INDEX, response[2]);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +/**
> > > + * pcie_doe_protocol_check() - check if this DOE mailbox supports specific protocol
> > > + * @doe: DOE state structure
> > > + * @vid: Vendor ID
> > > + * @protocol: Protocol number as defined by Vendor
> > > + * Returns: 0 on success, <0 on error
> > > + */
> > > +int pcie_doe_protocol_check(struct pcie_doe *doe, u16 vid, u8 protocol)    
> > 
> > Not clear to me that this is a comfortable API for a driver. I would
> > expect that at registration time all the supported protocols would be
> > retrieved and cached in the 'struct pcie_doe' context and then the
> > host driver could query from there without going back to the device
> > again.  
> 
> I'm not sure I follow.
> 
> Any driver will fall into one of the following categories:
> a) Already knows what protocols are available on a
>    given DOE instance perhaps because that's a characteristic of the hardware
>    supported, in which case it has no reason to check (unless driver writer
>    is paranoid)
> b) It has no way to know (e.g. class driver), then it makes sense to query
>    the DOE instance to find out what protocols are available.
> 
> Absolutely we could cache them, but it wouldn't change the interface
> presented to the driver. I think doing so at this stage is
> premature optimization.
> 
> We could present this at a different level and wrap it up as a
> find_doe that will return a DOE instance that supports the desired
> protocol, but then that puts the burden of reference counting etc
> for the different DOE instances on the core - the one thing I think
> we want to avoid.
> 
> So far we have no evidence any device will actually need that.
> 
> Of the existing protocols, only a few are allowed to coexist with
> each other and in well defined sets (CMA and IDE for example).
> 
> An alternative model we could look at (which is much more complex)
> is to have something like the following: 
> 
> struct pcie_doe_set - Central location which is responsible for
> all DOE mailboxes on a PCI device.
> 
> At init that scans all DOE mailboxes and builds a look up table
> from [vid, protocol] to [struct pcie_doe]
> Note this is 1 to 1, so if a protocol is supported on multiple
> mailboxes we use the first one.
> 
> pcie_doe_exchange(struct pcie_doe_set, u16 vid, u8 protocol...)
> Looks up the relevant DOE instance and does exchange on that.
> 
> So far I'm not convinced we should engage in this complexity.
> Nothing stops us adding it if and when it becomes apparent we
> actually need it.
> 
> An intermediate point would be to add basic list and reference
> counting infrastructure so that a driver can call
> 
> struct pcie_doe *pcie_doe_get(struct pci_dev, u16 vid, u8 protocol)
> void pci_doe_put(struct pci_doe *doe);
> 
> That means at least a list_head and possibly a lock being added
> to pci_dev. Not sure how Bjorn will feel about that.
> 
> I might see how bad this looks for v2.

Lifetime element of the DOE could be avoided by simply having

pcie_doe_register_all()
and
pcie_doe_unregister_all()

and so managing all DOE instances in one unit.

I'm not sure I like it, but certainly makes things simple.
After pcie_doe_register_all() call, all DOEs are ready to use
and we can have simple pcie_doe_find() to get one with
appropriate protocols.  There is never any need to specifically
release it because they are all cleaned up together in remove
/release path.

I'll put this together for a v2 and we can see how it shapes
up.

Jonathan

Re: [RFC PATCH 1/2] PCI/doe: Initial support PCI Data Object Exchange

[Dan Williams]

On Tue, Mar 16, 2021 at 9:31 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Mon, 15 Mar 2021 12:45:49 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > Hey Jonathan, happy to see this, some comments below...
>
> Hi Dan,
>
> Thanks for taking a look!
>
> >
> > On Wed, Mar 10, 2021 at 10:08 AM Jonathan Cameron
> > <Jonathan.Cameron@huawei.com> wrote:
> > >
> > > Introduced in an ECN to the PCI 5.0, DOE provides a config space
> > > based mailbox with standard protocol discovery.  Each mailbox
> > > is accessed through a DOE PCIE Extended Capability.
> > >
> > > A device may have 1 or more DOE mailboxes, each of which is allowed
> > > to support any number of protocols (some DOE protocols
> > > specifications apply additional restrictions).  A given protocol
> > > may be supported on more than one DOE mailbox on a given function.
> >
> > Are all those protocol instances shared?
> > I'm trying to mental model
> > whether, for example, an auxiliary driver instance could be loaded per
> > DOE mailbox, or if there would need to be coordination of a given
> > protocol no matter how many DOE mailboxes on that device implemented
> > that protocol.
>
> Just to check I've understood corectly, you mean multiple instances of same
> protocol across different DOE mailboxes on a given device?
>

Right.

> At DOE ECN level I don't think it is actually defined if they can
> interact or not.  I've trawled though the released protocols that I know of
> to see if there is a consensus but not finding much information.
>
> I would argue however that there would be no reason to have the OS make
> use of more than one DOE mailbox for the same protocol. Bit fiddly to
> handle, but doesn't seem impossible to only register a protocol with first
> DOE that supports it.
>
> CMA does talk about use of multiple methods to communicate with the device
> and the need for results consistency. However that is referring to out of
> band vs DOE rather than multiple DOEs.  Plus it isn't making statements
> about protocol coordination just responses to particular queries.
>
> Things might get crazy if you tried to do IDE setup from two different DOE
> mailboxes. The IDE ECN refers to "the specific instance of DOE used for..."
> implying I think that there might be multiple but software should only
> use one of them?
>
> My other gut feeling is that only some of the DOE mailboxes are ever going
> to be in the control of Linux. IDE calls out models where firmware or a TEE is
> responsible for it for example. I'm not sure how that is going to be communicated
> to the OS (can guess of course)
>
> Sub drivers are a plausible model that I'll think about some more - but
> for now it feels like too early to go that way..

Ok, fair enough.

>
> >
> > >
> > > The current infrastructure is fairly simplistic and pushes the burden
> > > of handling this many-to-many relantionship to the drivers. In many
> >
> > s/relantionship/relationship/
> >
> > > cases the arrangement will be static, making this straight forward.
> > >
> > > Open questions:
> > > * timeouts: The DOE specification allows for 1 second for some
> > >   operations, but notes that specific protocols may have different
> > >   requirements. Should we introduce the flexiblity now, or leave
> >
> > s/flexiblity/flexibility/
>
> Gah. One day I'll remember to spell check. Sorry about that.
>
> >
> > >   that to be implemented when support for such a protocol is added?
> >
> > If the timeout is property of the protocol then perhaps it should wait
> > and not be modeled at the transport level, but that's just an initial
> > reaction. I have not spent quality time with the DOE spec.
>
> I'm not sure it's possible to do so without breaking the abstraction of
> DOE request / response into a bunch of messy sub steps.  Perhaps there is
> a clean way of doing it but I can't immediately think of it.
>
> If a protocol comes along that varies the timeout we can just add
> a parameter to say what it is on a call by call basis.

Now that I've had a chance to take a look the spec seems to
unequivocally mandate the timeouts in "6.xx.1 Operation", where was
the per-protocol timeout implied?

> > > * DOE mailboxes may use MSI / MSIX to signal that the have prepared
> > >   a response. These require normal conditions are setup by the driver.
> > >   Should we move some of this into the DOE support (such as ensuring
> > >   bus mastering is enabled)?
> >
> > DOE support seems suitable to just be a library and leave the
> > host-device management to the host driver.
>
> Agreed.  Though might be worth some debug checks.
>
> Speaking from experience it's easy to spend half a day wondering why your
> interrupts aren't turning up (I was blaming QEMU) because bus mastering
> wasn't enabled.

Sure, no concern about validating assumptions in the library, but
leave control to the host.

> > > Testing conducted against QEMU using:
> > >
> > > https://lore.kernel.org/qemu-devel/1612900760-7361-1-git-send-email-cbrowy@avery-design.com/
> > > + fix for interrupt flag mentioned in that thread.
> > >
> >
> > I came across this the other day and made me wonder about SPDM
> > emulation as another test case:
> >
> > https://cfp.osfc.io/media/osfc2020/submissions/ECQ88N/resources/An_open_source_SPDM_implementation_for_secure_devi_kmIgAQe.pdf
>
> Nice!  Looking at CMA / IDE emulation was on my todo list and that looks like
> it might make that job a lot easier.
>
> >
> >
> > > Additional testing to be done, particularly around error handling.
> > >
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> Anything not commented on should be in v2.
>
> > > ---
> > >  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 +++-
> > >  5 files changed, 356 insertions(+), 1 deletion(-)
> >
> > > diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
> > > index b2980db88cc0..801fdd5fbfc1 100644
> > > --- a/drivers/pci/pcie/Makefile
> > > +++ b/drivers/pci/pcie/Makefile
> > > @@ -13,3 +13,4 @@ obj-$(CONFIG_PCIE_PME)                += pme.o
> > >  obj-$(CONFIG_PCIE_DPC)         += dpc.o
> > >  obj-$(CONFIG_PCIE_PTM)         += ptm.o
> > >  obj-$(CONFIG_PCIE_EDR)         += edr.o
> > > +obj-$(CONFIG_PCIE_DOE)         += doe.o
> > > diff --git a/drivers/pci/pcie/doe.c b/drivers/pci/pcie/doe.c
> > > new file mode 100644
> > > index 000000000000..b091ef379362
> > > --- /dev/null
> > > +++ b/drivers/pci/pcie/doe.c
> > > @@ -0,0 +1,284 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Data Object Exchange was added to the PCI spec as an ECN to 5.0.
> >
> > Perhaps just put the ECN link here?
>
> It's by number so I've left the title here as well as a link.

Ok.

>
> >
> > > + *
> > > + * Copyright (C) 2021 Huawei
> > > + *     Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > + */
> > > +
> > > +#include <linux/bitfield.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/jiffies.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/pci.h>
> > > +#include <linux/pcie-doe.h>
> > > +
> > > +static irqreturn_t doe_irq(int irq, void *data)
> > > +{
> > > +       struct pcie_doe *doe = data;
> > > +       struct pci_dev *pdev = doe->pdev;
> > > +       u32 val;
> > > +
> > > +       pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_STATUS, &val);
> > > +       if (FIELD_GET(PCI_DOE_STATUS_INT_STATUS, val)) {
> > > +               pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_STATUS,
> > > +                                      val);
> > > +               complete(&doe->c);
> > > +               return IRQ_HANDLED;
> > > +       }
> > > +       /* Leave the error case to be handled outside irq */
> > > +       if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) {
> > > +               complete(&doe->c);
> > > +               return IRQ_HANDLED;
> > > +       }
> >
> > Only one DOE command can be outstanding at a time per PCI device?
>
> No, unless I'm missing something, that is one command per DOE mailbox at a time.
> The completion is part of the pcie_doe structure, not the pci_dev.
> That represents a single DOE mailbox.
>
> There can be multiple commands in flight to multiple DOE mailboxes. Not clear
> that there ever will be in real use cases however.
>
> This comes up later wrt to async operation.  The mailbox only
> supports one request / response cycle at a time, they cannot be overlapped.

"6.xx.1 Operation" says "If a single DOE instance supports multiple
data object protocols, system firmware/software is permitted to
interleave requests/responses with different data object protocols."

...although I must say I don't understand how system software tracks
which response belongs to which request if the transactions are
interleaved.

> > This
> > seems insufficient in the multi-mailbox case / feels like there should
> > be a 'struct pcie_doe_request' object to track what it is to be
> > completed.
>
> No need for the complexity with one request / response in flight per
> mailbox at a time and each mailbox having separate state maintenance.

I think the workqueue proposal removes the need for pcie_doe_request,
but still allows for the possibility of interleaving requests.

>
> >
> > > +
> > > +       return IRQ_NONE;
> > > +}
> > > +
> > > +static int pcie_doe_abort(struct pcie_doe *doe)
> > > +{
> > > +       struct pci_dev *pdev = doe->pdev;
> > > +       int retry = 0;
> > > +       u32 val;
> > > +
> > > +       pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_CTRL,
> > > +                              PCI_DOE_CTRL_ABORT);
> > > +       /* Abort is allowed to take up to 1 second */
> > > +       do {
> > > +               retry++;
> > > +               pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_STATUS,
> > > +                                     &val);
> > > +               if (FIELD_GET(PCI_DOE_STATUS_ERROR, val) &&
> > > +                   !FIELD_GET(PCI_DOE_STATUS_BUSY, val))
> > > +                       return 0;
> > > +               usleep_range(1000, 2000);
> > > +       } while (retry < 1000);
> > > +
> > > +       return -EIO;
> >
> > What's the state of the mailbox after an abort failure?
>
> Good question.  I think the answer to that is dead device, reboot the machine
> or at least the device if you can do a hard enough slot reset.

...and hopefully that device is not part of an active interleave
otherwise a reset can take down "System RAM".

>
> The specification goes with...
> "It is strongly recommend that implementations ensure that the functionality
> of the DOE Abort bit is resilient, including that DOE Abort functionality is
> maintained even in cases where device firmware is malfunctioning"

Ok.

>
> So cross our fingers everyone obeys that strong recommendation or try to
> work out what to do?

What's the worst that can happen? </famous last words>

>
> >
> > > +}
> > > +
> > > +/**
> > > + * pcie_doe_init() - Initialise a Data Object Exchange mailbox
> > > + * @doe: state structure for the DOE mailbox
> > > + * @pdev: pci device which has this DOE mailbox
> > > + * @doe_offset: offset in configuration space of the DOE extended capability.
> > > + * @use_int: whether to use the optional interrupt
> > > + * Returns: 0 on success, <0 on error
> > > + *
> > > + * Caller responsible for calling pci_alloc_irq_vectors() including DOE
> > > + * interrupt.
> > > + */
> > > +int pcie_doe_init(struct pcie_doe *doe, struct pci_dev *pdev, int doe_offset,
> > > +                 bool use_int)
> > > +{
> > > +       u32 val;
> > > +       int rc;
> > > +
> > > +       mutex_init(&doe->lock);
> > > +       init_completion(&doe->c);
> > > +       doe->cap_offset = doe_offset;
> > > +       doe->pdev = pdev;
> > > +       /* Reset the mailbox by issuing an abort */
> > > +       rc = pcie_doe_abort(doe);
> > > +       if (rc)
> > > +               return rc;
> > > +
> > > +       pci_read_config_dword(pdev, doe_offset + PCI_DOE_CAP, &val);
> > > +
> > > +       if (use_int && FIELD_GET(PCI_DOE_CAP_INT, val)) {
> > > +               rc = devm_request_irq(&pdev->dev,
> >
> > Lets not hide devm semantics from the caller, so at a minimum this
> > function should be called pcim_pcie_doe_init() to indicate to the
> > caller that it has placed something into the devm stack. However, this
> > may not be convenient for the caller. I'd leave it to the user to call
> > a pcie_doe() unregister routine via devm_add_action_or_reset() if it
> > wants.
>
> >
> > Lastly, I don't expect _init() routines to fail so perhaps split this
> > into pure "init" and "register" functionality?
>
> I'm a bit doubtful on naming of register() but will go with that for v2.
>
> It's not registering with anything so that feels a bit wrong as a description
> for part 2 of setup.  Can leave that bike shedding for now though.
>

Ok, just searching for a name that implies symmetrical teardown
register/unregister, enable/disable, ... etc. init/deinit doesn't do
it for me.

> >
> > > +                                     pci_irq_vector(pdev,
> > > +                                                    FIELD_GET(PCI_DOE_CAP_IRQ, val)),
> > > +                                     doe_irq, 0, "DOE", doe);
> > > +               if (rc)
> > > +                       return rc;
> > > +
> > > +               doe->use_int = use_int;
> > > +               pci_write_config_dword(pdev, doe_offset + PCI_DOE_CTRL,
> > > +                                      FIELD_PREP(PCI_DOE_CTRL_INT_EN, 1));
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +
> > > +/**
> > > + * pcie_doe_exchange() - Send a request and receive a response
> > > + * @doe: DOE mailbox state structure
> > > + * @request: request data to be sent
> > > + * @request_sz: size of request in bytes
> > > + * @response: buffer into which to place the response
> > > + * @response_sz: size of available response buffer in bytes
> > > + *
> > > + * Return: 0 on success, < 0 on error
> > > + * Excess data will be discarded.
> > > + */
> > > +int pcie_doe_exchange(struct pcie_doe *doe, u32 *request, size_t request_sz,
> > > +                     u32 *response, size_t response_sz)
> >
> > Are requests made against a specific protocol?
>
> Yes, but the descriptive header is very brea.
>
> >
> > This interface feels under-decorated for a public API for host-drivers to use.
>
> I'll see what I can come up with for v2.
> Likely to look something like
>
> int pcie_doe_exchange(struct pci_doe *doe, u16 vid, u8 type,
>                       u32 *request_pl, size_t request_pl_sz,
>                       u32 *response_pl, size_t response_pl_sz)

I was thinking something like 'struct pcie_doe_object' pointers rather
than u32 arrays.

>
> and return received length or negative on error.
>
> The disadvantage is that at least some of the specs just have the
> header as their first few DW.  So there isn't a clear distinction
> between header and payload. May lead to people getting offsets wrong
> in a way they wouldn't do if driver was responsible for building the
> whole message.

Aren't they more likely to get offsets wrong with u32 arrays rather
than data structures?

>
> >
> > > +{
> > > +       struct pci_dev *pdev = doe->pdev;
> > > +       int ret = 0;
> > > +       int i;
> > > +       u32 val;
> > > +       int retry = -1;
> > > +       size_t length;
> > > +
> > > +       /* DOE requests must be a whole number of DW */
> > > +       if (request_sz % sizeof(u32))
> > > +               return -EINVAL;
> > > +
> > > +       /* Need at least 2 DW to get the length */
> > > +       if (response_sz < 2 * sizeof(u32))
> > > +               return -EINVAL;
> > > +
> > > +       mutex_lock(&doe->lock);
> > > +       /*
> > > +        * Check the DOE busy bit is not set.
> > > +        * If it is set, this could indicate someone other than Linux is
> > > +        * using the mailbox.
> > > +        */
> >
> > Ugh, makes me think we need to extend the support for blocking pci
> > device MMIO while a driver is attached to config-space as well. How
> > can a communication protocol work if initiators can trample each
> > other's state?
>
> Agreed. It is crazy. At very least we need a means of saying
> keep your hands off this DOE to the OS.
>
> We can't do it on a per protocol basis, which was what I was previously
> thinking, because we can't call the discovery protocol to see what
> a given DOE is for.

I'm specifically thinking of a mechanism that blocks pci-sysfs from
initiating config-cycles if a driver has claimed that range.

However, these MCTP to DOE tunnels that the SPDM presentation alluded
to make me nervous as there is no protocol to prevent an OS driver
agent and an MCTP agent from clobbering each other.

>
> >
> > > +       pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_STATUS, &val);
> > > +       if (FIELD_GET(PCI_DOE_STATUS_BUSY, val)) {
> > > +               ret = -EBUSY;
> > > +               goto unlock;
> > > +       }
> > > +
> > > +       if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) {
> > > +               ret = pcie_doe_abort(doe);
> > > +               if (ret)
> > > +                       goto unlock;
> > > +       }
> > > +
> > > +       for (i = 0; i < request_sz / 4; i++)
> > > +               pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_WRITE,
> > > +                                      request[i]);
> > > +
> > > +       reinit_completion(&doe->c);
> > > +       pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_CTRL,
> > > +                              PCI_DOE_CTRL_GO);
> > > +
> > > +       if (doe->use_int) {
> > > +               /*
> > > +                * Timeout of 1 second from 6.xx.1 ECN - Data Object Exchange
> > > +                * Note a protocol is allowed to specify a different timeout, so
> > > +                * that may need supporting in future.
> > > +                */
> > > +               if (!wait_for_completion_timeout(&doe->c,
> > > +                                                msecs_to_jiffies(1000))) {
> >
> > s/msecs_to_jiffies(1000)/HZ/
>
> huh. Missed that :)

Yeah, the shorthand that X*HZ == "X seconds worth of jiffies" is just
something I picked up from other drivers not explicit documentation.

>
> >
> > > +                       ret = -ETIMEDOUT;
> > > +                       goto unlock;
> > > +               }
> > > +
> > > +               pci_read_config_dword(pdev,
> > > +                                     doe->cap_offset + PCI_DOE_STATUS,
> > > +                                     &val);
> > > +               if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) {
> > > +                       pcie_doe_abort(doe);
> > > +                       ret = -EIO;
> > > +                       goto unlock;
> > > +               }
> > > +       } else {
> > > +               do {
> > > +                       retry++;
> > > +                       pci_read_config_dword(pdev,
> > > +                                             doe->cap_offset + PCI_DOE_STATUS,
> > > +                                             &val);
> > > +                       if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) {
> > > +                               pcie_doe_abort(doe);
> > > +                               ret = -EIO;
> > > +                               goto unlock;
> > > +                       }
> > > +
> > > +                       if (FIELD_GET(PCI_DOE_STATUS_DATA_OBJECT_READY, val))
> > > +                               break;
> > > +                       usleep_range(1000, 2000);
> > > +               } while (retry < 1000);
> > > +               if (!FIELD_GET(PCI_DOE_STATUS_DATA_OBJECT_READY, val)) {
> > > +                       ret = -ETIMEDOUT;
> > > +                       goto unlock;
> >
> > Rather than a lock and polling loop I'd organize this as a single
> > threaded delayed_workqueue that periodically services requests or
> > immediately runs the workqueue upon receipt of an interrupt. This
> > provides a software queuing model that can optionally be treated as
> > async / sync depending on the use case.
>
> Given it's single element in flight I don't think there is any benefit
> to enabling async.  The lock has to be held throughout anyway.
> It is always possible a particular caller wants to overlap this
> transaction with some other actions, but I'd rather put the burden
> on that clever caller which can spin this out to a thread of one type
> or another.
>
> We can revisit and split this in half if we have a user who benefits
> from the complexity.

I don't think it's complex. I think it's simpler to rationalize than
this pattern of taking a lock and going to sleep with the lock held.
You can eliminate the lock completely if the only access to a given
DOE is a single dedicated kthread. There are other examples of this
single-thread protocol handler pattern in the kernel, like libsas SMP
protocol.

> > > +               }
> > > +       }
> > > +
> > > +       /* Read the first two dwords to get the length */
> > > +       pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_READ,
> > > +                             &response[0]);
> > > +       pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_READ, 0);
> > > +       pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_READ,
> > > +                             &response[1]);
> > > +       pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_READ, 0);
> > > +       length = FIELD_GET(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH,
> > > +                          response[1]);
> > > +       if (length > SZ_1M)
>
> oops. That's exiting with mutex held. Fixed in v2.
>
> > > +               return -EIO;
> > > +
> > > +       for (i = 2; i < min(length, response_sz / 4); i++) {
> > > +               pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_READ,
> > > +                                     &response[i]);
> > > +               pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_READ, 0);
> > > +       }
> > > +       /* flush excess length */
> > > +       for (; i < length; i++) {
> > > +               pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_READ,
> > > +                                     &val);
> > > +               pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_READ, 0);
> > > +       }
> > > +       /* Final error check to pick up on any since Data Object Ready */
> > > +       pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_STATUS, &val);
> > > +       if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) {
> > > +               pcie_doe_abort(doe);
> > > +               ret = -EIO;
> > > +       }
> > > +unlock:
> > > +       mutex_unlock(&doe->lock);
> > > +
> > > +       return ret;
> > > +}
> > > +
> > > +
> > > +static int pcie_doe_discovery(struct pcie_doe *doe, u8 *index, u16 *vid, u8 *protocol)
> > > +{
> > > +       u32 request[3] = {
> >
> > Should this be a proper struct with named fields rather than an array?
>
> Well the field names are going to end up as dw0 dw1 etc as there isn't a lot more
> meaningful to call them.  We also want to keep them as u32 values throughout to
> avoid fiddly packing manipulation on different endian machines.

The DOE object format has dedicated space for type and length.

If anything the endian issue is more reason to have a proper data structure.

>
> This becomes rather simpler when it's just the payload due to changes in the
> interface in v2.
>
> >
> > > +               [0] = FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_VID, 0001) |
> > > +               FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_TYPE, 0),
> > > +               [1] = FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH, 3),
> > > +               [2] = FIELD_PREP(PCI_DOE_DATA_OBJECT_DISC_REQ_3_INDEX, *index)
> > > +       };
> > > +       u32 response[3];
> > > +       int ret;
> > > +
> > > +       ret = pcie_doe_exchange(doe, request, sizeof(request), response, sizeof(response));
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       *vid = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_VID, response[2]);
> > > +       *protocol = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_PROTOCOL, response[2]);
> > > +       *index = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_NEXT_INDEX, response[2]);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +/**
> > > + * pcie_doe_protocol_check() - check if this DOE mailbox supports specific protocol
> > > + * @doe: DOE state structure
> > > + * @vid: Vendor ID
> > > + * @protocol: Protocol number as defined by Vendor
> > > + * Returns: 0 on success, <0 on error
> > > + */
> > > +int pcie_doe_protocol_check(struct pcie_doe *doe, u16 vid, u8 protocol)
> >
> > Not clear to me that this is a comfortable API for a driver. I would
> > expect that at registration time all the supported protocols would be
> > retrieved and cached in the 'struct pcie_doe' context and then the
> > host driver could query from there without going back to the device
> > again.
>
> I'm not sure I follow.
>
> Any driver will fall into one of the following categories:
> a) Already knows what protocols are available on a
>    given DOE instance perhaps because that's a characteristic of the hardware
>    supported, in which case it has no reason to check (unless driver writer
>    is paranoid)
> b) It has no way to know (e.g. class driver), then it makes sense to query
>    the DOE instance to find out what protocols are available.

I was more thinking that the public interface is a protocol rather
than the raw DOE. So the library knows CDAT, SPDM, IDE... and drivers
never need to query the interface.

So this more of a question about where to draw the line of common code.

For example in the nfit driver there is usage of:

acpi_label_write()

...and:

acpi_evaluate_dsm()

...where the former abstracts the protocol and the latter is the raw
interface. Both can write to a label area, but only one is idiomatic.

Re: [RFC PATCH 1/2] PCI/doe: Initial support PCI Data Object Exchange

[Chris Browy]

Please address and clarify 2 queries below...


> On Mar 16, 2021, at 2:14 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> 
> On Tue, Mar 16, 2021 at 9:31 AM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
>> 
>> On Mon, 15 Mar 2021 12:45:49 -0700
>> Dan Williams <dan.j.williams@intel.com> wrote:
>> 
>>> Hey Jonathan, happy to see this, some comments below...
>> 
>> Hi Dan,
>> 
>> Thanks for taking a look!
>> 
>>> 
>>> On Wed, Mar 10, 2021 at 10:08 AM Jonathan Cameron
>>> <Jonathan.Cameron@huawei.com> wrote:
>>>> 
>>>> Introduced in an ECN to the PCI 5.0, DOE provides a config space
>>>> based mailbox with standard protocol discovery.  Each mailbox
>>>> is accessed through a DOE PCIE Extended Capability.
>>>> 
>>>> A device may have 1 or more DOE mailboxes, each of which is allowed
>>>> to support any number of protocols (some DOE protocols
>>>> specifications apply additional restrictions).  A given protocol
>>>> may be supported on more than one DOE mailbox on a given function.
>>> 
>>> Are all those protocol instances shared?
>>> I'm trying to mental model
>>> whether, for example, an auxiliary driver instance could be loaded per
>>> DOE mailbox, or if there would need to be coordination of a given
>>> protocol no matter how many DOE mailboxes on that device implemented
>>> that protocol.
>> 
>> Just to check I've understood corectly, you mean multiple instances of same
>> protocol across different DOE mailboxes on a given device?
>> 
> 
> Right.

Could you confirm this case for clarity?  A CXL device may have multiple VF/PF.
For example, PF=0 could have one or more DOE instances for CDAT protocol.  
The driver will scan PF=0 for all DOE instances and finding one or more of CDAT 
protocol will combine/manage them.  I had not considered multiple CDAT tables 
for single PF.  For CXL devices with multiple PF’s the same process would be 
carried out on PF=1-N.

> 
>> At DOE ECN level I don't think it is actually defined if they can
>> interact or not.  I've trawled though the released protocols that I know of
>> to see if there is a consensus but not finding much information.
>> 
>> I would argue however that there would be no reason to have the OS make
>> use of more than one DOE mailbox for the same protocol. Bit fiddly to
>> handle, but doesn't seem impossible to only register a protocol with first
>> DOE that supports it.
>> 
>> CMA does talk about use of multiple methods to communicate with the device
>> and the need for results consistency. However that is referring to out of
>> band vs DOE rather than multiple DOEs.  Plus it isn't making statements
>> about protocol coordination just responses to particular queries.
>> 
>> Things might get crazy if you tried to do IDE setup from two different DOE
>> mailboxes. The IDE ECN refers to "the specific instance of DOE used for..."
>> implying I think that there might be multiple but software should only
>> use one of them?
>> 
>> My other gut feeling is that only some of the DOE mailboxes are ever going
>> to be in the control of Linux. IDE calls out models where firmware or a TEE is
>> responsible for it for example. I'm not sure how that is going to be communicated
>> to the OS (can guess of course)
>> 
>> Sub drivers are a plausible model that I'll think about some more - but
>> for now it feels like too early to go that way..
> 
> Ok, fair enough.
> 
>> 
>>> 
>>>> 
>>>> The current infrastructure is fairly simplistic and pushes the burden
>>>> of handling this many-to-many relantionship to the drivers. In many
>>> 
>>> s/relantionship/relationship/
>>> 
>>>> cases the arrangement will be static, making this straight forward.
>>>> 
>>>> Open questions:
>>>> * timeouts: The DOE specification allows for 1 second for some
>>>>  operations, but notes that specific protocols may have different
>>>>  requirements. Should we introduce the flexiblity now, or leave
>>> 
>>> s/flexiblity/flexibility/
>> 
>> Gah. One day I'll remember to spell check. Sorry about that.
>> 
>>> 
>>>>  that to be implemented when support for such a protocol is added?
>>> 
>>> If the timeout is property of the protocol then perhaps it should wait
>>> and not be modeled at the transport level, but that's just an initial
>>> reaction. I have not spent quality time with the DOE spec.
>> 
>> I'm not sure it's possible to do so without breaking the abstraction of
>> DOE request / response into a bunch of messy sub steps.  Perhaps there is
>> a clean way of doing it but I can't immediately think of it.
>> 
>> If a protocol comes along that varies the timeout we can just add
>> a parameter to say what it is on a call by call basis.
> 
> Now that I've had a chance to take a look the spec seems to
> unequivocally mandate the timeouts in "6.xx.1 Operation", where was
> the per-protocol timeout implied?
> 
>>>> * DOE mailboxes may use MSI / MSIX to signal that the have prepared
>>>>  a response. These require normal conditions are setup by the driver.
>>>>  Should we move some of this into the DOE support (such as ensuring
>>>>  bus mastering is enabled)?
>>> 
>>> DOE support seems suitable to just be a library and leave the
>>> host-device management to the host driver.
>> 
>> Agreed.  Though might be worth some debug checks.
>> 
>> Speaking from experience it's easy to spend half a day wondering why your
>> interrupts aren't turning up (I was blaming QEMU) because bus mastering
>> wasn't enabled.
> 
> Sure, no concern about validating assumptions in the library, but
> leave control to the host.
> 
>>>> Testing conducted against QEMU using:
>>>> 
>>>> https://lore.kernel.org/qemu-devel/1612900760-7361-1-git-send-email-cbrowy@avery-design.com/
>>>> + fix for interrupt flag mentioned in that thread.
>>>> 
>>> 
>>> I came across this the other day and made me wonder about SPDM
>>> emulation as another test case:
>>> 
>>> https://cfp.osfc.io/media/osfc2020/submissions/ECQ88N/resources/An_open_source_SPDM_implementation_for_secure_devi_kmIgAQe.pdf
>> 
>> Nice!  Looking at CMA / IDE emulation was on my todo list and that looks like
>> it might make that job a lot easier.

Would it be useful to integrate the openspdm’s SpdmResponderEmu.c onto the QEMU’s CXL Type3 Device’s
DOE backend for CMA/IDE testing?  Doesn’t look hard to do.

>> 
>>> 
>>> 
>>>> Additional testing to be done, particularly around error handling.
>>>> 
>>>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> 
>> Anything not commented on should be in v2.
>> 
>>>> ---
>>>> 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 +++-
>>>> 5 files changed, 356 insertions(+), 1 deletion(-)
>>> 
>>>> diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
>>>> index b2980db88cc0..801fdd5fbfc1 100644
>>>> --- a/drivers/pci/pcie/Makefile
>>>> +++ b/drivers/pci/pcie/Makefile
>>>> @@ -13,3 +13,4 @@ obj-$(CONFIG_PCIE_PME)                += pme.o
>>>> obj-$(CONFIG_PCIE_DPC)         += dpc.o
>>>> obj-$(CONFIG_PCIE_PTM)         += ptm.o
>>>> obj-$(CONFIG_PCIE_EDR)         += edr.o
>>>> +obj-$(CONFIG_PCIE_DOE)         += doe.o
>>>> diff --git a/drivers/pci/pcie/doe.c b/drivers/pci/pcie/doe.c
>>>> new file mode 100644
>>>> index 000000000000..b091ef379362
>>>> --- /dev/null
>>>> +++ b/drivers/pci/pcie/doe.c
>>>> @@ -0,0 +1,284 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Data Object Exchange was added to the PCI spec as an ECN to 5.0.
>>> 
>>> Perhaps just put the ECN link here?
>> 
>> It's by number so I've left the title here as well as a link.
> 
> Ok.
> 
>> 
>>> 
>>>> + *
>>>> + * Copyright (C) 2021 Huawei
>>>> + *     Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>>> + */
>>>> +
>>>> +#include <linux/bitfield.h>
>>>> +#include <linux/delay.h>
>>>> +#include <linux/jiffies.h>
>>>> +#include <linux/mutex.h>
>>>> +#include <linux/pci.h>
>>>> +#include <linux/pcie-doe.h>
>>>> +
>>>> +static irqreturn_t doe_irq(int irq, void *data)
>>>> +{
>>>> +       struct pcie_doe *doe = data;
>>>> +       struct pci_dev *pdev = doe->pdev;
>>>> +       u32 val;
>>>> +
>>>> +       pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_STATUS, &val);
>>>> +       if (FIELD_GET(PCI_DOE_STATUS_INT_STATUS, val)) {
>>>> +               pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_STATUS,
>>>> +                                      val);
>>>> +               complete(&doe->c);
>>>> +               return IRQ_HANDLED;
>>>> +       }
>>>> +       /* Leave the error case to be handled outside irq */
>>>> +       if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) {
>>>> +               complete(&doe->c);
>>>> +               return IRQ_HANDLED;
>>>> +       }
>>> 
>>> Only one DOE command can be outstanding at a time per PCI device?
>> 
>> No, unless I'm missing something, that is one command per DOE mailbox at a time.
>> The completion is part of the pcie_doe structure, not the pci_dev.
>> That represents a single DOE mailbox.
>> 
>> There can be multiple commands in flight to multiple DOE mailboxes. Not clear
>> that there ever will be in real use cases however.
>> 
>> This comes up later wrt to async operation.  The mailbox only
>> supports one request / response cycle at a time, they cannot be overlapped.
> 
> "6.xx.1 Operation" says "If a single DOE instance supports multiple
> data object protocols, system firmware/software is permitted to
> interleave requests/responses with different data object protocols."
> 
> ...although I must say I don't understand how system software tracks
> which response belongs to which request if the transactions are
> interleaved.
> 
>>> This
>>> seems insufficient in the multi-mailbox case / feels like there should
>>> be a 'struct pcie_doe_request' object to track what it is to be
>>> completed.
>> 
>> No need for the complexity with one request / response in flight per
>> mailbox at a time and each mailbox having separate state maintenance.
> 
> I think the workqueue proposal removes the need for pcie_doe_request,
> but still allows for the possibility of interleaving requests.
> 
>> 
>>> 
>>>> +
>>>> +       return IRQ_NONE;
>>>> +}
>>>> +
>>>> +static int pcie_doe_abort(struct pcie_doe *doe)
>>>> +{
>>>> +       struct pci_dev *pdev = doe->pdev;
>>>> +       int retry = 0;
>>>> +       u32 val;
>>>> +
>>>> +       pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_CTRL,
>>>> +                              PCI_DOE_CTRL_ABORT);
>>>> +       /* Abort is allowed to take up to 1 second */
>>>> +       do {
>>>> +               retry++;
>>>> +               pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_STATUS,
>>>> +                                     &val);
>>>> +               if (FIELD_GET(PCI_DOE_STATUS_ERROR, val) &&
>>>> +                   !FIELD_GET(PCI_DOE_STATUS_BUSY, val))
>>>> +                       return 0;
>>>> +               usleep_range(1000, 2000);
>>>> +       } while (retry < 1000);
>>>> +
>>>> +       return -EIO;
>>> 
>>> What's the state of the mailbox after an abort failure?
>> 
>> Good question.  I think the answer to that is dead device, reboot the machine
>> or at least the device if you can do a hard enough slot reset.
> 
> ...and hopefully that device is not part of an active interleave
> otherwise a reset can take down "System RAM".
> 
>> 
>> The specification goes with...
>> "It is strongly recommend that implementations ensure that the functionality
>> of the DOE Abort bit is resilient, including that DOE Abort functionality is
>> maintained even in cases where device firmware is malfunctioning"
> 
> Ok.
> 
>> 
>> So cross our fingers everyone obeys that strong recommendation or try to
>> work out what to do?
> 
> What's the worst that can happen? </famous last words>
> 
>> 
>>> 
>>>> +}
>>>> +
>>>> +/**
>>>> + * pcie_doe_init() - Initialise a Data Object Exchange mailbox
>>>> + * @doe: state structure for the DOE mailbox
>>>> + * @pdev: pci device which has this DOE mailbox
>>>> + * @doe_offset: offset in configuration space of the DOE extended capability.
>>>> + * @use_int: whether to use the optional interrupt
>>>> + * Returns: 0 on success, <0 on error
>>>> + *
>>>> + * Caller responsible for calling pci_alloc_irq_vectors() including DOE
>>>> + * interrupt.
>>>> + */
>>>> +int pcie_doe_init(struct pcie_doe *doe, struct pci_dev *pdev, int doe_offset,
>>>> +                 bool use_int)
>>>> +{
>>>> +       u32 val;
>>>> +       int rc;
>>>> +
>>>> +       mutex_init(&doe->lock);
>>>> +       init_completion(&doe->c);
>>>> +       doe->cap_offset = doe_offset;
>>>> +       doe->pdev = pdev;
>>>> +       /* Reset the mailbox by issuing an abort */
>>>> +       rc = pcie_doe_abort(doe);
>>>> +       if (rc)
>>>> +               return rc;
>>>> +
>>>> +       pci_read_config_dword(pdev, doe_offset + PCI_DOE_CAP, &val);
>>>> +
>>>> +       if (use_int && FIELD_GET(PCI_DOE_CAP_INT, val)) {
>>>> +               rc = devm_request_irq(&pdev->dev,
>>> 
>>> Lets not hide devm semantics from the caller, so at a minimum this
>>> function should be called pcim_pcie_doe_init() to indicate to the
>>> caller that it has placed something into the devm stack. However, this
>>> may not be convenient for the caller. I'd leave it to the user to call
>>> a pcie_doe() unregister routine via devm_add_action_or_reset() if it
>>> wants.
>> 
>>> 
>>> Lastly, I don't expect _init() routines to fail so perhaps split this
>>> into pure "init" and "register" functionality?
>> 
>> I'm a bit doubtful on naming of register() but will go with that for v2.
>> 
>> It's not registering with anything so that feels a bit wrong as a description
>> for part 2 of setup.  Can leave that bike shedding for now though.
>> 
> 
> Ok, just searching for a name that implies symmetrical teardown
> register/unregister, enable/disable, ... etc. init/deinit doesn't do
> it for me.
> 
>>> 
>>>> +                                     pci_irq_vector(pdev,
>>>> +                                                    FIELD_GET(PCI_DOE_CAP_IRQ, val)),
>>>> +                                     doe_irq, 0, "DOE", doe);
>>>> +               if (rc)
>>>> +                       return rc;
>>>> +
>>>> +               doe->use_int = use_int;
>>>> +               pci_write_config_dword(pdev, doe_offset + PCI_DOE_CTRL,
>>>> +                                      FIELD_PREP(PCI_DOE_CTRL_INT_EN, 1));
>>>> +       }
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +
>>>> +/**
>>>> + * pcie_doe_exchange() - Send a request and receive a response
>>>> + * @doe: DOE mailbox state structure
>>>> + * @request: request data to be sent
>>>> + * @request_sz: size of request in bytes
>>>> + * @response: buffer into which to place the response
>>>> + * @response_sz: size of available response buffer in bytes
>>>> + *
>>>> + * Return: 0 on success, < 0 on error
>>>> + * Excess data will be discarded.
>>>> + */
>>>> +int pcie_doe_exchange(struct pcie_doe *doe, u32 *request, size_t request_sz,
>>>> +                     u32 *response, size_t response_sz)
>>> 
>>> Are requests made against a specific protocol?
>> 
>> Yes, but the descriptive header is very brea.
>> 
>>> 
>>> This interface feels under-decorated for a public API for host-drivers to use.
>> 
>> I'll see what I can come up with for v2.
>> Likely to look something like
>> 
>> int pcie_doe_exchange(struct pci_doe *doe, u16 vid, u8 type,
>>                      u32 *request_pl, size_t request_pl_sz,
>>                      u32 *response_pl, size_t response_pl_sz)
> 
> I was thinking something like 'struct pcie_doe_object' pointers rather
> than u32 arrays.
> 
>> 
>> and return received length or negative on error.
>> 
>> The disadvantage is that at least some of the specs just have the
>> header as their first few DW.  So there isn't a clear distinction
>> between header and payload. May lead to people getting offsets wrong
>> in a way they wouldn't do if driver was responsible for building the
>> whole message.
> 
> Aren't they more likely to get offsets wrong with u32 arrays rather
> than data structures?
> 
>> 
>>> 
>>>> +{
>>>> +       struct pci_dev *pdev = doe->pdev;
>>>> +       int ret = 0;
>>>> +       int i;
>>>> +       u32 val;
>>>> +       int retry = -1;
>>>> +       size_t length;
>>>> +
>>>> +       /* DOE requests must be a whole number of DW */
>>>> +       if (request_sz % sizeof(u32))
>>>> +               return -EINVAL;
>>>> +
>>>> +       /* Need at least 2 DW to get the length */
>>>> +       if (response_sz < 2 * sizeof(u32))
>>>> +               return -EINVAL;
>>>> +
>>>> +       mutex_lock(&doe->lock);
>>>> +       /*
>>>> +        * Check the DOE busy bit is not set.
>>>> +        * If it is set, this could indicate someone other than Linux is
>>>> +        * using the mailbox.
>>>> +        */
>>> 
>>> Ugh, makes me think we need to extend the support for blocking pci
>>> device MMIO while a driver is attached to config-space as well. How
>>> can a communication protocol work if initiators can trample each
>>> other's state?
>> 
>> Agreed. It is crazy. At very least we need a means of saying
>> keep your hands off this DOE to the OS.
>> 
>> We can't do it on a per protocol basis, which was what I was previously
>> thinking, because we can't call the discovery protocol to see what
>> a given DOE is for.
> 
> I'm specifically thinking of a mechanism that blocks pci-sysfs from
> initiating config-cycles if a driver has claimed that range.
> 
> However, these MCTP to DOE tunnels that the SPDM presentation alluded
> to make me nervous as there is no protocol to prevent an OS driver
> agent and an MCTP agent from clobbering each other.
> 
>> 
>>> 
>>>> +       pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_STATUS, &val);
>>>> +       if (FIELD_GET(PCI_DOE_STATUS_BUSY, val)) {
>>>> +               ret = -EBUSY;
>>>> +               goto unlock;
>>>> +       }
>>>> +
>>>> +       if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) {
>>>> +               ret = pcie_doe_abort(doe);
>>>> +               if (ret)
>>>> +                       goto unlock;
>>>> +       }
>>>> +
>>>> +       for (i = 0; i < request_sz / 4; i++)
>>>> +               pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_WRITE,
>>>> +                                      request[i]);
>>>> +
>>>> +       reinit_completion(&doe->c);
>>>> +       pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_CTRL,
>>>> +                              PCI_DOE_CTRL_GO);
>>>> +
>>>> +       if (doe->use_int) {
>>>> +               /*
>>>> +                * Timeout of 1 second from 6.xx.1 ECN - Data Object Exchange
>>>> +                * Note a protocol is allowed to specify a different timeout, so
>>>> +                * that may need supporting in future.
>>>> +                */
>>>> +               if (!wait_for_completion_timeout(&doe->c,
>>>> +                                                msecs_to_jiffies(1000))) {
>>> 
>>> s/msecs_to_jiffies(1000)/HZ/
>> 
>> huh. Missed that :)
> 
> Yeah, the shorthand that X*HZ == "X seconds worth of jiffies" is just
> something I picked up from other drivers not explicit documentation.
> 
>> 
>>> 
>>>> +                       ret = -ETIMEDOUT;
>>>> +                       goto unlock;
>>>> +               }
>>>> +
>>>> +               pci_read_config_dword(pdev,
>>>> +                                     doe->cap_offset + PCI_DOE_STATUS,
>>>> +                                     &val);
>>>> +               if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) {
>>>> +                       pcie_doe_abort(doe);
>>>> +                       ret = -EIO;
>>>> +                       goto unlock;
>>>> +               }
>>>> +       } else {
>>>> +               do {
>>>> +                       retry++;
>>>> +                       pci_read_config_dword(pdev,
>>>> +                                             doe->cap_offset + PCI_DOE_STATUS,
>>>> +                                             &val);
>>>> +                       if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) {
>>>> +                               pcie_doe_abort(doe);
>>>> +                               ret = -EIO;
>>>> +                               goto unlock;
>>>> +                       }
>>>> +
>>>> +                       if (FIELD_GET(PCI_DOE_STATUS_DATA_OBJECT_READY, val))
>>>> +                               break;
>>>> +                       usleep_range(1000, 2000);
>>>> +               } while (retry < 1000);
>>>> +               if (!FIELD_GET(PCI_DOE_STATUS_DATA_OBJECT_READY, val)) {
>>>> +                       ret = -ETIMEDOUT;
>>>> +                       goto unlock;
>>> 
>>> Rather than a lock and polling loop I'd organize this as a single
>>> threaded delayed_workqueue that periodically services requests or
>>> immediately runs the workqueue upon receipt of an interrupt. This
>>> provides a software queuing model that can optionally be treated as
>>> async / sync depending on the use case.
>> 
>> Given it's single element in flight I don't think there is any benefit
>> to enabling async.  The lock has to be held throughout anyway.
>> It is always possible a particular caller wants to overlap this
>> transaction with some other actions, but I'd rather put the burden
>> on that clever caller which can spin this out to a thread of one type
>> or another.
>> 
>> We can revisit and split this in half if we have a user who benefits
>> from the complexity.
> 
> I don't think it's complex. I think it's simpler to rationalize than
> this pattern of taking a lock and going to sleep with the lock held.
> You can eliminate the lock completely if the only access to a given
> DOE is a single dedicated kthread. There are other examples of this
> single-thread protocol handler pattern in the kernel, like libsas SMP
> protocol.
> 
>>>> +               }
>>>> +       }
>>>> +
>>>> +       /* Read the first two dwords to get the length */
>>>> +       pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_READ,
>>>> +                             &response[0]);
>>>> +       pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_READ, 0);
>>>> +       pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_READ,
>>>> +                             &response[1]);
>>>> +       pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_READ, 0);
>>>> +       length = FIELD_GET(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH,
>>>> +                          response[1]);
>>>> +       if (length > SZ_1M)
>> 
>> oops. That's exiting with mutex held. Fixed in v2.
>> 
>>>> +               return -EIO;
>>>> +
>>>> +       for (i = 2; i < min(length, response_sz / 4); i++) {
>>>> +               pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_READ,
>>>> +                                     &response[i]);
>>>> +               pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_READ, 0);
>>>> +       }
>>>> +       /* flush excess length */
>>>> +       for (; i < length; i++) {
>>>> +               pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_READ,
>>>> +                                     &val);
>>>> +               pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_READ, 0);
>>>> +       }
>>>> +       /* Final error check to pick up on any since Data Object Ready */
>>>> +       pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_STATUS, &val);
>>>> +       if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) {
>>>> +               pcie_doe_abort(doe);
>>>> +               ret = -EIO;
>>>> +       }
>>>> +unlock:
>>>> +       mutex_unlock(&doe->lock);
>>>> +
>>>> +       return ret;
>>>> +}
>>>> +
>>>> +
>>>> +static int pcie_doe_discovery(struct pcie_doe *doe, u8 *index, u16 *vid, u8 *protocol)
>>>> +{
>>>> +       u32 request[3] = {
>>> 
>>> Should this be a proper struct with named fields rather than an array?
>> 
>> Well the field names are going to end up as dw0 dw1 etc as there isn't a lot more
>> meaningful to call them.  We also want to keep them as u32 values throughout to
>> avoid fiddly packing manipulation on different endian machines.
> 
> The DOE object format has dedicated space for type and length.
> 
> If anything the endian issue is more reason to have a proper data structure.
> 
>> 
>> This becomes rather simpler when it's just the payload due to changes in the
>> interface in v2.
>> 
>>> 
>>>> +               [0] = FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_VID, 0001) |
>>>> +               FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_TYPE, 0),
>>>> +               [1] = FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH, 3),
>>>> +               [2] = FIELD_PREP(PCI_DOE_DATA_OBJECT_DISC_REQ_3_INDEX, *index)
>>>> +       };
>>>> +       u32 response[3];
>>>> +       int ret;
>>>> +
>>>> +       ret = pcie_doe_exchange(doe, request, sizeof(request), response, sizeof(response));
>>>> +       if (ret)
>>>> +               return ret;
>>>> +
>>>> +       *vid = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_VID, response[2]);
>>>> +       *protocol = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_PROTOCOL, response[2]);
>>>> +       *index = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_NEXT_INDEX, response[2]);
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +/**
>>>> + * pcie_doe_protocol_check() - check if this DOE mailbox supports specific protocol
>>>> + * @doe: DOE state structure
>>>> + * @vid: Vendor ID
>>>> + * @protocol: Protocol number as defined by Vendor
>>>> + * Returns: 0 on success, <0 on error
>>>> + */
>>>> +int pcie_doe_protocol_check(struct pcie_doe *doe, u16 vid, u8 protocol)
>>> 
>>> Not clear to me that this is a comfortable API for a driver. I would
>>> expect that at registration time all the supported protocols would be
>>> retrieved and cached in the 'struct pcie_doe' context and then the
>>> host driver could query from there without going back to the device
>>> again.
>> 
>> I'm not sure I follow.
>> 
>> Any driver will fall into one of the following categories:
>> a) Already knows what protocols are available on a
>>   given DOE instance perhaps because that's a characteristic of the hardware
>>   supported, in which case it has no reason to check (unless driver writer
>>   is paranoid)
>> b) It has no way to know (e.g. class driver), then it makes sense to query
>>   the DOE instance to find out what protocols are available.
> 
> I was more thinking that the public interface is a protocol rather
> than the raw DOE. So the library knows CDAT, SPDM, IDE... and drivers
> never need to query the interface.
> 
> So this more of a question about where to draw the line of common code.
> 
> For example in the nfit driver there is usage of:
> 
> acpi_label_write()
> 
> ...and:
> 
> acpi_evaluate_dsm()
> 
> ...where the former abstracts the protocol and the latter is the raw
> interface. Both can write to a label area, but only one is idiomatic.

Re: [RFC PATCH 1/2] PCI/doe: Initial support PCI Data Object Exchange

[Dan Williams]

Btw your mailer does something odd with the "In-Reply-To:" field, I
need to fix it up manually to include your address.

On Tue, Mar 16, 2021 at 4:28 PM Chris Browy <cbrowy@avery-design.com> wrote:
>
> Please address and clarify 2 queries below...
>
>
> > On Mar 16, 2021, at 2:14 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> >
> > On Tue, Mar 16, 2021 at 9:31 AM Jonathan Cameron
> > <Jonathan.Cameron@huawei.com> wrote:
> >>
> >> On Mon, 15 Mar 2021 12:45:49 -0700
> >> Dan Williams <dan.j.williams@intel.com> wrote:
> >>
> >>> Hey Jonathan, happy to see this, some comments below...
> >>
> >> Hi Dan,
> >>
> >> Thanks for taking a look!
> >>
> >>>
> >>> On Wed, Mar 10, 2021 at 10:08 AM Jonathan Cameron
> >>> <Jonathan.Cameron@huawei.com> wrote:
> >>>>
> >>>> Introduced in an ECN to the PCI 5.0, DOE provides a config space
> >>>> based mailbox with standard protocol discovery.  Each mailbox
> >>>> is accessed through a DOE PCIE Extended Capability.
> >>>>
> >>>> A device may have 1 or more DOE mailboxes, each of which is allowed
> >>>> to support any number of protocols (some DOE protocols
> >>>> specifications apply additional restrictions).  A given protocol
> >>>> may be supported on more than one DOE mailbox on a given function.
> >>>
> >>> Are all those protocol instances shared?
> >>> I'm trying to mental model
> >>> whether, for example, an auxiliary driver instance could be loaded per
> >>> DOE mailbox, or if there would need to be coordination of a given
> >>> protocol no matter how many DOE mailboxes on that device implemented
> >>> that protocol.
> >>
> >> Just to check I've understood corectly, you mean multiple instances of same
> >> protocol across different DOE mailboxes on a given device?
> >>
> >
> > Right.
>
> Could you confirm this case for clarity?  A CXL device may have multiple VF/PF.
> For example, PF=0 could have one or more DOE instances for CDAT protocol.
> The driver will scan PF=0 for all DOE instances and finding one or more of CDAT
> protocol will combine/manage them.  I had not considered multiple CDAT tables
> for single PF.  For CXL devices with multiple PF’s the same process would be
> carried out on PF=1-N.

This patch has nothing to do with CXL. This is a general discussion of
how a PCIE device implements a DOE mailbox or set of mailboxes. The
DOE definition is PF-only afaics from the DOE specification.

The CXL specification only says that a device can implement a CDAT per
DOE capability instance, so the CXL spec does not limit the number of
DOE instances to 1, but I can't think of a practical reason to support
more than one.

[..]
> >>> https://cfp.osfc.io/media/osfc2020/submissions/ECQ88N/resources/An_open_source_SPDM_implementation_for_secure_devi_kmIgAQe.pdf
> >>
> >> Nice!  Looking at CMA / IDE emulation was on my todo list and that looks like
> >> it might make that job a lot easier.
>
> Would it be useful to integrate the openspdm’s SpdmResponderEmu.c onto the QEMU’s CXL Type3 Device’s
> DOE backend for CMA/IDE testing?  Doesn’t look hard to do.

Yes, I do think it would be useful.

Re: [RFC PATCH 1/2] PCI/doe: Initial support PCI Data Object Exchange

[Jonathan Cameron]

On Wed, 17 Mar 2021 18:30:26 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Btw your mailer does something odd with the "In-Reply-To:" field, I
> need to fix it up manually to include your address.
> 
> On Tue, Mar 16, 2021 at 4:28 PM Chris Browy <cbrowy@avery-design.com> wrote:
> >
> > Please address and clarify 2 queries below...
> >
> >  
> > > On Mar 16, 2021, at 2:14 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> > >
> > > On Tue, Mar 16, 2021 at 9:31 AM Jonathan Cameron
> > > <Jonathan.Cameron@huawei.com> wrote:  
> > >>
> > >> On Mon, 15 Mar 2021 12:45:49 -0700
> > >> Dan Williams <dan.j.williams@intel.com> wrote:
> > >>  
> > >>> Hey Jonathan, happy to see this, some comments below...  
> > >>
> > >> Hi Dan,
> > >>
> > >> Thanks for taking a look!
> > >>  
> > >>>
> > >>> On Wed, Mar 10, 2021 at 10:08 AM Jonathan Cameron
> > >>> <Jonathan.Cameron@huawei.com> wrote:  
> > >>>>
> > >>>> Introduced in an ECN to the PCI 5.0, DOE provides a config space
> > >>>> based mailbox with standard protocol discovery.  Each mailbox
> > >>>> is accessed through a DOE PCIE Extended Capability.
> > >>>>
> > >>>> A device may have 1 or more DOE mailboxes, each of which is allowed
> > >>>> to support any number of protocols (some DOE protocols
> > >>>> specifications apply additional restrictions).  A given protocol
> > >>>> may be supported on more than one DOE mailbox on a given function.  
> > >>>
> > >>> Are all those protocol instances shared?
> > >>> I'm trying to mental model
> > >>> whether, for example, an auxiliary driver instance could be loaded per
> > >>> DOE mailbox, or if there would need to be coordination of a given
> > >>> protocol no matter how many DOE mailboxes on that device implemented
> > >>> that protocol.  
> > >>
> > >> Just to check I've understood corectly, you mean multiple instances of same
> > >> protocol across different DOE mailboxes on a given device?
> > >>  
> > >
> > > Right.  
> >
> > Could you confirm this case for clarity?  A CXL device may have multiple VF/PF.
> > For example, PF=0 could have one or more DOE instances for CDAT protocol.
> > The driver will scan PF=0 for all DOE instances and finding one or more of CDAT
> > protocol will combine/manage them.  I had not considered multiple CDAT tables
> > for single PF.  For CXL devices with multiple PF’s the same process would be
> > carried out on PF=1-N.  
> 
> This patch has nothing to do with CXL. This is a general discussion of
> how a PCIE device implements a DOE mailbox or set of mailboxes. The
> DOE definition is PF-only afaics from the DOE specification.
> 
> The CXL specification only says that a device can implement a CDAT per
> DOE capability instance, so the CXL spec does not limit the number of
> DOE instances to 1, but I can't think of a practical reason to support
> more than one.
> 
> [..]
> > >>> https://cfp.osfc.io/media/osfc2020/submissions/ECQ88N/resources/An_open_source_SPDM_implementation_for_secure_devi_kmIgAQe.pdf  
> > >>
> > >> Nice!  Looking at CMA / IDE emulation was on my todo list and that looks like
> > >> it might make that job a lot easier.  
> >
> > Would it be useful to integrate the openspdm’s SpdmResponderEmu.c onto the QEMU’s CXL Type3 Device’s
> > DOE backend for CMA/IDE testing?  Doesn’t look hard to do.  
> 
> Yes, I do think it would be useful.

Agreed.  Very useful indeed.

Jonathan

Re: [RFC PATCH 1/2] PCI/doe: Initial support PCI Data Object Exchange

[Jonathan Cameron]

On Thu, 18 Mar 2021 14:25:29 +0000
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Wed, 17 Mar 2021 18:30:26 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > Btw your mailer does something odd with the "In-Reply-To:" field, I
> > need to fix it up manually to include your address.
> > 
> > On Tue, Mar 16, 2021 at 4:28 PM Chris Browy <cbrowy@avery-design.com> wrote:  
> > >
> > > Please address and clarify 2 queries below...
> > >
> > >    
> > > > On Mar 16, 2021, at 2:14 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> > > >
> > > > On Tue, Mar 16, 2021 at 9:31 AM Jonathan Cameron
> > > > <Jonathan.Cameron@huawei.com> wrote:    
> > > >>
> > > >> On Mon, 15 Mar 2021 12:45:49 -0700
> > > >> Dan Williams <dan.j.williams@intel.com> wrote:
> > > >>    
> > > >>> Hey Jonathan, happy to see this, some comments below...    
> > > >>
> > > >> Hi Dan,
> > > >>
> > > >> Thanks for taking a look!
> > > >>    
> > > >>>
> > > >>> On Wed, Mar 10, 2021 at 10:08 AM Jonathan Cameron
> > > >>> <Jonathan.Cameron@huawei.com> wrote:    
> > > >>>>
> > > >>>> Introduced in an ECN to the PCI 5.0, DOE provides a config space
> > > >>>> based mailbox with standard protocol discovery.  Each mailbox
> > > >>>> is accessed through a DOE PCIE Extended Capability.
> > > >>>>
> > > >>>> A device may have 1 or more DOE mailboxes, each of which is allowed
> > > >>>> to support any number of protocols (some DOE protocols
> > > >>>> specifications apply additional restrictions).  A given protocol
> > > >>>> may be supported on more than one DOE mailbox on a given function.    
> > > >>>
> > > >>> Are all those protocol instances shared?
> > > >>> I'm trying to mental model
> > > >>> whether, for example, an auxiliary driver instance could be loaded per
> > > >>> DOE mailbox, or if there would need to be coordination of a given
> > > >>> protocol no matter how many DOE mailboxes on that device implemented
> > > >>> that protocol.    
> > > >>
> > > >> Just to check I've understood corectly, you mean multiple instances of same
> > > >> protocol across different DOE mailboxes on a given device?
> > > >>    
> > > >
> > > > Right.    
> > >
> > > Could you confirm this case for clarity?  A CXL device may have multiple VF/PF.
> > > For example, PF=0 could have one or more DOE instances for CDAT protocol.
> > > The driver will scan PF=0 for all DOE instances and finding one or more of CDAT
> > > protocol will combine/manage them.  I had not considered multiple CDAT tables
> > > for single PF.  For CXL devices with multiple PF’s the same process would be
> > > carried out on PF=1-N.    
> > 
> > This patch has nothing to do with CXL. This is a general discussion of
> > how a PCIE device implements a DOE mailbox or set of mailboxes. The
> > DOE definition is PF-only afaics from the DOE specification.
> > 
> > The CXL specification only says that a device can implement a CDAT per
> > DOE capability instance, so the CXL spec does not limit the number of
> > DOE instances to 1, but I can't think of a practical reason to support
> > more than one.
> > 
> > [..]  
> > > >>> https://cfp.osfc.io/media/osfc2020/submissions/ECQ88N/resources/An_open_source_SPDM_implementation_for_secure_devi_kmIgAQe.pdf    
> > > >>
> > > >> Nice!  Looking at CMA / IDE emulation was on my todo list and that looks like
> > > >> it might make that job a lot easier.    
> > >
> > > Would it be useful to integrate the openspdm’s SpdmResponderEmu.c onto the QEMU’s CXL Type3 Device’s
> > > DOE backend for CMA/IDE testing?  Doesn’t look hard to do.    
> > 
> > Yes, I do think it would be useful.  
> 
> Agreed.  Very useful indeed.
> 
> Jonathan
> 

Hi Chris,

Just wondering if this qemu/openspdm integration was something your team have
had time to look at?  I'd like to ideally get a second DOE usecase
implemented on the Linux side to prove out the implementation.

If it's fallen off your near term todo list I might see if I can hack
something together in the meantime.

Thanks,

Jonathan

Re: [RFC PATCH 1/2] PCI/doe: Initial support PCI Data Object Exchange

[Chris Browy]

> On Jun 17, 2021, at 1:12 PM, Jonathan Cameron <jonathan.cameron@huawei.com> wrote:
> 
> On Thu, 18 Mar 2021 14:25:29 +0000
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> 
>> On Wed, 17 Mar 2021 18:30:26 -0700
>> Dan Williams <dan.j.williams@intel.com> wrote:
>> 
>>> Btw your mailer does something odd with the "In-Reply-To:" field, I
>>> need to fix it up manually to include your address.
>>> 
>>> On Tue, Mar 16, 2021 at 4:28 PM Chris Browy <cbrowy@avery-design.com> wrote:  
>>>> 
>>>> Please address and clarify 2 queries below...
>>>> 
>>>> 
>>>>> On Mar 16, 2021, at 2:14 PM, Dan Williams <dan.j.williams@intel.com> wrote:
>>>>> 
>>>>> On Tue, Mar 16, 2021 at 9:31 AM Jonathan Cameron
>>>>> <Jonathan.Cameron@huawei.com> wrote:    
>>>>>> 
>>>>>> On Mon, 15 Mar 2021 12:45:49 -0700
>>>>>> Dan Williams <dan.j.williams@intel.com> wrote:
>>>>>> 
>>>>>>> Hey Jonathan, happy to see this, some comments below...    
>>>>>> 
>>>>>> Hi Dan,
>>>>>> 
>>>>>> Thanks for taking a look!
>>>>>> 
>>>>>>> 
>>>>>>> On Wed, Mar 10, 2021 at 10:08 AM Jonathan Cameron
>>>>>>> <Jonathan.Cameron@huawei.com> wrote:    
>>>>>>>> 
>>>>>>>> Introduced in an ECN to the PCI 5.0, DOE provides a config space
>>>>>>>> based mailbox with standard protocol discovery.  Each mailbox
>>>>>>>> is accessed through a DOE PCIE Extended Capability.
>>>>>>>> 
>>>>>>>> A device may have 1 or more DOE mailboxes, each of which is allowed
>>>>>>>> to support any number of protocols (some DOE protocols
>>>>>>>> specifications apply additional restrictions).  A given protocol
>>>>>>>> may be supported on more than one DOE mailbox on a given function.    
>>>>>>> 
>>>>>>> Are all those protocol instances shared?
>>>>>>> I'm trying to mental model
>>>>>>> whether, for example, an auxiliary driver instance could be loaded per
>>>>>>> DOE mailbox, or if there would need to be coordination of a given
>>>>>>> protocol no matter how many DOE mailboxes on that device implemented
>>>>>>> that protocol.    
>>>>>> 
>>>>>> Just to check I've understood corectly, you mean multiple instances of same
>>>>>> protocol across different DOE mailboxes on a given device?
>>>>>> 
>>>>> 
>>>>> Right.    
>>>> 
>>>> Could you confirm this case for clarity?  A CXL device may have multiple VF/PF.
>>>> For example, PF=0 could have one or more DOE instances for CDAT protocol.
>>>> The driver will scan PF=0 for all DOE instances and finding one or more of CDAT
>>>> protocol will combine/manage them.  I had not considered multiple CDAT tables
>>>> for single PF.  For CXL devices with multiple PF’s the same process would be
>>>> carried out on PF=1-N.    
>>> 
>>> This patch has nothing to do with CXL. This is a general discussion of
>>> how a PCIE device implements a DOE mailbox or set of mailboxes. The
>>> DOE definition is PF-only afaics from the DOE specification.
>>> 
>>> The CXL specification only says that a device can implement a CDAT per
>>> DOE capability instance, so the CXL spec does not limit the number of
>>> DOE instances to 1, but I can't think of a practical reason to support
>>> more than one.
>>> 
>>> [..]  
>>>>>>> https://cfp.osfc.io/media/osfc2020/submissions/ECQ88N/resources/An_open_source_SPDM_implementation_for_secure_devi_kmIgAQe.pdf    
>>>>>> 
>>>>>> Nice!  Looking at CMA / IDE emulation was on my todo list and that looks like
>>>>>> it might make that job a lot easier.    
>>>> 
>>>> Would it be useful to integrate the openspdm’s SpdmResponderEmu.c onto the QEMU’s CXL Type3 Device’s
>>>> DOE backend for CMA/IDE testing?  Doesn’t look hard to do.    
>>> 
>>> Yes, I do think it would be useful.  
>> 
>> Agreed.  Very useful indeed.
>> 
>> Jonathan
>> 
> 
> Hi Chris,
> 
> Just wondering if this qemu/openspdm integration was something your team have
> had time to look at?  I'd like to ideally get a second DOE usecase
> implemented on the Linux side to prove out the implementation.
> 
> If it's fallen off your near term todo list I might see if I can hack
> something together in the meantime.

We have been working on this.  The plan was 
1) implement modified version of openspdm requester running as QEMU app to pass DOE via CXL 
    driver IOCTL’s to do DOE (DONE)
2) enhance cxl_type3.c to call openspdm responder to implement device side SPDM (NOT DONE)
3) enhance the Avery CXL Type 3 device SystemVerilog model to call opensldm 
   responder to implement device side SPDM (DONE)

Currently we run 1) and 3) using our QEMU co-sim environment.

Huai-Cheng can probably finish 2) next week so we can run entirely in QEMU stand-alone environment.


> 
> Thanks,
> 
> Jonathan

Re: [RFC PATCH 1/2] PCI/doe: Initial support PCI Data Object Exchange

[Jonathan Cameron]

On Tue, 16 Mar 2021 11:14:05 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

...

> >  
> > >  
> > > >   that to be implemented when support for such a protocol is added?  
> > >
> > > If the timeout is property of the protocol then perhaps it should wait
> > > and not be modeled at the transport level, but that's just an initial
> > > reaction. I have not spent quality time with the DOE spec.  
> >
> > I'm not sure it's possible to do so without breaking the abstraction of
> > DOE request / response into a bunch of messy sub steps.  Perhaps there is
> > a clean way of doing it but I can't immediately think of it.
> >
> > If a protocol comes along that varies the timeout we can just add
> > a parameter to say what it is on a call by call basis.  
> 
> Now that I've had a chance to take a look the spec seems to
> unequivocally mandate the timeouts in "6.xx.1 Operation", where was
> the per-protocol timeout implied?

That paragraph starts with
"For request/response protocols, unless there is a protocol-specific
 requirement, a DOE instance must complete processing a received data object
 and, if a data object is required in response, must generate the response
 and Set the Data Object Ready bit in the DOE Status register within 1
 second after the DOE Go bit was Set in the DOE Control register...."

I read that as allowing a wealth of flexibility in what a protocol can specify
differently from the main DOE spec including the timeout.
Let's ignore it until it matters.

...

> > > > +obj-$(CONFIG_PCIE_DOE)         += doe.o
> > > > diff --git a/drivers/pci/pcie/doe.c b/drivers/pci/pcie/doe.c
> > > > new file mode 100644
> > > > index 000000000000..b091ef379362
> > > > --- /dev/null
> > > > +++ b/drivers/pci/pcie/doe.c
> > > > @@ -0,0 +1,284 @@

> >  
> > >  
> > > > + *
> > > > + * Copyright (C) 2021 Huawei
> > > > + *     Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > + */
> > > > +
> > > > +#include <linux/bitfield.h>
> > > > +#include <linux/delay.h>
> > > > +#include <linux/jiffies.h>
> > > > +#include <linux/mutex.h>
> > > > +#include <linux/pci.h>
> > > > +#include <linux/pcie-doe.h>
> > > > +
> > > > +static irqreturn_t doe_irq(int irq, void *data)
> > > > +{
> > > > +       struct pcie_doe *doe = data;
> > > > +       struct pci_dev *pdev = doe->pdev;
> > > > +       u32 val;
> > > > +
> > > > +       pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_STATUS, &val);
> > > > +       if (FIELD_GET(PCI_DOE_STATUS_INT_STATUS, val)) {
> > > > +               pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_STATUS,
> > > > +                                      val);
> > > > +               complete(&doe->c);
> > > > +               return IRQ_HANDLED;
> > > > +       }
> > > > +       /* Leave the error case to be handled outside irq */
> > > > +       if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) {
> > > > +               complete(&doe->c);
> > > > +               return IRQ_HANDLED;
> > > > +       }  
> > >
> > > Only one DOE command can be outstanding at a time per PCI device?  
> >
> > No, unless I'm missing something, that is one command per DOE mailbox at a time.
> > The completion is part of the pcie_doe structure, not the pci_dev.
> > That represents a single DOE mailbox.
> >
> > There can be multiple commands in flight to multiple DOE mailboxes. Not clear
> > that there ever will be in real use cases however.
> >
> > This comes up later wrt to async operation.  The mailbox only
> > supports one request / response cycle at a time, they cannot be overlapped.  
> 
> "6.xx.1 Operation" says "If a single DOE instance supports multiple
> data object protocols, system firmware/software is permitted to
> interleave requests/responses with different data object protocols."
> 
> ...although I must say I don't understand how system software tracks
> which response belongs to which request if the transactions are
> interleaved.

Ouch, I read that as meaning you could use the mailbox for
request protocol A / response protocol A ... request protocol B / response protocol B.

You are correct it is more general.  Still we don't have to use it
and I strongly suggest we don't.  It's a layer of complexity we probably
will never really need and it is fiddly to handle.

For tracking: you could do it, as the response carries the protocol ID.
That is enough to let you match against the protocol and each protocol can
only issue one request before it has to wait for a response.

One nasty element is that there is no interrupt to let you know BUSY bit has
fallen and you can send a new requests (as long as different protocol),
so this would need polling in all cases.

I think it would be a valid implementation to hold BUSY high until the
response to the previous response has been read out from the DOE.
Such an implementation would be enforcing no interleaving of different protocols
and we would have had to poll pointlessly whilst the previous message was being
handled and response read.

My current thought is to simply not support multi protocol interleaving.
If it turns out to be necessary sometime in the future we can look at
how to do it.

I see DOE as simple and lower performance so want to keep software
at a similar level.

> 
> > > This
> > > seems insufficient in the multi-mailbox case / feels like there should
> > > be a 'struct pcie_doe_request' object to track what it is to be
> > > completed.  
> >
> > No need for the complexity with one request / response in flight per
> > mailbox at a time and each mailbox having separate state maintenance.  
> 
> I think the workqueue proposal removes the need for pcie_doe_request,
> but still allows for the possibility of interleaving requests.

OK at some abstract level it allows for interleaving and we can try
to figure out how to actually do it if a need ever comes along.

> 
> >  
> > >  
> > > > +
> > > > +       return IRQ_NONE;
> > > > +}
> > > > +
> > > > +static int pcie_doe_abort(struct pcie_doe *doe)
> > > > +{
> > > > +       struct pci_dev *pdev = doe->pdev;
> > > > +       int retry = 0;
> > > > +       u32 val;
> > > > +
> > > > +       pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_CTRL,
> > > > +                              PCI_DOE_CTRL_ABORT);
> > > > +       /* Abort is allowed to take up to 1 second */
> > > > +       do {
> > > > +               retry++;
> > > > +               pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_STATUS,
> > > > +                                     &val);
> > > > +               if (FIELD_GET(PCI_DOE_STATUS_ERROR, val) &&
> > > > +                   !FIELD_GET(PCI_DOE_STATUS_BUSY, val))
> > > > +                       return 0;
> > > > +               usleep_range(1000, 2000);
> > > > +       } while (retry < 1000);
> > > > +
> > > > +       return -EIO;  
> > >
> > > What's the state of the mailbox after an abort failure?  
> >
> > Good question.  I think the answer to that is dead device, reboot the machine
> > or at least the device if you can do a hard enough slot reset.  
> 
> ...and hopefully that device is not part of an active interleave
> otherwise a reset can take down "System RAM".

Well - I suspect in most cases DOE death either breaks IDE in which case
likely *boom* anyway or it's informational stuff life CDAT in which case
print lots of warnings to suggest a smooth shutdown is a really good idea.

> 
> >
> > The specification goes with...
> > "It is strongly recommend that implementations ensure that the functionality
> > of the DOE Abort bit is resilient, including that DOE Abort functionality is
> > maintained even in cases where device firmware is malfunctioning"  
> 
> Ok.
> 
> >
> > So cross our fingers everyone obeys that strong recommendation or try to
> > work out what to do?  
> 
> What's the worst that can happen? </famous last words>

:)


> > >  
> > > > +                                     pci_irq_vector(pdev,
> > > > +                                                    FIELD_GET(PCI_DOE_CAP_IRQ, val)),
> > > > +                                     doe_irq, 0, "DOE", doe);
> > > > +               if (rc)
> > > > +                       return rc;
> > > > +
> > > > +               doe->use_int = use_int;
> > > > +               pci_write_config_dword(pdev, doe_offset + PCI_DOE_CTRL,
> > > > +                                      FIELD_PREP(PCI_DOE_CTRL_INT_EN, 1));
> > > > +       }
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +
> > > > +/**
> > > > + * pcie_doe_exchange() - Send a request and receive a response
> > > > + * @doe: DOE mailbox state structure
> > > > + * @request: request data to be sent
> > > > + * @request_sz: size of request in bytes
> > > > + * @response: buffer into which to place the response
> > > > + * @response_sz: size of available response buffer in bytes
> > > > + *
> > > > + * Return: 0 on success, < 0 on error
> > > > + * Excess data will be discarded.
> > > > + */
> > > > +int pcie_doe_exchange(struct pcie_doe *doe, u32 *request, size_t request_sz,
> > > > +                     u32 *response, size_t response_sz)  
> > >
> > > Are requests made against a specific protocol?  
> >
> > Yes, but the descriptive header is very brea.
> >  
> > >
> > > This interface feels under-decorated for a public API for host-drivers to use.  
> >
> > I'll see what I can come up with for v2.
> > Likely to look something like
> >
> > int pcie_doe_exchange(struct pci_doe *doe, u16 vid, u8 type,
> >                       u32 *request_pl, size_t request_pl_sz,
> >                       u32 *response_pl, size_t response_pl_sz)  
> 
> I was thinking something like 'struct pcie_doe_object' pointers rather
> than u32 arrays.

Possibly... I'm not convinced that it doesn't end up being
struct pcie_doe_object doe_obj;
DOE_OBJ_INIT(&doe_obj, vid, type, request_pl, request_pl_sz,
	     response_pl, response_pl_sz)

ret = pcie_doe_exchange(doe, &doe_obj);

If that's the pattern we see, why split it?
We might well have a struct pcie_doe_object internally but it doesn't
seem like a sensible external interface to me simply because we'd
just be filling it in and immediately passing it to a 'send' function.

> 
> >
> > and return received length or negative on error.
> >
> > The disadvantage is that at least some of the specs just have the
> > header as their first few DW.  So there isn't a clear distinction
> > between header and payload. May lead to people getting offsets wrong
> > in a way they wouldn't do if driver was responsible for building the
> > whole message.  
> 
> Aren't they more likely to get offsets wrong with u32 arrays rather
> than data structures?

I'm not sure what level you mean this at.  The CDAT patch review you
followed this with suggested just breaking out vid and type which is
fine because those are always packed the same and we can do appropriate
special handling.

If you meant the whole object as packed structure, then it is a whole
different matter.

Easy one to point is that u64 values are going to end up with their
top and bottom halves swapped.  Things get even messier if we break
up below the u32 level.

We can do this at a higher level by having wrappers that deal with
each protocol and do a serialize / deserialize for the protocol.
I'm not sure if that will make sense or not yet though.

> 
> >  
> > >  
> > > > +{
> > > > +       struct pci_dev *pdev = doe->pdev;
> > > > +       int ret = 0;
> > > > +       int i;
> > > > +       u32 val;
> > > > +       int retry = -1;
> > > > +       size_t length;
> > > > +
> > > > +       /* DOE requests must be a whole number of DW */
> > > > +       if (request_sz % sizeof(u32))
> > > > +               return -EINVAL;
> > > > +
> > > > +       /* Need at least 2 DW to get the length */
> > > > +       if (response_sz < 2 * sizeof(u32))
> > > > +               return -EINVAL;
> > > > +
> > > > +       mutex_lock(&doe->lock);
> > > > +       /*
> > > > +        * Check the DOE busy bit is not set.
> > > > +        * If it is set, this could indicate someone other than Linux is
> > > > +        * using the mailbox.
> > > > +        */  
> > >
> > > Ugh, makes me think we need to extend the support for blocking pci
> > > device MMIO while a driver is attached to config-space as well. How
> > > can a communication protocol work if initiators can trample each
> > > other's state?  
> >
> > Agreed. It is crazy. At very least we need a means of saying
> > keep your hands off this DOE to the OS.
> >
> > We can't do it on a per protocol basis, which was what I was previously
> > thinking, because we can't call the discovery protocol to see what
> > a given DOE is for.  
> 
> I'm specifically thinking of a mechanism that blocks pci-sysfs from
> initiating config-cycles if a driver has claimed that range.
> 
> However, these MCTP to DOE tunnels that the SPDM presentation alluded
> to make me nervous as there is no protocol to prevent an OS driver
> agent and an MCTP agent from clobbering each other.

Agreed.


> >  
> > >  
> > > > +                       ret = -ETIMEDOUT;
> > > > +                       goto unlock;
> > > > +               }
> > > > +
> > > > +               pci_read_config_dword(pdev,
> > > > +                                     doe->cap_offset + PCI_DOE_STATUS,
> > > > +                                     &val);
> > > > +               if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) {
> > > > +                       pcie_doe_abort(doe);
> > > > +                       ret = -EIO;
> > > > +                       goto unlock;
> > > > +               }
> > > > +       } else {
> > > > +               do {
> > > > +                       retry++;
> > > > +                       pci_read_config_dword(pdev,
> > > > +                                             doe->cap_offset + PCI_DOE_STATUS,
> > > > +                                             &val);
> > > > +                       if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) {
> > > > +                               pcie_doe_abort(doe);
> > > > +                               ret = -EIO;
> > > > +                               goto unlock;
> > > > +                       }
> > > > +
> > > > +                       if (FIELD_GET(PCI_DOE_STATUS_DATA_OBJECT_READY, val))
> > > > +                               break;
> > > > +                       usleep_range(1000, 2000);
> > > > +               } while (retry < 1000);
> > > > +               if (!FIELD_GET(PCI_DOE_STATUS_DATA_OBJECT_READY, val)) {
> > > > +                       ret = -ETIMEDOUT;
> > > > +                       goto unlock;  
> > >
> > > Rather than a lock and polling loop I'd organize this as a single
> > > threaded delayed_workqueue that periodically services requests or
> > > immediately runs the workqueue upon receipt of an interrupt. This
> > > provides a software queuing model that can optionally be treated as
> > > async / sync depending on the use case.  
> >
> > Given it's single element in flight I don't think there is any benefit
> > to enabling async.  The lock has to be held throughout anyway.
> > It is always possible a particular caller wants to overlap this
> > transaction with some other actions, but I'd rather put the burden
> > on that clever caller which can spin this out to a thread of one type
> > or another.
> >
> > We can revisit and split this in half if we have a user who benefits
> > from the complexity.  
> 
> I don't think it's complex. I think it's simpler to rationalize than
> this pattern of taking a lock and going to sleep with the lock held.
> You can eliminate the lock completely if the only access to a given
> DOE is a single dedicated kthread. There are other examples of this
> single-thread protocol handler pattern in the kernel, like libsas SMP
> protocol.

So for this, I've implemented a simple single threaded workqueue.
As I'm not supporting interleaving for now, it ends up being very similar
to the body of pcie_doe_exchange().  The lock is gone.

Async is trivial to implement (I haven't done so yet as don't have a user)
but there is a requirement for the caller to ensure lifetimes of the buffers
because we probably don't want to take copies when not necessary.  That
shouldn't be to onerous for the drivers.

One thing I don't understand is why you proposed a delayed work queue
above?  I'm not seeing that model in the libsas SMP for example.  As far
as I can tell that just processes work items asap.

Can you point to a more specific example if you thinks that we should
use one?

> 
> > > > +               }
> > > > +       }
> > > > +
> > > > +       /* Read the first two dwords to get the length */
> > > > +       pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_READ,
> > > > +                             &response[0]);
> > > > +       pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_READ, 0);
> > > > +       pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_READ,
> > > > +                             &response[1]);
> > > > +       pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_READ, 0);
> > > > +       length = FIELD_GET(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH,
> > > > +                          response[1]);
> > > > +       if (length > SZ_1M)  
> >
> > oops. That's exiting with mutex held. Fixed in v2.
> >  
> > > > +               return -EIO;
> > > > +
> > > > +       for (i = 2; i < min(length, response_sz / 4); i++) {
> > > > +               pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_READ,
> > > > +                                     &response[i]);
> > > > +               pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_READ, 0);
> > > > +       }
> > > > +       /* flush excess length */
> > > > +       for (; i < length; i++) {
> > > > +               pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_READ,
> > > > +                                     &val);
> > > > +               pci_write_config_dword(pdev, doe->cap_offset + PCI_DOE_READ, 0);
> > > > +       }
> > > > +       /* Final error check to pick up on any since Data Object Ready */
> > > > +       pci_read_config_dword(pdev, doe->cap_offset + PCI_DOE_STATUS, &val);
> > > > +       if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) {
> > > > +               pcie_doe_abort(doe);
> > > > +               ret = -EIO;
> > > > +       }
> > > > +unlock:
> > > > +       mutex_unlock(&doe->lock);
> > > > +
> > > > +       return ret;
> > > > +}
> > > > +
> > > > +
> > > > +static int pcie_doe_discovery(struct pcie_doe *doe, u8 *index, u16 *vid, u8 *protocol)
> > > > +{
> > > > +       u32 request[3] = {  
> > >
> > > Should this be a proper struct with named fields rather than an array?  
> >
> > Well the field names are going to end up as dw0 dw1 etc as there isn't a lot more
> > meaningful to call them.  We also want to keep them as u32 values throughout to
> > avoid fiddly packing manipulation on different endian machines.  
> 
> The DOE object format has dedicated space for type and length.
> 
> If anything the endian issue is more reason to have a proper data structure.

I'm fine with doing that for the first 2 DW but after that it's a lot messier
as mentioned above. It is protocol specific.  Of course we can always wrap the
individual protocols up with serializer / deserializer code to actually pack
the dwords.

> 
> >
> > This becomes rather simpler when it's just the payload due to changes in the
> > interface in v2.
> >  
> > >  
> > > > +               [0] = FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_VID, 0001) |
> > > > +               FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_TYPE, 0),
> > > > +               [1] = FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH, 3),
> > > > +               [2] = FIELD_PREP(PCI_DOE_DATA_OBJECT_DISC_REQ_3_INDEX, *index)
> > > > +       };
> > > > +       u32 response[3];
> > > > +       int ret;
> > > > +
> > > > +       ret = pcie_doe_exchange(doe, request, sizeof(request), response, sizeof(response));
> > > > +       if (ret)
> > > > +               return ret;
> > > > +
> > > > +       *vid = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_VID, response[2]);
> > > > +       *protocol = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_PROTOCOL, response[2]);
> > > > +       *index = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_NEXT_INDEX, response[2]);
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +/**
> > > > + * pcie_doe_protocol_check() - check if this DOE mailbox supports specific protocol
> > > > + * @doe: DOE state structure
> > > > + * @vid: Vendor ID
> > > > + * @protocol: Protocol number as defined by Vendor
> > > > + * Returns: 0 on success, <0 on error
> > > > + */
> > > > +int pcie_doe_protocol_check(struct pcie_doe *doe, u16 vid, u8 protocol)  
> > >
> > > Not clear to me that this is a comfortable API for a driver. I would
> > > expect that at registration time all the supported protocols would be
> > > retrieved and cached in the 'struct pcie_doe' context and then the
> > > host driver could query from there without going back to the device
> > > again.  
> >
> > I'm not sure I follow.
> >
> > Any driver will fall into one of the following categories:
> > a) Already knows what protocols are available on a
> >    given DOE instance perhaps because that's a characteristic of the hardware
> >    supported, in which case it has no reason to check (unless driver writer
> >    is paranoid)
> > b) It has no way to know (e.g. class driver), then it makes sense to query
> >    the DOE instance to find out what protocols are available.  
> 
> I was more thinking that the public interface is a protocol rather
> than the raw DOE. So the library knows CDAT, SPDM, IDE... and drivers
> never need to query the interface.
> 
> So this more of a question about where to draw the line of common code.
> 
> For example in the nfit driver there is usage of:
> 
> acpi_label_write()
> 
> ...and:
> 
> acpi_evaluate_dsm()
> 
> ...where the former abstracts the protocol and the latter is the raw
> interface. Both can write to a label area, but only one is idiomatic.

Ah. Got it. Makes sense to have another layer for at least commonly reused
protocols.  Exactly what that looks like is going to be protocol specific.

I've reworked the internal handling to do DOE mailboxes a bit like
pci_alloc_irq_vectors().  Basically if a driver uses a DOE at all it issues
one call to get them all and one call to free them all.  That call
goes and gets the protocols supported and caches their [vid type].

That lets a driver not care which protocols share a DOE and simply use:

struct pcie_doe *cdat_doe = pci_doe_find(pci_dev, vid, prot); etc.
struct pcie_doe *cma_doe = ...
struct pcie_doe *ide_doe;
struct pcie_doe *vendors_own_magic_doe;

Some of which happen to point to the same DOEs.  It's simpler than
reference counting, which we shouldn't need as lifetimes of these
should all be the same.

Jonathan

Re: [RFC PATCH 1/2] PCI/doe: Initial support PCI Data Object Exchange

[Dan Williams]

On Tue, Mar 23, 2021 at 11:25 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
[..]
> > I was thinking something like 'struct pcie_doe_object' pointers rather
> > than u32 arrays.
>
> Possibly... I'm not convinced that it doesn't end up being
> struct pcie_doe_object doe_obj;
> DOE_OBJ_INIT(&doe_obj, vid, type, request_pl, request_pl_sz,
>              response_pl, response_pl_sz)
>
> ret = pcie_doe_exchange(doe, &doe_obj);
>
> If that's the pattern we see, why split it?
> We might well have a struct pcie_doe_object internally but it doesn't
> seem like a sensible external interface to me simply because we'd
> just be filling it in and immediately passing it to a 'send' function.

I don't think there are going to be so many DOE users that would
justify DOE_OBJ_INIT(). I am thinking of a model where the payload
construction is open coded and typesafe similar to how
intel_bus_fwa_activate() [1] builds its mailbox payload.

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/acpi/nfit/intel.c#n546


> > > The disadvantage is that at least some of the specs just have the
> > > header as their first few DW.  So there isn't a clear distinction
> > > between header and payload. May lead to people getting offsets wrong
> > > in a way they wouldn't do if driver was responsible for building the
> > > whole message.
> >
> > Aren't they more likely to get offsets wrong with u32 arrays rather
> > than data structures?
>
> I'm not sure what level you mean this at.  The CDAT patch review you
> followed this with suggested just breaking out vid and type which is
> fine because those are always packed the same and we can do appropriate
> special handling.
>
> If you meant the whole object as packed structure, then it is a whole
> different matter.
>
> Easy one to point is that u64 values are going to end up with their
> top and bottom halves swapped.  Things get even messier if we break
> up below the u32 level.
>
> We can do this at a higher level by having wrappers that deal with
> each protocol and do a serialize / deserialize for the protocol.
> I'm not sure if that will make sense or not yet though.

Again, I think the protocols to support are limited (CDAT and IDE/SPDM
are the only ones on the horizon), so not much value to having a rich
set of wrappers and macros to obfuscate payload generation.

>
> One thing I don't understand is why you proposed a delayed work queue
> above?  I'm not seeing that model in the libsas SMP for example.  As far
> as I can tell that just processes work items asap.
>
> Can you point to a more specific example if you thinks that we should
> use one?

For polling on a timeout a delayed workqueue can poll at an interval
without need for any explicit sleep calls. There are several examples
of queue_delayed_work() in drivers/ being used to advance a state
machine after a protocol relative timeout.

[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.