Re: [PATCH v4 0/3] raw/ntb: add AMD NTB support

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 26 Oct 2026 11:26:01 -0400
David Marchand <david.marchand@redhat.com> wrote:

> >
> > The common NTB code validates the vendor ops once at probe time, and the
> > vendor abstraction is built around a small set of mandatory ops instead
> > of optional hooks with built-in fallbacks:
> >
> > - Patch 1 is a preparation commit that enforces the vendor ops once, at
> >   probe time, and removes the per-call NULL checks that are scattered
> >   throughout the common code.
> >
> > - Patch 2 generalizes the framework for multiple vendors. The link
> >   bring-up/tear-down is now expressed through mandatory ops
> >   (dev_handshake, peer_mw_validate, read_peer_config, link_cleanup); the
> >   former built-in scratchpad logic moved into the Intel driver. The
> >   mw_set_trans op is gone (it is now a vendor-internal concern of the
> >   handshake), and there is a single common interrupt handler with just
> >   one optional op (handle_hw_event) for vendor-specific hardware events.
> >
> > - Patch 3 adds the AMD hardware driver, its documentation, release note
> >   and MAINTAINERS entry.
> >
> > A few AMD-specific hardware details worth noting for reviewers:
> >
> > - Primary/secondary topology. One endpoint enumerates as the primary
> >   (device ID 0x14c0) and the other as the secondary (0x14c3), rather
> >   than the Intel back-to-back model.
> >
> > - Packed scratchpad handshake. The hardware exposes a single shared
> >   16-register scratchpad bank split into two disjoint 8-register sets
> >   (one per side), so the driver uses a packed handshake layout that
> >   fits in 8 registers, plugged in through the dev_handshake,
> >   peer_mw_validate and read_peer_config ops.
> >
> > - Outbound translation window. AMD NTB uses an outbound translation
> >   window: a write to a BARxx memory-window offset is forwarded to
> >   (xlat_base | offset) in the peer's memory rather than
> >   (xlat_base + offset). The translation base must therefore be aligned
> >   to a power of two >= the window length so that no offset bit collides
> >   with a set bit in the base. This is reported to applications through
> >   the rte_pmd_ntb_get_mem_align() API and honoured by the ntb example
> >   when reserving the memory window memzone.
> >
> > - Secondary-side link status. On the secondary side the device's own
> >   PCIe link status does not reflect the true inter-host link, so the
> >   speed and width are read from the upstream switch port via sysfs.
> >
> > Tested on a two-node setup (primary + secondary EPYC Embedded hosts)
> > with the dpdk-ntb example: the link comes up at the expected speed and
> > width on both sides, and file transfers are byte-correct end to end for
> > both small and large files.
> >
> > Changes since v3:
> >
> > - Simplified the probe-time vendor ops check (patch 1) to a plain
> >   per-member macro instead of an offsetof() table.
> >
> > - Removed the mw_size_align field from struct ntb_dev_info entirely
> >   (patch 2): applications must now use rte_pmd_ntb_get_mem_align(). This
> >   is documented in the API changes section of the release notes.
> >
> > - Removed the unused pmd_private field from struct ntb_hw and the whole
> >   struct amd_ntb_hw (patch 3): self_mmio is just ntb->hw_addr, peer_mmio
> >   becomes AMD_PEER_MMIO(ntb), the scratchpad offsets become an
> >   AMD_SPAD_BASE(ntb, peer) macro, and int_mask/peer_status/ctl_status
> >   were unused.
> >
> > - Sorted AMD alphabetically before Intel in meson.build, the ntb_hw
> >   header include, the PCI id mapping table and usertools/dpdk-devbind.py.
> >
> > - Moved the primary/secondary topology definitions (NTB_TOPO_PRI/SEC)
> >   from patch 2 to patch 3 where they are first used.
> >
> > - Updated the Limitation section of doc/guides/rawdevs/ntb.rst to list
> >   the supported AMD processors.  
> 
> - "external" build was broken for the example as spotted by the CI.
>   https://github.com/ovsrobot/dpdk/actions/runs/40296962846/job/121485246438
>   This is a known limitation of the .pc provided by DPDK (which Robin
> recently tried to fix).
>   I tested and squashed a simple fix:
> https://github.com/david-marchand/dpdk/commit/16b0d741ebf39b22ebb696c07f2ed44ad383bfc9
> 
> - Some commitlogs were not updated after the v4 changes, fixed.
> 
> - Some strange indentation issues were still there, fixed.
> 
> With all those fixes applied, I pushed this driver update.
> Thanks.


I think merge was premature. More detailed AI review reports:

Subject: Re: [PATCH 0/4] raw/ntb: AMD NTB support

Post-merge review of the raw/ntb series (1/4 - 3/4) against current
main. Findings below are for follow-up fixes.

Series summary
--------------
The ops-table refactor in 1/4 and 2/4 is reasonable. The main problems
are a type-confusion hole in the new public API, an unbounded
peer-supplied loop count that writes past peer_mw_base[], and AMD event
handling that cannot actually run as wired.


[PATCH 2/4] raw/ntb: generalize framework for multiple vendors
--------------------------------------------------------------

