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

[PATCH v4] 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 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

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

[Frank Li]

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
>

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

[Koichiro Den]

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

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

[Koichiro Den]

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