|WARNING| [v2] crypto/qat: fix IPsec MB header include for ARM

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Summary
This patch updates the QAT crypto driver to fix IPsec MB header includes and guards for ARM architecture, specifically handling DOCSIS BPI-related functionality.

---

## Errors

### 1. Incorrect header guard logic in `qat_sym_session.h`

**File:** `drivers/crypto/qat/qat_sym_session.h`

The header include logic is inverted and incomplete:

```c
#ifdef RTE_ARCH_ARM
#include <ipsec-mb.h>
#else
#include <intel-ipsec-mb.h>
#endif
```

**Issue:** The commit message states "guard DOCSIS BPI-related fields that depend on IPsec MB so they are not included for ARM build", but the code unconditionally includes `<ipsec-mb.h>` on ARM. This contradicts the intent to exclude IPsec MB functionality on ARM.

**Expected pattern (based on guards in other files):**
```c
#ifndef RTE_QAT_OPENSSL
#ifndef RTE_ARCH_ARM
#include <intel-ipsec-mb.h>
#endif
#endif
```

Either ARM builds should not include any IPsec MB header (if the library is unavailable), or the commit message is incorrect about the intent. The pattern in `qat_sym_session.c` (functions and fields guarded by `#ifndef RTE_ARCH_ARM`) suggests ARM should NOT have IPsec MB, so including the header is wrong.

---

### 2. Nested `#endif` comments are inconsistent

**Files:** `drivers/crypto/qat/qat_sym.h`, `drivers/crypto/qat/dev/qat_crypto_pmd_gens.h`

Multiple locations use:
```c
#endif
#endif
```
without comments, making the nesting unclear. Some `#endif` directives do have comments (`#endif /* RTE_ARCH_ARM */`), but this is inconsistent.

**Suggested fix:** Add comments to all `#endif` directives to clarify which `#ifdef`/`#ifndef` they close:
```c
#endif /* !RTE_ARCH_ARM */
#endif /* !RTE_QAT_OPENSSL */
```

This improves readability and reduces the chance of mismatched guards during future edits.

---

## Warnings

### 1. Session structure fields not conditionally compiled

**File:** `drivers/crypto/qat/qat_sym_session.h` (assumed, not shown in patch)

The patch guards the *initialization* and *usage* of DOCSIS BPI fields (`docsis_key_len`, `expkey`, `mb_mgr`, `dust`) with `#ifndef RTE_ARCH_ARM`, but does not show whether these fields are conditionally compiled in `struct qat_sym_session`.

If the structure definition includes these fields unconditionally, ARM builds waste memory on unused fields. If the fields are missing on ARM, the code as written will not compile (references to `session->docsis_key_len`, `session->expkey`, etc. in guarded sections will fail).

**Verification needed:** Ensure `struct qat_sym_session` in `qat_sym_session.h` conditionally compiles these fields:
```c
struct qat_sym_session {
	/* ... */
#ifndef RTE_QAT_OPENSSL
#ifndef RTE_ARCH_ARM
	uint8_t docsis_key_len;
	uint64_t *expkey;
	IMB_MGR *mb_mgr;
	/* other BPI-related fields */
#endif
#endif
};
```

If this is not done, either the code will fail to compile on ARM (if fields are missing) or will waste space (if fields are present but unused).

---

### 2. Dead code on ARM builds

**Files:** `drivers/crypto/qat/qat_sym.h` (lines 237-245), `drivers/crypto/qat/dev/qat_crypto_pmd_gens.h` (lines 99-106)

The `#ifndef RTE_ARCH_ARM` guards inside the `#else` branch of `#ifndef RTE_QAT_OPENSSL` create nested conditionals where the `bpi_cipher_ipsec()` calls are dead code on ARM:

```c
#else  /* !RTE_QAT_OPENSSL */
#ifndef RTE_ARCH_ARM
	bpi_cipher_ipsec(last_block, dst, iv, last_block_len, ctx->expkey,
		ctx->mb_mgr, ctx->docsis_key_len);
#endif
#endif
```

