All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 0/5] PCI Data Object Exchange support + CXL CDAT
@ 2021-05-24 13:39 Jonathan Cameron
  2021-05-24 13:39 ` [PATCH v4 1/5] PCI: Add vendor ID for the PCI SIG Jonathan Cameron
                   ` (5 more replies)
  0 siblings, 6 replies; 17+ messages in thread
From: Jonathan Cameron @ 2021-05-24 13:39 UTC (permalink / raw)
  To: linux-pci, linux-cxl, Dan Williams, Bjorn Helgaas, ira.weiny,
	Lorenzo Pieralisi
  Cc: Ben Widawsky, Chris Browy, linux-acpi, alison.schofield,
	vishal.l.verma, Fangjian, linuxarm, Jonathan Cameron

Series first introduces generic support for DOE mailboxes as defined
in the ECN to the PCIe r5.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 Attribute Table (CDAT) defined in [2]

Finally, in two patches that are not intended for merging, an example
of a generic IOCTL interface to perform synchronous exchanges from user
space in a kernel mediated fashion is introduced. The current consensus
seems in favour of not introducing such an interface, but instead
providing per protocol interfaces where appropriate. As this code was
developed in parallel with that discussion and may be of use to someone it
is included here.

Open questions.
* Do we need to prevent userspace access to the DOE when the kernel is
  managing it (registered by driver)? Discussion in thread:
  https://lore.kernel.org/linux-pci/20210419165451.2176200-1-Jonathan.Cameron@huawei.com/
* Does a generic userspace mediation interface make sense?
  Currently it seems not, but as it was nearly ready, I've included
  it in this patch set anyway.
* Move CXL cdat handling to a separate file, or wait until we can see
  how it is going to be used?
  
Changes since v3
Thanks to Ira, Bjorn and Dan for feedback.
* Addition of a generic IOCTL interface and demo program for discussion.
* Fixed an issue with accidentally disabling interrupts.
* Ensure IRQ_HANDLED always returned as clearing of BUSY can result
  in an interrupt, but there is no means of identifying this.
  The best that can be done is to eat the interrupt.
* Edited comments to more directly tie them to the code the referred to.
* Lock scope documentation update.
* Dropped the CDAT dump to log patch as we are hopefully getting closer
  to this being applied.
* Fixed the references to pcie_ missed when renaming to pci_ in v3.

Set based on cxl/next

All testing conducted against QEMU emulation of a CXL type 3 device
in conjunction with DOE mailbox patches v5 [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/1619454964-10190-1-git-send-email-cbrowy@avery-design.com/

Jonathan Cameron (5):
  PCI: Add vendor ID for the PCI SIG
  PCI/DOE: Add Data Object Exchange support
  cxl/mem: Add CDAT table reading from DOE
  DONOTMERGE PCI/DOE Add per DOE chrdev for ioctl based access
  DONOTMERGE: PCI/DOE: Add userspace example program to tools/pci

 drivers/cxl/Kconfig           |   1 +
 drivers/cxl/cxl.h             |  21 +
 drivers/cxl/mem.c             | 174 ++++++++
 drivers/cxl/mem.h             |   6 +
 drivers/pci/Kconfig           |   8 +
 drivers/pci/Makefile          |   1 +
 drivers/pci/doe.c             | 794 ++++++++++++++++++++++++++++++++++
 drivers/pci/pci-driver.c      |   3 +-
 include/linux/pci-doe.h       | 100 +++++
 include/linux/pci.h           |   3 +
 include/linux/pci_ids.h       |   1 +
 include/uapi/linux/pci_doe.h  |  32 ++
 include/uapi/linux/pci_regs.h |  29 +-
 tools/pci/Build               |   1 +
 tools/pci/Makefile            |   9 +-
 tools/pci/doetest.c           | 131 ++++++
 16 files changed, 1311 insertions(+), 3 deletions(-)
 create mode 100644 drivers/pci/doe.c
 create mode 100644 include/linux/pci-doe.h
 create mode 100644 include/uapi/linux/pci_doe.h
 create mode 100644 tools/pci/doetest.c


base-commit: 35c32e3095d396c750f5cdfdaa94cba83d9b23c6
-- 
2.19.1


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v4 1/5] PCI: Add vendor ID for the PCI SIG
  2021-05-24 13:39 [PATCH v4 0/5] PCI Data Object Exchange support + CXL CDAT Jonathan Cameron
@ 2021-05-24 13:39 ` Jonathan Cameron
  2021-06-10 15:17   ` Dan Williams
  2021-05-24 13:39 ` [PATCH v4 2/5] PCI/DOE: Add Data Object Exchange support Jonathan Cameron
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: Jonathan Cameron @ 2021-05-24 13:39 UTC (permalink / raw)
  To: linux-pci, linux-cxl, Dan Williams, Bjorn Helgaas, ira.weiny,
	Lorenzo Pieralisi
  Cc: Ben Widawsky, Chris Browy, linux-acpi, alison.schofield,
	vishal.l.verma, Fangjian, linuxarm, Jonathan Cameron

This ID is used in DOE headers to identify protocols that are defined
within the PCI Express Base Specification.

Specified in Table 7-x2 of the Data Object Exchange ECN (approved 12 March
2020) available from https://members.pcisig.com/wg/PCI-SIG/document/14143

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 include/linux/pci_ids.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 4c3fa5293d76..dcc8b4b14198 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -149,6 +149,7 @@
 #define PCI_CLASS_OTHERS		0xff
 
 /* Vendors and devices.  Sort key: vendor first, device next. */
+#define PCI_VENDOR_ID_PCI_SIG		0x0001
 
 #define PCI_VENDOR_ID_LOONGSON		0x0014
 
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v4 2/5] PCI/DOE: Add Data Object Exchange support
  2021-05-24 13:39 [PATCH v4 0/5] PCI Data Object Exchange support + CXL CDAT Jonathan Cameron
  2021-05-24 13:39 ` [PATCH v4 1/5] PCI: Add vendor ID for the PCI SIG Jonathan Cameron
