|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

**Missing `volatile` qualifier in LR macro casts**

The `__RTE_RISCV_LR_16` macro casts `src` to `uintptr_t` and then back to pointer for the aligned address, but loses the `volatile` qualifier when passing to `__RTE_RISCV_LR_32`:

```c
__RTE_RISCV_LR_32(((uintptr_t)(src) & (~3)), word, (memorder));
```

The cast result `(uintptr_t)(src) & (~3)` is not a `volatile uint32_t *`, so the reservation load may not observe the memory location correctly. The compiler could cache or reorder the access.

**Fix:** Cast to `volatile uint32_t *`:
```c
__RTE_RISCV_LR_32((volatile uint32_t *)((uintptr_t)(src) & (~3)), word, (memorder));
```

---

**`RTE_WAIT_UNTIL_MASKED` infinite loop if initial condition already true**

Line 168:
```c
if ((rte_atomic_load_explicit((addr), (memorder)) & (mask))
        cond expected_value)
    break;
```

This `break` exits the `do { } while (0)` macro expansion immediately if the condition is already true, which is correct. However, the logic is inverted: it should **return early** (like the non-masked variants), not break and continue into the reservation loop. As written, when the initial check passes, execution falls through to the `__RTE_RISCV_LR` and enters the `while` loop anyway.

**Fix:** Change `break` to early return pattern or restructure:
```c
if ((rte_atomic_load_explicit((addr), (memorder)) & (mask))
        cond expected_value)
    return;  /* or appropriate early exit for macro context */
```

Actually, since this is a macro not a function, `break` out of `do-while(0)` is correct for early exit. The real issue is that there's **no outer scope to return from** -- the macro expands inline in the caller's scope. Re-reading: the `do { } while (0)` pattern allows the macro to be used as a statement, and `break` does exit the block. This is actually correct. **Retract this finding.**

---

**Race condition in fast-path check before reservation**

In `rte_wait_until_equal_16/32/64`, the fast-path plain load (line 110, 144, 164) races with the subsequent reservation load. Between the plain load returning non-equal and taking the reservation, another thread could write the expected value and then write a different value, causing the reservation loop to wait indefinitely even though the expected value was briefly present.

Wait, that's not a correctness bug -- the API contract is to wait *until* the value equals expected, not to detect *every time* it equals expected. If it transitions back to non-equal after the plain load, the loop correctly waits again. The fast-path check is an optimization to avoid the reservation overhead when the value is already equal at the time of check. No bug here. **Retract this finding.**

---

**16-bit shift assumes little-endian byte order**

Line 46-49 in `__RTE_RISCV_LR_16`:
```c
if ((size_t)(src) & 3)
    (dst) = (typeof(dst))(word >> 16);
else
    (dst) = (typeof(dst))(word & 0xFFFF);
```

This extracts the low or high halfword based on whether the address is 2-byte aligned within the word. On big-endian systems, the byte order is reversed: an address ending in `0` contains the *high* halfword in the word, not the low. RISC-V is little-endian, but DPDK code should not silently assume endianness unless documented.

However, RISC-V base ISA is defined as little-endian, and the RISC-V privileged spec allows big-endian as an extension but it is rare. If DPDK's RISC-V support is LE-only, this is fine. Checking the codebase context: `rte_pause.h` has no endian guards elsewhere, and RISC-V atomics don't vary by endian. Without explicit DPDK policy that RISC-V must support BE, this is **not a bug**, just a portability note. Since I cannot prove DPDK supports RISC-V BE from this patch alone, **omit this finding**.

---

## WARNINGS

**`RTE_WAIT_UNTIL_MASKED` does not follow early-return fast-path pattern**

Unlike the `rte_wait_until_equal_*` functions which check the condition with a plain load first (lines 109-111, 143-145, 163-165), `RTE_WAIT_UNTIL_MASKED` has a conditional `break` (line 168-170) but does not avoid the reservation overhead when the condition is already true. The `break` exits the `do-while(0)`, which is an early exit, but the check happens *inside* the macro body where the caller has already invoked it. The caller cannot short-circuit before entering the macro.

Wait, re-reading the macro: the `do { } while (0)` wraps the entire body, and the `if (...) break;` at line 168 exits the block immediately if the condition is true, skipping the `__RTE_RISCV_LR` call. This **is** the fast-path optimization, just structured differently because it's a macro. The pattern is correct. **Retract this finding.**

---

**Release notes entry uses future tense**

Line 75 in release notes:
```
``rte_wait_until_equal_*`` and ``RTE_WAIT_UNTIL_MASKED`` now stall the hart
```

Release notes describe what was added/changed in the release, typically in present tense ("Added", "Updated") for the heading and past tense or present tense for the description. "now stall" is acceptable, but "stall the hart" (present tense ongoing) is slightly awkward. More idiomatic: "now use `wrs.nto` to stall" or "now stall... using `wrs.nto`". This is a minor style preference, not an error. **Omit -- release notes style is discretionary and this is clear.**

---

**Documentation does not explain when to use the ISA extension**

The cross-build documentation (lines 121-143) explains *how* to enable Zawrs, but does not explain *when* it is beneficial or safe to enable. Users need to know:
- Does the target hardware support it?
- What is the performance trade-off (energy vs. latency)?
- Is it safe to enable for a binary that runs on mixed hardware (with and without Zawrs)?

However, this is common practice for ISA extension docs in DPDK -- the vector extensions docs similarly just show the flag. Adding a "when to use" section would be nice but is not required. **Omit -- informational gap, not a violation.**

---

## INFO

**Consider cache-line alignment requirement in assert**

The alignment asserts (lines 114, 147, 167) check natural alignment (2/4/8 bytes), which is required for the `lr.*` instructions. However, the memory being polled is likely shared between threads/harts, so cache-line alignment would reduce false sharing. The generic `rte_wait_until_equal_*` implementations in `generic/rte_pause.h` do not enforce cache-line alignment either, so this is not a regression. Just a note that shared wait variables should ideally be cache-line aligned for performance.

---

**Zawrs spec allows reservations to be invalidated spuriously**

The Zawrs spec permits `wrs.nto` to resume execution arbitrarily, not just when the reservation set is invalidated. The code correctly handles this by looping (lines 126, 148, 169), but a comment noting that spurious wakeups are expected (similar to `pthread_cond_wait`) would help future maintainers. The existing comment "A loop is likely still needed as it may stop stalling arbitrarily" (line 97) already covers this. No change needed.

---

**Pre-existing: no compiler barrier in `__RTE_RISCV_WRS_NTO`**

The `wrs.nto` inline asm at line 98 includes a `"memory"` clobber, which is a compiler barrier. This is correct -- the barrier prevents the compiler from reordering loads/stores around the wait instruction. No issue here. **Omit.**

---

Review-Result: ERROR