Re: [PATCH 00/13] raw/ntb: post-merge fixes

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 4 Nov 2026 16:49:48 +0530
Raghavendra Ningoji <raghavendra.ningoji@amd.com> wrote:

> This series addresses the post-merge review of the AMD NTB support
> by Stephen. Some of the issues also exist in the Intel code and in
> released versions; those patches are tagged for stable.
> 
> - Patches 1-4 fix the type confusion in rte_pmd_ntb_get_mem_align(),
>   the unbounded peer memory window count, the off-by-one in the
>   address remapping and the memory window leaks.
> 
> - Patch 5 fixes the doorbell interrupts with vfio-pci: only MSI-X
>   vector 0 (DB0) was serviced, so the DB1/DB2 exchange of dev_stop
>   never completed and the stop waited for its 1s timeout.
> 
> - Patch 7 removes the AMD hardware event handling, which could not run
>   as wired, and documents that hardware events are not handled.
> 
> - Patches 8-9 fix the Rx path: a partially consumed packet was leaked
>   and desynchronized the ring, and an mbuf owned by the application
>   could be freed again on device stop.
> 
> - The other patches are documentation and cleanup.
> 
> The peer memory window size check suggested for the AMD handshake is
> not added: all the AMD scratchpads are in use, and the window limit
> register programmed with the local memzone length already bounds what
> the peer can reach.
> 
> Tested with vfio-pci on two AMD EPYC Embedded hosts: handshake,
> file transfer, and stop/quit with DB1/DB2 exchange.
> 
> Raghavendra Ningoji (13):
>   raw/ntb: fix device check in memory alignment query
>   raw/ntb: fix peer memory window count check
>   raw/ntb: fix memory window bound in address remapping
>   raw/ntb: fix memory leaks
>   raw/ntb: fix doorbell interrupts with VFIO MSI-X
>   raw/ntb: fix AMD processor names
>   raw/ntb: remove AMD hardware event handling
>   raw/ntb: fix Rx of incomplete packets
>   raw/ntb: fix double free of Rx mbufs on stop
>   examples/ntb: fix raw/ntb library linking
>   doc: add NTB device info change to ABI section
>   doc: add IOVA requirement for NTB
>   raw/ntb: remove unused AMD register definitions
> 
>  doc/guides/rawdevs/ntb.rst             |  12 ++-
>  doc/guides/rel_notes/release_26_11.rst |   6 +-
>  drivers/raw/ntb/ntb.c                  | 136 ++++++++++++++++++++-----
>  drivers/raw/ntb/ntb.h                  |   7 +-
>  drivers/raw/ntb/ntb_hw_amd.c           |  54 ++--------
>  drivers/raw/ntb/ntb_hw_amd.h           |  25 +----
>  drivers/raw/ntb/ntb_hw_intel.c         |   9 +-
>  examples/ntb/Makefile                  |   3 +-
>  8 files changed, 149 insertions(+), 103 deletions(-)
> 

Lots of AI reported issues here:

Subject: Re: [PATCH 00/13] raw/ntb: post-merge fixes

Series applies to main (26.11-rc2) and builds at every commit with
-Dwerror=true (raw/ntb + examples/ntb).

Patch 04/13 raw/ntb: fix memory leaks

Warning: mw_size is freed in dev_close, but rte_rawdev_close() does not
detach the device; it stays attached and its ops remain reachable.

  +	rte_free(hw->mw_size);
  +	hw->mw_size = NULL;

After close, rte_pmd_ntb_get_mem_align() passes the new checks from
patch 1 (attached, dev_ops == &ntb_ops, mw_id < mw_cnt) and on Intel
calls intel_ntb_get_mem_align(), which does

	return hw->mw_size[mw_id];

i.e. a NULL dereference. A configure after close does the same in
intel_ntb_dev_handshake() (hw->mw_size[i]), and ntb_dev_info_get()
now hands the application mw_cnt = 2 with mw_size = NULL; ntb_fwd
indexes ntb_info.mw_size[0] unconditionally.

Both vendors have a fixed count of 2 windows (XEON_MW_COUNT,
AMD_MW_COUNT), so drop the allocation and embed the array in
struct ntb_hw. It then lives exactly as long as dev_private, the
info->mw_size pointer stays valid, and the leak goes away with no
free at all:

	#define NTB_MAX_MW	2
	...
	uint64_t mw_size[NTB_MAX_MW];

with RTE_BUILD_BUG_ON(XEON_MW_COUNT > NTB_MAX_MW) (and AMD) in the
vendor init, and the rte_zmalloc()/NULL check removed there.

Patch 05/13 raw/ntb: fix doorbell interrupts with VFIO MSI-X

Warning: db_intr_handles is an array of process-private pointers
(handles come from rte_intr_instance_alloc(RTE_INTR_INSTANCE_F_PRIVATE))
used only by the control path. No reason to put it in hugepage memory.

  +	hw->db_intr_handles = rte_zmalloc("ntb_db_intr",
  +				nb_efd * sizeof(*hw->db_intr_handles), 0);

Use calloc(nb_efd, sizeof(*hw->db_intr_handles)) and free() in
ntb_db_intr_unregister().

Info (pre-existing, not introduced by this patch): peer_dev_up is a
plain uint8_t written by the interrupt thread (DB1/DB2 handlers,
"hw->peer_dev_up = 0;") and polled by ntb_dev_stop():

	while (hw->peer_dev_up && time_out) {

Now that DB2 is actually delivered, this loop is the normal stop path
rather than the timeout. It only works because rte_delay_us() is an
opaque call. Should be rte_atomic_store_explicit()/load with
release/acquire, as a separate patch.

Patch 08/13 raw/ntb: fix Rx of incomplete packets

Info (pre-existing, not introduced by this patch): the new on-stack

  +	struct rte_mbuf *new_mbufs[NTB_MAX_DESC_SIZE];

like the existing rx_desc[], is only bounded because nb_rx_desc is
assumed <= NTB_MAX_DESC_SIZE. ntb_rxq_setup() takes
"rxq->nb_rx_desc = rxq_conf->nb_desc;" without checking that, or that
it is a power of two (the ring indexes use & (nb_rx_desc - 1)); only
the ntb_fwd example range-checks it. The same function also leaks rxq
when rx_mp is NULL. Worth a separate fix:

	if (rxq_conf->rx_mp == NULL ||
	    rxq_conf->nb_desc < NTB_MIN_DESC_SIZE ||
	    rxq_conf->nb_desc > NTB_MAX_DESC_SIZE ||
	    !rte_is_power_of_2(rxq_conf->nb_desc))
		return -EINVAL;

placed before the rte_zmalloc_socket().

Review-Result: WARNING