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

[David Marchand]

Hello,

On Mon, 26 Oct 2026 at 18:00, Stephen Hemminger
<stephen at networkplumber.org> wrote:
> 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 at 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.

Please look into those warnings and send fixes if confirmed.

Thanks.


-- 
David Marchand