* [PATCH v4] PCI: endpoint: pci-epf-vntb: Honor MSI-X selection
@ 2026-10-14 5:32 Koichiro Den
2026-10-14 5:48 ` sashiko-bot
2026-10-14 14:39 ` Frank Li
0 siblings, 2 replies; 5+ messages in thread
From: Koichiro Den @ 2026-10-14 5:32 UTC (permalink / raw)
To: Manivannan Sadhasivam, Frank Li, Niklas Cassel, Bjorn Helgaas
Cc: Jon Mason, Dave Jiang, Allen Hubbe, Krzysztof Wilczyński,
Kishon Vijay Abraham I, ntb, linux-pci, linux-kernel
ntb_hw_epf tries MSI-X first and falls back to MSI. It reports the
result in COMMAND_CONFIGURE_DOORBELL. pci-epf-vntb ignores MSIX_ENABLE,
configures only MSI, and always raises peer doorbells with PCI_IRQ_MSI.
When MSI-X is selected, the host does not program MSI, so raising it can
issue a write to an invalid address. This was observed as IOMMU faults
on the RC, with no doorbell interrupts delivered.
Configure MSI-X when supported and use the selected type for peer
doorbells. Allocate the MSI-X Table and PBA in the config BAR and
configure db_count entries. Use this EPF-owned layout even if the EPC
has fixed, hardware-owned Table/PBA regions, as pci-epf-test and
nvmet-pci-epf already do.
Fixes: e35f56bb0330 ("PCI: endpoint: Support NTB transfer between RC and EP")
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes since v3:
- Reject MSI/MSI-X requests unsupported by the EPC. (Sashiko)
- Link to v3: https://lore.kernel.org/r/20260919172021.1055451-1-den@valinux.co.jp/
Changes in v3:
- Split out the vNTB MSI-X fix and use an EPF-owned layout. (Niklas)
Niklas and I both hit an issue with hardware-owned MSI-X on RK3588:
https://lore.kernel.org/r/mmcripxdekhktnn73ikylthajkdnincwu7p2nelcpcqzexpf22@rrmmzr6hfvzf/
- Link to v2: https://lore.kernel.org/r/20260830151948.3547577-4-den@valinux.co.jp/
drivers/pci/endpoint/functions/pci-epf-vntb.c | 63 ++++++++++++++-----
1 file changed, 47 insertions(+), 16 deletions(-)
diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
index 07ba338732d4..c30a73c537a0 100644
--- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
+++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
@@ -148,6 +148,7 @@ struct epf_ntb {
u16 vntb_vid;
bool linkup;
+ bool peer_msix;
/*
* True when doorbells are interrupt-driven (MSI or embedded), false
@@ -155,6 +156,7 @@ struct epf_ntb {
*/
bool msi_doorbell;
u32 spad_size;
+ u32 msix_table_offset;
enum pci_barno epf_ntb_bar[VNTB_BAR_NUM];
@@ -276,6 +278,7 @@ static void epf_ntb_teardown_mw(struct epf_ntb *ntb, u32 mw)
*/
static void epf_ntb_cmd_handler(struct work_struct *work)
{
+ const struct pci_epc_features *epc_features;
struct epf_ntb_ctrl *ctrl;
u32 command, argument;
struct epf_ntb *ntb;
@@ -308,6 +311,15 @@ static void epf_ntb_cmd_handler(struct work_struct *work)
switch (command) {
case COMMAND_CONFIGURE_DOORBELL:
+ epc_features = pci_epc_get_features(ntb->epf->epc,
+ ntb->epf->func_no,
+ ntb->epf->vfunc_no);
+ if (((argument & MSIX_ENABLE) && !epc_features->msix_capable) ||
+ (!(argument & MSIX_ENABLE) && !epc_features->msi_capable)) {
+ ctrl->command_status = COMMAND_STATUS_ERROR;
+ break;
+ }
+ ntb->peer_msix = argument & MSIX_ENABLE;
ctrl->command_status = COMMAND_STATUS_OK;
break;
case COMMAND_TEARDOWN_DOORBELL:
@@ -451,8 +463,9 @@ static void epf_ntb_config_spad_bar_free(struct epf_ntb *ntb)
* @ntb: NTB device that facilitates communication between HOST and VHOST
*
* Allocate the Local Memory mentioned in the above diagram. The size of
- * CONFIG REGION is sizeof(struct epf_ntb_ctrl) and size of SCRATCHPAD REGION
- * is obtained from "spad-count" configfs entry.
+ * CONFIG REGION is sizeof(struct epf_ntb_ctrl), plus space for the MSI-X
+ * Table and PBA if supported. The size of SCRATCHPAD REGION is obtained
+ * from "spad-count" configfs entry.
*
* Returns: Zero for success, or an error code in case of failure
*/
@@ -473,6 +486,12 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
spad_count = ntb->spad_count;
ctrl_size = ALIGN(sizeof(struct epf_ntb_ctrl), sizeof(u32));
+ if (epc_features->msix_capable) {
+ ntb->msix_table_offset = ALIGN(ctrl_size, 8);
+ ctrl_size = ntb->msix_table_offset +
+ ntb->db_count * PCI_MSIX_ENTRY_SIZE +
+ BITS_TO_U64(ntb->db_count) * sizeof(u64);
+ }
spad_size = 2 * spad_count * sizeof(u32);
base = pci_epf_alloc_space(epf, ctrl_size + spad_size,
@@ -513,12 +532,14 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
static int epf_ntb_configure_interrupt(struct epf_ntb *ntb)
{
const struct pci_epc_features *epc_features;
+ struct pci_epf *epf = ntb->epf;
struct device *dev;
int ret;
- dev = &ntb->epf->dev;
+ dev = &epf->dev;
- epc_features = pci_epc_get_features(ntb->epf->epc, ntb->epf->func_no, ntb->epf->vfunc_no);
+ epc_features = pci_epc_get_features(epf->epc, epf->func_no,
+ epf->vfunc_no);
if (!(epc_features->msix_capable || epc_features->msi_capable)) {
dev_err(dev, "MSI or MSI-X is required for doorbell\n");
@@ -532,16 +553,24 @@ static int epf_ntb_configure_interrupt(struct epf_ntb *ntb)
}
if (epc_features->msi_capable) {
- ret = pci_epc_set_msi(ntb->epf->epc,
- ntb->epf->func_no,
- ntb->epf->vfunc_no,
- 16);
+ ret = pci_epc_set_msi(epf->epc, epf->func_no, epf->vfunc_no, 16);
if (ret) {
dev_err(dev, "MSI configuration failed\n");
return ret;
}
}
+ if (epc_features->msix_capable) {
+ ret = pci_epc_set_msix(epf->epc, epf->func_no, epf->vfunc_no,
+ ntb->db_count,
+ ntb->epf_ntb_bar[BAR_CONFIG],
+ ntb->msix_table_offset);
+ if (ret) {
+ dev_err(dev, "MSI-X configuration failed\n");
+ return ret;
+ }
+ }
+
return 0;
}
@@ -1529,6 +1558,7 @@ static void vntb_epf_peer_db_work(struct work_struct *work)
struct epf_ntb *ntb = container_of(work, struct epf_ntb, peer_db_work);
struct pci_epf *epf = ntb->epf;
unsigned int budget = VNTB_PEER_DB_WORK_BUDGET;
+ unsigned int irq_type;
u8 func_no, vfunc_no;
unsigned int db_bit;
u32 interrupt_num;
@@ -1540,6 +1570,7 @@ static void vntb_epf_peer_db_work(struct work_struct *work)
func_no = epf->func_no;
vfunc_no = epf->vfunc_no;
+ irq_type = ntb->peer_msix ? PCI_IRQ_MSIX : PCI_IRQ_MSI;
/*
* Drain doorbells from peer_db_pending in snapshots (atomic64_xchg()).
@@ -1553,16 +1584,16 @@ static void vntb_epf_peer_db_work(struct work_struct *work)
while (db_bits) {
/*
- * pci_epc_raise_irq() for MSI expects a 1-based
- * interrupt number. The first usable doorbell starts
- * at EPF_IRQ_DB_START in the legacy slot layout.
+ * pci_epc_raise_irq() expects a 1-based interrupt
+ * number for MSI and MSI-X. The first usable doorbell
+ * starts at EPF_IRQ_DB_START in the legacy slot layout.
*
* Legacy mapping (kept for compatibility):
*
- * MSI #1 : link event (reserved)
- * MSI #2 : unused (historical offset)
- * MSI #3 : doorbell bit 0 (DB#0)
- * MSI #4 : doorbell bit 1 (DB#1)
+ * IRQ #1 : link event (reserved)
+ * IRQ #2 : unused (historical offset)
+ * IRQ #3 : doorbell bit 0 (DB#0)
+ * IRQ #4 : doorbell bit 1 (DB#1)
* ...
*
* Do not change this mapping to avoid breaking
@@ -1573,7 +1604,7 @@ static void vntb_epf_peer_db_work(struct work_struct *work)
db_bits &= ~BIT_ULL(db_bit);
ret = pci_epc_raise_irq(epf->epc, func_no, vfunc_no,
- PCI_IRQ_MSI, interrupt_num);
+ irq_type, interrupt_num);
if (ret)
dev_err(&epf->dev,
"Failed to raise IRQ for interrupt_num %u: %d\n",
base-commit: 5cdbd6d3a3a147fa9975e35f2602cbec9a72a932
--
2.54.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v4] PCI: endpoint: pci-epf-vntb: Honor MSI-X selection 2026-10-14 5:32 [PATCH v4] PCI: endpoint: pci-epf-vntb: Honor MSI-X selection Koichiro Den @ 2026-10-14 5:48 ` sashiko-bot 2026-10-14 14:39 ` Frank Li 1 sibling, 0 replies; 5+ messages in thread From: sashiko-bot @ 2026-10-14 5:48 UTC (permalink / raw) To: Koichiro Den; +Cc: ntb, linux-pci Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] pci: vntb endpoint triggers out-of-bounds MSI/MSI-X vectors -- commit d370d48a0be90734250ec28295637550cdfa6363 Author: Koichiro Den <den@valinux.co.jp> PCI: endpoint: pci-epf-vntb: Honor MSI-X selection ntb_hw_epf tries MSI-X first and falls back to MSI. It reports the result in COMMAND_CONFIGURE_DOORBELL. pci-epf-vntb ignores MSIX_ENABLE, configures only MSI, and always raises peer doorbells with PCI_IRQ_MSI. > diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c > index 07ba338732d4e..c30a73c537a0b 100644 > --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c > +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c [ ... ] > @@ -308,6 +311,15 @@ static void epf_ntb_cmd_handler(struct work_struct *work) > > switch (command) { > case COMMAND_CONFIGURE_DOORBELL: > + epc_features = pci_epc_get_features(ntb->epf->epc, > + ntb->epf->func_no, > + ntb->epf->vfunc_no); > + if (((argument & MSIX_ENABLE) && !epc_features->msix_capable) || > + (!(argument & MSIX_ENABLE) && !epc_features->msi_capable)) { > + ctrl->command_status = COMMAND_STATUS_ERROR; > + break; > + } > + ntb->peer_msix = argument & MSIX_ENABLE; [Severity: High] Does epf_ntb_cmd_handler in drivers/pci/endpoint/functions/pci-epf-vntb.c only extract MSIX_ENABLE from argument while failing to extract and apply the host's allocated doorbell count (argument & DB_COUNT_MASK)? If so, could this leave the endpoint's doorbell count unbound? > ctrl->command_status = COMMAND_STATUS_OK; > break; > case COMMAND_TEARDOWN_DOORBELL: [ ... ] > @@ -532,16 +553,24 @@ static int epf_ntb_configure_interrupt(struct epf_ntb *ntb) > } > > if (epc_features->msi_capable) { > - ret = pci_epc_set_msi(ntb->epf->epc, > - ntb->epf->func_no, > - ntb->epf->vfunc_no, > - 16); > + ret = pci_epc_set_msi(epf->epc, epf->func_no, epf->vfunc_no, 16); [Severity: High] Is MSI allocation hardcoded to 16 vectors here in epf_ntb_configure_interrupt, even though ntb->db_count can be up to 32? Could this mismatch cause out-of-bounds MSI raises when MSI fallback is used? > if (ret) { > dev_err(dev, "MSI configuration failed\n"); > return ret; > } > } [ ... ] > @@ -1553,16 +1584,16 @@ static void vntb_epf_peer_db_work(struct work_struct *work) > > while (db_bits) { > /* > - * pci_epc_raise_irq() for MSI expects a 1-based > - * interrupt number. The first usable doorbell starts > - * at EPF_IRQ_DB_START in the legacy slot layout. > + * pci_epc_raise_irq() expects a 1-based interrupt > + * number for MSI and MSI-X. The first usable doorbell > + * starts at EPF_IRQ_DB_START in the legacy slot layout. > * > * Legacy mapping (kept for compatibility): > * > - * MSI #1 : link event (reserved) > - * MSI #2 : unused (historical offset) > - * MSI #3 : doorbell bit 0 (DB#0) > - * MSI #4 : doorbell bit 1 (DB#1) > + * IRQ #1 : link event (reserved) > + * IRQ #2 : unused (historical offset) > + * IRQ #3 : doorbell bit 0 (DB#0) > + * IRQ #4 : doorbell bit 1 (DB#1) > * ... > * > * Do not change this mapping to avoid breaking > @@ -1573,7 +1604,7 @@ static void vntb_epf_peer_db_work(struct work_struct *work) > db_bits &= ~BIT_ULL(db_bit); > > ret = pci_epc_raise_irq(epf->epc, func_no, vfunc_no, > - PCI_IRQ_MSI, interrupt_num); > + irq_type, interrupt_num); [Severity: High] Since interrupt_num is calculated based on the un-negotiated db_count, could this loop in vntb_epf_peer_db_work raise IRQs exceeding the host's actual allocated vectors? If this happens when the endpoint's local doorbell is triggered, could it trigger an out-of-bounds access in the controller driver's MSI/MSI-X table, leading to a wild DMA write? > if (ret) > dev_err(&epf->dev, > "Failed to raise IRQ for interrupt_num %u: %d\n", -- Sashiko AI review · https://sashiko.dev/#/patchset/20261014053256.2378817-1-den@valinux.co.jp?part=1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4] PCI: endpoint: pci-epf-vntb: Honor MSI-X selection 2026-10-14 5:32 [PATCH v4] PCI: endpoint: pci-epf-vntb: Honor MSI-X selection Koichiro Den 2026-10-14 5:48 ` sashiko-bot @ 2026-10-14 14:39 ` Frank Li 2026-10-14 15:06 ` Koichiro Den 1 sibling, 1 reply; 5+ messages in thread From: Frank Li @ 2026-10-14 14:39 UTC (permalink / raw) To: Koichiro Den Cc: Manivannan Sadhasivam, Frank Li, Niklas Cassel, Bjorn Helgaas, Jon Mason, Dave Jiang, Allen Hubbe, Krzysztof Wilczyński, Kishon Vijay Abraham I, ntb, linux-pci, linux-kernel On Wed, Oct 14, 2026 at 02:32:56PM +0900, Koichiro Den wrote: > ntb_hw_epf tries MSI-X first and falls back to MSI. It reports the > result in COMMAND_CONFIGURE_DOORBELL. pci-epf-vntb ignores MSIX_ENABLE, > configures only MSI, and always raises peer doorbells with PCI_IRQ_MSI. > When MSI-X is selected, the host does not program MSI, so raising it can > issue a write to an invalid address. This was observed as IOMMU faults > on the RC, with no doorbell interrupts delivered. > > Configure MSI-X when supported and use the selected type for peer > doorbells. Allocate the MSI-X Table and PBA in the config BAR and > configure db_count entries. Use this EPF-owned layout even if the EPC > has fixed, hardware-owned Table/PBA regions, as pci-epf-test and > nvmet-pci-epf already do. > > Fixes: e35f56bb0330 ("PCI: endpoint: Support NTB transfer between RC and EP") > Signed-off-by: Koichiro Den <den@valinux.co.jp> > --- > Changes since v3: > - Reject MSI/MSI-X requests unsupported by the EPC. (Sashiko) > - Link to v3: https://lore.kernel.org/r/20260919172021.1055451-1-den@valinux.co.jp/ > > Changes in v3: > - Split out the vNTB MSI-X fix and use an EPF-owned layout. (Niklas) > Niklas and I both hit an issue with hardware-owned MSI-X on RK3588: > https://lore.kernel.org/r/mmcripxdekhktnn73ikylthajkdnincwu7p2nelcpcqzexpf22@rrmmzr6hfvzf/ > - Link to v2: https://lore.kernel.org/r/20260830151948.3547577-4-den@valinux.co.jp/ > > drivers/pci/endpoint/functions/pci-epf-vntb.c | 63 ++++++++++++++----- > 1 file changed, 47 insertions(+), 16 deletions(-) > > diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c > index 07ba338732d4..c30a73c537a0 100644 > --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c > +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c > @@ -148,6 +148,7 @@ struct epf_ntb { > u16 vntb_vid; > > bool linkup; > + bool peer_msix; > > /* > * True when doorbells are interrupt-driven (MSI or embedded), false > @@ -155,6 +156,7 @@ struct epf_ntb { > */ > bool msi_doorbell; > u32 spad_size; > + u32 msix_table_offset; > > enum pci_barno epf_ntb_bar[VNTB_BAR_NUM]; > > @@ -276,6 +278,7 @@ static void epf_ntb_teardown_mw(struct epf_ntb *ntb, u32 mw) > */ > static void epf_ntb_cmd_handler(struct work_struct *work) > { > + const struct pci_epc_features *epc_features; > struct epf_ntb_ctrl *ctrl; > u32 command, argument; > struct epf_ntb *ntb; > @@ -308,6 +311,15 @@ static void epf_ntb_cmd_handler(struct work_struct *work) > > switch (command) { > case COMMAND_CONFIGURE_DOORBELL: > + epc_features = pci_epc_get_features(ntb->epf->epc, > + ntb->epf->func_no, > + ntb->epf->vfunc_no); > + if (((argument & MSIX_ENABLE) && !epc_features->msix_capable) || > + (!(argument & MSIX_ENABLE) && !epc_features->msi_capable)) { > + ctrl->command_status = COMMAND_STATUS_ERROR; > + break; > + } > + ntb->peer_msix = argument & MSIX_ENABLE; > ctrl->command_status = COMMAND_STATUS_OK; > break; > case COMMAND_TEARDOWN_DOORBELL: > @@ -451,8 +463,9 @@ static void epf_ntb_config_spad_bar_free(struct epf_ntb *ntb) > * @ntb: NTB device that facilitates communication between HOST and VHOST > * > * Allocate the Local Memory mentioned in the above diagram. The size of > - * CONFIG REGION is sizeof(struct epf_ntb_ctrl) and size of SCRATCHPAD REGION > - * is obtained from "spad-count" configfs entry. > + * CONFIG REGION is sizeof(struct epf_ntb_ctrl), plus space for the MSI-X > + * Table and PBA if supported. The size of SCRATCHPAD REGION is obtained > + * from "spad-count" configfs entry. > * > * Returns: Zero for success, or an error code in case of failure > */ > @@ -473,6 +486,12 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb) > spad_count = ntb->spad_count; > > ctrl_size = ALIGN(sizeof(struct epf_ntb_ctrl), sizeof(u32)); > + if (epc_features->msix_capable) { > + ntb->msix_table_offset = ALIGN(ctrl_size, 8); > + ctrl_size = ntb->msix_table_offset + > + ntb->db_count * PCI_MSIX_ENTRY_SIZE + > + BITS_TO_U64(ntb->db_count) * sizeof(u64); > + } > spad_size = 2 * spad_count * sizeof(u32); > > base = pci_epf_alloc_space(epf, ctrl_size + spad_size, > @@ -513,12 +532,14 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb) > static int epf_ntb_configure_interrupt(struct epf_ntb *ntb) > { > const struct pci_epc_features *epc_features; > + struct pci_epf *epf = ntb->epf; > struct device *dev; > int ret; > > - dev = &ntb->epf->dev; > + dev = &epf->dev; > > - epc_features = pci_epc_get_features(ntb->epf->epc, ntb->epf->func_no, ntb->epf->vfunc_no); > + epc_features = pci_epc_get_features(epf->epc, epf->func_no, > + epf->vfunc_no); > > if (!(epc_features->msix_capable || epc_features->msi_capable)) { > dev_err(dev, "MSI or MSI-X is required for doorbell\n"); > @@ -532,16 +553,24 @@ static int epf_ntb_configure_interrupt(struct epf_ntb *ntb) > } > > if (epc_features->msi_capable) { > - ret = pci_epc_set_msi(ntb->epf->epc, > - ntb->epf->func_no, > - ntb->epf->vfunc_no, > - 16); > + ret = pci_epc_set_msi(epf->epc, epf->func_no, epf->vfunc_no, 16); > if (ret) { > dev_err(dev, "MSI configuration failed\n"); > return ret; > } > } This part belong code cleanup by local variable epf. Can you move this part to prepare patch? Or pci_epc_set_msix() use ntb->epf->epc, then replace all after this patch. > > + if (epc_features->msix_capable) { > + ret = pci_epc_set_msix(epf->epc, epf->func_no, epf->vfunc_no, > + ntb->db_count, > + ntb->epf_ntb_bar[BAR_CONFIG], > + ntb->msix_table_offset); > + if (ret) { > + dev_err(dev, "MSI-X configuration failed\n"); > + return ret; > + } > + } > + > return 0; > } > > @@ -1529,6 +1558,7 @@ static void vntb_epf_peer_db_work(struct work_struct *work) > struct epf_ntb *ntb = container_of(work, struct epf_ntb, peer_db_work); > struct pci_epf *epf = ntb->epf; > unsigned int budget = VNTB_PEER_DB_WORK_BUDGET; > + unsigned int irq_type; > u8 func_no, vfunc_no; > unsigned int db_bit; > u32 interrupt_num; > @@ -1540,6 +1570,7 @@ static void vntb_epf_peer_db_work(struct work_struct *work) > > func_no = epf->func_no; > vfunc_no = epf->vfunc_no; > + irq_type = ntb->peer_msix ? PCI_IRQ_MSIX : PCI_IRQ_MSI; > > /* > * Drain doorbells from peer_db_pending in snapshots (atomic64_xchg()). > @@ -1553,16 +1584,16 @@ static void vntb_epf_peer_db_work(struct work_struct *work) > > while (db_bits) { > /* > - * pci_epc_raise_irq() for MSI expects a 1-based > - * interrupt number. The first usable doorbell starts > - * at EPF_IRQ_DB_START in the legacy slot layout. > + * pci_epc_raise_irq() expects a 1-based interrupt > + * number for MSI and MSI-X. The first usable doorbell > + * starts at EPF_IRQ_DB_START in the legacy slot layout. > * > * Legacy mapping (kept for compatibility): > * > - * MSI #1 : link event (reserved) > - * MSI #2 : unused (historical offset) > - * MSI #3 : doorbell bit 0 (DB#0) > - * MSI #4 : doorbell bit 1 (DB#1) > + * IRQ #1 : link event (reserved) > + * IRQ #2 : unused (historical offset) > + * IRQ #3 : doorbell bit 0 (DB#0) > + * IRQ #4 : doorbell bit 1 (DB#1) > * ... Needn't update these comments, the key information is #n, there are not big difference between MSI and IRQ. Frank > * > * Do not change this mapping to avoid breaking > @@ -1573,7 +1604,7 @@ static void vntb_epf_peer_db_work(struct work_struct *work) > db_bits &= ~BIT_ULL(db_bit); > > ret = pci_epc_raise_irq(epf->epc, func_no, vfunc_no, > - PCI_IRQ_MSI, interrupt_num); > + irq_type, interrupt_num); > if (ret) > dev_err(&epf->dev, > "Failed to raise IRQ for interrupt_num %u: %d\n", > > base-commit: 5cdbd6d3a3a147fa9975e35f2602cbec9a72a932 > -- > 2.54.0 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4] PCI: endpoint: pci-epf-vntb: Honor MSI-X selection 2026-10-14 14:39 ` Frank Li @ 2026-10-14 15:06 ` Koichiro Den 2026-10-14 16:39 ` Koichiro Den 0 siblings, 1 reply; 5+ messages in thread From: Koichiro Den @ 2026-10-14 15:06 UTC (permalink / raw) To: Frank Li Cc: Manivannan Sadhasivam, Frank Li, Niklas Cassel, Bjorn Helgaas, Jon Mason, Dave Jiang, Allen Hubbe, Krzysztof Wilczyński, Kishon Vijay Abraham I, ntb, linux-pci, linux-kernel On Wed, Oct 14, 2026 at 09:39:33AM -0500, Frank Li wrote: > On Wed, Oct 14, 2026 at 02:32:56PM +0900, Koichiro Den wrote: > > ntb_hw_epf tries MSI-X first and falls back to MSI. It reports the > > result in COMMAND_CONFIGURE_DOORBELL. pci-epf-vntb ignores MSIX_ENABLE, > > configures only MSI, and always raises peer doorbells with PCI_IRQ_MSI. > > When MSI-X is selected, the host does not program MSI, so raising it can > > issue a write to an invalid address. This was observed as IOMMU faults > > on the RC, with no doorbell interrupts delivered. > > > > Configure MSI-X when supported and use the selected type for peer > > doorbells. Allocate the MSI-X Table and PBA in the config BAR and > > configure db_count entries. Use this EPF-owned layout even if the EPC > > has fixed, hardware-owned Table/PBA regions, as pci-epf-test and > > nvmet-pci-epf already do. > > > > Fixes: e35f56bb0330 ("PCI: endpoint: Support NTB transfer between RC and EP") > > Signed-off-by: Koichiro Den <den@valinux.co.jp> > > --- > > Changes since v3: > > - Reject MSI/MSI-X requests unsupported by the EPC. (Sashiko) > > - Link to v3: https://lore.kernel.org/r/20260919172021.1055451-1-den@valinux.co.jp/ > > > > Changes in v3: > > - Split out the vNTB MSI-X fix and use an EPF-owned layout. (Niklas) > > Niklas and I both hit an issue with hardware-owned MSI-X on RK3588: > > https://lore.kernel.org/r/mmcripxdekhktnn73ikylthajkdnincwu7p2nelcpcqzexpf22@rrmmzr6hfvzf/ > > - Link to v2: https://lore.kernel.org/r/20260830151948.3547577-4-den@valinux.co.jp/ > > > > drivers/pci/endpoint/functions/pci-epf-vntb.c | 63 ++++++++++++++----- > > 1 file changed, 47 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c > > index 07ba338732d4..c30a73c537a0 100644 > > --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c > > +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c > > @@ -148,6 +148,7 @@ struct epf_ntb { > > u16 vntb_vid; > > > > bool linkup; > > + bool peer_msix; > > > > /* > > * True when doorbells are interrupt-driven (MSI or embedded), false > > @@ -155,6 +156,7 @@ struct epf_ntb { > > */ > > bool msi_doorbell; > > u32 spad_size; > > + u32 msix_table_offset; > > > > enum pci_barno epf_ntb_bar[VNTB_BAR_NUM]; > > > > @@ -276,6 +278,7 @@ static void epf_ntb_teardown_mw(struct epf_ntb *ntb, u32 mw) > > */ > > static void epf_ntb_cmd_handler(struct work_struct *work) > > { > > + const struct pci_epc_features *epc_features; > > struct epf_ntb_ctrl *ctrl; > > u32 command, argument; > > struct epf_ntb *ntb; > > @@ -308,6 +311,15 @@ static void epf_ntb_cmd_handler(struct work_struct *work) > > > > switch (command) { > > case COMMAND_CONFIGURE_DOORBELL: > > + epc_features = pci_epc_get_features(ntb->epf->epc, > > + ntb->epf->func_no, > > + ntb->epf->vfunc_no); > > + if (((argument & MSIX_ENABLE) && !epc_features->msix_capable) || > > + (!(argument & MSIX_ENABLE) && !epc_features->msi_capable)) { > > + ctrl->command_status = COMMAND_STATUS_ERROR; > > + break; > > + } > > + ntb->peer_msix = argument & MSIX_ENABLE; > > ctrl->command_status = COMMAND_STATUS_OK; > > break; > > case COMMAND_TEARDOWN_DOORBELL: > > @@ -451,8 +463,9 @@ static void epf_ntb_config_spad_bar_free(struct epf_ntb *ntb) > > * @ntb: NTB device that facilitates communication between HOST and VHOST > > * > > * Allocate the Local Memory mentioned in the above diagram. The size of > > - * CONFIG REGION is sizeof(struct epf_ntb_ctrl) and size of SCRATCHPAD REGION > > - * is obtained from "spad-count" configfs entry. > > + * CONFIG REGION is sizeof(struct epf_ntb_ctrl), plus space for the MSI-X > > + * Table and PBA if supported. The size of SCRATCHPAD REGION is obtained > > + * from "spad-count" configfs entry. > > * > > * Returns: Zero for success, or an error code in case of failure > > */ > > @@ -473,6 +486,12 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb) > > spad_count = ntb->spad_count; > > > > ctrl_size = ALIGN(sizeof(struct epf_ntb_ctrl), sizeof(u32)); > > + if (epc_features->msix_capable) { > > + ntb->msix_table_offset = ALIGN(ctrl_size, 8); > > + ctrl_size = ntb->msix_table_offset + > > + ntb->db_count * PCI_MSIX_ENTRY_SIZE + > > + BITS_TO_U64(ntb->db_count) * sizeof(u64); > > + } > > spad_size = 2 * spad_count * sizeof(u32); > > > > base = pci_epf_alloc_space(epf, ctrl_size + spad_size, > > @@ -513,12 +532,14 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb) > > static int epf_ntb_configure_interrupt(struct epf_ntb *ntb) > > { > > const struct pci_epc_features *epc_features; > > + struct pci_epf *epf = ntb->epf; > > struct device *dev; > > int ret; > > > > - dev = &ntb->epf->dev; > > + dev = &epf->dev; > > > > - epc_features = pci_epc_get_features(ntb->epf->epc, ntb->epf->func_no, ntb->epf->vfunc_no); > > + epc_features = pci_epc_get_features(epf->epc, epf->func_no, > > + epf->vfunc_no); > > > > if (!(epc_features->msix_capable || epc_features->msi_capable)) { > > dev_err(dev, "MSI or MSI-X is required for doorbell\n"); > > @@ -532,16 +553,24 @@ static int epf_ntb_configure_interrupt(struct epf_ntb *ntb) > > } > > > > if (epc_features->msi_capable) { > > - ret = pci_epc_set_msi(ntb->epf->epc, > > - ntb->epf->func_no, > > - ntb->epf->vfunc_no, > > - 16); > > + ret = pci_epc_set_msi(epf->epc, epf->func_no, epf->vfunc_no, 16); > > if (ret) { > > dev_err(dev, "MSI configuration failed\n"); > > return ret; > > } > > } > > This part belong code cleanup by local variable epf. Can you move this > part to prepare patch? Or pci_epc_set_msix() use ntb->epf->epc, then > replace all after this patch. I remember Bjorn mentioning this clean-up somewhere, and I thought this patch was a good chance to do that. But yeah, it makes sense to split out the clean-up. > > > > > + if (epc_features->msix_capable) { > > + ret = pci_epc_set_msix(epf->epc, epf->func_no, epf->vfunc_no, > > + ntb->db_count, > > + ntb->epf_ntb_bar[BAR_CONFIG], > > + ntb->msix_table_offset); > > + if (ret) { > > + dev_err(dev, "MSI-X configuration failed\n"); > > + return ret; > > + } > > + } > > + > > return 0; > > } > > > > @@ -1529,6 +1558,7 @@ static void vntb_epf_peer_db_work(struct work_struct *work) > > struct epf_ntb *ntb = container_of(work, struct epf_ntb, peer_db_work); > > struct pci_epf *epf = ntb->epf; > > unsigned int budget = VNTB_PEER_DB_WORK_BUDGET; > > + unsigned int irq_type; > > u8 func_no, vfunc_no; > > unsigned int db_bit; > > u32 interrupt_num; > > @@ -1540,6 +1570,7 @@ static void vntb_epf_peer_db_work(struct work_struct *work) > > > > func_no = epf->func_no; > > vfunc_no = epf->vfunc_no; > > + irq_type = ntb->peer_msix ? PCI_IRQ_MSIX : PCI_IRQ_MSI; > > > > /* > > * Drain doorbells from peer_db_pending in snapshots (atomic64_xchg()). > > @@ -1553,16 +1584,16 @@ static void vntb_epf_peer_db_work(struct work_struct *work) > > > > while (db_bits) { > > /* > > - * pci_epc_raise_irq() for MSI expects a 1-based > > - * interrupt number. The first usable doorbell starts > > - * at EPF_IRQ_DB_START in the legacy slot layout. > > + * pci_epc_raise_irq() expects a 1-based interrupt > > + * number for MSI and MSI-X. The first usable doorbell > > + * starts at EPF_IRQ_DB_START in the legacy slot layout. > > * > > * Legacy mapping (kept for compatibility): > > * > > - * MSI #1 : link event (reserved) > > - * MSI #2 : unused (historical offset) > > - * MSI #3 : doorbell bit 0 (DB#0) > > - * MSI #4 : doorbell bit 1 (DB#1) > > + * IRQ #1 : link event (reserved) > > + * IRQ #2 : unused (historical offset) > > + * IRQ #3 : doorbell bit 0 (DB#0) > > + * IRQ #4 : doorbell bit 1 (DB#1) > > * ... > > Needn't update these comments, the key information is #n, there are not > big difference between MSI and IRQ. I guess your point is to keep the diff small. Otherwise, I would also be fine with s/MSI/MSI(-X)/. I don't have a strong opinion here, so I'll leave the labels unchanged. I'll send v5. Thanks for the review! Best regards, Koichiro Den > > Frank > > > * > > * Do not change this mapping to avoid breaking > > @@ -1573,7 +1604,7 @@ static void vntb_epf_peer_db_work(struct work_struct *work) > > db_bits &= ~BIT_ULL(db_bit); > > > > ret = pci_epc_raise_irq(epf->epc, func_no, vfunc_no, > > - PCI_IRQ_MSI, interrupt_num); > > + irq_type, interrupt_num); > > if (ret) > > dev_err(&epf->dev, > > "Failed to raise IRQ for interrupt_num %u: %d\n", > > > > base-commit: 5cdbd6d3a3a147fa9975e35f2602cbec9a72a932 > > -- > > 2.54.0 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4] PCI: endpoint: pci-epf-vntb: Honor MSI-X selection 2026-10-14 15:06 ` Koichiro Den @ 2026-10-14 16:39 ` Koichiro Den 0 siblings, 0 replies; 5+ messages in thread From: Koichiro Den @ 2026-10-14 16:39 UTC (permalink / raw) To: Frank Li, Manivannan Sadhasivam Cc: Frank Li, Niklas Cassel, Bjorn Helgaas, Jon Mason, Dave Jiang, Allen Hubbe, Krzysztof Wilczyński, Kishon Vijay Abraham I, ntb, linux-pci, linux-kernel On Thu, Oct 15, 2026 at 12:06:34AM +0900, Koichiro Den wrote: > On Wed, Oct 14, 2026 at 09:39:33AM -0500, Frank Li wrote: > > On Wed, Oct 14, 2026 at 02:32:56PM +0900, Koichiro Den wrote: > > > ntb_hw_epf tries MSI-X first and falls back to MSI. It reports the > > > result in COMMAND_CONFIGURE_DOORBELL. pci-epf-vntb ignores MSIX_ENABLE, > > > configures only MSI, and always raises peer doorbells with PCI_IRQ_MSI. > > > When MSI-X is selected, the host does not program MSI, so raising it can > > > issue a write to an invalid address. This was observed as IOMMU faults > > > on the RC, with no doorbell interrupts delivered. > > > > > > Configure MSI-X when supported and use the selected type for peer > > > doorbells. Allocate the MSI-X Table and PBA in the config BAR and > > > configure db_count entries. Use this EPF-owned layout even if the EPC > > > has fixed, hardware-owned Table/PBA regions, as pci-epf-test and > > > nvmet-pci-epf already do. > > > > > > Fixes: e35f56bb0330 ("PCI: endpoint: Support NTB transfer between RC and EP") > > > Signed-off-by: Koichiro Den <den@valinux.co.jp> > > > --- > > > Changes since v3: > > > - Reject MSI/MSI-X requests unsupported by the EPC. (Sashiko) > > > - Link to v3: https://lore.kernel.org/r/20260919172021.1055451-1-den@valinux.co.jp/ > > > > > > Changes in v3: > > > - Split out the vNTB MSI-X fix and use an EPF-owned layout. (Niklas) > > > Niklas and I both hit an issue with hardware-owned MSI-X on RK3588: > > > https://lore.kernel.org/r/mmcripxdekhktnn73ikylthajkdnincwu7p2nelcpcqzexpf22@rrmmzr6hfvzf/ > > > - Link to v2: https://lore.kernel.org/r/20260830151948.3547577-4-den@valinux.co.jp/ > > > > > > drivers/pci/endpoint/functions/pci-epf-vntb.c | 63 ++++++++++++++----- > > > 1 file changed, 47 insertions(+), 16 deletions(-) > > > > > > diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c > > > index 07ba338732d4..c30a73c537a0 100644 > > > --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c > > > +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c > > > @@ -148,6 +148,7 @@ struct epf_ntb { > > > u16 vntb_vid; > > > > > > bool linkup; > > > + bool peer_msix; > > > > > > /* > > > * True when doorbells are interrupt-driven (MSI or embedded), false > > > @@ -155,6 +156,7 @@ struct epf_ntb { > > > */ > > > bool msi_doorbell; > > > u32 spad_size; > > > + u32 msix_table_offset; > > > > > > enum pci_barno epf_ntb_bar[VNTB_BAR_NUM]; > > > > > > @@ -276,6 +278,7 @@ static void epf_ntb_teardown_mw(struct epf_ntb *ntb, u32 mw) > > > */ > > > static void epf_ntb_cmd_handler(struct work_struct *work) > > > { > > > + const struct pci_epc_features *epc_features; > > > struct epf_ntb_ctrl *ctrl; > > > u32 command, argument; > > > struct epf_ntb *ntb; > > > @@ -308,6 +311,15 @@ static void epf_ntb_cmd_handler(struct work_struct *work) > > > > > > switch (command) { > > > case COMMAND_CONFIGURE_DOORBELL: > > > + epc_features = pci_epc_get_features(ntb->epf->epc, > > > + ntb->epf->func_no, > > > + ntb->epf->vfunc_no); > > > + if (((argument & MSIX_ENABLE) && !epc_features->msix_capable) || > > > + (!(argument & MSIX_ENABLE) && !epc_features->msi_capable)) { > > > + ctrl->command_status = COMMAND_STATUS_ERROR; > > > + break; > > > + } > > > + ntb->peer_msix = argument & MSIX_ENABLE; > > > ctrl->command_status = COMMAND_STATUS_OK; > > > break; > > > case COMMAND_TEARDOWN_DOORBELL: > > > @@ -451,8 +463,9 @@ static void epf_ntb_config_spad_bar_free(struct epf_ntb *ntb) > > > * @ntb: NTB device that facilitates communication between HOST and VHOST > > > * > > > * Allocate the Local Memory mentioned in the above diagram. The size of > > > - * CONFIG REGION is sizeof(struct epf_ntb_ctrl) and size of SCRATCHPAD REGION > > > - * is obtained from "spad-count" configfs entry. > > > + * CONFIG REGION is sizeof(struct epf_ntb_ctrl), plus space for the MSI-X > > > + * Table and PBA if supported. The size of SCRATCHPAD REGION is obtained > > > + * from "spad-count" configfs entry. > > > * > > > * Returns: Zero for success, or an error code in case of failure > > > */ > > > @@ -473,6 +486,12 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb) > > > spad_count = ntb->spad_count; > > > > > > ctrl_size = ALIGN(sizeof(struct epf_ntb_ctrl), sizeof(u32)); > > > + if (epc_features->msix_capable) { > > > + ntb->msix_table_offset = ALIGN(ctrl_size, 8); > > > + ctrl_size = ntb->msix_table_offset + > > > + ntb->db_count * PCI_MSIX_ENTRY_SIZE + > > > + BITS_TO_U64(ntb->db_count) * sizeof(u64); > > > + } > > > spad_size = 2 * spad_count * sizeof(u32); > > > > > > base = pci_epf_alloc_space(epf, ctrl_size + spad_size, > > > @@ -513,12 +532,14 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb) > > > static int epf_ntb_configure_interrupt(struct epf_ntb *ntb) > > > { > > > const struct pci_epc_features *epc_features; > > > + struct pci_epf *epf = ntb->epf; > > > struct device *dev; > > > int ret; > > > > > > - dev = &ntb->epf->dev; > > > + dev = &epf->dev; > > > > > > - epc_features = pci_epc_get_features(ntb->epf->epc, ntb->epf->func_no, ntb->epf->vfunc_no); > > > + epc_features = pci_epc_get_features(epf->epc, epf->func_no, > > > + epf->vfunc_no); > > > > > > if (!(epc_features->msix_capable || epc_features->msi_capable)) { > > > dev_err(dev, "MSI or MSI-X is required for doorbell\n"); > > > @@ -532,16 +553,24 @@ static int epf_ntb_configure_interrupt(struct epf_ntb *ntb) > > > } > > > > > > if (epc_features->msi_capable) { > > > - ret = pci_epc_set_msi(ntb->epf->epc, > > > - ntb->epf->func_no, > > > - ntb->epf->vfunc_no, > > > - 16); > > > + ret = pci_epc_set_msi(epf->epc, epf->func_no, epf->vfunc_no, 16); > > > if (ret) { > > > dev_err(dev, "MSI configuration failed\n"); > > > return ret; > > > } > > > } > > > > This part belong code cleanup by local variable epf. Can you move this > > part to prepare patch? Or pci_epc_set_msix() use ntb->epf->epc, then > > replace all after this patch. > > I remember Bjorn mentioning this clean-up somewhere, and I thought this patch > was a good chance to do that. But yeah, it makes sense to split out the > clean-up. > > > > > > > > > + if (epc_features->msix_capable) { > > > + ret = pci_epc_set_msix(epf->epc, epf->func_no, epf->vfunc_no, > > > + ntb->db_count, > > > + ntb->epf_ntb_bar[BAR_CONFIG], > > > + ntb->msix_table_offset); > > > + if (ret) { > > > + dev_err(dev, "MSI-X configuration failed\n"); > > > + return ret; > > > + } > > > + } > > > + > > > return 0; > > > } > > > > > > @@ -1529,6 +1558,7 @@ static void vntb_epf_peer_db_work(struct work_struct *work) > > > struct epf_ntb *ntb = container_of(work, struct epf_ntb, peer_db_work); > > > struct pci_epf *epf = ntb->epf; > > > unsigned int budget = VNTB_PEER_DB_WORK_BUDGET; > > > + unsigned int irq_type; > > > u8 func_no, vfunc_no; > > > unsigned int db_bit; > > > u32 interrupt_num; > > > @@ -1540,6 +1570,7 @@ static void vntb_epf_peer_db_work(struct work_struct *work) > > > > > > func_no = epf->func_no; > > > vfunc_no = epf->vfunc_no; > > > + irq_type = ntb->peer_msix ? PCI_IRQ_MSIX : PCI_IRQ_MSI; > > > > > > /* > > > * Drain doorbells from peer_db_pending in snapshots (atomic64_xchg()). > > > @@ -1553,16 +1584,16 @@ static void vntb_epf_peer_db_work(struct work_struct *work) > > > > > > while (db_bits) { > > > /* > > > - * pci_epc_raise_irq() for MSI expects a 1-based > > > - * interrupt number. The first usable doorbell starts > > > - * at EPF_IRQ_DB_START in the legacy slot layout. > > > + * pci_epc_raise_irq() expects a 1-based interrupt > > > + * number for MSI and MSI-X. The first usable doorbell > > > + * starts at EPF_IRQ_DB_START in the legacy slot layout. > > > * > > > * Legacy mapping (kept for compatibility): > > > * > > > - * MSI #1 : link event (reserved) > > > - * MSI #2 : unused (historical offset) > > > - * MSI #3 : doorbell bit 0 (DB#0) > > > - * MSI #4 : doorbell bit 1 (DB#1) > > > + * IRQ #1 : link event (reserved) > > > + * IRQ #2 : unused (historical offset) > > > + * IRQ #3 : doorbell bit 0 (DB#0) > > > + * IRQ #4 : doorbell bit 1 (DB#1) > > > * ... > > > > Needn't update these comments, the key information is #n, there are not > > big difference between MSI and IRQ. > > I guess your point is to keep the diff small. Otherwise, I would also be fine > with s/MSI/MSI(-X)/. I don't have a strong opinion here, so I'll leave the > labels unchanged. I'll send v5. (+To: Mani) BTW, I'm thinking of basing the next revision on [1] (thanks for the review there too). This MSI-X fix conflicts with it, and it's not just a textual conflict; the new link-event notification also needs to care about the MSI vs MSI-X selection. Since [1] already has your Reviewed-by, I expect it to land in pci/endpoint first. Please let me know if you have any objections to adding [1] as a dependency. (vNTB-DMA series [2] also conflicts with these small fixes, so I'm hesitating to send its next revision too.) [1] https://lore.kernel.org/r/20261014071448.2473208-1-den@valinux.co.jp/ [2] https://lore.kernel.org/r/20261009045949.1109871-1-den@valinux.co.jp/ Best regards, Koichiro Den > > Thanks for the review! > > Best regards, > Koichiro Den > > > > > Frank > > > > > * > > > * Do not change this mapping to avoid breaking > > > @@ -1573,7 +1604,7 @@ static void vntb_epf_peer_db_work(struct work_struct *work) > > > db_bits &= ~BIT_ULL(db_bit); > > > > > > ret = pci_epc_raise_irq(epf->epc, func_no, vfunc_no, > > > - PCI_IRQ_MSI, interrupt_num); > > > + irq_type, interrupt_num); > > > if (ret) > > > dev_err(&epf->dev, > > > "Failed to raise IRQ for interrupt_num %u: %d\n", > > > > > > base-commit: 5cdbd6d3a3a147fa9975e35f2602cbec9a72a932 > > > -- > > > 2.54.0 > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-10-14 16:39 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-10-14 5:32 [PATCH v4] PCI: endpoint: pci-epf-vntb: Honor MSI-X selection Koichiro Den 2026-10-14 5:48 ` sashiko-bot 2026-10-14 14:39 ` Frank Li 2026-10-14 15:06 ` Koichiro Den 2026-10-14 16:39 ` Koichiro Den
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.