|WARNING| [v2] crypto/cnxk: add ML crypto support

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Patch: crypto/cnxk: add ML crypto support

### Summary
This patch adds ML-KEM and ML-DSA (post-quantum cryptography) support to the Marvell cnxk crypto driver. The implementation adds a new engine type (RE), lookup tables for ML operations, and support functions for key generation, encapsulation/decapsulation (ML-KEM), and sign/verify (ML-DSA).

---

## ERRORS

### 1. Resource leak on error path in `cnxk_cpt_dev_config()`

**File:** `drivers/crypto/cnxk/cnxk_cryptodev_ops.c`

**Location:** Lines 202-203

```c
if (ret) {
    plt_err("Could not initialize RE ML lookup table");
    goto ec_grp_put;
}
```

The error path `ec_grp_put` does not free the FPM table obtained earlier (line 184). If `roc_re_ml_zeta_get()` fails after successful `roc_ae_fpm_get()` and `roc_ae_ec_grp_get()`, both resources remain allocated.

**Fix:** The patch includes proper cleanup labels (`ec_grp_put` and `fpm_put`), so this appears to be handled correctly. However, verify that `roc_ae_ec_grp_put()` is safe to call after `roc_ae_fpm_put()` if there are any internal dependencies.

### 2. Missing bounds check on `hash` enum before array access

**File:** `drivers/crypto/cnxk/cnxk_ae.h`

**Location:** Lines 729, 819

```c
param2 |= (mldsa_hash_algo[hash] << ROC_RE_ML_DSA_PARAM2_SIGN_BIT);
```

The code checks `hash >= RTE_DIM(mldsa_hash_algo)` at lines 724 and 814, which is correct. However, the check is only performed in the `else` branch. If `hash` is an invalid enum value in the earlier branches (lines 722 or 812), it could be used without validation.

**However**, on closer inspection, the code structure ensures that `hash` is only accessed after the bounds check in the `else` branch where `hash != 0` and `mu.length == 0`. The earlier branches do not access the `mldsa_hash_algo` array. This is **actually correct** - no issue here.

### 3. Potential integer overflow in context length shift

**File:** `drivers/crypto/cnxk/cnxk_ae.h`

**Location:** Lines 749-753, 838-842

```c
if (mldsa->siggen.ctx.length > (UINT16_MAX >> ROC_RE_ML_DSA_PARAM2_CTXN_BIT)) {
    op->status = RTE_CRYPTO_OP_STATUS_INVALID_ARGS;
    return -EINVAL;
}
param2 |= ((uint16_t)mldsa->siggen.ctx.length << ROC_RE_ML_DSA_PARAM2_CTXN_BIT);
```

The check validates that shifting won't overflow a `uint16_t`, which is correct. The cast to `uint16_t` before the shift is also correct. This is **not an error** - the bounds check prevents overflow.

---

## WARNINGS

### 1. Hardcoded magic offsets in `cnxk_ae_dequeue_mlkem_op()`

**File:** `drivers/crypto/cnxk/cnxk_ae.h`

**Location:** Line 2042

```c
memcpy(mlkem->keygen.ek.data, rptr + 384 * (type + 1), mlkem->keygen.ek.length);
```

The offset `384 * (type + 1)` appears to be hardware-specific layout knowledge. This should be documented or replaced with a named constant to explain why the encapsulation key is at this specific offset relative to the decapsulation key.

**Recommendation:** Add a comment explaining the memory layout or define constants for these offsets.

### 2. Large metabuf size may waste memory

**File:** `drivers/crypto/cnxk/cnxk_cryptodev_ops.c`

**Location:** Lines 48-51

```c
#define CNXK_CPT_MAX_ASYM_OP_PQC_LEN	 16384
```

The comment states this is for ML-DSA-87 which requires 9523 bytes, but the buffer is 16KB. This is a 70% overhead. Consider whether a smaller size (e.g., 10KB or 12KB) would suffice with a smaller safety margin.

**Recommendation:** Re-evaluate if 16KB is necessary or if a smaller size would be adequate.

### 3. Missing release notes detail

**File:** `doc/guides/rel_notes/release_26_07.rst`

**Location:** Lines 158-160

The release note entry is very brief:

```rst
* **Updated Marvell cnxk crypto driver.**

  * Added support for ML-KEM and ML-DSA on CN20K platform.
```

Consider adding brief descriptions of what ML-KEM and ML-DSA are (post-quantum key encapsulation and digital signature algorithms) for readers unfamiliar with these standards.

