|WARNING| [v4,3/3] raw/ntb: add AMD NTB support

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/172065

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-10-26

# DPDK Patch Review

## Patch 1/3: raw/ntb: validate vendor ops at probe

### ERRORS

None.

### WARNINGS

None.

### INFO

**Pre-existing: `ntb_dev_intr_handler()` error path cleanup**

Lines removed at old L157-163 dropped a check around `get_link_status` but did not add the mandatory-ops enforcement in its place until the vendor_ops_check function later in the patch. Between those hunks, if `get_link_status` were NULL (which cannot happen in a correct vendor driver but is not yet enforced), the line at new L193:
```c
(*hw->ntb_ops->get_link_status)(dev);
```
would dereference NULL. This is a transient state within the patch itself and is resolved by the `ntb_vendor_ops_check()` introduction, so it does not survive to the final code. Not a finding against this patch, but noted for completeness.

---

## Patch 2/3: raw/ntb: generalize framework for multiple vendors

### ERRORS

**1. Malformed Rx stream guard has off-by-one in overflow check**

File: `drivers/raw/ntb/ntb.c`  
Lines: new L608-612

```c
if (unlikely(nb_mbufs >= rxq->nb_rx_desc)) {
    NTB_LOG(ERR, "Malformed rx stream (no EOP); "
        "aborting to avoid desc overflow.");
    goto end_of_rx;
}
```

This check triggers when `nb_mbufs >= rxq->nb_rx_desc`, but the correct condition to prevent overflow is `nb_mbufs >= rxq->nb_rx_desc`. The array `sw_ring[]` is indexed with `rxq->last_used`, and `rxq->nb_rx_desc` is the array size. The loop accesses `sw_ring[rxq->last_used]` (line new L615), so the valid range for `nb_mbufs` is `0 <= nb_mbufs < rxq->nb_rx_desc`. The current check using `>=` is correct. However, the comment "aborting to avoid desc overflow" and the condition itself suggest the intent is to prevent accessing `sw_ring[nb_rx_desc]`, which would be out of bounds.

**Correction:** The check is actually correct as written (`>=`). The loop increments `nb_mbufs` *after* using it as an index (via `rxq->last_used` which wraps modulo `nb_rx_desc`), so the guard `nb_mbufs >= rxq->nb_rx_desc` correctly prevents the `nb_mbufs`-th iteration from proceeding when all descriptors are consumed. This is not an error. Retracted.

**2. Missing `#include <eal_export.h>` placement in installed header scope**

File: `drivers/raw/ntb/ntb.c`  
Line: new L23

```c
#include <eal_export.h>
```

The `eal_export.h` header is an internal EAL header for export macros (`RTE_EXPORT_EXPERIMENTAL_SYMBOL`). Including it in `ntb.c` (a driver `.c` file) is acceptable, but if any macro or declaration from it were to leak into an installed header (`rte_pmd_ntb.h`), it would be an ABI/API hygiene issue. Checking the actual usage: the `RTE_EXPORT_EXPERIMENTAL_SYMBOL` macro is used in `ntb.c` at new L715, not in the header. The header at `drivers/raw/ntb/rte_pmd_ntb.h` (new lines) uses `__rte_experimental` from `rte_compat.h`, which is correct. No installed-header pollution occurs. Not an error.

**3. Release notes claim API removal but `mw_size_align` field removal is an ABI break**

File: `doc/guides/rel_notes/release_26_11.rst`  
Lines: new L548-551

```rst
* raw/ntb: Removed the ``mw_size_align`` field from ``struct ntb_dev_info``.
  Applications must now query the required memory window base-address
  alignment with the new ``rte_pmd_ntb_get_mem_align()`` function
  instead of relying on this field.
```

Removing a field from `struct ntb_dev_info` is an ABI break: the structure size changes, and any application compiled against the old ABI will have incorrect field offsets. The current release is 26.11 (per the patch date 2026-10-26 and the release notes file name). According to DPDK ABI policy, ABI breaks are only allowed on LTS releases (X.11) with prior deprecation notice. The patch does not show a deprecation notice being added in a prior release, nor does it use ABI versioning to provide a compat shim. Unless this is an LTS release (26.11 would be, as it ends in .11) *and* a deprecation notice was filed in 26.07 or earlier, this is an undocumented ABI break.

