Re: [PATCH v2 1/5] PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller

From: Kishon Vijay Abraham I
Date: Fri Sep 29 2023 - 05:30:44 EST


Hi Frank,

On 9/12/2023 3:39 AM, Frank Li wrote:
This commit introduces a common method for sending messages from the Root
Complex (RC) to the Endpoint (EP) by utilizing the platform MSI interrupt
controller, such as ARM GIC, as an EP doorbell. Maps the memory assigned
for the BAR region by the PCI host to the message address of the platform
MSI interrupt controller in the PCI EP. As a result, when the PCI RC writes
to the BAR region, it triggers an IRQ at the EP. This implementation serves
as a common method for all endpoint function drivers.

This would help avoid the polling used in current EPF drivers. Thanks!

However, it currently supports only one EP physical function due to
limitations in ARM MSI/IMS readiness.

Any such platform or architecture restrictions should not be handled in the endpoint core layer.

Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
---
drivers/pci/endpoint/pci-epc-core.c | 192 ++++++++++++++++++++++++++++
drivers/pci/endpoint/pci-epf-core.c | 44 +++++++
include/linux/pci-epc.h | 6 +
include/linux/pci-epf.h | 7 +
4 files changed, 249 insertions(+)

diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
index 5a4a8b0be6262..d336a99c6a94f 100644
--- a/drivers/pci/endpoint/pci-epc-core.c
+++ b/drivers/pci/endpoint/pci-epc-core.c
@@ -10,6 +10,7 @@
#include <linux/slab.h>
#include <linux/module.h>
+#include <linux/msi.h>
#include <linux/pci-epc.h>
#include <linux/pci-epf.h>
#include <linux/pci-ep-cfs.h>
@@ -783,6 +784,197 @@ void pci_epc_bme_notify(struct pci_epc *epc)
}
EXPORT_SYMBOL_GPL(pci_epc_bme_notify);
+/**
+ * pci_epc_alloc_doorbell() - alloc an address space to let RC trigger EP side IRQ by write data to
+ * the space.
+ *
+ * @epc: the EPC device that need doorbell address and data from RC.
+ * @func_no: the physical endpoint function number in the EPC device.
+ * @vfunc_no: the virtual endpoint function number in the physical function.
+ * @num_msgs: the total number of doorbell messages
+ *
+ * Return: 0 success, other is failure
+ */
+int pci_epc_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_msgs)
+{
+ int ret;
+
+ if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
+ return -EINVAL;
+
+ if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
+ return -EINVAL;
+
+ if (!epc->ops->alloc_doorbell)
+ return 0;
+
+ mutex_lock(&epc->lock);
+ ret = epc->ops->alloc_doorbell(epc, func_no, vfunc_no, num_msgs);
+ mutex_unlock(&epc->lock);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(pci_epc_alloc_doorbell);
+
+/**
+ * pci_epc_free_doorbell() - free resource allocated by pci_epc_alloc_doorbell()
+ *
+ * @epc: the EPC device that need doorbell address and data from RC.
+ * @func_no: the physical endpoint function number in the EPC device.
+ * @vfunc_no: the virtual endpoint function number in the physical function.
+ *
+ * Return: 0 success, other is failure
+ */
+void pci_epc_free_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
+{
+ if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
+ return;
+
+ if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
+ return;
+
+ if (!epc->ops->free_doorbell)
+ return;
+
+ mutex_lock(&epc->lock);
+ epc->ops->free_doorbell(epc, func_no, vfunc_no);
+ mutex_unlock(&epc->lock);
+}
+EXPORT_SYMBOL_GPL(pci_epc_free_doorbell);
+
+static irqreturn_t pci_epf_generic_doorbell_handler(int irq, void *data)
+{
+ struct pci_epf *epf = data;
+
+ if (epf->event_ops && epf->event_ops->doorbell)
+ epf->event_ops->doorbell(epf, irq - epf->virq_base);
+
+ return IRQ_HANDLED;
+}

IMO the handler should be directly implemented in the EPF drivers. There should be one API which returns the virq and the msi_msg to the EPF driver and the EPF driver should do request_irq.
+
+static void pci_epc_generic_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
+{
+ struct pci_epc *epc = NULL;
+ struct class_dev_iter iter;
+ struct pci_epf *epf;
+ struct device *dev;
+
+ class_dev_iter_init(&iter, pci_epc_class, NULL, NULL);
+ while ((dev = class_dev_iter_next(&iter))) {
+ if (dev->parent != desc->dev)
+ continue;

Ideally the msi_desc should be associated directly with the EPF device.
+
+ epc = to_pci_epc(dev);
+
+ class_dev_iter_exit(&iter);
+ break;
+ }
+
+ if (!epc)
+ return;
+
+ /* Only support one EPF for doorbell */
+ epf = list_first_entry_or_null(&epc->pci_epf, struct pci_epf, list);
+
+ if (!epf)
+ return;

If this is a platform restriction, this should be moved elsewhere.
+
+ if (epf->msg && desc->msi_index < epf->num_msgs)
+ epf->msg[desc->msi_index] = *msg;
+}
+
+
+/**
+ * pci_epc_generic_alloc_doorbell() - Common help function. Allocate address space from MSI
+ * controller
+ *
+ * @epc: the EPC device that need doorbell address and data from RC.
+ * @func_no: the physical endpoint function number in the EPC device.
+ * @vfunc_no: the virtual endpoint function number in the physical function.
+ * @num_msgs: the total number of doorbell messages
+ *
+ * Remark: use this function only if EPC driver just register one EPC device.
+ *
+ * Return: 0 success, other is failure
+ */
+int pci_epc_generic_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_msgs)
+{
+ struct pci_epf *epf;
+ struct device *dev;
+ int virq, last;
+ int ret;
+ int i;
+
+ if (IS_ERR_OR_NULL(epc))
+ return -EINVAL;
+
+ /* Currently only support one func and one vfunc for doorbell */
+ if (func_no || vfunc_no)
+ return -EINVAL;
+
+ epf = list_first_entry_or_null(&epc->pci_epf, struct pci_epf, list);
+ if (!epf)
+ return -EINVAL;
+
+ dev = epc->dev.parent;
+ ret = platform_msi_domain_alloc_irqs(dev, num_msgs, pci_epc_generic_write_msi_msg);
+ if (ret) {
+ dev_err(dev, "Failed to allocate MSI\n");
+ return -ENOMEM;
+ }

The alloc_irqs should be for a EPF device IMO.
+
+ last = -1;
+ for (i = 0; i < num_msgs; i++) {
+ virq = msi_get_virq(dev, i);
+ if (i == 0)
+ epf->virq_base = virq;
+
+ ret = request_irq(virq, pci_epf_generic_doorbell_handler, 0,
+ kasprintf(GFP_KERNEL, "pci-epc-doorbell%d", i), epf);
+
+ if (ret) {
+ dev_err(dev, "Failed to request doorbell\n");
+ goto err_free_irq;
+ }
+ last = i;
+ }
+
+ return 0;
+
+err_free_irq:
+ for (i = 0; i < last; i++)
+ kfree(free_irq(epf->virq_base + i, epf));
+ platform_msi_domain_free_irqs(dev);
+
+ return -EINVAL;
+}

Thanks,
Kishon