@ 2021-05-24 13:39 ` Jonathan Cameron
  2021-06-10 20:06   ` Dan Williams
  2021-05-24 13:39 ` [PATCH v4 3/5] cxl/mem: Add CDAT table reading from DOE Jonathan Cameron
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: Jonathan Cameron @ 2021-05-24 13:39 UTC (permalink / raw)
  To: linux-pci, linux-cxl, Dan Williams, Bjorn Helgaas, ira.weiny,
	Lorenzo Pieralisi
  Cc: Ben Widawsky, Chris Browy, linux-acpi, alison.schofield,
	vishal.l.verma, Fangjian, linuxarm, Jonathan Cameron

Introduced in a PCI ECN [1], DOE provides a config space based mailbox with
standard protocol discovery.  Each mailbox is accessed through a DOE
Extended Capability.

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

If a driver wishes to access any number of DOE instances / protocols it
makes a single call to pci_doe_register_all() which will find available
DOEs, create the required infrastructure and cache the protocols they
support.  pci_doe_find() can then retrieve a pointer to an appropriate DOE
instance.

A synchronous interface is provided in pci_doe_exchange_sync() to perform a
single query / response exchange.

Testing conducted against QEMU using:

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

[1] https://members.pcisig.com/wg/PCI-SIG/document/14143
    Data Object Exchange (DOE) - Approved 12 March 2020

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/pci/Kconfig           |   8 +
 drivers/pci/Makefile          |   1 +
 drivers/pci/doe.c             | 626 ++++++++++++++++++++++++++++++++++
 include/linux/pci-doe.h       |  87 +++++
 include/linux/pci.h           |   3 +
 include/uapi/linux/pci_regs.h |  29 +-
 6 files changed, 753 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index 0c473d75e625..a30c59cf5e27 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -190,6 +190,14 @@ config PCI_HYPERV
 	  The PCI device frontend driver allows the kernel to import arbitrary
 	  PCI devices from a PCI backend to support PCI driver domains.
 
+config PCI_DOE
+	bool
+	help
+	  This enables library support for the PCI Data Object Exchange
+	  capability. DOE provides a simple mailbox in PCI config space that is
+	  used by a number of different protocols.
+	  DOE is defined in the Data Object Exchange ECN to the PCIe r5.0 spec.
+
 choice
 	prompt "PCI Express hierarchy optimization setting"
 	default PCIE_BUS_DEFAULT
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index d62c4ac4ae1b..1b61c1a1c232 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_PCI_STUB)		+= pci-stub.o
 obj-$(CONFIG_PCI_PF_STUB)	+= pci-pf-stub.o
 obj-$(CONFIG_PCI_ECAM)		+= ecam.o
 obj-$(CONFIG_PCI_P2PDMA)	+= p2pdma.o
+obj-$(CONFIG_PCI_DOE)		+= doe.o
 obj-$(CONFIG_XEN_PCIDEV_FRONTEND) += xen-pcifront.o
 
 # Endpoint library must be initialized before its users
diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
new file mode 100644
index 000000000000..27514313ed6a
--- /dev/null
+++ b/drivers/pci/doe.c
@@ -0,0 +1,626 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Data Object Exchange ECN
+ * https://members.pcisig.com/wg/PCI-SIG/document/14143
+ *
+ * Copyright (C) 2021 Huawei
+ *     Jonathan Cameron <Jonathan.Cameron@huawei.com>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/delay.h>
+#include <linux/jiffies.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/pci.h>
+#include <linux/pci-doe.h>
+#include <linux/workqueue.h>
+
+#define PCI_DOE_PROTOCOL_DISCOVERY 0
+
+#define PCI_DOE_BUSY_MAX_RETRIES 16
+#define PCI_DOE_POLL_INTERVAL (HZ / 128)
+
+/* Timeout of 1 second from 6.xx.1 (Operation), ECN - Data Object Exchange */
+#define PCI_DOE_TIMEOUT HZ
+
+static irqreturn_t pci_doe_irq(int irq, void *data)
+{
+	struct pci_doe *doe = data;
+	struct pci_dev *pdev = doe->pdev;
+	u32 val;
+
+	pci_read_config_dword(pdev, doe->cap + PCI_DOE_STATUS, &val);
+	if (FIELD_GET(PCI_DOE_STATUS_INT_STATUS, val)) {
+		pci_write_config_dword(pdev, doe->cap + PCI_DOE_STATUS, val);
+		mod_delayed_work(system_wq, &doe->statemachine, 0);
+		return IRQ_HANDLED;
+	}
+	/* Leave the error case to be handled outside IRQ */
+	if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) {
+		mod_delayed_work(system_wq, &doe->statemachine, 0);
+		return IRQ_HANDLED;
+	}
+
+	/*
+	 * Busy being cleared can result in an interrupt, but as
+	 * the original Busy may not have been detected, there is no
+	 * way to separate such an interrupt from a spurious interrupt.
+	 */
+	return IRQ_HANDLED;
+}
+
+/*
+ * Only call when safe to directly access the DOE, either because no tasks yet
+ * queued, or called from doe_statemachine_work() which has exclusive access to
+ * the DOE config space.
+ */
+static void pci_doe_abort_start(struct pci_doe *doe)
+{
+	struct pci_dev *pdev = doe->pdev;
+	u32 val;
+
+	val = PCI_DOE_CTRL_ABORT;
+	if (doe->irq)
+		val |= PCI_DOE_CTRL_INT_EN;
+	pci_write_config_dword(pdev, doe->cap + PCI_DOE_CTRL, val);
+
+	doe->timeout_jiffies = jiffies + HZ;
+	schedule_delayed_work(&doe->statemachine, HZ);
+}
+
+static int pci_doe_send_req(struct pci_doe *doe, struct pci_doe_exchange *ex)
+{
+	struct pci_dev *pdev = doe->pdev;
+	u32 val;
+	int i;
+
+	/*
+	 * Check the DOE busy bit is not set. If it is set, this could indicate
+	 * someone other than Linux (e.g. firmware) is using the mailbox. Note
+	 * it is expected that firmware and OS will negotiate access rights via
+	 * an, as yet to be defined method.
+	 */
+	pci_read_config_dword(pdev, doe->cap + PCI_DOE_STATUS, &val);
+	if (FIELD_GET(PCI_DOE_STATUS_BUSY, val))
+		return -EBUSY;
+
+	if (FIELD_GET(PCI_DOE_STATUS_ERROR, val))
+		return -EIO;
+
+	/* Write DOE Header */
+	val = FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_VID, ex->vid) |
+		FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_TYPE, ex->protocol);
+	pci_write_config_dword(pdev, doe->cap + PCI_DOE_WRITE, val);
+	/* Length is 2 DW of header + length of payload in DW */
+	pci_write_config_dword(pdev, doe->cap + PCI_DOE_WRITE,
+			       FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH,
+					  2 + ex->request_pl_sz / sizeof(u32)));
+	for (i = 0; i < ex->request_pl_sz / sizeof(u32); i++)
+		pci_write_config_dword(pdev, doe->cap + PCI_DOE_WRITE,
+				       ex->request_pl[i]);
+
+	val = PCI_DOE_CTRL_GO;
+	if (doe->irq)
+		val |= PCI_DOE_CTRL_INT_EN;
+
+	pci_write_config_dword(pdev, doe->cap + PCI_DOE_CTRL, val);
+	/* Request is sent - now wait for poll or IRQ */
+	return 0;
+}
+
+static int pci_doe_recv_resp(struct pci_doe *doe, struct pci_doe_exchange *ex)
+{
+	struct pci_dev *pdev = doe->pdev;
+	size_t length;
+	u32 val;
+	int i;
+
+	/* Read the first dword to get the protocol */
+	pci_read_config_dword(pdev, doe->cap + PCI_DOE_READ, &val);
+	if ((FIELD_GET(PCI_DOE_DATA_OBJECT_HEADER_1_VID, val) != ex->vid) ||
+	    (FIELD_GET(PCI_DOE_DATA_OBJECT_HEADER_1_TYPE, val) != ex->protocol)) {
+		pci_err(pdev,
+			"Expected [VID, Protocol] = [%x, %x], got [%x, %x]\n",
+			ex->vid, ex->protocol,
+			FIELD_GET(PCI_DOE_DATA_OBJECT_HEADER_1_VID, val),
+			FIELD_GET(PCI_DOE_DATA_OBJECT_HEADER_1_TYPE, val));
+		return -EIO;
+	}
+
+	pci_write_config_dword(pdev, doe->cap + PCI_DOE_READ, 0);
+	/* Read the second dword to get the length */
+	pci_read_config_dword(pdev, doe->cap + PCI_DOE_READ, &val);
+	pci_write_config_dword(pdev, doe->cap + PCI_DOE_READ, 0);
+
+	length = FIELD_GET(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH, val);
+	if (length > SZ_1M || length < 2)
+		return -EIO;
+
+	/* First 2 dwords have already been read */
+	length -= 2;
+	/* Read the rest of the response payload */
+	for (i = 0; i < min(length, ex->response_pl_sz / sizeof(u32)); i++) {
+		pci_read_config_dword(pdev, doe->cap + PCI_DOE_READ,
+				      &ex->response_pl[i]);
+		pci_write_config_dword(pdev, doe->cap + PCI_DOE_READ, 0);
+	}
+
+	/* Flush excess length */
+	for (; i < length; i++) {
+		pci_read_config_dword(pdev, doe->cap + PCI_DOE_READ, &val);
+		pci_write_config_dword(pdev, doe->cap + PCI_DOE_READ, 0);
+	}
+	/* Final error check to pick up on any since Data Object Ready */
+	pci_read_config_dword(pdev, doe->cap + PCI_DOE_STATUS, &val);
+	if (FIELD_GET(PCI_DOE_STATUS_ERROR, val))
+		return -EIO;
+
+	return min(length, ex->response_pl_sz / sizeof(u32)) * sizeof(u32);
+}
+
+static void pci_doe_task_complete(void *private)
+{
+	complete(private);
+}
+
+/**
+ * struct pci_doe_task - description of a query / response task
+ * @h: Head to add it to the list of outstanding tasks
+ * @ex: The details of the task to be done
+ * @rv: Return value.  Length of received response or error
+ * @cb: Callback for completion of task
+ * @private: Private data passed to callback on completion
+ */
+struct pci_doe_task {
+	struct list_head h;
+	struct pci_doe_exchange *ex;
+	int rv;
+	void (*cb)(void *private);
+	void *private;
+};
+
+/**
+ * pci_doe_exchange_sync() - Send a request, then wait for and receive response
+ * @doe: DOE mailbox state structure
+ * @ex: Description of the buffers and Vendor ID + type used in this
+ *      request/response pair
+ *
+ * Excess data will be discarded.
+ *
+ * RETURNS: payload in bytes on success, < 0 on error
+ */
+int pci_doe_exchange_sync(struct pci_doe *doe, struct pci_doe_exchange *ex)
+{
+	struct pci_doe_task task;
+	DECLARE_COMPLETION_ONSTACK(c);
+	int only_task;
+
+	/* DOE requests must be a whole number of DW */
+	if (ex->request_pl_sz % sizeof(u32))
+		return -EINVAL;
+
+	task.ex = ex;
+	task.cb = pci_doe_task_complete;
+	task.private = &c;
+
+	mutex_lock(&doe->tasks_lock);
+	if (doe->dead) {
+		mutex_unlock(&doe->tasks_lock);
+		return -EIO;
+	}
+	only_task = list_empty(&doe->tasks);
+	list_add_tail(&task.h, &doe->tasks);
+	if (only_task)
+		schedule_delayed_work(&doe->statemachine, 0);
+	mutex_unlock(&doe->tasks_lock);
+	wait_for_completion(&c);
+
+	return task.rv;
+}
+EXPORT_SYMBOL_GPL(pci_doe_exchange_sync);
+
+static void doe_statemachine_work(struct work_struct *work)
+{
+	struct delayed_work *w = to_delayed_work(work);
+	struct pci_doe *doe = container_of(w, struct pci_doe, statemachine);
+	struct pci_dev *pdev = doe->pdev;
+	struct pci_doe_task *task;
+	bool abort;
+	u32 val;
+	int rc;
+
+	mutex_lock(&doe->tasks_lock);
+	task = list_first_entry_or_null(&doe->tasks, struct pci_doe_task, h);
+	abort = doe->abort;
+	doe->abort = false;
+	mutex_unlock(&doe->tasks_lock);
+
+	if (abort) {
+		/*
+		 * Currently only used during init - care needed if we want to
+		 * generally expose pci_doe_abort() as it would impact queries
+		 * in flight.
+		 */
+		WARN_ON(task);
+		doe->state = DOE_WAIT_ABORT;
+		pci_doe_abort_start(doe);
+		return;
+	}
+
+	switch (doe->state) {
+	case DOE_IDLE:
+		if (task == NULL)
+			return;
+
+		/* Nothing currently in flight so queue a task */
+		rc = pci_doe_send_req(doe, task->ex);
+		/*
+		 * The specification does not provide any guidance on how long
+		 * some other entity could keep the DOE busy, so try for 1
+		 * second then fail. Busy handling is best effort only, because
+		 * there is no way of avoiding racing against another user of
+		 * the DOE.
+		 */
+		if (rc == -EBUSY) {
+			doe->busy_retries++;
+			if (doe->busy_retries == PCI_DOE_BUSY_MAX_RETRIES) {
+				/* Long enough, fail this request */
+				pci_WARN(pdev, true, "DOE busy for too long\n");
+				doe->busy_retries = 0;
+				goto err_busy;
+			}
+			schedule_delayed_work(w, HZ / PCI_DOE_BUSY_MAX_RETRIES);
+			return;
+		}
+		if (rc)
+			goto err_abort;
+		doe->busy_retries = 0;
+
+		doe->state = DOE_WAIT_RESP;
+		doe->timeout_jiffies = jiffies + HZ;
+		/* Now poll or wait for IRQ with timeout */
+		if (doe->irq > 0)
+			schedule_delayed_work(w, PCI_DOE_TIMEOUT);
+		else
+			schedule_delayed_work(w, PCI_DOE_POLL_INTERVAL);
+		return;
+
+	case DOE_WAIT_RESP:
+		/* Not possible to get here with NULL task */
+		pci_read_config_dword(pdev, doe->cap + PCI_DOE_STATUS, &val);
+		if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) {
+			rc = -EIO;
+			goto err_abort;
+		}
+
+		if (!FIELD_GET(PCI_DOE_STATUS_DATA_OBJECT_READY, val)) {
+			/* If not yet at timeout reschedule otherwise abort */
+			if (time_after(jiffies, doe->timeout_jiffies)) {
+				rc = -ETIMEDOUT;
+				goto err_abort;
+			}
+			schedule_delayed_work(w, PCI_DOE_POLL_INTERVAL);
+			return;
+		}
+
+		rc  = pci_doe_recv_resp(doe, task->ex);
+		if (rc < 0)
+			goto err_abort;
+
+		doe->state = DOE_IDLE;
+
+		mutex_lock(&doe->tasks_lock);
+		list_del(&task->h);
+		if (!list_empty(&doe->tasks))
+			schedule_delayed_work(w, 0);
+		mutex_unlock(&doe->tasks_lock);
+
+		/* Set the return value to the length of received payload */
+		task->rv = rc;
+		task->cb(task->private);
+		return;
+
+	case DOE_WAIT_ABORT:
+	case DOE_WAIT_ABORT_ON_ERR:
+		pci_read_config_dword(pdev, doe->cap + PCI_DOE_STATUS, &val);
+
+		if (!FIELD_GET(PCI_DOE_STATUS_ERROR, val) &&
+		    !FIELD_GET(PCI_DOE_STATUS_BUSY, val)) {
+			/* Back to normal state - carry on */
+			mutex_lock(&doe->tasks_lock);
+			if (!list_empty(&doe->tasks))
+				schedule_delayed_work(w, 0);
+			mutex_unlock(&doe->tasks_lock);
+
+			/*
+			 * For deliberately triggered abort, someone is
+			 * waiting.
+			 */
+			if (doe->state == DOE_WAIT_ABORT)
+				complete(&doe->abort_c);
+			doe->state = DOE_IDLE;
+
+			return;
+		}
+		if (time_after(jiffies, doe->timeout_jiffies)) {
+			struct pci_doe_task *t, *n;
+
+			/* We are dead - abort all queued tasks */
+			pci_err(pdev, "DOE ABORT timed out\n");
+			mutex_lock(&doe->tasks_lock);
+			doe->dead = true;
+			list_for_each_entry_safe(t, n, &doe->tasks, h) {
+				t->rv = -EIO;
+				t->cb(t->private);
+				list_del(&t->h);
+			}
+
+			mutex_unlock(&doe->tasks_lock);
+			if (doe->state == DOE_WAIT_ABORT)
+				complete(&doe->abort_c);
+		}
+		return;
+	}
+
+err_abort:
+	doe->state = DOE_WAIT_ABORT_ON_ERR;
+	pci_doe_abort_start(doe);
+err_busy:
+	mutex_lock(&doe->tasks_lock);
+	list_del(&task->h);
+	mutex_unlock(&doe->tasks_lock);
+
+	task->rv = rc;
+	task->cb(task->private);
+	/*
+	 * If we got here via err_busy, and the queue isn't empty then we need
+	 * to go again.
+	 */
+	if (doe->state == DOE_IDLE) {
+		mutex_lock(&doe->tasks_lock);
+		if (!list_empty(&doe->tasks))
+			schedule_delayed_work(w, 0);
+		mutex_unlock(&doe->tasks_lock);
+	}
+}
+
+static int pci_doe_discovery(struct pci_doe *doe, u8 *index, u16 *vid,
+			     u8 *protocol)
+{
+	u32 request_pl = FIELD_PREP(PCI_DOE_DATA_OBJECT_DISC_REQ_3_INDEX, *index);
+	u32 response_pl;
+	struct pci_doe_exchange ex = {
+		.vid = PCI_VENDOR_ID_PCI_SIG,
+		.protocol = PCI_DOE_PROTOCOL_DISCOVERY,
+		.request_pl = &request_pl,
+		.request_pl_sz = sizeof(request_pl),
+		.response_pl = &response_pl,
+		.response_pl_sz = sizeof(response_pl),
+	};
+	int ret;
+
+	ret = pci_doe_exchange_sync(doe, &ex);
+	if (ret < 0)
+		return ret;
+
+	if (ret != sizeof(response_pl))
+		return -EIO;
+
+	*vid = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_VID, response_pl);
+	*protocol = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_PROTOCOL, response_pl);
+	*index = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_NEXT_INDEX, response_pl);
+
+	return 0;
+}
+
+static int pci_doe_cache_protocols(struct pci_doe *doe)
+{
+	u8 index = 0;
+	int rc;
+
+	/* Discovery protocol must always be supported and must report itself */
+	doe->num_prots = 1;
+	doe->prots = kzalloc(sizeof(*doe->prots) * doe->num_prots, GFP_KERNEL);
+	if (doe->prots == NULL)
+		return -ENOMEM;
+
+	do {
+		struct pci_doe_prot *prot, *prot_new;
+
+		prot = &doe->prots[doe->num_prots - 1];
+		rc = pci_doe_discovery(doe, &index, &prot->vid, &prot->type);
+		if (rc)
+			goto err_free_prots;
+
+		if (index) {
+			prot_new = krealloc(doe->prots,
+					    sizeof(*doe->prots) * doe->num_prots,
+					    GFP_KERNEL);
+			if (prot_new == NULL) {
+				rc = -ENOMEM;
+				goto err_free_prots;
+			}
+			doe->prots = prot_new;
+			doe->num_prots++;
+		}
+	} while (index);
+
+	return 0;
+
+err_free_prots:
+	kfree(doe->prots);
+	return rc;
+}
+
+static void pci_doe_init(struct pci_doe *doe, struct pci_dev *pdev,
+			 int doe_offset)
+{
+	mutex_init(&doe->tasks_lock);
+	init_completion(&doe->abort_c);
+	doe->cap = doe_offset;
+	doe->pdev = pdev;
+	INIT_LIST_HEAD(&doe->tasks);
+	INIT_DELAYED_WORK(&doe->statemachine, doe_statemachine_work);
+}
+
+static int pci_doe_abort(struct pci_doe *doe)
+{
+	reinit_completion(&doe->abort_c);
+	mutex_lock(&doe->tasks_lock);
+	doe->abort = true;
+	mutex_unlock(&doe->tasks_lock);
+	schedule_delayed_work(&doe->statemachine, 0);
+	wait_for_completion(&doe->abort_c);
+
+	if (doe->dead)
+		return -EIO;
+
+	return 0;
+}
+
+static int pci_doe_register(struct pci_doe *doe)
+{
+	struct pci_dev *pdev = doe->pdev;
+	bool poll = !pci_dev_msi_enabled(pdev);
+	int rc, irq;
+	u32 val;
+
+	pci_read_config_dword(pdev, doe->cap + PCI_DOE_CAP, &val);
+
+	if (!poll && FIELD_GET(PCI_DOE_CAP_INT, val)) {
+		irq = pci_irq_vector(pdev, FIELD_GET(PCI_DOE_CAP_IRQ, val));
+		if (irq < 0)
+			return irq;
+
+		doe->irq_name = kasprintf(GFP_KERNEL, "DOE[%s]_%x",
+					  dev_name(&pdev->dev), doe->cap);
+		if (!doe->irq_name)
+			return -ENOMEM;
+
+		rc = request_irq(irq, pci_doe_irq, 0, doe->irq_name, doe);
+		if (rc)
+			goto err_free_name;
+
+		doe->irq = irq;
+		pci_write_config_dword(pdev, doe->cap + PCI_DOE_CTRL,
+				       PCI_DOE_CTRL_INT_EN);
+	}
+
+	/* Reset the mailbox by issuing an abort */
+	rc = pci_doe_abort(doe);
+	if (rc)
+		goto err_free_irqs;
+
+	return 0;
+
+err_free_irqs:
+	if (doe->irq > 0)
+		free_irq(doe->irq, doe);
+err_free_name:
+	kfree(doe->irq_name);
+
+	return rc;
+}
+
+static void pci_doe_unregister(struct pci_doe *doe)
+{
+	if (doe->irq > 0)
+		free_irq(doe->irq, doe);
+	kfree(doe->irq_name);
+}
+
+void pci_doe_unregister_all(struct pci_dev *pdev)
+{
+	struct pci_doe *doe, *next;
+
+	list_for_each_entry_safe(doe, next, &pdev->doe_list, h) {
+		/* First halt the state machine */
+		cancel_delayed_work_sync(&doe->statemachine);
+		kfree(doe->prots);
+		pci_doe_unregister(doe);
+		kfree(doe);
+	}
+}
+EXPORT_SYMBOL_GPL(pci_doe_unregister_all);
+
+/**
+ * pci_doe_register_all() - Find and register all DOE mailboxes
+ * @pdev: PCI device whose DOE mailboxes we are finding
+ *
+ * Will locate any DOE mailboxes present on the device and cache the protocols
+ * so that pci_doe_find() can be used to retrieve a suitable DOE instance.
+ *
+ * DOE mailboxes are available until pci_doe_unregister_all() is called.
+ *
+ * RETURNS: 0 on success, < 0 on error
+ */
+int pci_doe_register_all(struct pci_dev *pdev)
+{
+	struct pci_doe *doe;
+	int pos = 0;
+	int rc;
+
+	INIT_LIST_HEAD(&pdev->doe_list);
+
+	/* Walk the DOE extended capabilities and add to per pci_dev list */
+	while (true) {
+		pos = pci_find_next_ext_capability(pdev, pos,
+						   PCI_EXT_CAP_ID_DOE);
+		if (!pos)
+			return 0;
+
+		doe = kzalloc(sizeof(*doe), GFP_KERNEL);
+		if (!doe) {
+			rc = -ENOMEM;
+			goto err_free_does;
+		}
+
+		pci_doe_init(doe, pdev, pos);
+		rc = pci_doe_register(doe);
+		if (rc) {
+			kfree(doe);
+			goto err_free_does;
+		}
+
+		rc = pci_doe_cache_protocols(doe);
+		if (rc) {
+			pci_doe_unregister(doe);
+			kfree(doe);
+			goto err_free_does;
+		}
+
+		list_add(&doe->h, &pdev->doe_list);
+	}
+
+err_free_does:
+	pci_doe_unregister_all(pdev);
+
+	return rc;
+}
+EXPORT_SYMBOL_GPL(pci_doe_register_all);
+
+/**
+ * pci_doe_find() - Find the first DOE instance that supports a given protocol
+ * @pdev: Device on which to find the DOE instance
+ * @vid: Vendor ID
+ * @type: Specific protocol for this vendor
+ *
+ * RETURNS: Pointer to DOE instance on success, NULL on no suitable instance
+ * available
+ */
+struct pci_doe *pci_doe_find(struct pci_dev *pdev, u16 vid, u8 type)
+{
+	struct pci_doe *doe;
+	int i;
+
+	list_for_each_entry(doe, &pdev->doe_list, h) {
+		for (i = 0; i < doe->num_prots; i++)
+			if ((doe->prots[i].vid == vid) &&
+			    (doe->prots[i].type == type))
+				return doe;
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(pci_doe_find);
diff --git a/include/linux/pci-doe.h b/include/linux/pci-doe.h
new file mode 100644
index 000000000000..b2624e505458
--- /dev/null
+++ b/include/linux/pci-doe.h
@@ -0,0 +1,87 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Data Object Exchange was added as an ECN to the PCIe r5.0 spec.
+ *
+ * Copyright (C) 2021 Huawei
+ *     Jonathan Cameron <Jonathan.Cameron@huawei.com>
+ */
+
+#include <linux/completion.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+
+#ifndef LINUX_PCI_DOE_H
+#define LINUX_PCI_DOE_H
+
+struct pci_doe_prot {
+	u16 vid;
+	u8 type;
+};
+
+struct workqueue_struct;
+
+enum pci_doe_state {
+	DOE_IDLE,
+	DOE_WAIT_RESP,
+	DOE_WAIT_ABORT,
+	DOE_WAIT_ABORT_ON_ERR,
+};
+
+struct pci_doe_exchange {
+	u16 vid;
+	u8 protocol;
+	u32 *request_pl;
+	size_t request_pl_sz;
+	u32 *response_pl;
+	size_t response_pl_sz;
+};
+
+/**
+ * struct pci_doe - State to support use of DOE mailbox
+ * @cap: Config space offset to base of DOE capability
+ * @pdev: PCI device that hosts this DOE
+ * @abort_c: Completion used for initial abort handling
+ * @irq: Interrupt used for signaling DOE ready or abort
+ * @irq_name: Name used to identify the irq for a particular DOE
+ * @prots: Cache of identifiers for protocols supported
+ * @num_prots: Size of prots cache
+ * @h: Used for DOE instance lifetime management
+ * @wq: Workqueue used to handle state machine and polling / timeouts
+ * @tasks: List of task in flight + pending
+ * @tasks_lock: Protect the tasks list and abort state
+ * @statemachine: Work item for the DOE state machine
+ * @state: Current state of this DOE
+ * @timeout_jiffies: 1 second after GO set
+ * @busy_retries: Count of retry attempts
+ * @abort: Request a manual abort (e.g. on init)
+ * @dead: Used to mark a DOE for which an ABORT has timed out. Further messages
+ *        will immediately be aborted with error
+ */
+struct pci_doe {
+	int cap;
+	struct pci_dev *pdev;
+	struct completion abort_c;
+	int irq;
+	char *irq_name;
+	struct pci_doe_prot *prots;
+	int num_prots;
+	struct list_head h;
+
+	struct workqueue_struct *wq;
+	struct list_head tasks;
+	struct mutex tasks_lock;
+	struct delayed_work statemachine;
+	enum pci_doe_state state;
+	unsigned long timeout_jiffies;
+	unsigned int busy_retries;
+	unsigned int abort:1;
+	unsigned int dead:1;
+};
+
+int pci_doe_register_all(struct pci_dev *pdev);
+void pci_doe_unregister_all(struct pci_dev *pdev);
+struct pci_doe *pci_doe_find(struct pci_dev *pdev, u16 vid, u8 type);
+
+int pci_doe_exchange_sync(struct pci_doe *doe, struct pci_doe_exchange *ex);
+
+#endif
diff --git a/include/linux/pci.h b/include/linux/pci.h
index c20211e59a57..2250a00ad8c2 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -332,6 +332,9 @@ struct pci_dev {
 #ifdef CONFIG_PCIEPORTBUS
 	struct rcec_ea	*rcec_ea;	/* RCEC cached endpoint association */
 	struct pci_dev  *rcec;          /* Associated RCEC device */
+#endif
+#ifdef CONFIG_PCI_DOE
+	struct list_head doe_list;	/* Data Object Exchange mailboxes */
 #endif
 	u8		pcie_cap;	/* PCIe capability offset */
 	u8		msi_cap;	/* MSI capability offset */
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index e709ae8235e7..b97df1d8bd19 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


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v4 3/5] cxl/mem: Add CDAT table reading from DOE
  2021-05-24 13:39 [PATCH v4 0/5] PCI Data Object Exchange support + CXL CDAT Jonathan Cameron
  2021-05-24 13:39 ` [PATCH v4 1/5] PCI: Add vendor ID for the PCI SIG Jonathan Cameron
  2021-05-24 13:39 ` [PATCH v4 2/5] PCI/DOE: Add Data Object Exchange support Jonathan Cameron
@ 2021-05-24 13:39 ` Jonathan Cameron
  2021-06-10 21:46   ` Dan Williams
  2021-05-24 13:39 ` [PATCH v4 4/5] DONOTMERGE: PCI/DOE: Add per DOE chrdev for ioctl based access Jonathan Cameron
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: Jonathan Cameron @ 2021-05-24 13:39 UTC (permalink / raw)
  To: linux-pci, linux-cxl, Dan Williams, Bjorn Helgaas, ira.weiny,
	Lorenzo Pieralisi
  Cc: Ben Widawsky, Chris Browy, linux-acpi, alison.schofield,
	vishal.l.verma, Fangjian, linuxarm, Jonathan Cameron

