|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

**1. RTE_WAIT_UNTIL_MASKED uses `break` in do-while(0) without enclosing loop**

Line: `break;` in the RTE_WAIT_UNTIL_MASKED macro (lib/eal/riscv/include/rte_pause.h:176)

```c
if ((rte_atomic_load_explicit((addr), (memorder)) & (mask))      \
		cond expected_value)                             \
	break;                                                   \
```

This `break` statement is inside a `do { ... } while (0)` macro with no surrounding loop. If the fast-path condition is met, the break will cause a compilation error ("break statement not within loop"). The intent appears to be to return early, so this should be `break` only if the macro is documented to require use inside a loop, or it should use a different construct (goto to end, or restructure as compound statement with early return check outside the do-while).

The generic implementation in `generic/rte_pause.h` uses this macro inside `while (1)` loops in the `rte_wait_until_equal_*` functions, but this RISC-V version defines standalone functions that don't use the macro in that way. The macro as written cannot be used correctly.

**Suggested fix:**
Either document that `RTE_WAIT_UNTIL_MASKED` must only be used inside a loop (and verify all usage sites comply), or restructure to avoid the bare `break`:

```c
#define RTE_WAIT_UNTIL_MASKED(addr, mask, cond, expected, memorder) do { \
	/* ... validation ... */                                         \
	typeof(*(addr)) expected_value = (expected);                     \
	typeof(*(addr)) value;                                           \
	value = rte_atomic_load_explicit((addr), (memorder));            \
	if (!((value & (mask)) cond expected_value)) {                   \
		__RTE_RISCV_LR((addr), value, memorder, size);           \
		while (!((value & (mask)) cond expected_value)) {        \
			__RTE_RISCV_WRS_NTO();                           \
			__RTE_RISCV_LR((addr), value, memorder, size);   \
		}                                                        \
	}                                                                \
} while (0)
```

---

**2. Missing blank line between declarations and statements**

Multiple functions violate the "empty line between declarations and statements" rule:

In `rte_wait_until_equal_16` (line 106):
```c
uint16_t value;

RTE_ASSERT(memorder == rte_memory_order_acquire ||
```
Should have a blank line after the `uint16_t value;` declaration.

Same issue in `rte_wait_until_equal_32` (line 129) and `rte_wait_until_equal_64` (line 152).

---

## WARNINGS

**1. Missing `__rte_experimental` tag on new public functions**

The functions `rte_wait_until_equal_16`, `rte_wait_until_equal_32`, and `rte_wait_until_equal_64` are new inline functions in a public header (`lib/eal/riscv/include/rte_pause.h`). While they match the signature of generic helpers, this is the first architecture-specific implementation that makes them available. If these are new to the RISC-V architecture's public API surface, they should be marked `__rte_experimental` unless they are overriding a stable generic API.

However, on closer inspection, these appear to be architecture-specific overrides of generic helpers that applications use indirectly. If the generic versions are already stable API, then no experimental tag is needed. This is a judgment call based on whether RISC-V is adding new API or implementing existing API.

Given the context (overriding generic helpers, release notes describe this as an optimization not new API), this is likely acceptable. Downgrading to informational note rather than warning.

---

**2. Macro parameter evaluation in __RTE_RISCV_LR_16**

Line 76-77:
```c
if ((size_t)(src) & 3)                                          \
	(dst) = (typeof(dst))(word >> 16);                      \
```

The macro evaluates `(src)` twice: once in the `__RTE_RISCV_LR_32` call (line 74) and again in the `if` condition (line 75). If `src` has side effects (e.g., `ptr++`), this would cause incorrect behavior. While the macro is internal and presumably only called with simple lvalues, using a temporary would be safer:

```c
#define __RTE_RISCV_LR_16(src, dst, memorder) do {                      \
	uint32_t word;                                                  \
	const void *__src = (src);                                      \
	__RTE_RISCV_LR_32(((uintptr_t)__src & (~3)), word, (memorder)); \
	if ((size_t)__src & 3)                                          \
		(dst) = (typeof(dst))(word >> 16);                      \
	else                                                            \
		(dst) = (typeof(dst))(word & 0xFFFF);                   \
} while (0)
```

This is a **Warning** because the current implementation works as long as callers follow the implicit contract, but it's fragile.

---

## INFO

**1. Inline assembly clobbers**

The inline assembly blocks specify `"memory"` clobbers, which is correct for synchronization primitives. The register constraints look appropriate (output `=r`, input `r`).

**2. Release notes update is appropriate**

The release notes entry correctly describes the new feature and documents the build-time flag. The cross-build documentation update is helpful.

**3. Code structure follows generic/rte_pause.h pattern**

The implementation correctly follows the pattern established by the generic wait helpers, with proper memorder checks via RTE_ASSERT.

---

## SUMMARY

The patch has one critical error (the `break` in RTE_WAIT_UNTIL_MASKED cannot work in all contexts) and one warning-level issue (macro parameter re-evaluation in __RTE_RISCV_LR_16). The core idea and implementation are sound, but the macro needs correction before merge.

Review-Result: ERROR