| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20230902045214.GA2913@thinkpad>
Date: Sat, 2 Sep 2023 10:22:14 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To: Frank Li <Frank.Li@....com>
Cc: tglx@...utronix.de, aisheng.dong@....com, bhelgaas@...gle.com,
devicetree@...r.kernel.org, festevam@...il.com,
imx@...ts.linux.dev, jdmason@...zu.us, kernel@...gutronix.de,
kishon@...com, krzysztof.kozlowski+dt@...aro.org, kw@...ux.com,
linux-arm-kernel@...ts.infradead.org, linux-imx@....com,
linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
lorenzo.pieralisi@....com, lpieralisi@...nel.org, maz@...nel.org,
ntb@...ts.linux.dev, peng.fan@....com, robh+dt@...nel.org,
s.hauer@...gutronix.de, shawnguo@...nel.org
Subject: Re: [PATCH 1/3] PCI: endpoint: Add RC-to-EP doorbell support using
platform MSI controller
On Wed, Apr 26, 2023 at 04:34:34PM -0400, 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.
>
> However, it currently supports only one EP physical function due to limitations
> in ARM MSI/IMS readiness.
>
I've provided generic comments below, but I will do one more thorough review
after seeing epf-test driver patch.
> Signed-off-by: Frank Li <Frank.Li@....com>
> ---
> drivers/pci/endpoint/pci-epf-core.c | 109 ++++++++++++++++++++++++++++
> include/linux/pci-epf.h | 16 ++++
> 2 files changed, 125 insertions(+)
>
> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> index 355a6f56fcea..94ac82bf84c5 100644
> --- a/drivers/pci/endpoint/pci-epf-core.c
> +++ b/drivers/pci/endpoint/pci-epf-core.c
> @@ -6,10 +6,12 @@
> * Author: Kishon Vijay Abraham I <kishon@...com>
> */
>
> +#include <linux/irqreturn.h>
Why is this needed?
> #include <linux/device.h>
> #include <linux/dma-mapping.h>
> #include <linux/slab.h>
> #include <linux/module.h>
> +#include <linux/msi.h>
>
> #include <linux/pci-epc.h>
> #include <linux/pci-epf.h>
> @@ -300,6 +302,113 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
> }
> EXPORT_SYMBOL_GPL(pci_epf_alloc_space);
>
> +static enum irqreturn pci_epf_interrupt_handler(int irq, void *data)
static irqreturn_t
s/pci_epf_interrupt_handler/pci_epf_doorbell_handler
> +{
> + 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;
> +}
> +
> +static void pci_epf_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> +{
> + struct pci_epc *epc = container_of(desc->dev, struct pci_epc, dev);
> + struct pci_epf *epf;
> +
> + /* Todo: Need check correct epf if multi epf supported */
> + list_for_each_entry(epf, &epc->pci_epf, list) {
> + if (epf->msg && desc->msi_index < epf->num_msgs)
> + epf->msg[desc->msi_index] = *msg;
> + }
> +}
> +
> +int pci_epf_alloc_doorbell(struct pci_epf *epf, u16 num_msgs)
> +{
> + struct irq_domain *domain;
> + struct pci_epc *epc;
> + struct device *dev;
> + int virq;
> + int ret;
> + int i;
> +
> + epc = epf->epc;
> + dev = &epc->dev;
"epc_dev" to make it explicit
> +
> + /*
> + * Current only support 1 function.
What does this mean exactly? Even a single EPC can support multiple EPFs
> + * PCI IMS(interrupt message store) ARM support have not been
> + * ready yet.
No need to mention platform irq controller name.
> + */
> + if (epc->function_num_map != 1)
Why can't you use, epf->func_no?
> + return -EOPNOTSUPP;
> +
> + domain = dev_get_msi_domain(dev->parent);
> + if (!domain)
> + return -EOPNTSUPP;
Newline
> + dev_set_msi_domain(dev, domain);
> +
> + /* use parent of_node to get device id information */
> + dev->of_node = dev->parent->of_node;
> +
Why do you need of_node assignment inside EPF core?
> + epf->msg = kcalloc(num_msgs, sizeof(struct msi_msg), GFP_KERNEL);
> + if (!epf->msg)
> + return -ENOMEM;
> +
> + epf->num_msgs = num_msgs;
> +
Move this to the start of the function, after checks.
> + ret = platform_msi_domain_alloc_irqs(dev, num_msgs, pci_epf_write_msi_msg);
> + if (ret) {
> + dev_err(dev, "Can't allocate MSI from system MSI controller\n");
"Failed to allocate MSI"
> + goto err_mem;
err_free_mem
> + }
> +
> + 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_interrupt_handler, 0,
> + "pci-epf-doorbell", epf);
IRQ name should have an index, otherwise all of them will have the same name.
> +
> + if (ret) {
> + dev_err(dev, "Failure request doorbell IRQ\n");
"Failed to request doorbell"
> + goto err_irq;
err_free_irq
> + }
> + }
> +
> + epf->num_msgs = num_msgs;
Newline
> + return ret;
> +
> +err_irq:
> + platform_msi_domain_free_irqs(dev);
> +err_mem:
> + kfree(epf->msg);
> + epf->msg = NULL;
> + epf->num_msgs = 0;
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(pci_epf_alloc_doorbell);
> +
> +void pci_epf_free_doorbell(struct pci_epf *epf)
> +{
> + struct pci_epc *epc;
> + int i;
> +
> + epc = epf->epc;
> +
> + for (i = 0; i < epf->num_msgs; i++)
> + free_irq(epf->virq_base + i, epf);
> +
> + platform_msi_domain_free_irqs(&epc->dev);
> + kfree(epf->msg);
> + epf->msg = NULL;
> + epf->num_msgs = 0;
> +}
> +EXPORT_SYMBOL_GPL(pci_epf_free_doorbell);
> +
> static void pci_epf_remove_cfs(struct pci_epf_driver *driver)
> {
> struct config_group *group, *tmp;
> diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> index b8441db2fa52..e187e3ee48d2 100644
> --- a/include/linux/pci-epf.h
> +++ b/include/linux/pci-epf.h
> @@ -75,6 +75,7 @@ struct pci_epf_ops {
> struct pci_epc_event_ops {
> int (*core_init)(struct pci_epf *epf);
> int (*link_up)(struct pci_epf *epf);
> + int (*doorbell)(struct pci_epf *epf, int index);
> };
>
> /**
> @@ -173,6 +174,9 @@ struct pci_epf {
> unsigned long vfunction_num_map;
> struct list_head pci_vepf;
> const struct pci_epc_event_ops *event_ops;
> + struct msi_msg *msg;
> + u16 num_msgs;
> + int virq_base;
> };
>
> /**
> @@ -216,4 +220,16 @@ int pci_epf_bind(struct pci_epf *epf);
> void pci_epf_unbind(struct pci_epf *epf);
> int pci_epf_add_vepf(struct pci_epf *epf_pf, struct pci_epf *epf_vf);
> void pci_epf_remove_vepf(struct pci_epf *epf_pf, struct pci_epf *epf_vf);
> +int pci_epf_alloc_doorbell(struct pci_epf *epf, u16 nums);
> +void pci_epf_free_doorbell(struct pci_epf *epf);
> +
> +static inline struct msi_msg *epf_get_msg(struct pci_epf *epf)
> +{
> + return epf->msg;
> +}
> +
> +static inline u16 epf_get_msg_num(struct pci_epf *epf)
> +{
> + return epf->num_msgs;
> +}
I don't see a need for these two functions as they are doing just dereferences.
- Mani
> #endif /* __LINUX_PCI_EPF_H */
> --
> 2.34.1
>
--
மணிவண்ணன் சதாசிவம்
Powered by blists - more mailing lists