This patch provides a sysfs binary attribute to allow dumping of the whole
table.

Binary dumping is modeled 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.

This does not support table updates at runtime. It will always provide
whatever was there when first cached. Handling of table updates can be
implemented later.

Once we have more users, this code can move out to driver/cxl/cdat.c or
similar.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/cxl/Kconfig |   1 +
 drivers/cxl/cxl.h   |  21 ++++++
 drivers/cxl/mem.c   | 174 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/cxl/mem.h   |   6 ++
 4 files changed, 202 insertions(+)

diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
index 97dc4d751651..26cad9fa29f7 100644
--- a/drivers/cxl/Kconfig
+++ b/drivers/cxl/Kconfig
@@ -15,6 +15,7 @@ if CXL_BUS
 
 config CXL_MEM
 	tristate "CXL.mem: Memory Devices"
+	select PCI_DOE
 	help
 	  The CXL.mem protocol allows a device to act as a provider of
 	  "System RAM" and/or "Persistent Memory" that is fully coherent
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index d49e0cb679fa..e649a286aace 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/pci-doe.h>
 
 /* CXL 2.0 8.2.8.1 Device Capabilities Array Register */
 #define CXLDEV_CAP_ARRAY_OFFSET 0x0
@@ -69,5 +70,25 @@ struct cxl_regs {
 void cxl_setup_device_regs(struct device *dev, void __iomem *base,
 			   struct cxl_device_regs *regs);
 
+/*
+ * Address space properties derived from:
+ * CXL 2.0 8.2.5.12.7 CXL HDM Decoder 0 Control Register
+ */
+#define CXL_ADDRSPACE_RAM   BIT(0)
+#define CXL_ADDRSPACE_PMEM  BIT(1)
+#define CXL_ADDRSPACE_TYPE2 BIT(2)
+#define CXL_ADDRSPACE_TYPE3 BIT(3)
+#define CXL_ADDRSPACE_MASK  GENMASK(3, 0)
+
+#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)
+
 extern struct bus_type cxl_bus_type;
 #endif /* __CXL_H__ */
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index c5fdf2c57181..4224d1de311e 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -14,6 +14,7 @@
 #include "pci.h"
 #include "cxl.h"
 #include "mem.h"
+#include "cdat.h"
 
 /**
  * DOC: cxl mem
@@ -926,6 +927,85 @@ static int cxl_mem_setup_mailbox(struct cxl_mem *cxlm)
 	return 0;
 }
 
+#define CDAT_DOE_REQ(entry_handle)					\
+	(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 pci_doe *doe)
+{
+	u32 cdat_request_pl = CDAT_DOE_REQ(0);
+	u32 cdat_response_pl[32];
+	struct pci_doe_exchange ex = {
+		.vid = PCI_DVSEC_VENDOR_ID_CXL,
+		.protocol = CXL_DOE_PROTOCOL_TABLE_ACCESS,
+		.request_pl = &cdat_request_pl,
+		.request_pl_sz = sizeof(cdat_request_pl),
+		.response_pl = cdat_response_pl,
+		.response_pl_sz = sizeof(cdat_response_pl),
+	};
+
+	ssize_t rc;
+
+	rc = pci_doe_exchange_sync(doe, &ex);
+	if (rc < 0)
+		return rc;
+	if (rc < 1)
+		return -EIO;
+
+	return cdat_response_pl[1];
+}
+
+static int cdat_to_buffer(struct pci_doe *doe, u32 *buffer, size_t length)
+{
+	int entry_handle = 0;
+	int rc;
+
+	do {
+		u32 cdat_request_pl = CDAT_DOE_REQ(entry_handle);
+		u32 cdat_response_pl[32];
+		struct pci_doe_exchange ex = {
+			.vid = PCI_DVSEC_VENDOR_ID_CXL,
+			.protocol = CXL_DOE_PROTOCOL_TABLE_ACCESS,
+			.request_pl = &cdat_request_pl,
+			.request_pl_sz = sizeof(cdat_request_pl),
+			.response_pl = cdat_response_pl,
+			.response_pl_sz = sizeof(cdat_response_pl),
+		};
+		size_t entry_dw;
+		u32 *entry;
+
+		rc = pci_doe_exchange_sync(doe, &ex);
+		if (rc < 0)
+			return rc;
+
+		entry = cdat_response_pl + 1;
+		entry_dw = rc / sizeof(u32);
+		/* Skip Header */
+		entry_dw -= 1;
+		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_pl[0]);
+
+	} while (entry_handle != 0xFFFF);
+
+	return 0;
+}
+
+static void cxl_mem_free_irq_vectors(void *data)
+{
+	pci_free_irq_vectors(data);
+}
+
+static void cxl_mem_doe_unregister_all(void *data)
+{
+	pci_doe_unregister_all(data);
+}
+
 static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev, u32 reg_lo,
 				      u32 reg_hi)
 {
@@ -933,6 +1013,7 @@ static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev, u32 reg_lo,
 	struct cxl_mem *cxlm;
 	void __iomem *regs;
 	u64 offset;
+	int irqs;
 	u8 bar;
 	int rc;
 
@@ -971,6 +1052,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");
+	} 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);
+		rc = devm_add_action_or_reset(dev, cxl_mem_free_irq_vectors,
+					       pdev);
+		if (rc)
+			return NULL;
+	}
+
+	/*
+	 * Find a DOE mailbox that supports CDAT.
+	 * Supporting other DOE protocols will require more complexity.
+	 */
+	rc = pci_doe_register_all(pdev);
+	if (rc < 0)
+		return NULL;
+
+	rc = devm_add_action_or_reset(dev, cxl_mem_doe_unregister_all, pdev);
+	if (rc)
+		return NULL;
+
+	cxlm->table_doe = pci_doe_find(pdev, PCI_DVSEC_VENDOR_ID_CXL,
+				       CXL_DOE_PROTOCOL_TABLE_ACCESS);
+
 	dev_dbg(dev, "Mapped CXL Memory Device resource\n");
 	return cxlm;
 }
@@ -1060,6 +1179,31 @@ static ssize_t pmem_size_show(struct device *dev, struct device_attribute *attr,
 	return sysfs_emit(buf, "%#llx\n", len);
 }
 
+static ssize_t CDAT_read(struct file *filp, struct kobject *kobj,
+			 struct bin_attribute *bin_attr, char *buf,
+			 loff_t offset, size_t count)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
+
+	return memory_read_from_buffer(buf, count, &offset, cxlmd->cdat_table,
+				       cxlmd->cdat_length);
+}
+
+static BIN_ATTR_RO(CDAT, 0);
+
+static umode_t cxl_memdev_bin_attr_is_visible(struct kobject *kobj,
+					      struct bin_attribute *attr, int i)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
+
+	if ((attr == &bin_attr_CDAT) && cxlmd->cdat_table)
+		return 0400;
+
+	return 0;
+}
+
 static struct device_attribute dev_attr_pmem_size =
 	__ATTR(size, 0444, pmem_size_show, NULL);
 