Error: rte_pmd_ntb_get_mem_align() trusts any rawdev id.

  It checks only dev_id < RTE_RAWDEV_MAX_DEVS and dev_private != NULL,
  then casts dev_private to struct ntb_hw. Passing the id of an
  unattached slot or of a non-NTB rawdev (dpaa2, ifpga, cnxk...) calls
  through a garbage ntb_ops pointer. Check the device is valid and is
  ours:

	if (!rte_rawdev_pmd_is_valid_dev(dev_id))
		return 0;
	dev = rte_rawdev_pmd_get_dev(dev_id);
	if (dev->dev_ops != &ntb_ops)
		return 0;

  Also, an out of range mw_id returns RTE_CACHE_LINE_SIZE, which
  contradicts the doxygen ("0 on error"). Return 0.

Warning: Rx EOP guard leaks the partial chain and desyncs the ring.

  On the new "Malformed rx stream" path the descriptors already walked
  have advanced last_used/last_avail and been chained under `first`,
  but nb_rx is not incremented, so the chain is never returned or
  freed. If nb_rx == 0 the refilled descriptors are not published
  either (avail_cnt not written) while rxq->last_avail has moved.
  The mbuf-alloc failure path has the same bug already; the guard
  copies it. Free the partial chain and only advance ring indexes for
  completed packets.

  The guard also bounds on nb_rx_desc, not on the descriptors the peer
  actually published (used_cnt - last_used), so it still reads entries
  the peer has not written. This is a fix for code in released
  versions; it should be its own patch with Fixes: and
  Cc: stable@dpdk.org, not folded into the refactor.

Info: removing mw_size_align changes the layout of the installed
  struct ntb_dev_info; note it under ABI Changes as well.

Info: examples/ntb/Makefile adds -lrte_raw_ntb to LDFLAGS, which also
  hits the static target. Other examples (l2fwd-crypto,
  vm_power_manager) use LDFLAGS_SHARED.


[PATCH 3/4] raw/ntb: support AMD NTB
------------------------------------

Error: peer_used_mws from the scratchpad overflows peer_mw_base[].

  amd_ntb_read_peer_config():

	ntb->peer_used_mws = (info >> 16) & 0xff;
	for (i = 0; i < ntb->peer_used_mws; i++)
		...
		ntb->peer_mw_base[i] = ...

  peer_mw_base is rte_zmalloc'd with mw_cnt (2) entries. A peer
  advertising more than 2 (bug, version skew, or just garbage in an
  uninitialised scratchpad) writes past the allocation. Intel has the
  same pattern; fix both:

	if (peer_used_mws > ntb->mw_cnt) {
		NTB_LOG(ERR, "Peer used mws %u > mw cnt %u",
			peer_used_mws, ntb->mw_cnt);
		return -EINVAL;
	}

Warning: hardware events are never delivered.

  - AMD_INTMASK_OFFSET is defined but never written, so event
    delivery depends on the reset default.
  - ntb_init_hw() enables db_cnt (16) eventfds; the comment in
    amd_ntb_vector_bind() says events have their own MSI-X vectors
    (AMD_MSIX_VECTOR_COUNT is 24), and those are never enabled.
  - handle_hw_event is only called at the bottom of
    ntb_dev_intr_handler(), after doorbells 0/1/2 have already
    returned early, so it runs only on a stray doorbell.

  As written amd_ntb_handle_hw_event() is effectively dead code.
  Either wire up the event vector and INTMASK, or drop it.

Warning: doorbell interrupts with vfio-pci MSI-X.

  amd_ntb_vector_bind() is a no-op and doorbells map 1:1 to MSI-X
  vectors. The callback is registered on the device intr handle only,
  so with vfio-pci only vector 0 (DB0) reaches ntb_dev_intr_handler().
  DB1 (peer going down) and DB2 are lost, so the stop handshake in
  ntb_dev_stop() never completes. Was this tested with vfio-pci, or
  only igb_uio? If uio only, say so in ntb.rst.

Warning: amd_ntb_handle_hw_event() switches on a bitmask.

  INTSTAT can have several bits set at once; any combination falls to
  default and is logged as unknown and never acked to the SMU. Loop
  over the set bits, or ack all of (event & AMD_EVENT_INTMASK) after
  handling D0. Also drop the duplicated fallthrough comment; one
  /* fallthrough */ is enough.

Warning: ioremap range check is off by one.

	addr <= ntb->peer_mw_base[i] + ntb->mw_size[i]

  accepts one past the end of the window. Should be <. Copied from
  intel_ntb_ioremap(), which needs the same fix.

Warning: ntb->mw_size is never freed.

  Allocated in amd_ntb_dev_init(), not freed on later ntb_init_hw()
  failure or in ntb_destroy(). Same leak exists on the Intel side;
  free it in common code at release.

Info: unused defines in ntb_hw_amd.h: AMD_SPADMUTEX_OFFSET,
  AMD_DBFM_OFFSET, AMD_INTMASK_OFFSET, AMD_MSIX_VECTOR_COUNT.

Info: amd_ntb_mw_set_trans() checks IOVA alignment, but
  rte_memzone_reserve_aligned() aligns the VA. With IOVA as PA and a
  window larger than the hugepage size the IOVA is not guaranteed to
  be aligned, and configure fails with -EINVAL. Worth a line in
  ntb.rst that large windows need IOVA as VA (or 1G pages).

Info: amd_ntb_peer_mw_validate() only compares the window count; the
  Intel version also checks sizes. Mismatched BAR sizes between hosts
  go undetected.