| 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: <HE1PR0401MB2331DFA02184D883F3FD4CDD880F9@HE1PR0401MB2331.eurprd04.prod.outlook.com>
Date: Thu, 24 Nov 2022 22:02:13 +0000
From: Frank Li <frank.li@....com>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
CC: "lpieralisi@...nel.org" <lpieralisi@...nel.org>,
Aisheng Dong <aisheng.dong@....com>,
"bhelgaas@...gle.com" <bhelgaas@...gle.com>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"festevam@...il.com" <festevam@...il.com>,
"imx@...ts.linux.dev" <imx@...ts.linux.dev>,
"jdmason@...zu.us" <jdmason@...zu.us>,
"kernel@...gutronix.de" <kernel@...gutronix.de>,
"kishon@...com" <kishon@...com>,
"krzysztof.kozlowski+dt@...aro.org"
<krzysztof.kozlowski+dt@...aro.org>, "kw@...ux.com" <kw@...ux.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
dl-linux-imx <linux-imx@....com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
"lorenzo.pieralisi@....com" <lorenzo.pieralisi@....com>,
"lznuaa@...il.com" <lznuaa@...il.com>,
"maz@...nel.org" <maz@...nel.org>,
"ntb@...ts.linux.dev" <ntb@...ts.linux.dev>,
Peng Fan <peng.fan@....com>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
"shawnguo@...nel.org" <shawnguo@...nel.org>,
"tglx@...utronix.de" <tglx@...utronix.de>
Subject: RE: [EXT] Re: [PATCH v13 2/2] PCI: endpoint: pci-epf-vntb: using
platform MSI as doorbell
> -----Original Message-----
> From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
> Sent: Thursday, November 24, 2022 12:45 PM
> To: Frank Li <frank.li@....com>
> Cc: lpieralisi@...nel.org; Aisheng Dong <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; dl-linux-imx <linux-imx@....com>; linux-
> kernel@...r.kernel.org; linux-pci@...r.kernel.org;
> lorenzo.pieralisi@....com; lznuaa@...il.com; maz@...nel.org;
> ntb@...ts.linux.dev; Peng Fan <peng.fan@....com>; robh+dt@...nel.org;
> s.hauer@...gutronix.de; shawnguo@...nel.org; tglx@...utronix.de
> Subject: Re: [EXT] Re: [PATCH v13 2/2] PCI: endpoint: pci-epf-vntb: using
> platform MSI as doorbell
>
> Caution: EXT Email
>
> On Thu, Nov 24, 2022 at 06:03:40PM +0000, Frank Li wrote:
> >
> >
> > > -----Original Message-----
> > > From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
> > > Sent: Thursday, November 24, 2022 3:00 AM
> > > To: Frank Li <frank.li@....com>
> > > Cc: lpieralisi@...nel.org; Aisheng Dong <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; dl-linux-imx <linux-imx@....com>;
> linux-
> > > kernel@...r.kernel.org; linux-pci@...r.kernel.org;
> > > lorenzo.pieralisi@....com; lznuaa@...il.com; maz@...nel.org;
> > > ntb@...ts.linux.dev; Peng Fan <peng.fan@....com>; robh+dt@...nel.org;
> > > s.hauer@...gutronix.de; shawnguo@...nel.org; tglx@...utronix.de
> > > Subject: [EXT] Re: [PATCH v13 2/2] PCI: endpoint: pci-epf-vntb: using
> platform
> > > MSI as doorbell
> > >
> > > Caution: EXT Email
> > >
> > > On Thu, Nov 24, 2022 at 12:50:36AM -0500, Frank Li wrote:
> > > > ┌────────────┐ ┌──────────────
> ─
> > > ────────────────────┐ ┌───────
> ──
> > > ───────┐
> > > > │ │ │ │ │ │
> > > > │ │ │ PCI Endpoint │ │ PCI Host │
> > > > │ │ │ │ │ │
> > > > │ │◄──┤ 1.platform_msi_domain_alloc_irqs()│ │
> > > │
> > > > │ │ │ │ │ │
> > > > │ MSI ├──►│ 2.write_msi_msg() ├──►├─
> BAR<n>
> > > │
> > > > │ Controller │ │ update doorbell register address│ │ │
> > > > │ │ │ for BAR │ │ │
> > > > │ │ │ │ │ 3. Write BAR<n>│
> > > > │ │◄──┼─────────────────────
> ─
> > > ─────────────┼───┤ │
> > > > │ │ │ │ │ │
> > > > │ ├──►│ 4.Irq Handle │ │ │
> > > > │ │ │ │ │ │
> > > > │ │ │ │ │ │
> > > > └────────────┘ └──────────────
> ─
> > > ────────────────────┘ └───────
> ──
> > > ───────┘
> > > >
> > >
> > > There are at least couple of BAR regions used in this patch but they were
> not
> > > mentioned in the above diagram.
> >
> > This patch just affected one BAR regions. Do you like "BAR[DB]"?
> >
> > Do you want to me draw other BARs, which used by this function?
> >
>
> It'd be good to just mention DB BAR.
>
> > >
> > > The subject should be:
> > >
> > > "PCI: endpoint: pci-epf-vntb: Use EP MSI controller to handle DB from
> host"
> > >
> > > > Using platform MSI interrupt controller as endpoint(EP)'s doorbell.
> > > >
> > >
> > > Above line is not needed.
> > >
> > > > The memory assigned for BAR region by the PCI host is mapped to the
> > >
> > > Which BAR? (BAR 1 aka. DB BAR)? There are multiple BAR regions
> exposed by
> > > this function driver.
> > >
> > > > message address of platform msi interrupt controller in PCI Endpoint.
> > >
> > > s/msi/MSI. Also, use either Endpoint or EP, pick one but not both.
> > >
> > > > Such that, whenever the PCI host writes to the BAR region, it will
> > > > trigger an IRQ in the EP.
> > > >
> > > > Basic working follow as
> > >
> > > "work flow is"?
> > >
> > > > 1. EP function driver call platform_msi_domain_alloc_irqs() alloc a
> > >
> > > pci-epf-vntb function driver calls platform_msi_domain_alloc_irqs() to
> > > allocate
> > > MSI's from the platform MSI controller.
> > >
> > > > MSI irq from MSI controller with call back function write_msi_msg();
> > > > 2. write_msg_msg will config BAR and map to address defined in
> msi_msg;
> > >
> > > The epf_ntb_write_msi_msg() passed as a callback will write the offset of
> the
> > > MSI controller's MSI address dedicated for each MSI to the doorbell
> register
> > > db_offset and also writes the MSI data to db_data register in the CTRL
> BAR
> > > region.
> > >
> > > > 3. Host side trigger an IRQ at Endpoint by write to BAR region.
> > > >
> > >
> > > Finally, the host can trigger doorbell by reading the offset of the doorbell
> > > from db_offset register and writing the data read from db_data register
> in
> > > CTRL
> > > BAR region to the computed address in the DB BAR region.
> > >
> > > > Add MSI doorbell support for pci-epf-vntb. Query if system has an MSI
> > > > controller. Set up doorbell address according to struct msi_msg.
> > > >
> > > > So PCI host can write this doorbell address to trigger EP side's IRQ.
> > > >
> > > > If no MSI controller exists, fall back to software polling.
> > > >
> > >
> > > "Add doorbell support to pci-epf-vntb function driver making use of the
> > > platform
> > > MSI controller. If the MSI controller is not available, fallback to the polling
> > > method."
> > >
> > > Also, please move this paragraph to the beginning of the description.
> > >
> > > > Signed-off-by: Frank Li <Frank.Li@....com>
> > > > ---
> > > > drivers/pci/endpoint/functions/pci-epf-vntb.c | 146 +++++++++++++++-
> --
> > > > 1 file changed, 125 insertions(+), 21 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > > b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > > > index 0d744975f815..f770a068e58c 100644
> > > > --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > > > +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > > > @@ -44,6 +44,7 @@
> > > > #include <linux/pci-epc.h>
> > > > #include <linux/pci-epf.h>
> > > > #include <linux/ntb.h>
> > > > +#include <linux/msi.h>
> > > >
> > > > static struct workqueue_struct *kpcintb_workqueue;
> > > >
> > > > @@ -137,11 +138,14 @@ struct epf_ntb {
> > > > struct epf_ntb_ctrl *reg;
> > > >
> > > > u32 *epf_db;
> > > > + phys_addr_t epf_db_phys;
> > > >
> > > > phys_addr_t vpci_mw_phy[MAX_MW];
> > > > void __iomem *vpci_mw_addr[MAX_MW];
> > > >
> > > > struct delayed_work cmd_handler;
> > > > +
> > > > + int msi_virqbase;
> > > > };
> > >
> > > You should add kernel doc comments for this struct in a separate patch. It
> > > will
> > > help in understanding the driver better.
> > >
> > > >
> > > > #define to_epf_ntb(epf_group) container_of((epf_group), struct
> epf_ntb,
> > > group)
> > > > @@ -256,11 +260,13 @@ static void epf_ntb_cmd_handler(struct
> > > work_struct *work)
> > > >
> > > > ntb = container_of(work, struct epf_ntb, cmd_handler.work);
> > > >
> > > > - for (i = 1; i < ntb->db_count; i++) {
> > > > - if (ntb->epf_db[i]) {
> > > > - ntb->db |= 1 << (i - 1);
> > > > - ntb_db_event(&ntb->ntb, i);
> > > > - ntb->epf_db[i] = 0;
> > >
> > > A comment here stating that polling is implemented would be better.
> > >
> > > > + if (!ntb->epf_db_phys) {
> > > > + for (i = 1; i < ntb->db_count; i++) {
> > > > + if (ntb->epf_db[i]) {
> > > > + ntb->db |= 1 << (i - 1);
> > > > + ntb_db_event(&ntb->ntb, i);
> > > > + ntb->epf_db[i] = 0;
> > > > + }
> > > > }
> > > > }
> > > >
> > > > @@ -518,6 +524,28 @@ static int epf_ntb_configure_interrupt(struct
> > > epf_ntb *ntb)
> > > > return 0;
> > > > }
> > > >
> > > > +static int epf_ntb_db_size(struct epf_ntb *ntb)
> > > > +{
> > > > + const struct pci_epc_features *epc_features;
> > > > + size_t size = sizeof(u32) * ntb->db_count;
> > > > + u32 align;
> > > > +
> > > > + epc_features = pci_epc_get_features(ntb->epf->epc,
> > > > + ntb->epf->func_no,
> > > > + ntb->epf->vfunc_no);
> > > > + align = epc_features->align;
> > > > +
> > > > + if (size < 128)
> > >
> > > Shouldn't this be (size > 128)?
> >
> > This is one coming from pci-epf-ntb.c.
> > Not sure there are some EP hardware have such limitation.
> >
>
> I'm not sure if that is correct though. drivers/ntb/hw/epf/ntb_hw_epf.c sets
> the upper limit to 32 (NTB_EPF_MAX_DB_COUNT + 1) DBs, in that case the
> size
> cannot go beyond 128.
I don’t think so. Please check
drivers/pci/endpoint/functions/pci-epf-ntb.c
Looks like some EP hardware have mini windows map size requirement.
I think it is not important for this patch.
>
> Thanks,
> Mani
> --
> மணிவண்ணன் சதாசிவம்
Powered by blists - more mailing lists