@@ -1069,6 +1213,11 @@ static struct attribute *cxl_memdev_attributes[] = {
 	NULL,
 };
 
+static struct bin_attribute *cxl_memdev_bin_attributes[] = {
+	&bin_attr_CDAT,
+	NULL,
+};
+
 static struct attribute *cxl_memdev_pmem_attributes[] = {
 	&dev_attr_pmem_size.attr,
 	NULL,
@@ -1081,6 +1230,8 @@ static struct attribute *cxl_memdev_ram_attributes[] = {
 
 static struct attribute_group cxl_memdev_attribute_group = {
 	.attrs = cxl_memdev_attributes,
+	.bin_attrs = cxl_memdev_bin_attributes,
+	.is_bin_visible = cxl_memdev_bin_attr_is_visible,
 };
 
 static struct attribute_group cxl_memdev_ram_attribute_group = {
@@ -1158,6 +1309,25 @@ static struct cxl_memdev *cxl_memdev_alloc(struct cxl_mem *cxlm)
 	return ERR_PTR(rc);
 }
 
+static int cxl_cache_cdat_table(struct cxl_memdev *cxlmd)
+{
+	struct cxl_mem *cxlm = cxlmd->cxlm;
+	struct device *dev = &cxlmd->dev;
+	ssize_t cdat_length;
+
+	if (cxlm->table_doe == NULL)
+		return 0;
+
+	cdat_length = cdat_get_length(cxlm->table_doe);
+	if (cdat_length < 0)
+		return cdat_length;
+
+	cxlmd->cdat_length = cdat_length;
+	cxlmd->cdat_table = devm_kzalloc(dev->parent, cdat_length, GFP_KERNEL);
+
+	return cdat_to_buffer(cxlm->table_doe, cxlmd->cdat_table, cxlmd->cdat_length);
+}
+
 static int cxl_mem_add_memdev(struct cxl_mem *cxlm)
 {
 	struct cxl_memdev *cxlmd;
@@ -1180,6 +1350,10 @@ static int cxl_mem_add_memdev(struct cxl_mem *cxlm)
 	 */
 	cxlmd->cxlm = cxlm;
 
+	rc = cxl_cache_cdat_table(cxlmd);
+	if (rc)
+		goto err;
+
 	cdev = &cxlmd->cdev;
 	rc = cdev_device_add(cdev, dev);
 	if (rc)
diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h
index 0a3f70316872..fb26155a8fb3 100644
--- a/drivers/cxl/mem.h
+++ b/drivers/cxl/mem.h
@@ -38,12 +38,16 @@
  * @cdev: char dev core object for ioctl operations
  * @cxlm: pointer to the parent device driver data
  * @id: id number of this memdev instance.
+ * @cdat_table: cache of CDAT table
+ * @cdat_length: length of cached CDAT table
  */
 struct cxl_memdev {
 	struct device dev;
 	struct cdev cdev;
 	struct cxl_mem *cxlm;
 	int id;
+	void *cdat_table;
+	size_t cdat_length;
 };
 
 /**
@@ -51,6 +55,7 @@ struct cxl_memdev {
  * @pdev: The PCI device associated with this CXL device.
  * @base: IO mappings to the device's MMIO
  * @cxlmd: Logical memory device chardev / interface
+ * @table_doe: Data exchange object mailbox used to read tables
  * @regs: Parsed register blocks
  * @payload_size: Size of space for payload
  *                (CXL 2.0 8.2.8.4.3 Mailbox Capabilities Register)
@@ -65,6 +70,7 @@ struct cxl_mem {
 	void __iomem *base;
 	struct cxl_memdev *cxlmd;
 
+	struct pci_doe *table_doe;
 	struct cxl_regs regs;
 
 	size_t payload_size;
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v4 4/5] DONOTMERGE: PCI/DOE: Add per DOE chrdev for ioctl based access
  2021-05-24 13:39 [PATCH v4 0/5] PCI Data Object Exchange support + CXL CDAT Jonathan Cameron
                   ` (2 preceding siblings ...)
  2021-05-24 13:39 ` [PATCH v4 3/5] cxl/mem: Add CDAT table reading from DOE Jonathan Cameron
@ 2021-05-24 13:39 ` Jonathan Cameron
  2021-05-25 10:26     ` kernel test robot
  2021-05-24 13:39 ` [PATCH v4 5/5] DONOTMERGE: PCI/DOE: Add userspace example program to tools/pci Jonathan Cameron
  2021-06-10 14:30 ` [PATCH v4 0/5] PCI Data Object Exchange support + CXL CDAT Jonathan Cameron
  5 siblings, 1 reply; 17+ messages in thread
From: Jonathan Cameron @ 2021-05-24 13:39 UTC (permalink / raw)
  To: linux-pci, linux-cxl, Dan Williams, Bjorn Helgaas, ira.weiny,
	Lorenzo Pieralisi
  Cc: Ben Widawsky, Chris Browy, linux-acpi, alison.schofield,
	vishal.l.verma, Fangjian, linuxarm, Jonathan Cameron

It is not safe to access DOE mailboxes directly from userspace at the same
time as the kernel may be accessing them. An implementation note in the
ECN suggest use of a lock for this purpose, but in general, mediation is
needed.  Here we provide that mediation by providing a simple IOCTL
interface allowing userspace to issue requests to the DOE and in a
synchronous fashion receive the response.

There is no sanity checking of the messages sent so this is not an
appropriate interface to expose to userspace, but may be of use to others.

Current discussions suggstion that we will instead implement protocol
specific interfaces where needed.  The first of these is the CDAT
interface.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/pci/doe.c            | 188 +++++++++++++++++++++++++++++++++--
 drivers/pci/pci-driver.c     |   3 +-
 include/linux/pci-doe.h      |  13 +++
 include/uapi/linux/pci_doe.h |  32 ++++++
 4 files changed, 225 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
index 27514313ed6a..2d20f59e42c6 100644
--- a/drivers/pci/doe.c
+++ b/drivers/pci/doe.c
@@ -15,6 +15,10 @@
 #include <linux/pci.h>
 #include <linux/pci-doe.h>
 #include <linux/workqueue.h>
+#include <uapi/linux/pci_doe.h>
+
+/* Maximum number of DOE instances in the system */
+#define PCI_DOE_MAX_CNT 65536
 
 #define PCI_DOE_PROTOCOL_DISCOVERY 0
 
@@ -24,6 +28,10 @@
 /* Timeout of 1 second from 6.xx.1 (Operation), ECN - Data Object Exchange */
 #define PCI_DOE_TIMEOUT HZ
 
+static int pci_doe_major;
+static DEFINE_IDA(pci_doe_ida);
+static DECLARE_RWSEM(pci_doe_rwsem);
+
 static irqreturn_t pci_doe_irq(int irq, void *data)
 {
 	struct pci_doe *doe = data;
@@ -479,6 +487,126 @@ static int pci_doe_abort(struct pci_doe *doe)
 	return 0;
 }
 
+static void pci_doe_release(struct device *dev)
+{
+	struct pci_doe *doe = container_of(dev, struct pci_doe, dev);
+
+	ida_free(&pci_doe_ida, MINOR(doe->dev.devt));
+	kfree(doe);
+}
+
+static char *pci_doe_devnode(struct device *dev, umode_t *mode, kuid_t *uid,
+			     kgid_t *gid)
+{
+	return kasprintf(GFP_KERNEL, "pcidoe/%s", dev_name(dev));
+}
+
+static const struct device_type pci_doe_type = {
+	.name = "pci_doe",
+	.release = pci_doe_release,
+	.devnode = pci_doe_devnode,
+};
+
+static long __pci_doe_ioctl(struct pci_doe *doe, unsigned int cmd,
+			    unsigned long arg)
+{
+	struct pci_doe_uexchange __user *uex;
+	struct pci_doe_uexchange ex;
+	struct pci_doe_exchange exchange;
+	u32 *request_pl;
+	u32 *response_pl;
+	int ret;
+
+	if (cmd != PCI_DOE_EXCHANGE)
+		return -ENOTTY;
+
+	uex = (void __user *)arg;
+	if (copy_from_user(&ex, uex, sizeof(ex)))
+		return -EFAULT;
+
+	/* Cap size at something sensible */
+	request_pl = vmemdup_user(u64_to_user_ptr(ex.in.payload), ex.in.size);
+	if (!request_pl)
+		return -ENOMEM;
+
+	response_pl = kvzalloc(ex.out.size, GFP_KERNEL);
+	if (!response_pl) {
+		ret = -ENOMEM;
+		goto free_request;
+	}
+
+	exchange.vid = ex.vid;
+	exchange.protocol = ex.protocol;
+	exchange.request_pl = request_pl;
+	exchange.request_pl_sz = ex.in.size;
+	exchange.response_pl = response_pl;
+	exchange.response_pl_sz = ex.out.size;
+	ret = pci_doe_exchange_sync(doe, &exchange);
+	if (ret < 0)
+		goto free_response;
+	ret = 0;
+
+	if (copy_to_user(u64_to_user_ptr(ex.out.payload), response_pl, ex.out.size)) {
+		ret = -EFAULT;
+		goto free_response;
+	}
+
+	/* No useful value to return currently */
+	ex.retval = 0;
+	if (copy_to_user(uex, &ex, sizeof(ex))) {
+		ret = -EFAULT;
+		goto free_response;
+	}
+
+free_response:
+	kvfree(response_pl);
+free_request:
+	kvfree(request_pl);
+
+	return ret;
+}
+
+static long pci_doe_ioctl(struct file *file, unsigned int cmd,
+			    unsigned long arg)
+{
+	struct pci_doe *doe = file->private_data;
+	int rc = -ENXIO;
+
+	down_read(&pci_doe_rwsem);
+	if (!doe->going_down)
+		rc = __pci_doe_ioctl(doe, cmd, arg);
+	up_read(&pci_doe_rwsem);
+
+	return rc;
+}
+
+static int pci_doe_open(struct inode *inode, struct file *file)
+{
+	struct pci_doe *doe = container_of(inode->i_cdev, typeof(*doe), cdev);
+
+	get_device(&doe->dev);
+	file->private_data = doe;
+
+	return 0;
+}
+
+static int pci_doe_file_release(struct inode *inode, struct file *file)
+{
+	struct pci_doe *doe = container_of(inode->i_cdev, typeof(*doe), cdev);
+
+	put_device(&doe->dev);
+
+	return 0;
+}
+static const struct file_operations pci_doe_fops = {
+	.owner = THIS_MODULE,
+	.unlocked_ioctl = pci_doe_ioctl,
+	.open = pci_doe_open,
+	.release = pci_doe_file_release,
+	.compat_ioctl = compat_ptr_ioctl,
+	.llseek = noop_llseek,
+};
+
 static int pci_doe_register(struct pci_doe *doe)
 {
 	struct pci_dev *pdev = doe->pdev;
@@ -486,17 +614,35 @@ static int pci_doe_register(struct pci_doe *doe)
 	int rc, irq;
 	u32 val;
 
+	rc = ida_alloc_range(&pci_doe_ida, 0, PCI_DOE_MAX_CNT - 1, GFP_KERNEL);
+	if (rc < 0)
+		return rc;
+
+	device_initialize(&doe->dev);
+	doe->dev.parent = &pdev->dev;
+	doe->dev.devt = MKDEV(pci_doe_major, rc);
+	doe->dev.type = &pci_doe_type;
+	device_set_pm_not_required(&doe->dev);
+	rc = dev_set_name(&doe->dev, "doe[%s]_%x", dev_name(&pdev->dev), doe->cap);
+	if (rc)
+		goto err_put_device;
+
+	cdev_init(&doe->cdev, &pci_doe_fops);
+
 	pci_read_config_dword(pdev, doe->cap + PCI_DOE_CAP, &val);
 
 	if (!poll && FIELD_GET(PCI_DOE_CAP_INT, val)) {
-		irq = pci_irq_vector(pdev, FIELD_GET(PCI_DOE_CAP_IRQ, val));
-		if (irq < 0)
-			return irq;
+		rc = pci_irq_vector(pdev, FIELD_GET(PCI_DOE_CAP_IRQ, val));
+		if (rc < 0)
+			goto err_put_device;
+		irq = rc;
 
 		doe->irq_name = kasprintf(GFP_KERNEL, "DOE[%s]_%x",
 					  dev_name(&pdev->dev), doe->cap);
-		if (!doe->irq_name)
-			return -ENOMEM;
+		if (!doe->irq_name) {
+			rc = -ENOMEM;
+			goto err_put_device;
+		}
 
 		rc = request_irq(irq, pci_doe_irq, 0, doe->irq_name, doe);
 		if (rc)
@@ -512,6 +658,10 @@ static int pci_doe_register(struct pci_doe *doe)
 	if (rc)
 		goto err_free_irqs;
 
+	rc = cdev_device_add(&doe->cdev, &doe->dev);
+	if (rc)
+		goto err_free_irqs;
+
 	return 0;
 
 err_free_irqs:
@@ -519,15 +669,22 @@ static int pci_doe_register(struct pci_doe *doe)
 		free_irq(doe->irq, doe);
 err_free_name:
 	kfree(doe->irq_name);
+err_put_device:
+	put_device(&doe->dev);
 
 	return rc;
 }
 
 static void pci_doe_unregister(struct pci_doe *doe)
 {
+	cdev_device_del(&doe->cdev, &doe->dev);
+	down_write(&pci_doe_rwsem);
+	doe->going_down = 1;
+	up_write(&pci_doe_rwsem);
 	if (doe->irq > 0)
 		free_irq(doe->irq, doe);
 	kfree(doe->irq_name);
+	put_device(&doe->dev);
 }
 
 void pci_doe_unregister_all(struct pci_dev *pdev)
@@ -539,7 +696,6 @@ void pci_doe_unregister_all(struct pci_dev *pdev)
 		cancel_delayed_work_sync(&doe->statemachine);
 		kfree(doe->prots);
 		pci_doe_unregister(doe);
-		kfree(doe);
 	}
 }
 EXPORT_SYMBOL_GPL(pci_doe_unregister_all);
@@ -559,6 +715,7 @@ int pci_doe_register_all(struct pci_dev *pdev)
 {
 	struct pci_doe *doe;
 	int pos = 0;
+
 	int rc;
 
 	INIT_LIST_HEAD(&pdev->doe_list);
@@ -578,15 +735,12 @@ int pci_doe_register_all(struct pci_dev *pdev)
 
 		pci_doe_init(doe, pdev, pos);
 		rc = pci_doe_register(doe);
-		if (rc) {
-			kfree(doe);
+		if (rc)
 			goto err_free_does;
-		}
 
 		rc = pci_doe_cache_protocols(doe);
 		if (rc) {
 			pci_doe_unregister(doe);
-			kfree(doe);
 			goto err_free_does;
 		}
 
@@ -624,3 +778,17 @@ struct pci_doe *pci_doe_find(struct pci_dev *pdev, u16 vid, u8 type)
 	return NULL;
 }
 EXPORT_SYMBOL_GPL(pci_doe_find);
+
+int pci_doe_sys_init(void)
+{
+	dev_t devt;
+	int rc;
+
+	rc = alloc_chrdev_region(&devt, 0, PCI_DOE_MAX_CNT, "pcidoe");
+	if (rc)
+		return rc;
+	pci_doe_major = MAJOR(devt);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pci_doe_sys_init);
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index ec44a79e951a..e2077a2b866f 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -20,6 +20,7 @@
 #include <linux/of_device.h>
 #include <linux/acpi.h>
 #include <linux/dma-map-ops.h>
+#include <linux/pci-doe.h>
 #include "pci.h"
 #include "pcie/portdrv.h"
 
@@ -1655,6 +1656,6 @@ static int __init pci_driver_init(void)
 		return ret;
 #endif
 	dma_debug_add_bus(&pci_bus_type);
-	return 0;
+	return pci_doe_sys_init();
 }
 postcore_initcall(pci_driver_init);
diff --git a/include/linux/pci-doe.h b/include/linux/pci-doe.h
index b2624e505458..bdc5f15f14ab 100644
--- a/include/linux/pci-doe.h
+++ b/include/linux/pci-doe.h
@@ -6,6 +6,7 @@
  *     Jonathan Cameron <Jonathan.Cameron@huawei.com>
  */
 
+#include <linux/cdev.h>
 #include <linux/completion.h>
 #include <linux/list.h>
 #include <linux/mutex.h>
@@ -56,8 +57,11 @@ struct pci_doe_exchange {
  * @abort: Request a manual abort (e.g. on init)
  * @dead: Used to mark a DOE for which an ABORT has timed out. Further messages
  *        will immediately be aborted with error
+ * @going_down: Mark DOE as removed
  */
 struct pci_doe {
+	struct device dev;
+	struct cdev cdev;
 	int cap;
 	struct pci_dev *pdev;
 	struct completion abort_c;
@@ -76,6 +80,7 @@ struct pci_doe {
 	unsigned int busy_retries;
 	unsigned int abort:1;
 	unsigned int dead:1;
+	unsigned int going_down:1;
 };
 
 int pci_doe_register_all(struct pci_dev *pdev);
@@ -84,4 +89,12 @@ struct pci_doe *pci_doe_find(struct pci_dev *pdev, u16 vid, u8 type);
 
 int pci_doe_exchange_sync(struct pci_doe *doe, struct pci_doe_exchange *ex);
 
+#ifdef CONFIG_PCI_DOE
+int pci_doe_sys_init(void);
+#else
+static inline int pci_doe_sys_init(void)
+{
+	return 0;
+}
+#endif /* CONFIG_PCI_DOE */
 #endif
diff --git a/include/uapi/linux/pci_doe.h b/include/uapi/linux/pci_doe.h
new file mode 100644
index 000000000000..d01a27561df7
--- /dev/null
+++ b/include/uapi/linux/pci_doe.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Userspace interface for safely accessing a PCI Data Exchange Object
+ * mailbox that has been registered by a driver.
+ */
+
+#ifndef LINUX_PCI_DOE_UAPI_H
+#define LINUX_PCI_DOE_UAPI_H
+
+#include <linux/ioctl.h>
+#include <linux/types.h>
+
+struct pci_doe_uexchange {
+	__u16 vid;
+	__u8 protocol;
+	__u8 rsvd;
+	__u32 retval;
+	struct {
+		__s32 size;
+		__u32 rsvd;
+		__u64 payload;
+	} in;
+	struct {
+		__s32 size;
+		__u32 rsvd;
+		__u64 payload;
+	} out;
+};
+
+#define PCI_DOE_EXCHANGE _IOWR(0xDA, 1, struct pci_doe_uexchange)
+
+#endif /* LINUX_PCI_DOE_UAPI_H */
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v4 5/5] DONOTMERGE: PCI/DOE: Add userspace example program to tools/pci
  2021-05-24 13:39 [PATCH v4 0/5] PCI Data Object Exchange support + CXL CDAT Jonathan Cameron
                   ` (3 preceding siblings ...)
  2021-05-24 13:39 ` [PATCH v4 4/5] DONOTMERGE: PCI/DOE: Add per DOE chrdev for ioctl based access Jonathan Cameron
@ 2021-05-24 13:39 ` Jonathan Cameron
  2021-06-10 14:30 ` [PATCH v4 0/5] PCI Data Object Exchange support + CXL CDAT Jonathan Cameron
  5 siblings, 0 replies; 17+ messages in thread
From: Jonathan Cameron @ 2021-05-24 13:39 UTC (permalink / raw)
  To: linux-pci, linux-cxl, Dan Williams, Bjorn Helgaas, ira.weiny,
	Lorenzo Pieralisi
  Cc: Ben Widawsky, Chris Browy, linux-acpi, alison.schofield,
	vishal.l.verma, Fangjian, linuxarm, Jonathan Cameron

The example uses the Discovery Protocol to illustrate the
use of the IOCTL interface to access the DOE mailboxes form
userspace.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 tools/pci/Build     |   1 +
 tools/pci/Makefile  |   9 ++-
 tools/pci/doetest.c | 131 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 140 insertions(+), 1 deletion(-)

diff --git a/tools/pci/Build b/tools/pci/Build
index c375aea21790..af4521bebf93 100644
--- a/tools/pci/Build
+++ b/tools/pci/Build
@@ -1 +1,2 @@
 pcitest-y += pcitest.o
+doetest-y += doetest.o
diff --git a/tools/pci/Makefile b/tools/pci/Makefile
index 4b95a5176355..b2e54afe583c 100644
--- a/tools/pci/Makefile
+++ b/tools/pci/Makefile
@@ -14,7 +14,7 @@ MAKEFLAGS += -r
 
 CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include
 
-ALL_TARGETS := pcitest
+ALL_TARGETS := pcitest doetest
 ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS))
 
 SCRIPTS := pcitest.sh
