[PATCH v3] PCI: endpoint: pci-epf-vntb: Honor MSI-X selection

[PATCH v3] PCI: endpoint: pci-epf-vntb: Honor MSI-X selection

[Koichiro Den]

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 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/

v2: https://lore.kernel.org/r/20260830151948.3547577-4-den@valinux.co.jp/

@Frank, I did not carry over your R-b tag due to the change. I'm sending
this minimal fix separately so we can fix vNTB raising MSI when the host
selected MSI-X, without waiting for the RK3588 issue to be resolved.
Please take another look, thanks.

 drivers/pci/endpoint/functions/pci-epf-vntb.c | 54 +++++++++++++------
 1 file changed, 38 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..2f6f3da1d73f 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];
 
@@ -308,6 +310,7 @@ static void epf_ntb_cmd_handler(struct work_struct *work)
 
 	switch (command) {
 	case COMMAND_CONFIGURE_DOORBELL:
+		ntb->peer_msix = argument & MSIX_ENABLE;
 		ctrl->command_status = COMMAND_STATUS_OK;
 		break;
 	case COMMAND_TEARDOWN_DOORBELL:
@@ -451,8 +454,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 +477,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 +523,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 +544,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 +1549,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 +1561,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 +1575,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 +1595,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.51.0

Re: [PATCH v3] PCI: endpoint: pci-epf-vntb: Honor MSI-X selection

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pci: epf-vntb: missing msix_capable check leads to remote DoS
--

commit f3657c59adbc44294a8ce54de921ee0a386471b0
Author: Koichiro Den <den@valinux.co.jp>
Subject: PCI: endpoint: pci-epf-vntb: Honor MSI-X selection

This commit modifies the vNTB endpoint function to configure MSI-X when
supported by the hardware and uses the selected type for peer doorbells.
It allocates the MSI-X Table and PBA in the config BAR and configures
db_count entries.

> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c

[ ... ]

> @@ -308,6 +310,7 @@ static void epf_ntb_cmd_handler(struct work_struct *work)
>  
>  	switch (command) {
>  	case COMMAND_CONFIGURE_DOORBELL:
> +		ntb->peer_msix = argument & MSIX_ENABLE;

[Severity: High]
Does this allow the remote host to unconditionally enable MSI-X locally?

Since this takes the MSIX_ENABLE flag directly from the remote host's 
argument, could it bypass the local endpoint's hardware capabilities check 
for epc_features->msix_capable?

>  		ctrl->command_status = COMMAND_STATUS_OK;
>  		break;
>  	case COMMAND_TEARDOWN_DOORBELL:

[ ... ]

> @@ -1540,6 +1561,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()).

[ ... ]

> @@ -1573,7 +1595,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]
Can this cause a crash in the endpoint controller driver if MSI-X is not
actually supported locally?

If the remote host requests MSI-X but the local endpoint does not support
it, pci_epc_set_msix() will never be called to initialize the local MSI-X
table.

When vntb_epf_peer_db_work() later calls pci_epc_raise_irq() with
PCI_IRQ_MSIX based on the unvalidated ntb->peer_msix flag, will
the underlying controller drivers (like DesignWare) attempt to read
uninitialized hardware BIR and table offsets? Could this result in
dereferencing a NULL or out-of-bounds ep_func->epf_bar[bir]?

>  			if (ret)
>  				dev_err(&epf->dev,
>  					"Failed to raise IRQ for interrupt_num %u: %d\n",

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260919172021.1055451-1-den@valinux.co.jp?part=1