|WARNING| [RFC] crypto: add RSA-specific capability parameters

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# Review of RFC PATCH: crypto: add RSA-specific capability parameters

## Summary
This patch adds RSA-specific capability reporting to the cryptodev API. The code is generally well-structured, but there are several issues that need to be addressed before merging.

---

## Errors

**1. ABI break without versioning**

The patch modifies `struct rte_cryptodev_asymmetric_xform_capability` by adding a new union member `rsa_capa`. This is an ABI change that requires proper handling. Since this appears to target a development release (2026-07-31 is in the future, suggesting 26.11 or later), new API must be marked as experimental and the ABI implications must be documented in release notes.

**Fix:** 
- Mark any new API accessing `rsa_capa` with `__rte_experimental`
- Document the ABI change in release notes
- Consider whether this should be a new structure version or if the union extension is ABI-compatible (union extensions typically are, but verify)

---

## Warnings

**1. Missing release notes**

The patch adds new API structure members but does not update release notes in `doc/guides/rel_notes/`. This is a significant API addition that applications will need to discover.

**Fix:** Add an entry to the current release notes under "New Features" describing the new RSA capability reporting mechanism.

**2. Incomplete documentation of MGF1 hash algorithm encoding**

The documentation states "Each bit corresponds to enum rte_crypto_auth_algorithm" but does not specify how the bit position relates to the enum value. Is it `(1ULL << RTE_CRYPTO_AUTH_SHA256)` or some other encoding?

**Fix:**
```c
/**< Bitmask of hash algorithms supported for MGF1 mask generation.
 * Bit N is set if algorithm enum value N is supported.
 * For example, bit (1ULL << RTE_CRYPTO_AUTH_SHA256) indicates SHA-256 support.
 * ...
```

**3. Unclear semantics when capabilities are "not reported"**

The documentation says "A value of 0 means padding capability is not reported" and similar for MGF1. It's unclear whether this means:
- The PMD doesn't support the feature at all
- The PMD supports it but doesn't advertise capabilities
- The capability query mechanism is not implemented

**Fix:** Clarify the semantics. Suggest:
```c
/**< Bitmask of supported RSA padding schemes.
 * Each bit corresponds to enum rte_crypto_rsa_padding_type.
 * A value of 0 means the PMD does not advertise padding capabilities
 * and applications should assume only RTE_CRYPTO_RSA_PADDING_NONE is supported.
 */
```

**4. Inconsistent documentation of zero values**

For `modlen.min/max/increment`, the comment says "PMD default applies", but for `pad_types` and `mgf1_hash_algos` it says "capability is not reported". This inconsistency may confuse users.

**Fix:** Use consistent language. Either all zeros mean "not reported" or all mean "default applies", but clarify what the default is.

**5. Missing validation of bit positions**

The documentation references `enum rte_crypto_rsa_padding_type` and `enum rte_crypto_auth_algorithm` but doesn't state whether these enums are guaranteed to be in a range suitable for bitmasks (< 64 for `uint64_t`, < 8 for `uint8_t`).

If these enums can exceed the bitmask width, the documentation should note which values are valid for the bitmask.

**6. No example usage**

New API structures benefit from example code showing how applications should query and interpret the capabilities. Consider adding a code snippet in the documentation or referencing where this will be used.

---

## Info

**1. Consider explicit bitmask helper macros**

The patch uses raw bitmask values (`uint8_t`, `uint64_t`). Consider defining helper macros for setting/testing bits to improve type safety and readability:

```c
#define RTE_CRYPTO_RSA_PAD_TYPE(x)  (1U << (x))
#define RTE_CRYPTO_MGF1_HASH_ALGO(x) (1ULL << (x))
```

**2. Alignment with existing patterns**

The patch follows the existing union-based capability reporting pattern, which is good for consistency. However, verify that `hash_algos` at the end of the struct and the new `rsa_capa` don't create confusion about which applies to RSA (both do, for different purposes).

**3. Future extensibility**

The current design adds RSA-specific capabilities. If other asymmetric algorithms (DSA, ECDSA, etc.) need similar treatment in the future, will this pattern scale? Consider documenting the intended pattern for future extensions.

**4. Typo in modlen documentation**

The updated comment for `modlen` says "Value 0 means implementation default" (missing 's' in "means"). While this is a pre-existing issue being preserved, consider fixing it:

```c
/**< Range of modulus length supported by modulus based xforms
 * such as MODEXP, MODINV, DH, and DSA.
 * Values of 0 mean implementation default.
 */
```

---

## Additional Observations

**Code style:** The patch follows DPDK code style conventions correctly (indentation, comment formatting, struct layout).

**No functional code changes:** This patch only adds data structures and documentation. No .c files are modified, so there are no executable code paths to review for correctness bugs.

**Testing:** Since this is an RFC and only adds capability structures, no tests are included. When this moves to a full patch, ensure that:
- At least one PMD is updated to populate these fields
- Test code demonstrates querying the new capabilities
- The feature matrix in documentation is updated if applicable

---

## Recommendation

This is a reasonable API extension that addresses a real gap in RSA capability reporting. The main blocking issues are:

1. Missing release notes (Warning)
2. Unclear ABI compatibility story (Error if this targets a stable release)
3. Ambiguous documentation of zero-value semantics (Warning)

Address these issues before promoting from RFC to a formal patch submission.