@@ -30,6 +30,7 @@ include $(srctree)/tools/build/Makefile.include
 $(OUTPUT)include/linux/: ../../include/uapi/linux/
 	mkdir -p $(OUTPUT)include/linux/ 2>&1 || true
 	ln -sf $(CURDIR)/../../include/uapi/linux/pcitest.h $@
+	ln -sf $(CURDIR)/../../include/uapi/linux/pci_doe.h $@
 
 prepare: $(OUTPUT)include/linux/
 
@@ -39,6 +40,12 @@ $(PCITEST_IN): prepare FORCE
 $(OUTPUT)pcitest: $(PCITEST_IN)
 	$(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) $< -o $@
 
+DOETEST_IN := $(OUTPUT)doetest-in.o
+$(DOETEST_IN): prepare FORCE
+	$(Q)$(MAKE) $(build)=doetest
+$(OUTPUT)doetest: $(DOETEST_IN)
+	$(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) $< -o $@
+
 clean:
 	rm -f $(ALL_PROGRAMS)
 	rm -rf $(OUTPUT)include/
diff --git a/tools/pci/doetest.c b/tools/pci/doetest.c
new file mode 100644
index 000000000000..b2db847b1503
--- /dev/null
+++ b/tools/pci/doetest.c
@@ -0,0 +1,131 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Example user of the DOE userspace interface.
+ *
+ * Jonathan Cameron <Jonathan.Cameron@huawei.com>
+ */
+
+#include <unistd.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include <stdint.h>
+#include <fcntl.h>
+#include <linux/types.h>
+#include <stdio.h>
+#include <sys/ioctl.h>
+#include <getopt.h>
+#include <string.h>
+
+struct pci_doe_uexchange {
+	__u16 vid;
+	__u8 protocol;
+	__u8 rsvd;
+	__u32 retval;
+	struct {
+		__s32 size;
+		__u32 rsvd;
+		__u64 payload;
+	} in;
+	struct {
+		__s32 size;
+		__u32 rsvd;
+		__u64 payload;
+	} out;
+};
+
+#define PCI_DOE_EXCHANGE _IOWR(0xDA, 1, struct pci_doe_uexchange)
+
+int doe_list_protocols(int fd)
+{
+	__u32 outbuf = 0;
+	__u32 inbuf; /* Start with index 0 */
+	struct pci_doe_uexchange ex = {
+		.vid = 33,
+		.protocol = 1,
+		.in.size = sizeof(inbuf),
+		.in.payload = (__u64)&inbuf,
+		.out.size = sizeof(outbuf),
+		.out.payload = (__u64)&outbuf,
+		.vid = 0x01, /* PCI SIG */
+		.protocol = 0x00,
+	};
+	int rc;
+	uint8_t index = 0;
+
+	do {
+		inbuf = index;
+		rc = ioctl(fd, PCI_DOE_EXCHANGE, &ex);
+		if (rc) {
+			printf("IOCTL error: %d\n", rc);
+			return rc;
+		}
+		if (ex.retval) {
+			printf("DOE return value indicates failure: %d\n", ex.retval);
+			return ex.retval;
+		}
+		index = outbuf >> 24;
+
+		printf("VID: %#x, Protocol: %#x\n", outbuf & 0xffff, (outbuf >> 16) & 0xff);
+	} while (index);
+
+	return 0;
+}
+
+static const struct option longopts[] = {
+	{ "filename",		1, 0, 'f' },
+	{ }
+};
+
+static void print_usage(void)
+{
+	fprintf(stderr, "Usage: doe [options]...\n"
+		"Example userspace access to a PCI DOE mailbox\n"
+		"  -f <filename>	Path to chardev /dev/pcidoe/...\n"
+		"  -l			List supported protocols\n");
+}
+
+int main(int argc, char **argv)
+{
+	char *filename = NULL;
+	bool run_discovery = false;
+	int fd, c;
+	int rc = 0;
+
+	while ((c = getopt_long(argc, argv, "?f:l", longopts, NULL)) != -1) {
+		switch (c) {
+		case 'f':
+			filename = strdup(optarg);
+			break;
+		case 'l':
+			run_discovery = true;
+			break;
+		case '?':
+			print_usage();
+			goto free_filename;
+		}
+	}
+	if (!filename) {
+		fprintf(stderr, "Filename must be supplied using -f FILENAME\n");
+		rc = -1;
+		/* No need to actually free the filename, but keep exit path simple */
+		goto free_filename;
+	}
+
+	fd = open(filename, 0);
+	if (fd == -1) {
+		fprintf(stderr, "Could not open file %s\n", filename);
+		rc = -1;
+		goto free_filename;
+	}
+	if (run_discovery) {
+		rc = doe_list_protocols(fd);
+		if (rc)
+			goto close_fd;
+	}
+close_fd:
+	close(fd);
+free_filename:
+	free(filename);
+
+	return rc;
+}
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 4/5] DONOTMERGE: PCI/DOE: Add per DOE chrdev for ioctl based access
  2021-05-24 13:39 ` [PATCH v4 4/5] DONOTMERGE: PCI/DOE: Add per DOE chrdev for ioctl based access Jonathan Cameron
@ 2021-05-25 10:26     ` kernel test robot
  0 siblings, 0 replies; 17+ messages in thread
From: kernel test robot @ 2021-05-25 10:26 UTC (permalink / raw)
  To: Jonathan Cameron, linux-pci, linux-cxl, Dan Williams,
	Bjorn Helgaas, ira.weiny, Lorenzo Pieralisi
  Cc: kbuild-all, Ben Widawsky, Chris Browy, linux-acpi, alison.schofield