On ARM builds with `RTE_QAT_OPENSSL` undefined, this leaves the entire `#else` block empty except for the debug log. This means BPI cipher operations are silently skipped on ARM, which may be the intended behavior but is not explicitly handled.

**Suggested improvement:** Add an explicit compile-time assertion or runtime error when BPI is required but ARM is detected, to fail loudly rather than silently:
```c
#else  /* !RTE_QAT_OPENSSL */
#ifndef RTE_ARCH_ARM
	bpi_cipher_ipsec(last_block, dst, iv, last_block_len, ctx->expkey,
		ctx->mb_mgr, ctx->docsis_key_len);
#else
	/* BPI not supported on ARM */
	RTE_BUILD_BUG_ON(1);  /* or return an error at configure time */
#endif
#endif
```

Alternatively, document that DOCSIS BPI is not supported on ARM and ensure `qat_sym_session_configure_cipher()` returns an error when DES/AES BPI is requested on ARM.

---

### 3. Error handling for `ipsec_mb_ctx_init()` on ARM

**File:** `drivers/crypto/qat/qat_sym_session.c` (lines 479-492, 511-527)

The calls to `ipsec_mb_ctx_init()` are guarded with `#ifndef RTE_ARCH_ARM`, but the subsequent error check (`if (ret != 0)`) is not:

```c
#else
#ifndef RTE_ARCH_ARM
	session->docsis_key_len = cipher_xform->key.length;
	ret = ipsec_mb_ctx_init(...);
#endif
#endif
if (ret != 0) {
	QAT_LOG(ERR, "failed to create DES BPI ctx");
	return -EINVAL;
}
```

On ARM, `ret` is never set by `ipsec_mb_ctx_init()` (the call is compiled out), so the `if (ret != 0)` check evaluates an uninitialized or stale value of `ret`. This could incorrectly fail configuration or incorrectly succeed.

**Suggested fix:** Initialize `ret = 0` before the conditional IPsec MB call, or guard the error check as well:
```c
#else
	int ret = 0;  /* Assume success on ARM where BPI is not used */
#ifndef RTE_ARCH_ARM
	session->docsis_key_len = cipher_xform->key.length;
	ret = ipsec_mb_ctx_init(...);
#endif
#endif
if (ret != 0) {
	QAT_LOG(ERR, "failed to create DES BPI ctx");
	return -EINVAL;
}
```

Or reject BPI configuration explicitly on ARM:
```c
#else
#ifdef RTE_ARCH_ARM
	QAT_LOG(ERR, "DOCSIS BPI not supported on ARM");
	return -ENOTSUP;
#else
	session->docsis_key_len = cipher_xform->key.length;
	ret = ipsec_mb_ctx_init(...);
#endif
#endif
```

---

## Info

### 1. Commit message does not explain ARM limitation

The commit message states the patch is to "fix IPsec MB header include for ARM" and "guard DOCSIS BPI-related fields", but does not explain *why* DOCSIS BPI is unavailable on ARM. Is this a hardware limitation, a library limitation (intel-ipsec-mb not available on ARM), or a policy decision?

A brief explanation in the commit message would help future maintainers understand the context (e.g., "Intel IPsec MB library is x86-only; ARM builds cannot support DOCSIS BPI cipher modes").

---

### 2. Consider compile-time feature capability

Instead of scattering `#ifndef RTE_ARCH_ARM` throughout the code, consider defining a feature macro at the top of the driver:

```c
/* qat_sym_session.h or qat_sym_capabilities.h */
#if !defined(RTE_QAT_OPENSSL) && !defined(RTE_ARCH_ARM)
#define QAT_SYM_DOCSIS_BPI_SUPPORTED 1
#endif
```

Then use `#ifdef QAT_SYM_DOCSIS_BPI_SUPPORTED` in all guarded sections. This centralizes the platform/build logic and makes future changes easier.

---

## Conclusion

The patch correctly identifies the need to exclude DOCSIS BPI functionality on ARM, but the implementation has correctness issues (header include logic, uninitialized `ret` variable) and consistency issues (missing `#endif` comments, no explicit error when BPI is requested on ARM). Address the Errors section before merging; the Warnings section highlights potential runtime issues that should also be fixed.