| Message ID | 20241015-ep-msi-v3-4-cedc89a16c1a@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: EP: Add RC-to-EP doorbell with platform MSI controller | expand |
Hello Frank, On Tue, Oct 15, 2024 at 06:07:17PM -0400, Frank Li wrote: > Add three registers: doorbell_bar, doorbell_addr, and doorbell_data, > along with doorbell_done. Use pci_epf_alloc_doorbell() to allocate a > doorbell address space. > > Enable the Root Complex (RC) side driver to trigger pci-epc-test's doorbell > callback handler by writing doorbell_data to the mapped doorbell_bar's > address space. > > Set doorbell_done in the doorbell callback to indicate completion. > > To avoid broken compatibility, use new PID/VID and set RevID bigger than 0. > So only new pcitest program can distinguish with/without doorbell support > and avoid wrongly write test data to doorbell bar. > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > --- > drivers/pci/endpoint/functions/pci-epf-test.c | 58 ++++++++++++++++++++++++++- > 1 file changed, 56 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c > index 7c2ed6eae53ad..c054d621353a6 100644 > --- a/drivers/pci/endpoint/functions/pci-epf-test.c > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c > @@ -11,12 +11,14 @@ > #include <linux/dmaengine.h> > #include <linux/io.h> > #include <linux/module.h> > +#include <linux/msi.h> > #include <linux/slab.h> > #include <linux/pci_ids.h> > #include <linux/random.h> > > #include <linux/pci-epc.h> > #include <linux/pci-epf.h> > +#include <linux/pci-ep-msi.h> > #include <linux/pci_regs.h> > > #define IRQ_TYPE_INTX 0 > @@ -39,6 +41,7 @@ > #define STATUS_IRQ_RAISED BIT(6) > #define STATUS_SRC_ADDR_INVALID BIT(7) > #define STATUS_DST_ADDR_INVALID BIT(8) > +#define STATUS_DOORBELL_SUCCESS BIT(9) > > #define FLAG_USE_DMA BIT(0) > > @@ -50,6 +53,7 @@ struct pci_epf_test { > void *reg[PCI_STD_NUM_BARS]; > struct pci_epf *epf; > enum pci_barno test_reg_bar; > + enum pci_barno doorbell_bar; > size_t msix_table_offset; > struct delayed_work cmd_handler; > struct dma_chan *dma_chan_tx; > @@ -74,6 +78,9 @@ struct pci_epf_test_reg { > u32 irq_type; > u32 irq_number; > u32 flags; > + u32 doorbell_bar; > + u32 doorbell_addr; > + u32 doorbell_data; > } __packed; > > static struct pci_epf_header test_header = { > @@ -695,7 +702,7 @@ static int pci_epf_test_set_bar(struct pci_epf *epf) > enum pci_barno test_reg_bar = epf_test->test_reg_bar; > > for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) { > - if (!epf_test->reg[bar]) > + if (!epf_test->reg[bar] && bar != epf_test->doorbell_bar) > continue; > > ret = pci_epc_set_bar(epc, epf->func_no, epf->vfunc_no, > @@ -810,11 +817,24 @@ static int pci_epf_test_link_down(struct pci_epf *epf) > return 0; > } > > +static int pci_epf_test_doorbell(struct pci_epf *epf, int index) > +{ > + struct pci_epf_test *epf_test = epf_get_drvdata(epf); > + enum pci_barno test_reg_bar = epf_test->test_reg_bar; > + struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar]; > + > + reg->status |= STATUS_DOORBELL_SUCCESS; > + pci_epf_test_raise_irq(epf_test, reg); > + > + return 0; > +} > + > static const struct pci_epc_event_ops pci_epf_test_event_ops = { > .epc_init = pci_epf_test_epc_init, > .epc_deinit = pci_epf_test_epc_deinit, > .link_up = pci_epf_test_link_up, > .link_down = pci_epf_test_link_down, > + .doorbell = pci_epf_test_doorbell, > }; > > static int pci_epf_test_alloc_space(struct pci_epf *epf) > @@ -853,7 +873,7 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf) > if (bar == NO_BAR) > break; > > - if (bar == test_reg_bar) > + if (bar == test_reg_bar || bar == epf_test->doorbell_bar) > continue; > > base = pci_epf_alloc_space(epf, bar_size[bar], bar, > @@ -887,7 +907,11 @@ static int pci_epf_test_bind(struct pci_epf *epf) > struct pci_epf_test *epf_test = epf_get_drvdata(epf); > const struct pci_epc_features *epc_features; > enum pci_barno test_reg_bar = BAR_0; > + enum pci_barno doorbell_bar = NO_BAR; > struct pci_epc *epc = epf->epc; > + struct msi_msg *msg; > + u64 doorbell_addr; > + u32 align; > > if (WARN_ON_ONCE(!epc)) > return -EINVAL; > @@ -905,10 +929,40 @@ static int pci_epf_test_bind(struct pci_epf *epf) > epf_test->test_reg_bar = test_reg_bar; > epf_test->epc_features = epc_features; > > + align = epc_features->align; > + align = align ? align : 128; > + > + /* Only revid >=1 support RC-to-EP Door bell */ > + ret = epf->header->revid > 0 ? pci_epf_alloc_doorbell(epf, 1) : -EINVAL; I really, really don't like this idea. This means that you would need to write a revid > 1 in configfs to test this. I also don't think that it is right that pci-epf-test takes ownership of "rev". How about something like this instead: My thinking is that you add a doorbell_capable struct member to epc_features, and then populate CAPS_DOORBELL_SUPPORT based on epc_features in pci_epf_test_init_caps() (similar to how my proposal sets CAPS_MSI_SUPPORT). From 0f6bb535c6d56e03e9b3550194deec04a1c1d370 Mon Sep 17 00:00:00 2001 From: Niklas Cassel <cassel@kernel.org> Date: Fri, 18 Oct 2024 10:32:39 +0200 Subject: [PATCH] PCI: endpoint: pci-epf-test: Add support for exposing EPC capabilities Currently, there is no way for the pci-endpoint-test driver (RC side), to know which features the EPC supports. Expose some of the EPC:s capabilities in the test_reg_bar, such that the pci-endpoint-test driver can know if a feature (e.g. MSI-X or DMA) is supported before attempting to test it. Signed-off-by: Niklas Cassel <cassel@kernel.org> --- drivers/misc/pci_endpoint_test.c | 34 +++++++++++++++ drivers/pci/endpoint/functions/pci-epf-test.c | 43 +++++++++++++++++++ 2 files changed, 77 insertions(+) diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c index 3aaaf47fa4ee..7eb045dc81b6 100644 --- a/drivers/misc/pci_endpoint_test.c +++ b/drivers/misc/pci_endpoint_test.c @@ -69,6 +69,20 @@ #define PCI_ENDPOINT_TEST_FLAGS 0x2c #define FLAG_USE_DMA BIT(0) +#define CAPS_MAGIC 0x25ccf687 +#define PCI_ENDPOINT_TEST_CAPS_MAGIC 0x30 +#define PCI_ENDPOINT_TEST_CAPS_VERSION 0x34 +#define PCI_ENDPOINT_TEST_CAPS 0x38 + +#define CAPS_MSI_SUPPORT BIT(0) +#define CAPS_MSIX_SUPPORT BIT(1) +#define CAPS_DMA_SUPPORT BIT(2) +#define CAPS_DMA_IS_PRIVATE BIT(3) /* only valid if DMA_SUPPORT */ +#define CAPS_DOORBELL_SUPPORT BIT(4) +#define CAPS_DOORBELL_BAR_MASK GENMASK(7, 5) /* only valid if DOORBELL_SUPPORT */ +#define CAPS_DOORBELL_BAR_SHIFT 5 +#define CAPS_DOORBELL_BAR(x) (((x) & CAPS_DOORBELL_BAR_MASK) >> CAPS_DOORBELL_BAR_SHIFT) + #define PCI_DEVICE_ID_TI_AM654 0xb00c #define PCI_DEVICE_ID_TI_J7200 0xb00f #define PCI_DEVICE_ID_TI_AM64 0xb010 @@ -805,6 +819,24 @@ static const struct file_operations pci_endpoint_test_fops = { .unlocked_ioctl = pci_endpoint_test_ioctl, }; +static void pci_endpoint_get_caps(struct pci_endpoint_test *test) +{ + u32 caps_magic, caps; + + /* check if endpoint has CAPS support */ + caps_magic = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CAPS_MAGIC); + if (caps_magic != CAPS_MAGIC) + return; + + caps = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CAPS); + pr_info("CAPS: MSI support: %u\n", (caps & CAPS_MSI_SUPPORT) ? 1 : 0); + pr_info("CAPS: MSI-X support: %u\n", (caps & CAPS_MSIX_SUPPORT) ? 1 : 0); + pr_info("CAPS: DMA support: %u\n", (caps & CAPS_DMA_SUPPORT) ? 1 : 0); + pr_info("CAPS: DMA is private: %u\n", (caps & CAPS_DMA_IS_PRIVATE) ? 1 : 0); + pr_info("CAPS: DOORBELL support: %u\n", (caps & CAPS_DOORBELL_SUPPORT) ? 1 : 0); + pr_info("CAPS: DOORBELL BAR: %lu\n", CAPS_DOORBELL_BAR(caps)); +} + static int pci_endpoint_test_probe(struct pci_dev *pdev, const struct pci_device_id *ent) { @@ -906,6 +938,8 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev, goto err_kfree_test_name; } + pci_endpoint_get_caps(test); + misc_device = &test->miscdev; misc_device->minor = MISC_DYNAMIC_MINOR; misc_device->name = kstrdup(name, GFP_KERNEL); diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c index a73bc0771d35..2dd90e2e8565 100644 --- a/drivers/pci/endpoint/functions/pci-epf-test.c +++ b/drivers/pci/endpoint/functions/pci-epf-test.c @@ -44,6 +44,18 @@ #define TIMER_RESOLUTION 1 +#define CAPS_MAGIC 0x25ccf687 +#define CAPS_VERSION 0x1 + +#define CAPS_MSI_SUPPORT BIT(0) +#define CAPS_MSIX_SUPPORT BIT(1) +#define CAPS_DMA_SUPPORT BIT(2) +#define CAPS_DMA_IS_PRIVATE BIT(3) /* only valid if DMA_SUPPORT */ +#define CAPS_DOORBELL_SUPPORT BIT(4) +#define CAPS_DOORBELL_BAR_MASK GENMASK(7, 5) /* only valid if DOORBELL_SUPPORT */ +#define CAPS_DOORBELL_BAR_SHIFT 5 +#define CAPS_DOORBELL_BAR(x) (((x) & CAPS_DOORBELL_BAR_MASK) >> CAPS_DOORBELL_BAR_SHIFT) + static struct workqueue_struct *kpcitest_workqueue; struct pci_epf_test { @@ -74,6 +86,9 @@ struct pci_epf_test_reg { u32 irq_type; u32 irq_number; u32 flags; + u32 caps_magic; + u32 caps_version; + u32 caps; } __packed; static struct pci_epf_header test_header = { @@ -741,6 +756,32 @@ static void pci_epf_test_clear_bar(struct pci_epf *epf) } } +static void pci_epf_test_init_caps(struct pci_epf *epf) +{ + struct pci_epf_test *epf_test = epf_get_drvdata(epf); + const struct pci_epc_features *epc_features = epf_test->epc_features; + enum pci_barno test_reg_bar = epf_test->test_reg_bar; + struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar]; + u32 caps = 0; + + reg->caps_magic = cpu_to_le32(CAPS_MAGIC); + reg->caps_version = cpu_to_le32(CAPS_VERSION); + + if (epc_features->msi_capable) + caps |= CAPS_MSI_SUPPORT; + + if (epc_features->msix_capable) + caps |= CAPS_MSIX_SUPPORT; + + if (epf_test->dma_supported) + caps |= CAPS_DMA_SUPPORT; + + if (epf_test->dma_private) + caps |= CAPS_DMA_IS_PRIVATE; + + reg->caps = cpu_to_le64(caps); +} + static int pci_epf_test_epc_init(struct pci_epf *epf) { struct pci_epf_test *epf_test = epf_get_drvdata(epf); @@ -765,6 +806,8 @@ static int pci_epf_test_epc_init(struct pci_epf *epf) } } + pci_epf_test_init_caps(epf); + ret = pci_epf_test_set_bar(epf); if (ret) return ret;
On Fri, Oct 18, 2024 at 04:01:05PM +0200, Niklas Cassel wrote: > @@ -741,6 +756,32 @@ static void pci_epf_test_clear_bar(struct pci_epf *epf) > } > } > > +static void pci_epf_test_init_caps(struct pci_epf *epf) > +{ > + struct pci_epf_test *epf_test = epf_get_drvdata(epf); > + const struct pci_epc_features *epc_features = epf_test->epc_features; > + enum pci_barno test_reg_bar = epf_test->test_reg_bar; > + struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar]; > + u32 caps = 0; > + > + reg->caps_magic = cpu_to_le32(CAPS_MAGIC); > + reg->caps_version = cpu_to_le32(CAPS_VERSION); > + > + if (epc_features->msi_capable) > + caps |= CAPS_MSI_SUPPORT; > + > + if (epc_features->msix_capable) > + caps |= CAPS_MSIX_SUPPORT; > + > + if (epf_test->dma_supported) > + caps |= CAPS_DMA_SUPPORT; > + > + if (epf_test->dma_private) > + caps |= CAPS_DMA_IS_PRIVATE; > + > + reg->caps = cpu_to_le64(caps); opps, this should have been cpu_to_le32(caps); Kind regards, Niklas
On Fri, Oct 18, 2024 at 04:01:05PM +0200, Niklas Cassel wrote: > Hello Frank, > > On Tue, Oct 15, 2024 at 06:07:17PM -0400, Frank Li wrote: > > Add three registers: doorbell_bar, doorbell_addr, and doorbell_data, > > along with doorbell_done. Use pci_epf_alloc_doorbell() to allocate a > > doorbell address space. > > > > Enable the Root Complex (RC) side driver to trigger pci-epc-test's doorbell > > callback handler by writing doorbell_data to the mapped doorbell_bar's > > address space. > > > > Set doorbell_done in the doorbell callback to indicate completion. > > > > To avoid broken compatibility, use new PID/VID and set RevID bigger than 0. > > So only new pcitest program can distinguish with/without doorbell support > > and avoid wrongly write test data to doorbell bar. > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > --- > > drivers/pci/endpoint/functions/pci-epf-test.c | 58 ++++++++++++++++++++++++++- > > 1 file changed, 56 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c > > index 7c2ed6eae53ad..c054d621353a6 100644 > > --- a/drivers/pci/endpoint/functions/pci-epf-test.c > > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c > > @@ -11,12 +11,14 @@ > > #include <linux/dmaengine.h> > > #include <linux/io.h> > > #include <linux/module.h> > > +#include <linux/msi.h> > > #include <linux/slab.h> > > #include <linux/pci_ids.h> > > #include <linux/random.h> > > > > #include <linux/pci-epc.h> > > #include <linux/pci-epf.h> > > +#include <linux/pci-ep-msi.h> > > #include <linux/pci_regs.h> > > > > #define IRQ_TYPE_INTX 0 > > @@ -39,6 +41,7 @@ > > #define STATUS_IRQ_RAISED BIT(6) > > #define STATUS_SRC_ADDR_INVALID BIT(7) > > #define STATUS_DST_ADDR_INVALID BIT(8) > > +#define STATUS_DOORBELL_SUCCESS BIT(9) > > > > #define FLAG_USE_DMA BIT(0) > > > > @@ -50,6 +53,7 @@ struct pci_epf_test { > > void *reg[PCI_STD_NUM_BARS]; > > struct pci_epf *epf; > > enum pci_barno test_reg_bar; > > + enum pci_barno doorbell_bar; > > size_t msix_table_offset; > > struct delayed_work cmd_handler; > > struct dma_chan *dma_chan_tx; > > @@ -74,6 +78,9 @@ struct pci_epf_test_reg { > > u32 irq_type; > > u32 irq_number; > > u32 flags; > > + u32 doorbell_bar; > > + u32 doorbell_addr; > > + u32 doorbell_data; > > } __packed; > > > > static struct pci_epf_header test_header = { > > @@ -695,7 +702,7 @@ static int pci_epf_test_set_bar(struct pci_epf *epf) > > enum pci_barno test_reg_bar = epf_test->test_reg_bar; > > > > for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) { > > - if (!epf_test->reg[bar]) > > + if (!epf_test->reg[bar] && bar != epf_test->doorbell_bar) > > continue; > > > > ret = pci_epc_set_bar(epc, epf->func_no, epf->vfunc_no, > > @@ -810,11 +817,24 @@ static int pci_epf_test_link_down(struct pci_epf *epf) > > return 0; > > } > > > > +static int pci_epf_test_doorbell(struct pci_epf *epf, int index) > > +{ > > + struct pci_epf_test *epf_test = epf_get_drvdata(epf); > > + enum pci_barno test_reg_bar = epf_test->test_reg_bar; > > + struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar]; > > + > > + reg->status |= STATUS_DOORBELL_SUCCESS; > > + pci_epf_test_raise_irq(epf_test, reg); > > + > > + return 0; > > +} > > + > > static const struct pci_epc_event_ops pci_epf_test_event_ops = { > > .epc_init = pci_epf_test_epc_init, > > .epc_deinit = pci_epf_test_epc_deinit, > > .link_up = pci_epf_test_link_up, > > .link_down = pci_epf_test_link_down, > > + .doorbell = pci_epf_test_doorbell, > > }; > > > > static int pci_epf_test_alloc_space(struct pci_epf *epf) > > @@ -853,7 +873,7 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf) > > if (bar == NO_BAR) > > break; > > > > - if (bar == test_reg_bar) > > + if (bar == test_reg_bar || bar == epf_test->doorbell_bar) > > continue; > > > > base = pci_epf_alloc_space(epf, bar_size[bar], bar, > > @@ -887,7 +907,11 @@ static int pci_epf_test_bind(struct pci_epf *epf) > > struct pci_epf_test *epf_test = epf_get_drvdata(epf); > > const struct pci_epc_features *epc_features; > > enum pci_barno test_reg_bar = BAR_0; > > + enum pci_barno doorbell_bar = NO_BAR; > > struct pci_epc *epc = epf->epc; > > + struct msi_msg *msg; > > + u64 doorbell_addr; > > + u32 align; > > > > if (WARN_ON_ONCE(!epc)) > > return -EINVAL; > > @@ -905,10 +929,40 @@ static int pci_epf_test_bind(struct pci_epf *epf) > > epf_test->test_reg_bar = test_reg_bar; > > epf_test->epc_features = epc_features; > > > > + align = epc_features->align; > > + align = align ? align : 128; > > + > > + /* Only revid >=1 support RC-to-EP Door bell */ > > + ret = epf->header->revid > 0 ? pci_epf_alloc_doorbell(epf, 1) : -EINVAL; > > I really, really don't like this idea. > > This means that you would need to write a revid > 1 in configfs to test this. > I also don't think that it is right that pci-epf-test takes ownership of "rev". > > How about something like this instead: > > My thinking is that you add a doorbell_capable struct member to epc_features, > and then populate CAPS_DOORBELL_SUPPORT based on epc_features in > pci_epf_test_init_caps() (similar to how my proposal sets CAPS_MSI_SUPPORT). The primary issue is that the doorbell is not a capability of the EPC itself; rather, it's a capability of the entire system that requires an external MSI/ITS controller. The CAPS_DOORBELL_SUPPORT should handle this feature. Even we needn't CAPS_DOORBELL_SUPPORT, just call pci_epf_alloc_doorbell(), if error return, means not support DOORBELL. One potential problem is that if the EPC supports CAPS_DOORBELL_SUPPORT, but the user continues to use older PID/VID values to enable EPF testing, the pcitest tool may treat the doorbell BAR as a normal BAR. This could lead to confusion for users as to why their system breaks after a kernel update. To use the doorbell functionality, the revid can clearly inform users that this feature breaks previous compatibility. Users will need to update the host-side driver, PID/VID values, and the pcitest tools accordingly. Frank > > > From 0f6bb535c6d56e03e9b3550194deec04a1c1d370 Mon Sep 17 00:00:00 2001 > From: Niklas Cassel <cassel@kernel.org> > Date: Fri, 18 Oct 2024 10:32:39 +0200 > Subject: [PATCH] PCI: endpoint: pci-epf-test: Add support for exposing EPC > capabilities > > Currently, there is no way for the pci-endpoint-test driver (RC side), > to know which features the EPC supports. > > Expose some of the EPC:s capabilities in the test_reg_bar, such that > the pci-endpoint-test driver can know if a feature (e.g. MSI-X or DMA) > is supported before attempting to test it. > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > --- > drivers/misc/pci_endpoint_test.c | 34 +++++++++++++++ > drivers/pci/endpoint/functions/pci-epf-test.c | 43 +++++++++++++++++++ > 2 files changed, 77 insertions(+) > > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c > index 3aaaf47fa4ee..7eb045dc81b6 100644 > --- a/drivers/misc/pci_endpoint_test.c > +++ b/drivers/misc/pci_endpoint_test.c > @@ -69,6 +69,20 @@ > #define PCI_ENDPOINT_TEST_FLAGS 0x2c > #define FLAG_USE_DMA BIT(0) > > +#define CAPS_MAGIC 0x25ccf687 > +#define PCI_ENDPOINT_TEST_CAPS_MAGIC 0x30 > +#define PCI_ENDPOINT_TEST_CAPS_VERSION 0x34 > +#define PCI_ENDPOINT_TEST_CAPS 0x38 > + > +#define CAPS_MSI_SUPPORT BIT(0) > +#define CAPS_MSIX_SUPPORT BIT(1) > +#define CAPS_DMA_SUPPORT BIT(2) > +#define CAPS_DMA_IS_PRIVATE BIT(3) /* only valid if DMA_SUPPORT */ > +#define CAPS_DOORBELL_SUPPORT BIT(4) > +#define CAPS_DOORBELL_BAR_MASK GENMASK(7, 5) /* only valid if DOORBELL_SUPPORT */ > +#define CAPS_DOORBELL_BAR_SHIFT 5 > +#define CAPS_DOORBELL_BAR(x) (((x) & CAPS_DOORBELL_BAR_MASK) >> CAPS_DOORBELL_BAR_SHIFT) > + > #define PCI_DEVICE_ID_TI_AM654 0xb00c > #define PCI_DEVICE_ID_TI_J7200 0xb00f > #define PCI_DEVICE_ID_TI_AM64 0xb010 > @@ -805,6 +819,24 @@ static const struct file_operations pci_endpoint_test_fops = { > .unlocked_ioctl = pci_endpoint_test_ioctl, > }; > > +static void pci_endpoint_get_caps(struct pci_endpoint_test *test) > +{ > + u32 caps_magic, caps; > + > + /* check if endpoint has CAPS support */ > + caps_magic = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CAPS_MAGIC); > + if (caps_magic != CAPS_MAGIC) > + return; > + > + caps = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CAPS); > + pr_info("CAPS: MSI support: %u\n", (caps & CAPS_MSI_SUPPORT) ? 1 : 0); > + pr_info("CAPS: MSI-X support: %u\n", (caps & CAPS_MSIX_SUPPORT) ? 1 : 0); > + pr_info("CAPS: DMA support: %u\n", (caps & CAPS_DMA_SUPPORT) ? 1 : 0); > + pr_info("CAPS: DMA is private: %u\n", (caps & CAPS_DMA_IS_PRIVATE) ? 1 : 0); > + pr_info("CAPS: DOORBELL support: %u\n", (caps & CAPS_DOORBELL_SUPPORT) ? 1 : 0); > + pr_info("CAPS: DOORBELL BAR: %lu\n", CAPS_DOORBELL_BAR(caps)); > +} > + > static int pci_endpoint_test_probe(struct pci_dev *pdev, > const struct pci_device_id *ent) > { > @@ -906,6 +938,8 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev, > goto err_kfree_test_name; > } > > + pci_endpoint_get_caps(test); > + > misc_device = &test->miscdev; > misc_device->minor = MISC_DYNAMIC_MINOR; > misc_device->name = kstrdup(name, GFP_KERNEL); > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c > index a73bc0771d35..2dd90e2e8565 100644 > --- a/drivers/pci/endpoint/functions/pci-epf-test.c > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c > @@ -44,6 +44,18 @@ > > #define TIMER_RESOLUTION 1 > > +#define CAPS_MAGIC 0x25ccf687 > +#define CAPS_VERSION 0x1 > + > +#define CAPS_MSI_SUPPORT BIT(0) > +#define CAPS_MSIX_SUPPORT BIT(1) > +#define CAPS_DMA_SUPPORT BIT(2) > +#define CAPS_DMA_IS_PRIVATE BIT(3) /* only valid if DMA_SUPPORT */ > +#define CAPS_DOORBELL_SUPPORT BIT(4) > +#define CAPS_DOORBELL_BAR_MASK GENMASK(7, 5) /* only valid if DOORBELL_SUPPORT */ > +#define CAPS_DOORBELL_BAR_SHIFT 5 > +#define CAPS_DOORBELL_BAR(x) (((x) & CAPS_DOORBELL_BAR_MASK) >> CAPS_DOORBELL_BAR_SHIFT) > + > static struct workqueue_struct *kpcitest_workqueue; > > struct pci_epf_test { > @@ -74,6 +86,9 @@ struct pci_epf_test_reg { > u32 irq_type; > u32 irq_number; > u32 flags; > + u32 caps_magic; > + u32 caps_version; > + u32 caps; > } __packed; > > static struct pci_epf_header test_header = { > @@ -741,6 +756,32 @@ static void pci_epf_test_clear_bar(struct pci_epf *epf) > } > } > > +static void pci_epf_test_init_caps(struct pci_epf *epf) > +{ > + struct pci_epf_test *epf_test = epf_get_drvdata(epf); > + const struct pci_epc_features *epc_features = epf_test->epc_features; > + enum pci_barno test_reg_bar = epf_test->test_reg_bar; > + struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar]; > + u32 caps = 0; > + > + reg->caps_magic = cpu_to_le32(CAPS_MAGIC); > + reg->caps_version = cpu_to_le32(CAPS_VERSION); > + > + if (epc_features->msi_capable) > + caps |= CAPS_MSI_SUPPORT; > + > + if (epc_features->msix_capable) > + caps |= CAPS_MSIX_SUPPORT; > + > + if (epf_test->dma_supported) > + caps |= CAPS_DMA_SUPPORT; > + > + if (epf_test->dma_private) > + caps |= CAPS_DMA_IS_PRIVATE; > + > + reg->caps = cpu_to_le64(caps); > +} > + > static int pci_epf_test_epc_init(struct pci_epf *epf) > { > struct pci_epf_test *epf_test = epf_get_drvdata(epf); > @@ -765,6 +806,8 @@ static int pci_epf_test_epc_init(struct pci_epf *epf) > } > } > > + pci_epf_test_init_caps(epf); > + > ret = pci_epf_test_set_bar(epf); > if (ret) > return ret; > -- > 2.47.0 >
On Fri, Oct 18, 2024 at 11:13:34AM -0400, Frank Li wrote: > On Fri, Oct 18, 2024 at 04:01:05PM +0200, Niklas Cassel wrote: > > > + /* Only revid >=1 support RC-to-EP Door bell */ > > > + ret = epf->header->revid > 0 ? pci_epf_alloc_doorbell(epf, 1) : -EINVAL; > > > > I really, really don't like this idea. > > > > This means that you would need to write a revid > 1 in configfs to test this. > > I also don't think that it is right that pci-epf-test takes ownership of "rev". > > > > How about something like this instead: > > > > My thinking is that you add a doorbell_capable struct member to epc_features, > > and then populate CAPS_DOORBELL_SUPPORT based on epc_features in > > pci_epf_test_init_caps() (similar to how my proposal sets CAPS_MSI_SUPPORT). > > The primary issue is that the doorbell is not a capability of the EPC > itself; rather, it's a capability of the entire system that requires an > external MSI/ITS controller. The CAPS_DOORBELL_SUPPORT should handle this > feature. Even we needn't CAPS_DOORBELL_SUPPORT, just call > pci_epf_alloc_doorbell(), if error return, means not support DOORBELL. Well, the idea is that CAPS_DOORBELL_SUPPORT bit is to tell the host side driver (pci-endpoint-test.c) that the EPF supports doorbell. In other words, if pcitest -B is executed, but the EP (pci-epf-test) does not set the CAPS_DOORBELL_SUPPORT bit to one in the CAPS register, the host side driver (pci-endpoint-test.c) can error out immediately, no need to even trying to send any command to the EP. (We can do the same with MSI and MSI-X, no need to send a command to the EP if the EP has CAPS (CAPS_MAGIC is set), but does not indicate support for MSI/MSI-X.) > To use the doorbell functionality, the revid can clearly inform users that > this feature breaks previous compatibility. Users will need to update the > host-side driver, PID/VID values, and the pcitest tools accordingly. I still really don't like the revid idea. > One potential problem is that if the EPC supports CAPS_DOORBELL_SUPPORT, > but the user continues to use older PID/VID values to enable EPF testing, > the pcitest tool may treat the doorbell BAR as a normal BAR. This could > lead to confusion for users as to why their system breaks after a kernel > update. How about we add a new pcitest --set-doorbell-mode option (that is similar to pcitest -i which sets the interrupt mode to use). That way, we can do: ./pcitest --set-doorbell-mode 1 (This will enable doorbell for e.g. BAR0, pci-epf-test will call pci_epf_alloc_doorbell() when receiving this command from the RC side. The command will return failure if pci_epf_alloc_doorbell() returned failure.) ./pcitest -B (This will perform the doorbell test) ./pcitest --set-doorbell-mode 0 (This will disable the doorbell for BAR0, so it will again not trigger IRQs when BAR0 is written, and pcitest's tests to read/write the BARs will again behave as expected.) (We probably also need another option pcitest --get-doorbell-mode.) I think this should solve all your concerns. Thoughts? Kind regards, Niklas
On Sun, Oct 20, 2024 at 04:41:25PM +0200, Niklas Cassel wrote: > On Fri, Oct 18, 2024 at 11:13:34AM -0400, Frank Li wrote: > > On Fri, Oct 18, 2024 at 04:01:05PM +0200, Niklas Cassel wrote: > > How about we add a new pcitest --set-doorbell-mode option > (that is similar to pcitest -i which sets the interrupt mode to use). > > That way, we can do: > ./pcitest --set-doorbell-mode 1 > (This will enable doorbell for e.g. BAR0, pci-epf-test will call > pci_epf_alloc_doorbell() when receiving this command from the RC side. > The command will return failure if pci_epf_alloc_doorbell() returned failure.) > > ./pcitest -B > (This will perform the doorbell test) > > ./pcitest --set-doorbell-mode 0 > (This will disable the doorbell for BAR0, > so it will again not trigger IRQs when BAR0 is written, > and pcitest's tests to read/write the BARs will again behave as expected.) > > (We probably also need another option pcitest --get-doorbell-mode.) > > I think this should solve all your concerns. Thoughts? And just to clarify, if we go with the --set-doorbell-mode approach, then my previous idea (introducing capabilities in pci-epf-test and pci-endpoint-test) is no longer a necessity. Kind regards, Niklas
On Mon, Oct 21, 2024 at 08:55:38AM +0200, Niklas Cassel wrote: > On Sun, Oct 20, 2024 at 04:41:25PM +0200, Niklas Cassel wrote: > > On Fri, Oct 18, 2024 at 11:13:34AM -0400, Frank Li wrote: > > > On Fri, Oct 18, 2024 at 04:01:05PM +0200, Niklas Cassel wrote: > > > > How about we add a new pcitest --set-doorbell-mode option > > (that is similar to pcitest -i which sets the interrupt mode to use). > > > > That way, we can do: > > ./pcitest --set-doorbell-mode 1 > > (This will enable doorbell for e.g. BAR0, pci-epf-test will call > > pci_epf_alloc_doorbell() when receiving this command from the RC side. > > The command will return failure if pci_epf_alloc_doorbell() returned failure.) > > > > ./pcitest -B > > (This will perform the doorbell test) > > > > ./pcitest --set-doorbell-mode 0 > > (This will disable the doorbell for BAR0, > > so it will again not trigger IRQs when BAR0 is written, > > and pcitest's tests to read/write the BARs will again behave as expected.) > > > > (We probably also need another option pcitest --get-doorbell-mode.) > > > > I think this should solve all your concerns. Thoughts? > > And just to clarify, if we go with the --set-doorbell-mode approach, > then my previous idea (introducing capabilities in pci-epf-test and > pci-endpoint-test) is no longer a necessity. Yes, the problem is that it needs dynamatic switch bar mapping address. I am not sure all EPC support it. DWC should support it because I did it for vntb driver. But bar's size should be issue. PCI don't support change bar's size after pci bus scan devices. ITS's offset is 0x40. Anyway, ITS + DWC should work. Frank > > > Kind regards, > Niklas
diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c index 7c2ed6eae53ad..c054d621353a6 100644 --- a/drivers/pci/endpoint/functions/pci-epf-test.c +++ b/drivers/pci/endpoint/functions/pci-epf-test.c @@ -11,12 +11,14 @@ #include <linux/dmaengine.h> #include <linux/io.h> #include <linux/module.h> +#include <linux/msi.h> #include <linux/slab.h> #include <linux/pci_ids.h> #include <linux/random.h> #include <linux/pci-epc.h> #include <linux/pci-epf.h> +#include <linux/pci-ep-msi.h> #include <linux/pci_regs.h> #define IRQ_TYPE_INTX 0 @@ -39,6 +41,7 @@ #define STATUS_IRQ_RAISED BIT(6) #define STATUS_SRC_ADDR_INVALID BIT(7) #define STATUS_DST_ADDR_INVALID BIT(8) +#define STATUS_DOORBELL_SUCCESS BIT(9) #define FLAG_USE_DMA BIT(0) @@ -50,6 +53,7 @@ struct pci_epf_test { void *reg[PCI_STD_NUM_BARS]; struct pci_epf *epf; enum pci_barno test_reg_bar; + enum pci_barno doorbell_bar; size_t msix_table_offset; struct delayed_work cmd_handler; struct dma_chan *dma_chan_tx; @@ -74,6 +78,9 @@ struct pci_epf_test_reg { u32 irq_type; u32 irq_number; u32 flags; + u32 doorbell_bar; + u32 doorbell_addr; + u32 doorbell_data; } __packed; static struct pci_epf_header test_header = { @@ -695,7 +702,7 @@ static int pci_epf_test_set_bar(struct pci_epf *epf) enum pci_barno test_reg_bar = epf_test->test_reg_bar; for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) { - if (!epf_test->reg[bar]) + if (!epf_test->reg[bar] && bar != epf_test->doorbell_bar) continue; ret = pci_epc_set_bar(epc, epf->func_no, epf->vfunc_no, @@ -810,11 +817,24 @@ static int pci_epf_test_link_down(struct pci_epf *epf) return 0; } +static int pci_epf_test_doorbell(struct pci_epf *epf, int index) +{ + struct pci_epf_test *epf_test = epf_get_drvdata(epf); + enum pci_barno test_reg_bar = epf_test->test_reg_bar; + struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar]; + + reg->status |= STATUS_DOORBELL_SUCCESS; + pci_epf_test_raise_irq(epf_test, reg); + + return 0; +} + static const struct pci_epc_event_ops pci_epf_test_event_ops = { .epc_init = pci_epf_test_epc_init, .epc_deinit = pci_epf_test_epc_deinit, .link_up = pci_epf_test_link_up, .link_down = pci_epf_test_link_down, + .doorbell = pci_epf_test_doorbell, }; static int pci_epf_test_alloc_space(struct pci_epf *epf) @@ -853,7 +873,7 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf) if (bar == NO_BAR) break; - if (bar == test_reg_bar) + if (bar == test_reg_bar || bar == epf_test->doorbell_bar) continue; base = pci_epf_alloc_space(epf, bar_size[bar], bar, @@ -887,7 +907,11 @@ static int pci_epf_test_bind(struct pci_epf *epf) struct pci_epf_test *epf_test = epf_get_drvdata(epf); const struct pci_epc_features *epc_features; enum pci_barno test_reg_bar = BAR_0; + enum pci_barno doorbell_bar = NO_BAR; struct pci_epc *epc = epf->epc; + struct msi_msg *msg; + u64 doorbell_addr; + u32 align; if (WARN_ON_ONCE(!epc)) return -EINVAL; @@ -905,10 +929,40 @@ static int pci_epf_test_bind(struct pci_epf *epf) epf_test->test_reg_bar = test_reg_bar; epf_test->epc_features = epc_features; + align = epc_features->align; + align = align ? align : 128; + + /* Only revid >=1 support RC-to-EP Door bell */ + ret = epf->header->revid > 0 ? pci_epf_alloc_doorbell(epf, 1) : -EINVAL; + if (!ret) { + msg = epf->msg; + doorbell_bar = pci_epc_get_next_free_bar(epc_features, test_reg_bar + 1); + + if (doorbell_bar > 0) { + epf_test->doorbell_bar = doorbell_bar; + doorbell_addr = msg->address_hi; + doorbell_addr <<= 32; + doorbell_addr |= msg->address_lo; + epf->bar[doorbell_bar].phys_addr = round_down(doorbell_addr, align); + epf->bar[doorbell_bar].barno = doorbell_bar; + epf->bar[doorbell_bar].size = align; + } else { + pci_epf_free_doorbell(epf); + } + } + ret = pci_epf_test_alloc_space(epf); if (ret) return ret; + if (doorbell_bar > 0) { + struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar]; + + reg->doorbell_addr = doorbell_addr & (align - 1); + reg->doorbell_data = msg->data; + reg->doorbell_bar = doorbell_bar; + } + return 0; }
Add three registers: doorbell_bar, doorbell_addr, and doorbell_data, along with doorbell_done. Use pci_epf_alloc_doorbell() to allocate a doorbell address space. Enable the Root Complex (RC) side driver to trigger pci-epc-test's doorbell callback handler by writing doorbell_data to the mapped doorbell_bar's address space. Set doorbell_done in the doorbell callback to indicate completion. To avoid broken compatibility, use new PID/VID and set RevID bigger than 0. So only new pcitest program can distinguish with/without doorbell support and avoid wrongly write test data to doorbell bar. Signed-off-by: Frank Li <Frank.Li@nxp.com> --- drivers/pci/endpoint/functions/pci-epf-test.c | 58 ++++++++++++++++++++++++++- 1 file changed, 56 insertions(+), 2 deletions(-)