[-- Attachment #1: Type: text/plain, Size: 1598 bytes --]

Hi Jonathan,

I love your patch! Yet something to improve:

[auto build test ERROR on 35c32e3095d396c750f5cdfdaa94cba83d9b23c6]

url:    https://github.com/0day-ci/linux/commits/Jonathan-Cameron/PCI-Data-Object-Exchange-support-CXL-CDAT/20210524-214403
base:   35c32e3095d396c750f5cdfdaa94cba83d9b23c6
config: x86_64-rhel-8.3-kselftests (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/22a6e8bee6206990d309c9c61941f67868c3a12c
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jonathan-Cameron/PCI-Data-Object-Exchange-support-CXL-CDAT/20210524-214403
        git checkout 22a6e8bee6206990d309c9c61941f67868c3a12c
        # save the attached .config to linux build tree
        make W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> error: include/uapi/linux/pci_doe.h: missing "WITH Linux-syscall-note" for SPDX-License-Identifier
   make[2]: *** [scripts/Makefile.headersinst:63: usr/include/linux/pci_doe.h] Error 1
   make[2]: Target '__headers' not remade because of errors.
   make[1]: *** [Makefile:1333: headers] Error 2
   make[1]: Target 'headers_install' not remade because of errors.
   make: *** [Makefile:215: __sub-make] Error 2
   make: Target 'headers_install' not remade because of errors.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 41780 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 4/5] DONOTMERGE: PCI/DOE: Add per DOE chrdev for ioctl based access
@ 2021-05-25 10:26     ` kernel test robot
  0 siblings, 0 replies; 17+ messages in thread
From: kernel test robot @ 2021-05-25 10:26 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 1634 bytes --]

Hi Jonathan,

I love your patch! Yet something to improve:

[auto build test ERROR on 35c32e3095d396c750f5cdfdaa94cba83d9b23c6]

url:    https://github.com/0day-ci/linux/commits/Jonathan-Cameron/PCI-Data-Object-Exchange-support-CXL-CDAT/20210524-214403
base:   35c32e3095d396c750f5cdfdaa94cba83d9b23c6
config: x86_64-rhel-8.3-kselftests (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/22a6e8bee6206990d309c9c61941f67868c3a12c
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jonathan-Cameron/PCI-Data-Object-Exchange-support-CXL-CDAT/20210524-214403
        git checkout 22a6e8bee6206990d309c9c61941f67868c3a12c
        # save the attached .config to linux build tree
        make W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> error: include/uapi/linux/pci_doe.h: missing "WITH Linux-syscall-note" for SPDX-License-Identifier
   make[2]: *** [scripts/Makefile.headersinst:63: usr/include/linux/pci_doe.h] Error 1
   make[2]: Target '__headers' not remade because of errors.
   make[1]: *** [Makefile:1333: headers] Error 2
   make[1]: Target 'headers_install' not remade because of errors.
   make: *** [Makefile:215: __sub-make] Error 2
   make: Target 'headers_install' not remade because of errors.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 41780 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 0/5] PCI Data Object Exchange support + CXL CDAT
  2021-05-24 13:39 [PATCH v4 0/5] PCI Data Object Exchange support + CXL CDAT Jonathan Cameron
                   ` (4 preceding siblings ...)
  2021-05-24 13:39 ` [PATCH v4 5/5] DONOTMERGE: PCI/DOE: Add userspace example program to tools/pci Jonathan Cameron
@ 2021-06-10 14:30 ` Jonathan Cameron
  5 siblings, 0 replies; 17+ messages in thread
From: Jonathan Cameron @ 2021-06-10 14:30 UTC (permalink / raw)
  To: linux-pci, linux-cxl, Dan Williams, Bjorn Helgaas, ira.weiny,
	Lorenzo Pieralisi
  Cc: Ben Widawsky, Chris Browy, linux-acpi, alison.schofield,
	vishal.l.verma, Fangjian, linuxarm

On Mon, 24 May 2021 21:39:33 +0800
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> Series first introduces generic support for DOE mailboxes as defined
> in the ECN to the PCIe r5.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 Attribute Table (CDAT) defined in [2]
> 
> Finally, in two patches that are not intended for merging, an example
> of a generic IOCTL interface to perform synchronous exchanges from user
> space in a kernel mediated fashion is introduced. The current consensus
> seems in favour of not introducing such an interface, but instead
> providing per protocol interfaces where appropriate. As this code was
> developed in parallel with that discussion and may be of use to someone it
> is included here.
>

Gentle, not particularly urgent, request for review on this set.

I am currently missing ABI docs for the CDAT binary attribute, but
that could be handled in a follow up patch.

Thanks,

Jonathan
 
> Open questions.
> * Do we need to prevent userspace access to the DOE when the kernel is
>   managing it (registered by driver)? Discussion in thread:
>   https://lore.kernel.org/linux-pci/20210419165451.2176200-1-Jonathan.Cameron@huawei.com/
> * Does a generic userspace mediation interface make sense?
>   Currently it seems not, but as it was nearly ready, I've included
>   it in this patch set anyway.
> * Move CXL cdat handling to a separate file, or wait until we can see
>   how it is going to be used?
>   
> Changes since v3
> Thanks to Ira, Bjorn and Dan for feedback.
> * Addition of a generic IOCTL interface and demo program for discussion.
> * Fixed an issue with accidentally disabling interrupts.
> * Ensure IRQ_HANDLED always returned as clearing of BUSY can result
>   in an interrupt, but there is no means of identifying this.
>   The best that can be done is to eat the interrupt.
> * Edited comments to more directly tie them to the code the referred to.
> * Lock scope documentation update.
> * Dropped the CDAT dump to log patch as we are hopefully getting closer
>   to this being applied.
> * Fixed the references to pcie_ missed when renaming to pci_ in v3.
> 
> Set based on cxl/next
> 
> All testing conducted against QEMU emulation of a CXL type 3 device
> in conjunction with DOE mailbox patches v5 [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/1619454964-10190-1-git-send-email-cbrowy@avery-design.com/
> 
> Jonathan Cameron (5):
>   PCI: Add vendor ID for the PCI SIG
>   PCI/DOE: Add Data Object Exchange support
>   cxl/mem: Add CDAT table reading from DOE
>   DONOTMERGE PCI/DOE Add per DOE chrdev for ioctl based access
>   DONOTMERGE: PCI/DOE: Add userspace example program to tools/pci
> 
>  drivers/cxl/Kconfig           |   1 +
>  drivers/cxl/cxl.h             |  21 +
>  drivers/cxl/mem.c             | 174 ++++++++
>  drivers/cxl/mem.h             |   6 +
>  drivers/pci/Kconfig           |   8 +
>  drivers/pci/Makefile          |   1 +
>  drivers/pci/doe.c             | 794 ++++++++++++++++++++++++++++++++++
>  drivers/pci/pci-driver.c      |   3 +-
>  include/linux/pci-doe.h       | 100 +++++
>  include/linux/pci.h           |   3 +
>  include/linux/pci_ids.h       |   1 +
>  include/uapi/linux/pci_doe.h  |  32 ++
>  include/uapi/linux/pci_regs.h |  29 +-
>  tools/pci/Build               |   1 +
>  tools/pci/Makefile            |   9 +-
>  tools/pci/doetest.c           | 131 ++++++
>  16 files changed, 1311 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/pci/doe.c
>  create mode 100644 include/linux/pci-doe.h
>  create mode 100644 include/uapi/linux/pci_doe.h
>  create mode 100644 tools/pci/doetest.c
> 
> 
> base-commit: 35c32e3095d396c750f5cdfdaa94cba83d9b23c6


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 1/5] PCI: Add vendor ID for the PCI SIG
  2021-05-24 13:39 ` [PATCH v4 1/5] PCI: Add vendor ID for the PCI SIG Jonathan Cameron
@ 2021-06-10 15:17   ` Dan Williams
  2021-06-10 17:39     ` Jonathan Cameron
  0 siblings, 1 reply; 17+ messages in thread
From: Dan Williams @ 2021-06-10 15:17 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Linux PCI, linux-cxl, Bjorn Helgaas, Weiny, Ira,
	Lorenzo Pieralisi, Ben Widawsky, Chris Browy, Linux ACPI,
	Schofield, Alison, Vishal L Verma, Fangjian, Linuxarm

On Mon, May 24, 2021 at 6:41 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> This ID is used in DOE headers to identify protocols that are defined
> within the PCI Express Base Specification.
>
> Specified in Table 7-x2 of the Data Object Exchange ECN (approved 12 March
> 2020) available from https://members.pcisig.com/wg/PCI-SIG/document/14143
>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  include/linux/pci_ids.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 4c3fa5293d76..dcc8b4b14198 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -149,6 +149,7 @@
>  #define PCI_CLASS_OTHERS               0xff
>
>  /* Vendors and devices.  Sort key: vendor first, device next. */
> +#define PCI_VENDOR_ID_PCI_SIG          0x0001

Should this not be:

PCI_DOE_VENDOR_ID_PCI_SIG?

...because I don't think this value will ever show up at the typical
config-offset 0 vendor-id, will it?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 1/5] PCI: Add vendor ID for the PCI SIG
  2021-06-10 15:17   ` Dan Williams
@ 2021-06-10 17:39     ` Jonathan Cameron
  2021-06-10 20:10       ` Dan Williams
  0 siblings, 1 reply; 17+ messages in thread
From: Jonathan Cameron @ 2021-06-10 17:39 UTC (permalink / raw)
  To: Dan Williams
  Cc: Linux PCI, linux-cxl, Bjorn Helgaas, Weiny, Ira,
	Lorenzo Pieralisi, Ben Widawsky, Chris Browy, Linux ACPI,
	Schofield, Alison, Vishal L Verma, Fangjian, Linuxarm

On Thu, 10 Jun 2021 08:17:23 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> On Mon, May 24, 2021 at 6:41 AM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > This ID is used in DOE headers to identify protocols that are defined
> > within the PCI Express Base Specification.
> >
> > Specified in Table 7-x2 of the Data Object Exchange ECN (approved 12 March
> > 2020) available from https://members.pcisig.com/wg/PCI-SIG/document/14143
> >
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> >  include/linux/pci_ids.h | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> > index 4c3fa5293d76..dcc8b4b14198 100644
> > --- a/include/linux/pci_ids.h
> > +++ b/include/linux/pci_ids.h
> > @@ -149,6 +149,7 @@
> >  #define PCI_CLASS_OTHERS               0xff
> >
> >  /* Vendors and devices.  Sort key: vendor first, device next. */
> > +#define PCI_VENDOR_ID_PCI_SIG          0x0001  
> 
> Should this not be:
> 
> PCI_DOE_VENDOR_ID_PCI_SIG?
> 
> ...because I don't think this value will ever show up at the typical
> config-offset 0 vendor-id, will it?

Good question.

Whilst I agree it is unlikely to turn up as a conventional vendor-id
(though I've not found any text ruling it out) it already turns up
in locations other than DOE.

Many of them aren't software visible, but potentially places
like SPDM are in which you would have a registry ID of 0x3 (PCI-SIG)
followed by the PCI vendor ID (this one).  Those are used in SPDM
vendor defined requests / responses.

That SPDM feature is then used in IDE establishment.
The IDE ECN (via pcisig.com) has the following:
"The VendorID field of the VENDOR_DEFINED_REQUEST/
 VENDOR_DEFINED_RESPONSE must contain the value 0001h, which is assigned to
 the PCI-SIG."

Which to my reading, isn't quite the same as saying it's a vendor ID,
but nearly so.

Now, I argued the *_DVSEC_* naming in the CXL one based on the spec saying
that was all it could be used for but I may well have been wrong longer
term.

I'm fine with renaming it to the PCI_DOE_* version then dropping the DOE
when it gets used for something else though if that works for people.

At least this time naming isn't made awkward by legalese.

Jonathan

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 2/5] PCI/DOE: Add Data Object Exchange support
  2021-05-24 13:39 ` [PATCH v4 2/5] PCI/DOE: Add Data Object Exchange support Jonathan Cameron
@ 2021-06-10 20:06   ` Dan Williams
  2021-07-07 19:54     ` Dan Williams
  0 siblings, 1 reply; 17+ messages in thread
From: Dan Williams @ 2021-06-10 20:06 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Linux PCI, linux-cxl, Bjorn Helgaas, Weiny, Ira,
	Lorenzo Pieralisi, Ben Widawsky, Chris Browy, Linux ACPI,
	Schofield, Alison, Vishal L Verma, Fangjian, Linuxarm

On Mon, May 24, 2021 at 6:41 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> Introduced in a PCI ECN [1], DOE provides a config space based mailbox with
> standard protocol discovery.  Each mailbox is accessed through a DOE
> Extended Capability.
>
> A device may have 1 or more DOE mailboxes, each of which is allowed to
> support any number of protocols (some DOE protocol specifications apply
> additional restrictions).  A given protocol may be supported on more than
> one DOE mailbox on a given function.
>
> If a driver wishes to access any number of DOE instances / protocols it
> makes a single call to pci_doe_register_all() which will find available
> DOEs, create the required infrastructure and cache the protocols they
> support.  pci_doe_find() can then retrieve a pointer to an appropriate DOE
> instance.
>
> A synchronous interface is provided in pci_doe_exchange_sync() to perform a
> single query / response exchange.
>
> Testing conducted against QEMU using:
>
> https://lore.kernel.org/qemu-devel/1619454964-10190-1-git-send-email-cbrowy@avery-design.com/

Nice.

I was hoping that by now QEMU upstream would have given us some
indication that this useful work that has a chance of being merged. I
fear it's only us CXL practitioner's that care. Perhaps the PCI IDE
support will get them to move on at least the DOE patches?

>
> [1] https://members.pcisig.com/wg/PCI-SIG/document/14143
>     Data Object Exchange (DOE) - Approved 12 March 2020
>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

The core logic of this looks good to me. The interfaces for other code
to make use of this I feel can lean heavier on existing mechanics. A
few points come to mind:

- Does this need to support anything more than queue depth 1? I know
the specification seems to allow for some overlapping and queueing,
but I don't think there are any use cases that are precluded if the
max number of tasks in flight for a given DOE is one.

- Once its queue depth 1 then the list of tasks can be replaced with a
wait_queue_head_t where submitters wait for the previous task to
finish.

- This appears to be the prototypical scenario for deploying the new
auxiliary bus facility. Rather than custom code device-like facilities
(lists and parents etc) in 'struct pci_doe' just make pci_doe a device
directly (auxiliary-device) and separate the infrastructure that
drives that device to a driver (auxiliary-driver). That makes the
lifetime management more idiomatic, allows for user space to have
typical driver-binding controls to manage kernel-user DOE conflicts,
and it allows for typical driver services like devm.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 1/5] PCI: Add vendor ID for the PCI SIG
  2021-06-10 17:39     ` Jonathan Cameron
@ 2021-06-10 20:10       ` Dan Williams
  0 siblings, 0 replies; 17+ messages in thread
From: Dan Williams @ 2021-06-10 20:10 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Linux PCI, linux-cxl, Bjorn Helgaas, Weiny, Ira,
	Lorenzo Pieralisi, Ben Widawsky, Chris Browy, Linux ACPI,
	Schofield, Alison, Vishal L Verma, Fangjian, Linuxarm

On Thu, Jun 10, 2021 at 10:39 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Thu, 10 Jun 2021 08:17:23 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > On Mon, May 24, 2021 at 6:41 AM Jonathan Cameron
> > <Jonathan.Cameron@huawei.com> wrote:
> > >
> > > This ID is used in DOE headers to identify protocols that are defined
> > > within the PCI Express Base Specification.
> > >
> > > Specified in Table 7-x2 of the Data Object Exchange ECN (approved 12 March
> > > 2020) available from https://members.pcisig.com/wg/PCI-SIG/document/14143
> > >
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > ---
> > >  include/linux/pci_ids.h | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> > > index 4c3fa5293d76..dcc8b4b14198 100644
> > > --- a/include/linux/pci_ids.h
> > > +++ b/include/linux/pci_ids.h
> > > @@ -149,6 +149,7 @@
> > >  #define PCI_CLASS_OTHERS               0xff
> > >
> > >  /* Vendors and devices.  Sort key: vendor first, device next. */
> > > +#define PCI_VENDOR_ID_PCI_SIG          0x0001
> >
> > Should this not be:
> >
> > PCI_DOE_VENDOR_ID_PCI_SIG?
> >
> > ...because I don't think this value will ever show up at the typical
> > config-offset 0 vendor-id, will it?
>
> Good question.
>
> Whilst I agree it is unlikely to turn up as a conventional vendor-id
> (though I've not found any text ruling it out) it already turns up
> in locations other than DOE.
>
> Many of them aren't software visible, but potentially places
> like SPDM are in which you would have a registry ID of 0x3 (PCI-SIG)
> followed by the PCI vendor ID (this one).  Those are used in SPDM
> vendor defined requests / responses.
>
> That SPDM feature is then used in IDE establishment.
> The IDE ECN (via pcisig.com) has the following:
> "The VendorID field of the VENDOR_DEFINED_REQUEST/
>  VENDOR_DEFINED_RESPONSE must contain the value 0001h, which is assigned to
>  the PCI-SIG."
>
> Which to my reading, isn't quite the same as saying it's a vendor ID,
> but nearly so.
>
> Now, I argued the *_DVSEC_* naming in the CXL one based on the spec saying
> that was all it could be used for but I may well have been wrong longer
> term.
>
> I'm fine with renaming it to the PCI_DOE_* version then dropping the DOE
> when it gets used for something else though if that works for people.
>
> At least this time naming isn't made awkward by legalese.

For fun I did a lookup for vendor-id 1 and it came back "Fry's
Electronics Counterfeit Flash Drive"

https://pcilookup.com/?ven=0001&dev=&action=submit

The potential for it to be used in other places outside of DOE makes
me think the way you have it here is fine.

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 3/5] cxl/mem: Add CDAT table reading from DOE
  2021-05-24 13:39 ` [PATCH v4 3/5] cxl/mem: Add CDAT table reading from DOE Jonathan Cameron
@ 2021-06-10 21:46   ` Dan Williams
  0 siblings, 0 replies; 17+ messages in thread
From: Dan Williams @ 2021-06-10 21:46 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Linux PCI, linux-cxl, Bjorn Helgaas, Weiny, Ira,
	Lorenzo Pieralisi, Ben Widawsky, Chris Browy, Linux ACPI,
	Schofield, Alison, Vishal L Verma, Fangjian, Linuxarm

On Mon, May 24, 2021 at 6:42 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> This patch provides a sysfs binary attribute to allow dumping of the whole
> table.
>
> Binary dumping is modeled 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.
>
> This does not support table updates at runtime. It will always provide
> whatever was there when first cached. Handling of table updates can be
> implemented later.
>
> Once we have more users, this code can move out to driver/cxl/cdat.c or
> similar.
>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/cxl/Kconfig |   1 +
>  drivers/cxl/cxl.h   |  21 ++++++
>  drivers/cxl/mem.c   | 174 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/cxl/mem.h   |   6 ++
>  4 files changed, 202 insertions(+)
>
> diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
> index 97dc4d751651..26cad9fa29f7 100644
> --- a/drivers/cxl/Kconfig
> +++ b/drivers/cxl/Kconfig
> @@ -15,6 +15,7 @@ if CXL_BUS
>
>  config CXL_MEM
>         tristate "CXL.mem: Memory Devices"
> +       select PCI_DOE
>         help
>           The CXL.mem protocol allows a device to act as a provider of
>           "System RAM" and/or "Persistent Memory" that is fully coherent
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index d49e0cb679fa..e649a286aace 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/pci-doe.h>
>
>  /* CXL 2.0 8.2.8.1 Device Capabilities Array Register */
>  #define CXLDEV_CAP_ARRAY_OFFSET 0x0
> @@ -69,5 +70,25 @@ struct cxl_regs {
>  void cxl_setup_device_regs(struct device *dev, void __iomem *base,
>                            struct cxl_device_regs *regs);
>
> +/*
> + * Address space properties derived from:
> + * CXL 2.0 8.2.5.12.7 CXL HDM Decoder 0 Control Register
> + */
> +#define CXL_ADDRSPACE_RAM   BIT(0)
> +#define CXL_ADDRSPACE_PMEM  BIT(1)
> +#define CXL_ADDRSPACE_TYPE2 BIT(2)
> +#define CXL_ADDRSPACE_TYPE3 BIT(3)
> +#define CXL_ADDRSPACE_MASK  GENMASK(3, 0)

Looks like this got picked up from a rebase... they're now decoder
flags, but no need to include them in the CDAT patch.

> +
> +#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)
> +
>  extern struct bus_type cxl_bus_type;
>  #endif /* __CXL_H__ */
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index c5fdf2c57181..4224d1de311e 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -14,6 +14,7 @@
>  #include "pci.h"
>  #include "cxl.h"
>  #include "mem.h"
> +#include "cdat.h"
>
>  /**
>   * DOC: cxl mem
> @@ -926,6 +927,85 @@ static int cxl_mem_setup_mailbox(struct cxl_mem *cxlm)
>         return 0;
>  }
>
> +#define CDAT_DOE_REQ(entry_handle)                                     \
> +       (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 pci_doe *doe)
> +{
> +       u32 cdat_request_pl = CDAT_DOE_REQ(0);
> +       u32 cdat_response_pl[32];
> +       struct pci_doe_exchange ex = {
> +               .vid = PCI_DVSEC_VENDOR_ID_CXL,
> +               .protocol = CXL_DOE_PROTOCOL_TABLE_ACCESS,
> +               .request_pl = &cdat_request_pl,
> +               .request_pl_sz = sizeof(cdat_request_pl),
> +               .response_pl = cdat_response_pl,
> +               .response_pl_sz = sizeof(cdat_response_pl),
> +       };
> +
> +       ssize_t rc;
> +
> +       rc = pci_doe_exchange_sync(doe, &ex);
> +       if (rc < 0)
> +               return rc;
> +       if (rc < 1)
> +               return -EIO;
> +
> +       return cdat_response_pl[1];
> +}
> +
> +static int cdat_to_buffer(struct pci_doe *doe, u32 *buffer, size_t length)
> +{
> +       int entry_handle = 0;
> +       int rc;
> +
> +       do {
> +               u32 cdat_request_pl = CDAT_DOE_REQ(entry_handle);
> +               u32 cdat_response_pl[32];
> +               struct pci_doe_exchange ex = {
> +                       .vid = PCI_DVSEC_VENDOR_ID_CXL,
> +                       .protocol = CXL_DOE_PROTOCOL_TABLE_ACCESS,
> +                       .request_pl = &cdat_request_pl,
> +                       .request_pl_sz = sizeof(cdat_request_pl),
> +                       .response_pl = cdat_response_pl,
> +                       .response_pl_sz = sizeof(cdat_response_pl),
> +               };
> +               size_t entry_dw;
> +               u32 *entry;
> +
> +               rc = pci_doe_exchange_sync(doe, &ex);
> +               if (rc < 0)
> +                       return rc;
> +
> +               entry = cdat_response_pl + 1;

I think:

entry = &cdat_response_pl[1];

...is less ambiguous, otherwise you need to backtrack and remember
that cdat_response_pl is an array not a struct. Perhaps a comment or a
symbol name for "1" would help here too?


> +               entry_dw = rc / sizeof(u32);
> +               /* Skip Header */
> +               entry_dw -= 1;
> +               entry_dw = min(length / 4, entry_dw);

sometimes sizeof(u32) sometimes "4"?

> +               memcpy(buffer, entry, entry_dw * sizeof(u32));
> +               length -= entry_dw * sizeof(u32);

Why not keep entry_dw in bytes, it seems this conversion to a word
count is causing more trouble than it is worth above.

> +               buffer += entry_dw;
> +               entry_handle = FIELD_GET(CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE, cdat_response_pl[0]);
> +
> +       } while (entry_handle != 0xFFFF);

Shouldn't this also break out if length goes to zero?

> +
> +       return 0;
> +}
> +
> +static void cxl_mem_free_irq_vectors(void *data)
> +{
> +       pci_free_irq_vectors(data);
> +}
> +
> +static void cxl_mem_doe_unregister_all(void *data)
> +{
> +       pci_doe_unregister_all(data);
> +}
> +
>  static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev, u32 reg_lo,
>                                       u32 reg_hi)
>  {
> @@ -933,6 +1013,7 @@ static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev, u32 reg_lo,
>         struct cxl_mem *cxlm;
>         void __iomem *regs;
>         u64 offset;
> +       int irqs;
>         u8 bar;
>         int rc;
>
> @@ -971,6 +1052,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");
> +       } 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);
> +               rc = devm_add_action_or_reset(dev, cxl_mem_free_irq_vectors,
> +                                              pdev);
> +               if (rc)
> +                       return NULL;
> +       }
> +
> +       /*
> +        * Find a DOE mailbox that supports CDAT.
> +        * Supporting other DOE protocols will require more complexity.
> +        */
> +       rc = pci_doe_register_all(pdev);
> +       if (rc < 0)
> +               return NULL;
> +
> +       rc = devm_add_action_or_reset(dev, cxl_mem_doe_unregister_all, pdev);
> +       if (rc)
> +               return NULL;
> +
> +       cxlm->table_doe = pci_doe_find(pdev, PCI_DVSEC_VENDOR_ID_CXL,
> +                                      CXL_DOE_PROTOCOL_TABLE_ACCESS);
> +

cxl_mem_create() is about allocating / initializing the @cxlm object.
I think the above belongs in its own hardware init function. Well the
interrupt init belongs in its own init function before
cxl_mem_create() and the DOE / CDAT registration can come sometime
later after cxl_mem_identify() succeeds. I.e. it's not until the
driver has successfully setup mailbox communications should it worry
about talking to the DOE.

>         dev_dbg(dev, "Mapped CXL Memory Device resource\n");
>         return cxlm;
>  }
> @@ -1060,6 +1179,31 @@ static ssize_t pmem_size_show(struct device *dev, struct device_attribute *attr,
>         return sysfs_emit(buf, "%#llx\n", len);
>  }
>
> +static ssize_t CDAT_read(struct file *filp, struct kobject *kobj,
> +                        struct bin_attribute *bin_attr, char *buf,
> +                        loff_t offset, size_t count)
> +{
> +       struct device *dev = kobj_to_dev(kobj);
> +       struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> +
> +       return memory_read_from_buffer(buf, count, &offset, cxlmd->cdat_table,
> +                                      cxlmd->cdat_length);
> +}
> +
> +static BIN_ATTR_RO(CDAT, 0);
> +
> +static umode_t cxl_memdev_bin_attr_is_visible(struct kobject *kobj,
> +                                             struct bin_attribute *attr, int i)
> +{
> +       struct device *dev = kobj_to_dev(kobj);
> +       struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> +
> +       if ((attr == &bin_attr_CDAT) && cxlmd->cdat_table)
> +               return 0400;
> +
> +       return 0;
> +}
> +
>  static struct device_attribute dev_attr_pmem_size =
>         __ATTR(size, 0444, pmem_size_show, NULL);
>
> @@ -1069,6 +1213,11 @@ static struct attribute *cxl_memdev_attributes[] = {
>         NULL,
>  };
>
> +static struct bin_attribute *cxl_memdev_bin_attributes[] = {
> +       &bin_attr_CDAT,
> +       NULL,
> +};
> +
>  static struct attribute *cxl_memdev_pmem_attributes[] = {
>         &dev_attr_pmem_size.attr,
>         NULL,
> @@ -1081,6 +1230,8 @@ static struct attribute *cxl_memdev_ram_attributes[] = {
>
>  static struct attribute_group cxl_memdev_attribute_group = {
>         .attrs = cxl_memdev_attributes,
> +       .bin_attrs = cxl_memdev_bin_attributes,
> +       .is_bin_visible = cxl_memdev_bin_attr_is_visible,
>  };
>
>  static struct attribute_group cxl_memdev_ram_attribute_group = {
> @@ -1158,6 +1309,25 @@ static struct cxl_memdev *cxl_memdev_alloc(struct cxl_mem *cxlm)
>         return ERR_PTR(rc);
>  }
>
> +static int cxl_cache_cdat_table(struct cxl_memdev *cxlmd)
> +{
> +       struct cxl_mem *cxlm = cxlmd->cxlm;
> +       struct device *dev = &cxlmd->dev;
> +       ssize_t cdat_length;
> +
> +       if (cxlm->table_doe == NULL)
> +               return 0;
> +
> +       cdat_length = cdat_get_length(cxlm->table_doe);
> +       if (cdat_length < 0)
> +               return cdat_length;
> +
> +       cxlmd->cdat_length = cdat_length;
> +       cxlmd->cdat_table = devm_kzalloc(dev->parent, cdat_length, GFP_KERNEL);

I'm not sure how big these CDATs can get, but I don't think there is
any requirement for the memory to be contiguous, so perhaps this
should be kvzalloc() to be kind to the page allocator.

> +
> +       return cdat_to_buffer(cxlm->table_doe, cxlmd->cdat_table, cxlmd->cdat_length);
> +}
> +
>  static int cxl_mem_add_memdev(struct cxl_mem *cxlm)
>  {
>         struct cxl_memdev *cxlmd;
> @@ -1180,6 +1350,10 @@ static int cxl_mem_add_memdev(struct cxl_mem *cxlm)
>          */
>         cxlmd->cxlm = cxlm;
>
> +       rc = cxl_cache_cdat_table(cxlmd);
> +       if (rc)
> +               goto err;
> +
>         cdev = &cxlmd->cdev;
>         rc = cdev_device_add(cdev, dev);
>         if (rc)
> diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h
> index 0a3f70316872..fb26155a8fb3 100644
> --- a/drivers/cxl/mem.h
> +++ b/drivers/cxl/mem.h
> @@ -38,12 +38,16 @@
>   * @cdev: char dev core object for ioctl operations
>   * @cxlm: pointer to the parent device driver data
>   * @id: id number of this memdev instance.
> + * @cdat_table: cache of CDAT table
> + * @cdat_length: length of cached CDAT table
>   */
>  struct cxl_memdev {
>         struct device dev;
>         struct cdev cdev;
>         struct cxl_mem *cxlm;
>         int id;
> +       void *cdat_table;
> +       size_t cdat_length;
>  };
>
>  /**
> @@ -51,6 +55,7 @@ struct cxl_memdev {
>   * @pdev: The PCI device associated with this CXL device.
>   * @base: IO mappings to the device's MMIO
>   * @cxlmd: Logical memory device chardev / interface
> + * @table_doe: Data exchange object mailbox used to read tables
>   * @regs: Parsed register blocks
>   * @payload_size: Size of space for payload
>   *                (CXL 2.0 8.2.8.4.3 Mailbox Capabilities Register)
> @@ -65,6 +70,7 @@ struct cxl_mem {
>         void __iomem *base;
>         struct cxl_memdev *cxlmd;
>
> +       struct pci_doe *table_doe;
>         struct cxl_regs regs;
>
>         size_t payload_size;

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 2/5] PCI/DOE: Add Data Object Exchange support
  2021-06-10 20:06   ` Dan Williams
@ 2021-07-07 19:54     ` Dan Williams
  2021-07-08  8:48       ` Lorenzo Pieralisi
  0 siblings, 1 reply; 17+ messages in thread
From: Dan Williams @ 2021-07-07 19:54 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Linux PCI, linux-cxl, Bjorn Helgaas, Weiny, Ira,
	Lorenzo Pieralisi, Ben Widawsky, Chris Browy, Linux ACPI,
	Schofield, Alison, Vishal L Verma, Fangjian, Linuxarm

On Thu, Jun 10, 2021 at 1:06 PM Dan Williams <dan.j.williams@intel.com> wrote:
>
> On Mon, May 24, 2021 at 6:41 AM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > Introduced in a PCI ECN [1], DOE provides a config space based mailbox with
> > standard protocol discovery.  Each mailbox is accessed through a DOE
> > Extended Capability.
> >
> > A device may have 1 or more DOE mailboxes, each of which is allowed to
> > support any number of protocols (some DOE protocol specifications apply
> > additional restrictions).  A given protocol may be supported on more than
> > one DOE mailbox on a given function.
> >
> > If a driver wishes to access any number of DOE instances / protocols it
> > makes a single call to pci_doe_register_all() which will find available
> > DOEs, create the required infrastructure and cache the protocols they
> > support.  pci_doe_find() can then retrieve a pointer to an appropriate DOE
> > instance.
> >
> > A synchronous interface is provided in pci_doe_exchange_sync() to perform a
> > single query / response exchange.
> >
> > Testing conducted against QEMU using:
> >
> > https://lore.kernel.org/qemu-devel/1619454964-10190-1-git-send-email-cbrowy@avery-design.com/
>
> Nice.
>
> I was hoping that by now QEMU upstream would have given us some
> indication that this useful work that has a chance of being merged. I
> fear it's only us CXL practitioner's that care. Perhaps the PCI IDE
> support will get them to move on at least the DOE patches?
>
> >
> > [1] https://members.pcisig.com/wg/PCI-SIG/document/14143
> >     Data Object Exchange (DOE) - Approved 12 March 2020
> >
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> The core logic of this looks good to me. The interfaces for other code
> to make use of this I feel can lean heavier on existing mechanics. A
> few points come to mind:
>
> - Does this need to support anything more than queue depth 1? I know
> the specification seems to allow for some overlapping and queueing,
> but I don't think there are any use cases that are precluded if the
> max number of tasks in flight for a given DOE is one.
>
> - Once its queue depth 1 then the list of tasks can be replaced with a
> wait_queue_head_t where submitters wait for the previous task to
> finish.
>
> - This appears to be the prototypical scenario for deploying the new
> auxiliary bus facility. Rather than custom code device-like facilities
> (lists and parents etc) in 'struct pci_doe' just make pci_doe a device
> directly (auxiliary-device) and separate the infrastructure that
> drives that device to a driver (auxiliary-driver). That makes the
> lifetime management more idiomatic, allows for user space to have
> typical driver-binding controls to manage kernel-user DOE conflicts,
> and it allows for typical driver services like devm.

Hi Jonathan,

Are you waiting on me to take a shot at refactoring the DOE driver
into this proposed auxiliary device/driver organization? I am happy to
do that if you've moved on to looking at the kernel-side SPDM
implementation [1].

I would expect DOE,  SPDM, and IDE would be a useful topic to discuss
at the the Plumbers PCI Microconference assuming we do not solve all
the open issues before September.

[1]: https://lore.kernel.org/r/20210629132520.00000d1f@Huawei.com

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 2/5] PCI/DOE: Add Data Object Exchange support
  2021-07-07 19:54     ` Dan Williams
@ 2021-07-08  8:48       ` Lorenzo Pieralisi
  2021-07-08 17:28         ` Jonathan Cameron
  0 siblings, 1 reply; 17+ messages in thread
From: Lorenzo Pieralisi @ 2021-07-08  8:48 UTC (permalink / raw)
  To: Dan Williams
  Cc: Jonathan Cameron, Linux PCI, linux-cxl, Bjorn Helgaas, Weiny,
	Ira, Ben Widawsky, Chris Browy, Linux ACPI, Schofield, Alison,
	Vishal L Verma, Fangjian, Linuxarm

On Wed, Jul 07, 2021 at 12:54:33PM -0700, Dan Williams wrote:
> On Thu, Jun 10, 2021 at 1:06 PM Dan Williams <dan.j.williams@intel.com> wrote:
> >
> > On Mon, May 24, 2021 at 6:41 AM Jonathan Cameron
> > <Jonathan.Cameron@huawei.com> wrote:
> > >
> > > Introduced in a PCI ECN [1], DOE provides a config space based mailbox with
> > > standard protocol discovery.  Each mailbox is accessed through a DOE
> > > Extended Capability.
> > >
> > > A device may have 1 or more DOE mailboxes, each of which is allowed to
> > > support any number of protocols (some DOE protocol specifications apply
> > > additional restrictions).  A given protocol may be supported on more than
> > > one DOE mailbox on a given function.
> > >
> > > If a driver wishes to access any number of DOE instances / protocols it
> > > makes a single call to pci_doe_register_all() which will find available
> > > DOEs, create the required infrastructure and cache the protocols they
> > > support.  pci_doe_find() can then retrieve a pointer to an appropriate DOE
> > > instance.
> > >
> > > A synchronous interface is provided in pci_doe_exchange_sync() to perform a
> > > single query / response exchange.
> > >
> > > Testing conducted against QEMU using:
> > >
> > > https://lore.kernel.org/qemu-devel/1619454964-10190-1-git-send-email-cbrowy@avery-design.com/
> >
> > Nice.
> >
> > I was hoping that by now QEMU upstream would have given us some
> > indication that this useful work that has a chance of being merged. I
> > fear it's only us CXL practitioner's that care. Perhaps the PCI IDE
> > support will get them to move on at least the DOE patches?
> >
> > >
> > > [1] https://members.pcisig.com/wg/PCI-SIG/document/14143
> > >     Data Object Exchange (DOE) - Approved 12 March 2020
> > >
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > The core logic of this looks good to me. The interfaces for other code
> > to make use of this I feel can lean heavier on existing mechanics. A
> > few points come to mind:
> >
> > - Does this need to support anything more than queue depth 1? I know
> > the specification seems to allow for some overlapping and queueing,
> > but I don't think there are any use cases that are precluded if the
> > max number of tasks in flight for a given DOE is one.
> >
> > - Once its queue depth 1 then the list of tasks can be replaced with a
> > wait_queue_head_t where submitters wait for the previous task to
> > finish.
> >
> > - This appears to be the prototypical scenario for deploying the new
> > auxiliary bus facility. Rather than custom code device-like facilities
> > (lists and parents etc) in 'struct pci_doe' just make pci_doe a device
> > directly (auxiliary-device) and separate the infrastructure that
> > drives that device to a driver (auxiliary-driver). That makes the
> > lifetime management more idiomatic, allows for user space to have
> > typical driver-binding controls to manage kernel-user DOE conflicts,
> > and it allows for typical driver services like devm.
> 
> Hi Jonathan,
> 
> Are you waiting on me to take a shot at refactoring the DOE driver
> into this proposed auxiliary device/driver organization? I am happy to
> do that if you've moved on to looking at the kernel-side SPDM
> implementation [1].
> 
> I would expect DOE,  SPDM, and IDE would be a useful topic to discuss
> at the the Plumbers PCI Microconference assuming we do not solve all
> the open issues before September.

Definitely, I will make sure we schedule a slot on these topics.

Thanks,
Lorenzo

> [1]: https://lore.kernel.org/r/20210629132520.00000d1f@Huawei.com

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 2/5] PCI/DOE: Add Data Object Exchange support
  2021-07-08  8:48       ` Lorenzo Pieralisi