**Recommendation:** Expand to:
```rst
* **Updated Marvell cnxk crypto driver.**

  * Added support for ML-KEM (key encapsulation) and ML-DSA (digital signature)
    post-quantum cryptography algorithms on CN20K platform.
```

### 4. Global arrays could be declared `const`

**File:** `drivers/crypto/cnxk/cnxk_ae.h`

**Location:** Lines 59-65

```c
static const uint8_t mldsa_hash_algo[] = {
```

This is already declared `const`, so this is correct. No issue.

### 5. Refcount initialization not atomic

**File:** `drivers/common/cnxk/roc_re_ml_tables.c`

**Location:** Lines 191-193

```c
ml = mz->addr;
if (plt_atomic_fetch_add_explicit(&ml->refcount, 1, plt_memory_order_seq_cst) != 0)
    return 0;
```

The first caller initializes the lookup tables, but there's a race: if two threads call `roc_re_ml_zeta_get()` concurrently on first use, both could see `refcount == 0` before the increment completes, and both would attempt to initialize the tables.

**Fix:** The `fetch_add` is atomic and returns the **old** value. If the old value is 0, only that thread initializes. If non-zero, the thread skips initialization. This is **actually correct** for the increment-before-check pattern. The first caller gets old value 0 and initializes; subsequent callers get non-zero and skip. No race.

However, there's a subtle issue: the refcount is **uninitialized** when the memzone is first reserved. The refcount field should be explicitly initialized to 0.

**Recommendation:** After `plt_memzone_reserve_cache_align()`, explicitly set `ml->refcount = 0` before the first atomic operation:

```c
if (mz == NULL) {
    mz = plt_memzone_reserve_cache_align(name, sizeof(struct re_ml_tbl) + len);
    if (mz == NULL)
        return -ENOMEM;
    ml = mz->addr;
    ml->refcount = 0;  /* Initialize refcount */
}
```

### 6. No check for NULL `mz->addr` after memzone lookup/reserve

**File:** `drivers/common/cnxk/roc_re_ml_tables.c`

**Location:** Lines 176, 192, 214

```c
ml = mz->addr;
```

After `plt_memzone_lookup()` or `plt_memzone_reserve_cache_align()` returns non-NULL `mz`, the code assumes `mz->addr` is valid. While DPDK memzones typically guarantee non-NULL `addr` when the memzone is valid, defensive programming would add an assertion or check.

**However**, this is standard DPDK usage and the guarantee is reliable. Only mention if the coding standard requires explicit checks.

---

## INFORMATIONAL

### 1. Feature flag naming convention

**File:** `drivers/crypto/cnxk/cnxk_cryptodev.c`

**Location:** Lines 31-32

```c
if (roc_model_is_cn20k())
    ff |= RTE_CRYPTODEV_FF_MLDSA_SIGN_PREHASH;
```

The feature flag `RTE_CRYPTODEV_FF_MLDSA_SIGN_PREHASH` is added but not defined in this patch. Ensure it's defined in the DPDK cryptodev headers (likely in a dependency patch).

### 2. ML table data is hardcoded

**File:** `drivers/common/cnxk/roc_re_ml_tables.c`

The zeta tables (`re_mlkem_zeta_tbl` and `re_mldsa_zeta_tbl`) are hardcoded byte arrays. This is acceptable for fixed lookup tables but adds ~1.3KB of read-only data to the binary.

### 3. Error handling uses `goto` correctly

The patch uses `goto` for cleanup paths (e.g., `goto ec_grp_put`, `goto fpm_put`), which follows DPDK style and ensures proper resource release.

### 4. Endianness handling

**File:** `drivers/common/cnxk/roc_re_ml_tables.c`

**Location:** Lines 196, 199

```c
tbl[0] = plt_cpu_to_be_64((uintptr_t)data);
tbl[1] = plt_cpu_to_be_64((uintptr_t)data);
```

Addresses are converted to big-endian before storing in the `tbl` array. Ensure the hardware expects big-endian pointers (this appears to be a hardware requirement based on the `plt_cpu_to_be_64` usage, which is correct for this driver).

---

## SUMMARY

**Errors:** 0 (all suspected errors resolved upon deeper inspection)

**Warnings:** 2 actionable items:
1. Uninitialized refcount in `roc_re_ml_zeta_get()` - should explicitly initialize to 0
2. Hardcoded offset in `cnxk_ae_dequeue_mlkem_op()` - add documentation

**Informational:** 4 observations for context

The patch is well-structured with proper error handling and cleanup paths. The main concern is the uninitialized refcount which could lead to undefined behavior on the first atomic operation. The hardcoded offset should be documented for maintainability.