Hello, On Mon, 26 Oct 2026 at 18:00, Stephen Hemminger <[email protected]> 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: [email protected], 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