@ 2021-07-08 17:28         ` Jonathan Cameron
  0 siblings, 0 replies; 17+ messages in thread
From: Jonathan Cameron @ 2021-07-08 17:28 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Dan Williams, Linux PCI, linux-cxl, Bjorn Helgaas, Weiny, Ira,
	Ben Widawsky, Chris Browy, Linux ACPI, Schofield, Alison,
	Vishal L Verma, Fangjian, Linuxarm

On Thu, 8 Jul 2021 09:48:15 +0100
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:

> On Wed, Jul 07, 2021 at 12:54:33PM -0700, Dan Williams wrote:
> > On Thu, Jun 10, 2021 at 1:06 PM Dan Williams <dan.j.williams@intel.com> wrote:  
> > >
> > > On Mon, May 24, 2021 at 6:41 AM Jonathan Cameron
> > > <Jonathan.Cameron@huawei.com> wrote:  
> > > >
> > > > Introduced in a PCI ECN [1], DOE provides a config space based mailbox with
> > > > standard protocol discovery.  Each mailbox is accessed through a DOE
> > > > Extended Capability.
> > > >
> > > > A device may have 1 or more DOE mailboxes, each of which is allowed to
> > > > support any number of protocols (some DOE protocol specifications apply
> > > > additional restrictions).  A given protocol may be supported on more than
> > > > one DOE mailbox on a given function.
> > > >
> > > > If a driver wishes to access any number of DOE instances / protocols it
> > > > makes a single call to pci_doe_register_all() which will find available
> > > > DOEs, create the required infrastructure and cache the protocols they
> > > > support.  pci_doe_find() can then retrieve a pointer to an appropriate DOE
> > > > instance.
> > > >
> > > > A synchronous interface is provided in pci_doe_exchange_sync() to perform a
> > > > single query / response exchange.
> > > >
> > > > Testing conducted against QEMU using:
> > > >
> > > > https://lore.kernel.org/qemu-devel/1619454964-10190-1-git-send-email-cbrowy@avery-design.com/  
> > >
> > > Nice.
> > >
> > > I was hoping that by now QEMU upstream would have given us some
> > > indication that this useful work that has a chance of being merged. I
> > > fear it's only us CXL practitioner's that care. Perhaps the PCI IDE
> > > support will get them to move on at least the DOE patches?
> > >  
> > > >
> > > > [1] https://members.pcisig.com/wg/PCI-SIG/document/14143
> > > >     Data Object Exchange (DOE) - Approved 12 March 2020
> > > >
> > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> > >
> > > The core logic of this looks good to me. The interfaces for other code
> > > to make use of this I feel can lean heavier on existing mechanics. A
> > > few points come to mind:
> > >
> > > - Does this need to support anything more than queue depth 1? I know
> > > the specification seems to allow for some overlapping and queueing,
> > > but I don't think there are any use cases that are precluded if the
> > > max number of tasks in flight for a given DOE is one.

We aren't handling overlapping today, so a queue of one and wait on access
to that would work fine.  Queuing is more likely to be about multiple protocols
sharing a DOE (where allowed) as I'm not seeing a need in any individual protocol
to queue lots of things up at once. You could queue a few things in CMA as they
aren't all dependent on each other (though the order matters for building the
measurements) but I'm not sure why you would.

> > >
> > > - Once its queue depth 1 then the list of tasks can be replaced with a
> > > wait_queue_head_t where submitters wait for the previous task to
> > > finish.

I'll go with a resounding 'maybe' on that... I'm not sure I like that the
async path in the submitter could be waiting a very long time (if we think
anyone will actually do async...)  I'm probably missing something.

> > >
> > > - This appears to be the prototypical scenario for deploying the new
> > > auxiliary bus facility. Rather than custom code device-like facilities
> > > (lists and parents etc) in 'struct pci_doe' just make pci_doe a device
> > > directly (auxiliary-device) and separate the infrastructure that
> > > drives that device to a driver (auxiliary-driver). That makes the
> > > lifetime management more idiomatic, allows for user space to have
> > > typical driver-binding controls to manage kernel-user DOE conflicts,
> > > and it allows for typical driver services like devm.  
> > 
> > Hi Jonathan,
> > 
> > Are you waiting on me to take a shot at refactoring the DOE driver
> > into this proposed auxiliary device/driver organization? I am happy to
> > do that if you've moved on to looking at the kernel-side SPDM
> > implementation [1].

I wasn't really expecting you to take a shot, but if you have time, that
would be great. I've not looked at the auxiliary bus stuff yet.

Whilst I have a PoC up and running for SPDM, it's a bunch of dirty hacks and
I suspect that, even when cleaned up, it's going to keep me busy for a while.

Mostly I've developed a steadily growing hatred for the different ways in
which signatures can be stored and hand decoding records or digging in
openspdm to figure out what order and encoding things are in this time...

> > 
> > I would expect DOE,  SPDM, and IDE would be a useful topic to discuss
> > at the the Plumbers PCI Microconference assuming we do not solve all
> > the open issues before September.  
> 
> Definitely, I will make sure we schedule a slot on these topics.

+1

I'm sure there will be some issues left :)

> 
> Thanks,
> Lorenzo
> 
> > [1]: https://lore.kernel.org/r/20210629132520.00000d1f@Huawei.com  


^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2021-07-08 17:28 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-24 13:39 [PATCH v4 0/5] PCI Data Object Exchange support + CXL CDAT Jonathan Cameron
2021-05-24 13:39 ` [PATCH v4 1/5] PCI: Add vendor ID for the PCI SIG Jonathan Cameron
2021-06-10 15:17   ` Dan Williams
2021-06-10 17:39     ` Jonathan Cameron
2021-06-10 20:10       ` Dan Williams
2021-05-24 13:39 ` [PATCH v4 2/5] PCI/DOE: Add Data Object Exchange support Jonathan Cameron
2021-06-10 20:06   ` Dan Williams
2021-07-07 19:54     ` Dan Williams
2021-07-08  8:48       ` Lorenzo Pieralisi
2021-07-08 17:28         ` Jonathan Cameron
2021-05-24 13:39 ` [PATCH v4 3/5] cxl/mem: Add CDAT table reading from DOE Jonathan Cameron
2021-06-10 21:46   ` Dan Williams
2021-05-24 13:39 ` [PATCH v4 4/5] DONOTMERGE: PCI/DOE: Add per DOE chrdev for ioctl based access Jonathan Cameron
2021-05-25 10:26   ` kernel test robot
2021-05-25 10:26     ` kernel test robot
2021-05-24 13:39 ` [PATCH v4 5/5] DONOTMERGE: PCI/DOE: Add userspace example program to tools/pci Jonathan Cameron
2021-06-10 14:30 ` [PATCH v4 0/5] PCI Data Object Exchange support + CXL CDAT Jonathan Cameron

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.