However, checking the structure definition in `drivers/raw/ntb/rte_pmd_ntb.h` (patch 2 diff):
```c
struct ntb_dev_info {
	uint32_t ntb_hdr_size;
-	/**< memzone needs to be mw size align or not. */
-	uint8_t mw_size_align;
	uint8_t mw_cnt;
	uint64_t *mw_size;
};
```

The `ntb_dev_info` structure is defined in an installed header (`rte_pmd_ntb.h`), so it is part of the public ABI. Removing `mw_size_align` breaks the ABI. The release notes acknowledge the removal, which is good, but the ABI change section of the release notes does not list this. The patch should either:
- Add an entry under "ABI Changes" in the release notes, or  
- Provide ABI versioning for the structure (unlikely for a simple info struct), or  
- Confirm a deprecation notice was filed in a prior release.

The "API Changes" section includes this note, but "API Changes" is for source-level changes. ABI changes belong in "ABI Changes". **Flag as an error: ABI break without corresponding "ABI Changes" entry.**

---

## Patch 3/3: raw/ntb: add AMD NTB support

### ERRORS

**1. File descriptor leak on error path in `amd_ntb_read_lnksta_sysfs()`**

File: `drivers/raw/ntb/ntb_hw_amd.c`  
Lines: new L234-270

The function opens a file descriptor:
```c
fd = open(config_path, O_RDONLY);
if (fd < 0)
    return -errno;
```

Several `pread()` calls follow, each with an error path `goto out`. The `out` label closes `fd`. However, if the first `pread()` at line ~243 fails:
```c
if (pread(fd, &status, sizeof(status), RTE_PCI_STATUS) !=
    sizeof(status))
    goto out;
```
the function jumps to `out`, sets `ret = -EIO`, and closes `fd`. This is correct.

But if the second condition at line ~246:
```c
if (!(status & RTE_PCI_STATUS_CAP_LIST))
    goto out;
```
triggers, the code also goes to `out`, closes `fd`, and returns `-EIO` (the initialized value of `ret`). This is also correct.

The loop starting at line ~254:
```c
while (pos) {
```
has two `pread()` calls and multiple `goto out` branches, all of which reach the cleanup label. The final successful read at line ~262:
```c
if (pread(fd, link_status, sizeof(*link_status),
          pos + RTE_PCI_EXP_LNKSTA) ==
    sizeof(*link_status))
    ret = 0;
goto out;
```
sets `ret = 0` and then falls through to `out`. All paths reach `close(fd)`. No leak. Not an error.

**2. Potential alignment violation in `amd_ntb_mw_set_trans()` check**

File: `drivers/raw/ntb/ntb_hw_amd.c`  
Lines: new L120-127

```c
if (addr & (rte_align64pow2(size) - 1)) {
    NTB_LOG(ERR, "mw%d translation base 0x%" PRIx64 " is not "
        "aligned to a power of two >= size 0x%" PRIx64
        "; window writes would alias.", mw_idx, addr, size);
    return -EINVAL;
}
```

The comment in the code and the commit message explain that the AMD NTB hardware uses an outbound translation of the form `(base | offset)`, so the base must be aligned to a power of two >= `size` to avoid bit collisions. The check `addr & (rte_align64pow2(size) - 1)` enforces this: `rte_align64pow2(size)` returns the next power of two >= `size`, and the mask checks alignment. This is correct.

However, the hardware also requires >= 4K alignment (per the `mem_align` op later in the file, line new L653). If `size < 4096`, `rte_align64pow2(size)` could return a value less than 4096, and the alignment check here would accept a base aligned to (say) 2048 bytes, which the hardware would reject. Checking the `mem_align` implementation:
```c
return RTE_MAX((uint64_t)RTE_PGSIZE_4K, rte_align64pow2(mw_len));
```
So the application is told to align to `max(4K, pow2(size))`. If the application obeys this (and `ntb_fwd` is updated to call `rte_pmd_ntb_get_mem_align()`, which patch 2 shows at line new L1162), the base will always be at least 4K-aligned. The check here in `mw_set_trans` is a runtime validation of the application's input. It should enforce the full hardware requirement. **Flag as an error: the alignment check does not enforce the 4K minimum; it should be `addr & (RTE_MAX(RTE_PGSIZE_4K, rte_align64pow2(size)) - 1)`.**

