|WARNING| crypto/cnxk: fix out of place AES GCM

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-06-12

# DPDK Patch Review

## Patch Summary
This patch fixes an issue in the cnxk crypto driver where AES-GCM out-of-place operations with AAD (Additional Authentication Data) were incorrectly writing AAD to the output buffer. The fix redirects passthrough data (AAD) to a metadata buffer to keep the output buffer header clean.

---

## Errors

None identified.

---

## Warnings

### 1. Repeated code blocks (code duplication)

**Location:** `drivers/crypto/cnxk/cnxk_se.h` - identical blocks appear 4 times in `sg_inst_prep()` and `sg2_inst_prep()`

The same passthrough handling logic is duplicated in four separate locations within the file. While each instance is contextually different (different scatter/gather variants), the logic is identical.

**Why it matters:** Code duplication makes maintenance harder - if the logic needs updating (e.g., error handling, bounds checking), all four copies must be changed identically. Future bugs or enhancements require quadruple the effort.

**Suggested fix:** Extract the common logic into a helper function:

```c
static inline int
handle_passthrough_to_meta(struct roc_sg_comp *scatter_comp, int i,
			   uint32_t passthrough_len,
			   struct roc_se_buf_ptr *meta_buf,
			   uint32_t *size, uint32_t *dst_offset,
			   uint32_t *aad_offset)
{
	if (!passthrough_len)
		return i;

	if (unlikely(passthrough_len > ROC_SE_MAX_AAD_SIZE)) {
		plt_dp_err("Passthrough length %u exceeds reserved space %u",
			   passthrough_len, ROC_SE_MAX_AAD_SIZE);
		return -1;
	}

	uint64_t meta_passthrough = (uint64_t)meta_buf->vaddr +
				    meta_buf->size - ROC_SE_MAX_AAD_SIZE;
	i = fill_sg_comp(scatter_comp, i, meta_passthrough, passthrough_len);
	*size -= passthrough_len;
	*dst_offset = passthrough_len;
	*aad_offset = 0;
	return i;
}

/* Similar for sg2 variant if fill_sg2_comp signature differs */
```

Then use it at all four call sites:

```c
uint32_t dst_offset = 0;

i = handle_passthrough_to_meta(scatter_comp, i, passthrough_len,
			       &params->meta_buf, &size,
			       &dst_offset, &aad_offset);
if (i < 0)
	return -1;

if (size)
	i = fill_sg_comp_from_iov(scatter_comp, i, params->dst_iov,
				  dst_offset, &size, aad_buf, aad_offset);
```

---

### 2. Variable declaration at point of use (C99 style)

**Location:** All four duplicated blocks declare `dst_offset` and `meta_passthrough` at point of use

```c
uint32_t dst_offset = 0;
/* ... */
uint64_t meta_passthrough = ...;
```

While C99-style declarations are acceptable per the guidelines, the existing function uses declaration-at-top style for other variables. For consistency within this function, consider declaring these at the function start.

**Suggested fix (if not using helper):** Move declarations to match the function's existing style, or keep as-is for locality. Both are acceptable; this is a minor consistency note.

---

### 3. Hardcoded offset calculation reused in four places

**Location:** All four blocks compute `meta_buf.size - ROC_SE_MAX_AAD_SIZE`

The expression `params->meta_buf.size - ROC_SE_MAX_AAD_SIZE` is computed identically in all four code paths. This is a candidate for a symbolic constant or helper function to clarify intent and avoid errors if the calculation ever changes.

**Suggested fix:** Either use the helper function from Warning #1, or define a macro:

```c
#define ROC_SE_META_PASSTHROUGH_OFFSET(meta_buf) \
	((uint64_t)(meta_buf)->vaddr + (meta_buf)->size - ROC_SE_MAX_AAD_SIZE)
```

Then use:
```c
uint64_t meta_passthrough = ROC_SE_META_PASSTHROUGH_OFFSET(&params->meta_buf);
```

---

### 4. Missing release notes

**Location:** No updates to `doc/guides/rel_notes/release_*.rst`

This patch fixes a functional bug (AAD incorrectly appearing in output buffer for AES-GCM out-of-place operations) affecting a production driver. Per the guidelines, bug fixes should be documented in the release notes, especially when they correct observable incorrect behavior.

**Suggested fix:** Add a brief entry to the current release notes under "Fixed Issues" or "Resolved Issues":

```rst
* **crypto/cnxk: Fixed AES-GCM out-of-place with AAD**

  Fixed an issue where AAD (Additional Authentication Data) was incorrectly
  written to the output buffer header in AES-GCM out-of-place operations.
  AAD is now redirected to the metadata buffer, keeping the output buffer clean.
```

---

## Info

### 1. Increased `ROC_SE_MAX_AAD_SIZE` from 64 to 1024

**Location:** `drivers/common/cnxk/roc_se.h`

```c
-#define ROC_SE_MAX_AAD_SIZE 64
+#define ROC_SE_MAX_AAD_SIZE 1024
```

This 16x increase in maximum AAD size changes a hardware/driver constant. While the patch description focuses on the passthrough fix, this change has broader implications:

- Increases metadata buffer size requirement (`cnxk_cryptodev_ops.c` adds `ROC_SE_MAX_AAD_SIZE` to `mlen`)
- May affect memory allocation patterns and cache behavior
- Aligns with common AEAD use cases (some protocols use larger AAD)

**Consider:** Document why 1024 was chosen (protocol requirement, hardware limit, alignment). If this is driven by a specific use case or standard (e.g., TLS 1.3, IPsec), mention it in the commit message or comments.

---

### 2. Error message uses `%u` for `uint32_t` with no cast

**Location:** All four error messages

```c
plt_dp_err("Passthrough length %u exceeds reserved space %u",
	   passthrough_len, ROC_SE_MAX_AAD_SIZE);
```

`passthrough_len` is `uint32_t`, `%u` expects `unsigned int`. On most DPDK platforms these are the same size, but for strict portability, consider `PRIu32` or an explicit cast. Given DPDK's existing codebase often uses `%u` for `uint32_t` in practice and this is in driver code (not portable library), this is acceptable as-is but worth noting.

---

### 3. `.mailmap` entry

**Location:** `.mailmap` addition

```
+Daphne Priscilla <df at marvell.com>
```

This is correct and follows the existing format. No issue.

---

## Summary

**Overall assessment:** The patch correctly fixes the AAD passthrough issue. The logic is sound, error handling is present, and bounds checks are in place. The primary concern is code duplication (Warning #1) which increases maintenance burden. Extracting the repeated logic into a helper function would improve maintainability.

**Correctness:** No bugs identified. The fix properly diverts passthrough data to the metadata buffer and includes bounds checking.

**Style/Process:**
- Code duplication should be refactored (Warning)
- Release notes update recommended for this user-visible bug fix (Warning)

**Recommendation:** Address code duplication via helper function. Add release notes entry. Consider documenting the rationale for the 1024-byte AAD limit.