|WARNING| [v2] eal/riscv: add support for zawrs extension

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-11-08

# DPDK Patch Review

## ERRORS

**Error 1: Incorrect macro syntax in RTE_WAIT_UNTIL_MASKED**

Line 165: `if ((rte_atomic_load_explicit((addr), (memorder)) & (mask)) cond expected_value)`

The macro uses `cond` as a token-pasting parameter, but the `if` statement syntax is wrong.
The code writes `if (... cond expected_value)` where `cond` is expected to be an operator like `==` or `!=`,
but the syntax would produce `if (... == expected_value)` followed by `break;` outside any loop.
The `break;` at line 167 is not inside a loop context that the macro itself creates.

**Suggested fix:**
Wrap the entire macro body in a `do { ... } while (0)` loop so the `break;` at line 167 escapes the macro.
The current `do { ... } while (0)` only wraps the later loop, not the early-exit path.

---

**Error 2: Type mismatch in __RTE_RISCV_LR_16**

Line 75: `__RTE_RISCV_LR_32(((uintptr_t)(src) & (~3)), word, (memorder));`

`__RTE_RISCV_LR_32` expects its first argument to be a pointer (`"r" (src)` in the asm constraint),
but here it receives `(uintptr_t)(src) & (~3)`, which is an integer.
The assembly constraint `"r"` will compile, but this violates the design:
the `lr.w` instruction requires a register holding the address, not the address value itself.
The code happens to work because the integer is placed in a register,
but the pattern is wrong and fragile.

**Suggested fix:**
```c
#define __RTE_RISCV_LR_16(src, dst, memorder) do {                      \
	uint32_t word;                                                  \
	uint32_t *aligned_src = (uint32_t *)((uintptr_t)(src) & (~3UL)); \
	__RTE_RISCV_LR_32(aligned_src, word, (memorder));               \
	if ((uintptr_t)(src) & 2)                                       \
		(dst) = (typeof(dst))(word >> 16);                      \
	else                                                            \
		(dst) = (typeof(dst))(word & 0xFFFF);                   \
} while (0)
```

Note: the test `if ((size_t)(src) & 3)` at line 76 is also wrong --
it should be `& 2` to test the second bit (selecting upper vs lower halfword in the word).
The current code would also trigger on `& 1`, which cannot occur given the assertion,
but the intent is to distinguish addresses `...0` and `...2` within a word.

---

**Error 3: Macro __RTE_RISCV_LR missing `do { ... } while (0)`**

Line 82: `#define __RTE_RISCV_LR(src, dst, memorder, size) { ... }`

The macro body uses braces `{ }` instead of `do { ... } while (0)`.
If used in a context like `if (cond) __RTE_RISCV_LR(...); else ...`,
the semicolon after the macro call terminates the `if`, breaking the `else`.

**Suggested fix:**
```c
#define __RTE_RISCV_LR(src, dst, memorder, size) do {             \
	RTE_BUILD_BUG_ON(size != 16 && size != 32 && size != 64); \
	if (size == 16)                                           \
		__RTE_RISCV_LR_16(src, dst, memorder);            \
	else if (size == 32)                                      \
		__RTE_RISCV_LR_32(src, dst, memorder);            \
	else if (size == 64)                                      \
		__RTE_RISCV_LR_64(src, dst, memorder);            \
} while (0)
```

---

## WARNINGS

**Warning 1: Missing RTE_EXPORT_SYMBOL annotations**

The patch adds three new public functions: `rte_wait_until_equal_16`, `rte_wait_until_equal_32`, `rte_wait_until_equal_64`.
These are `static inline` in a header, so they do not require `RTE_EXPORT_*` macros.
However, the generic versions of these functions (in `lib/eal/include/generic/rte_pause.h`)
are also inline and are not annotated.
This is acceptable for inline-only API, but the pattern is worth noting.

(No action required -- inline functions in headers do not need export annotations.)

---

**Warning 2: Memory order validation incomplete**

Lines 95-96 (and similar in the 32-bit and 64-bit variants):
```c
RTE_ASSERT(memorder == rte_memory_order_acquire ||
	memorder == rte_memory_order_relaxed);
```

The assertion only fires in debug builds.
In production, if a caller passes `rte_memory_order_release` or `rte_memory_order_seq_cst`,
the code proceeds with the `else` branch in `__RTE_RISCV_LR_*`, using the `.aq` (acquire) variant,
which happens to work for seq_cst but is wrong for release.

This is not a bug (release memorder makes no sense for a load),
but the code silently accepts invalid input in non-debug builds.

**Suggested improvement:**
Use `RTE_BUILD_BUG_ON` if memorder can be constant, or add an `else return;` fallback:
```c
if (memorder != rte_memory_order_acquire &&
    memorder != rte_memory_order_relaxed)
	return;  /* invalid memorder, do nothing */
```

---

**Warning 3: Inconsistent assertion check in RTE_WAIT_UNTIL_MASKED**

Line 158: `RTE_ASSERT(((size_t)(addr) & (sizeof(*(addr)) - 1)) == 0);`

The runtime assertion checks natural alignment, but the macro already has a compile-time check
at line 157 for memorder validity.
The alignment check should also be compile-time when `addr` is a constant expression,
but `RTE_ASSERT` is runtime-only.

**Suggested improvement:**
This is acceptable as-is (alignment is usually runtime-determined),
but consider documenting that unaligned `addr` is UB even in non-debug builds
(the `lr.*` instruction will trap).

---

## INFO

**Info 1: Release notes describe build-time feature, not runtime detection**

The release notes (line 75-76 in the patch) say:
```
On RISC-V targets built with the Zawrs extension in the ISA string
(for example ``-Dcpu_instruction_set=rv64gcv_zawrs``),
```

This is accurate. The feature is build-time only (`#ifdef RTE_RISCV_FEATURE_ZAWRS`),
not runtime-detected.
If a binary is built without Zawrs and run on a Zawrs-capable CPU, it will not use `wrs.nto`.

This is acceptable for RISC-V where ISA features are typically baked in at compile time,
but worth noting.

---

**Info 2: Signed vs unsigned integer in __RTE_RISCV_LR_16 shift**

Line 77: `(dst) = (typeof(dst))(word >> 16);`

If `dst` is a signed `int16_t`, the cast happens after the shift.
`word` is `uint32_t`, so the shift is unsigned (no sign extension issue here),
but the pattern could be clearer by using an unsigned intermediate explicitly.

This is not a bug -- the code is correct as written.

---

**Info 3: Documentation of wrs.nto behavior**

The comment at line 63-66 says:
```
* Wait-on-Reservation-Set extension instruction, it stalls execution until the
* reservation set is invalidated or an interrupt is observed.
* A loop is likely still needed as it may stop stalling arbitrarily.
```

The Zawrs specification permits `wrs.nto` to resume spuriously even without an interrupt
or reservation invalidation (for example, on a timer or implementation-defined event).
The comment correctly notes this ("may stop stalling arbitrarily"),
and the code correctly loops after every wake.

No issue -- this is good documentation.

---

Review-Result: ERROR