**Actually**, re-reading the code flow: the application calls `rte_pmd_ntb_get_mem_align()` to reserve the memzone with the correct alignment, then the memzone's IOVA is passed to `mw_set_trans` via the `dev_handshake` op (patch 2 new L553). The IOVA comes from a memzone reserved with the alignment returned by `mem_align`, so the check in `mw_set_trans` is verifying the memzone allocation honored the requirement. If the application used the wrong alignment (ignored `get_mem_align`), this check catches it. The check as written (`rte_align64pow2(size)`) is what the `mem_align` op advertises (modulo the 4K floor), so it is consistent. However, the hardware constraint is `>= max(4K, pow2(size))`, so the check should enforce that. The current check does not reject a `size=2048, addr=0x800`-aligned base, which the hardware would not accept (because 0x800 < 4K). **Confirmed as an error.**

Wait, re-checking: the hardware requires the *XLAT register* to have at least 4K alignment (per the commit message "the XLAT register also requires at least 4K alignment"). The offset masking into `(base | offset)` requires `base` to be aligned to `pow2(size)` so that offset bits don't collide with base bits. These are two separate constraints:
- Software constraint (bit aliasing): `align(base) >= pow2(size)`
- Hardware constraint (XLAT reg): `align(base) >= 4K`

The `mem_align` op enforces `max(4K, pow2(size))`, which satisfies both. The check in `mw_set_trans` enforces `pow2(size)` only. If `size < 4K` and the application ignored `mem_align` and provided a base aligned to (say) 512 bytes, the `mw_set_trans` check would pass (if `pow2(size) = 512`), but the hardware would reject the write to the XLAT register (< 4K alignment). The check should be:
```c
if (addr & (RTE_MAX(RTE_PGSIZE_4K, rte_align64pow2(size)) - 1))
```
to catch this case. **Confirmed error.**

**3. Missing error handling for `rte_pci_read_config()` in `amd_ntb_read_pcie_link_status()`**

File: `drivers/raw/ntb/ntb_hw_amd.c`  
Lines: new L186-220

The function calls `rte_pci_read_config()` five times. Each call checks `ret != sizeof(...)` and returns `-EIO` on failure, which is correct. However, the function does not initialize `*link_status` before returning success. If the loop at line ~203 completes without finding `RTE_PCI_CAP_ID_EXP`, the function reaches line ~220:
```c
return -ENOTSUP;
```
without setting `*link_status`. The caller at line new L340 (in `amd_ntb_get_link_status()`):
```c
if (ret == 0) {
    ntb->link_speed = AMD_LNK_STA_SPEED(link_status);
    ntb->link_width = AMD_LNK_STA_WIDTH(link_status);
}
```
uses `link_status` only when `ret == 0`. If `ret != 0`, `link_status` is not used. So the uninitialized-output case does not cause a bug. Not an error.

**4. Potential race on `ntb->db_mask` in `amd_ntb_db_set_mask()`**

File: `drivers/raw/ntb/ntb_hw_amd.c`  
Lines: new L421-431

```c
static int
amd_ntb_db_set_mask(const struct rte_rawdev *dev, uint64_t db_mask)
{
	struct ntb_hw *ntb = dev->dev_private;

	if (db_mask & ~ntb->db_valid_mask)
		return -EINVAL;

	ntb->db_mask |= db_mask;
	rte_write16((uint16_t)ntb->db_mask, (char *)ntb->hw_addr +
		    AMD_DBMASK_OFFSET);

	return 0;
}
```

The line `ntb->db_mask |= db_mask;` is a read-modify-write of `ntb->db_mask` with no atomic protection. If two threads call `db_set_mask` concurrently (e.g., during queue setup on different cores), the `|=` could race:
- Thread A reads `db_mask = 0x1`
- Thread B reads `db_mask = 0x1`
- Thread A computes `0x1 | 0x2 = 0x3`, writes back
- Thread B computes `0x1 | 0x4 = 0x5`, writes back
- Final value is `0x5`, but `0x2` from thread A is lost.

However, the NTB driver's device start/stop paths are expected to be serialized by the application (per ethdev/rawdev API model: one thread calls dev_start, dev_stop, queue_setup, etc.). The guidelines note (Info point 33) that control-path callbacks (configure, start, stop, queue setup) are serialized by the application, so a PMD does not need to lock one against another. `db_set_mask` is called from `ntb_dev_configure()` (patch 1 removed line at old L1404, now via `ntb_init_hw` at new L1304 in patch 2). This is a control-path operation. The race would only occur if the application called `dev_configure` or `dev_start` concurrently on the same device, which violates the API contract. Not a driver bug. Not an error.

**5. Memory leak in `amd_ntb_dev_warning: review has no Review-Result line, severity inferred from prose (response may be truncated)