Re: [PATCH v1 2/3] raw/ntb: add AMD NTB support

[Raghavendra Ningoji <raghavendra.ningoji@amd.com>]

On Fri, Sep 25, 2026 at 10:22:17AM +0000, Bruce Richardson wrote:
> Reviewing changes to common code only, please see inline below. I think
> there is quite a bit of complexity introduced by the alignment constraints
> which could do with being simplified.

Thanks Bruce, agreed on all counts. v2 reworks the alignment handling
exactly along the lines you suggested. Point-by-point below.

> > -                'ntb_hw_intel.c')
> > +                'ntb_hw_intel.c',
> > +                'ntb_hw_amd.c')
>
> Very minor nit, but consider putting the ")" on the next line and putting a
> comma after the 'ntb_hw_amd.c' entry (since meson allows a trailing comma).

Done in v2:

  sources = files(
          'ntb.c',
          'ntb_hw_intel.c',
          'ntb_hw_amd.c',
  )

> > +	/**
> > +	 * AMD NTB uses an outbound translation window: writes to a BARxx
> > +	 ...
> > +	 */
>
> Consider shortening the coment here. Also see other feedback below
> regarding this field.

The whole block is gone in v2 - ntb_dev_info_get() no longer sets any
per-vendor alignment field (see below).

> > +	uint64_t mw_addr_align;
>
> Again, shorten the comment here to just a line or two.
> Also, in terms of how it is used, remove the special case for 0 == cache
> aligned, and instead change code assignment above to be cache aligned by
> default. [...]
>
> This is really complex to have in the app [...] I would suggest that,
> rather than trying to expose this via a single addr_align value - which it
> turns out isn't actually the alignment needed - you add a separate API
> called "get_mem_align" or something similar, and then hide the complexity
> of this calculation in the driver. Then you can drop the mw_addr_align in
> the info struct.

Done. v2 drops mw_addr_align entirely and adds an experimental API:

  uint64_t rte_pmd_ntb_get_mem_align(uint16_t dev_id, uint32_t mw_id,
                                     uint64_t mw_len);

backed by a new mem_align op. The driver returns the actual base-address
alignment required for a window of the given length, so the app has no
hardware-specific logic. The AMD driver returns
max(4K, rte_align64pow2(mw_len)); the Intel driver returns the memory
window size (matching the previous mw_size_align behaviour). The app
just reserves the memzone with the returned value, no branching:

  align = rte_pmd_ntb_get_mem_align(dev_id, mz_id, mz_len);
  mz = rte_memzone_reserve_aligned(mz_name, mz_len, socket_id,
                                   RTE_MEMZONE_IOVA_CONTIG, align);

The alignment rule itself now lives in amd_ntb_get_mem_align() with a
short comment; the misaligned-base check stays in amd_ntb_mw_set_trans().

Thanks,
Raghavendra