cryptobench: per-SoC AEAD seal rate, in software and on the cipher engine by widgetii · Pull Request #187 · OpenIPC/ipctool

widgetii · 2026-09-02T09:50:07Z

Closes #186.

Adds ipctool cryptobench, beside cpubench and membw, which are the same kind of per-SoC probe:

ipctool cryptobench [--json] [--bytes N] [--iters N] [--no-hw]

The finding that reframes the proposal

The proposal offers the hardware engine as the thing that might rescue AES-GCM on these parts. It cannot, and not because it is slow — gen-4 HiSilicon and Goke silicon has no AEAD mode at all. CHIP_AES_CCM_GCM_SUPPORT is defined for hi3569v100 alone; the block does ECB/CBC/CTR/CFB/OFB and nothing else. There is no hardware sealing to enable, on any part OpenIPC currently ships.

cryptobench establishes that by asking — it configures a channel for AES-GCM and reports what the driver answers — rather than by asserting it, so a part that does carry the mode reports itself instead of inheriting a hardcoded "unsupported".

What the engine can do is AES-CTR, the confidentiality half of AES-GCM. The authentication half would still be GHASH on the CPU, which is the expensive half. So ChaCha20's advantage on these cameras is not a software artifact waiting to be engineered away. It is the answer.

Numbers

Lab gk7205v200 (Cortex-A7), 1100-byte packets, majestic stopped, µs per packet:

	wall	cpu	MB/s
AES-128-GCM	141.4	141.4	7.4
AES-256-GCM	167.6	167.6	6.3
ChaCha20-Poly1305	45.1	45.1	23.3
engine AES-CTR, one packet per ioctl	83.9	44.1	12.5
engine AES-CTR, 15 packets per ioctl	49.7	12.3	21.1
engine, 16 bytes — the per-ioctl floor	37.4	34.1	—

ChaCha20 wins by 3.14×, so your reported ordering holds and is wider on this part. Different chip and a different implementation from ring, so I would not read much into 3.14 vs 2.3 beyond the direction — which is the argument for having the probe in-tree and reproducible rather than comparing two people's one-off harnesses.

The engine rows carry their own lesson, which is why the 16-byte row is there: nearly half the cost of a 1100-byte packet is the trip, not the cipher. A burst of 15 packets under one ioctl (OpenIPC/openhisilicon#217) cuts CPU per packet from 44 µs to 12 while wall clock barely moves — the engine is a poor bargain per packet and a good one per burst.

Your three questions

1. Language and deps — C, in-tree. Not a style preference: ipctool ships as a static musl binary (CMakeLists.txt:9-12, and release.yml builds with the default), musl's dlopen is a no-op when static, and all three CI toolchains carry only libmbedcrypto.so with no .a. The platform mbedTLS is genuinely unreachable from the shipped binary, so linking it would trade the single static binary for a benchmark.

That does cost the thing you correctly flagged — measuring the library our stacks use — so the implementation is built to keep the comparison honest rather than to be small: four runtime-generated AES T-tables and 4-bit GHASH tables, which is what mbedTLS itself does by default (MBEDTLS_AES_ROM_TABLES is off). A compact S-box implementation would be several times slower and would inflate ChaCha20's lead into a number about this file rather than about the silicon.

2. Where the numbers go — the tool generates the table. Output is YAML by default and --json on request, so the wiki holds pasted tool output rather than hand-typed numbers, and a row can be regenerated by whoever doubts it.

3. Scope — yes, it belongs here. It is a capability probe of the running chip, which is exactly cpubench and membw's remit, and it reads /dev/cipher, so it needs to run on the camera anyway. Cost is 19–26 KB of .text (arm32 +19252, arm64 +22248, mips32 +26128); all three CI targets cross-build clean and stay statically linked.

And for what it is worth: "interesting, wrong repo" was not the right answer. The proposal was right that this is per-chip and right that packet size is the design point not to compromise on — --bytes defaults to 1200 for that reason.

Correctness before numbers

A benchmark of a wrong implementation is worth nothing, and a wrong one is invisible from the outside because ciphertext is supposed to look like noise. Every primitive is checked against a published vector before it is timed, and a row that fails is reported verified: false with no numbers rather than as a fast wrong answer:

AES-128/256-GCM — GCM spec test cases 2 and 14
ChaCha20-Poly1305 — RFC 8439 §2.8.2
engine AES-CTR — NIST SP 800-38A F.5.1, plus a vector spanning a 32-bit counter wrap
engine AES-CTR batched — fifteen packages, a distinct IV and distinct plaintext each, every one compared against the software CTR above

The batch vector is separate from the single-packet one on purpose, and it earns its keep: a driver that accepted the burst but applied the channel's one IV to all fifteen packages would return correct ciphertext for package 0 and silent nonsense for the rest, which is precisely the shape of the benchmark's real jobs. It passes on a gk7205v200, so OpenIPC/openhisilicon#217 does honour per-package IVs — previously assumed, now checked. It also runs at a length that is not a multiple of the block, which establishes that the driver rounds the descriptor itself.

The counter-wrap vector is not optional either. An engine carrying only the low 32 bits of the counter agrees with software for every IV that is not near a wrap, which is nearly all of them — it would pass F.5.1, ship, and then disagree on one packet in millions with nothing to say so.

The §2.8.2 vector earned its keep during development: the first cut of this used Poly1305's own short-final-block padding instead of RFC 8439's pad16() for the MAC input. That is correct for any message which is a multiple of 16 bytes and wrong for every other one, and nothing but the vector would have caught it.

Measurement discipline

Three things the numbers depend on:

Both clocks on every row. The hardware rows differ by ~2× between them, because the driver sleeps on its completion interrupt and hands the core back — that gap is the offload. Quoting either clock alone hides the point. cpu_wall near 1.00 on a hardware row means the run was contending for CPU; --help says to stop majestic.
Nothing is printed between timed blocks. Results are buffered and printed once. A single printf to a serial console or an ssh pipe between two identical runs has been measured making the second one read twice as slow.
Packet-sized, and the floor is reported. The 16-byte row is almost entirely syscall and channel programming, and it is the number to reason with before building anything on this engine.

Degradation, tested on the board rather than written

no /dev/cipher (rmmod open_cipher) → hardware: absent, software half still runs
a driver that rejects the batched command → batched: unsupported, single-packet and floor rows still measured
non-HiSilicon hardware → same absent path

The /dev/cipher ABI is transcribed rather than linked, the way majestic does it: libhi_cipher.so is a thin ioctl wrapper and most images do not ship it, so a DT_NEEDED on it would stop ipctool starting on the very cameras it is meant to inspect. Commands are derived from struct sizes with the vendor's own macro, so a struct that does not match the driver's yields a command it does not recognise rather than one it misreads.

Second commit, unrelated but found by this work

cYAML_Print() returned NULL — meaning ipctool printed nothing at all and exited 0 — if any string anywhere in the document contained a non-ASCII byte. char is signed, so every UTF-8 byte tested as a control character, fell into the \u expansion and overran its scratch buffer. JSON mode was unaffected, so the two output modes disagreed about whether the machine had any hardware. Sensor names and U-Boot environments are passed through verbatim, so this was reachable from ordinary output; my note: strings are just what tripped it. Fixed by comparing as unsigned char, with two test cases. The test harness was also computing a per-case verdict and then unconditionally returning 0, so it now exits non-zero when a case fails.

`char` is signed on x86 and ARM alike, so `*c < 32` in print_string() is true for every byte of a UTF-8 sequence, not just for control characters. Those bytes then fell into the \u expansion below, whose scratch buffer holds ten bytes: 0xe2 read as a negative int expands to eleven bytes with its terminator, so the TRY() fails and the failure propagates all the way out of cYAML_Print(), which returns NULL. The caller then prints nothing at all. Not one mangled string in an otherwise fine document -- no document. `ipctool` in its default output mode exits 0 having said nothing, which reads as "this tool found nothing to report" rather than as a failure, and JSON mode is unaffected, so the two modes disagree about whether the machine has any hardware. This is reachable from ordinary output: sensor names, U-Boot environments and vendor strings are read off the device and passed through verbatim. Compare as `unsigned char`. Bytes >= 128 then take the pass-through path, which is what we want -- YAML is UTF-8 and they need no escaping -- and control characters keep being expanded exactly as before. Two test cases cover both halves. The harness was also computing a per-case verdict and then unconditionally returning 0, so no regression in it could ever fail a build; it now exits non-zero when a case fails.

Closes #186, which asks for a per-SoC probe of AES-128-GCM, AES-256-GCM and ChaCha20-Poly1305 on packet-sized buffers after ChaCha20 measured 2.3x AES-128-GCM on a GK7202V300. Adds `ipctool cryptobench`, beside cpubench and membw, which are the same kind of per-SoC probe. ipctool cryptobench [--json] [--bytes N] [--iters N] [--no-hw] Measured on a lab gk7205v200 (Cortex-A7, 1100-byte packets, majestic stopped), microseconds per packet: | | wall | cpu | MB/s | |---|---|---|---| | AES-128-GCM | 141.4 | 141.4 | 7.4 | | AES-256-GCM | 167.6 | 167.6 | 6.3 | | ChaCha20-Poly1305 | 45.1 | 45.1 | 23.3 | | engine AES-CTR, one packet per ioctl | 83.9 | 44.1 | 12.5 | | engine AES-CTR, 15 packets per ioctl | 49.7 | 12.3 | 21.1 | | engine, 16 bytes (the per-ioctl floor) | 37.4 | 34.1 | - | ChaCha20 wins by 3.14x here, so the reported ordering holds and is wider on this part. Written in C, in-tree. ipctool ships as a static musl binary (CMakeLists.txt:9-12, and release.yml builds with the default), musl's dlopen is a no-op when static, and all three CI toolchains carry only libmbedcrypto.so with no .a -- so the platform mbedTLS is not reachable from the shipped binary and linking it would trade the single static binary for a benchmark. The AES uses four runtime-generated T-tables and GHASH uses 4-bit tables, which is what mbedTLS itself does by default; a compact implementation would be several times slower and would inflate ChaCha20's lead into a number about this file rather than about the silicon. THE HARDWARE HALF REFRAMES THE PROPOSAL. The Cipher engine on HiSilicon and Goke parts cannot seal at all: gen-4 silicon has no GCM or CCM mode, and cryptobench establishes that by configuring a channel for AES-GCM and reporting what the driver answers, rather than by asserting it, so a part that does carry the mode reports itself. What the engine can do is AES-CTR, the confidentiality half; the authentication half would still be GHASH on the CPU. So ChaCha20's advantage on these cameras is not a software artifact waiting to be engineered away -- it is the answer. The engine rows also carry their own lesson, which is why the 16-byte row is there. Nearly half the cost of a 1100-byte packet is the trip, not the cipher, and a burst of 15 under one ioctl (openhisilicon#217) cuts CPU per packet from 44 us to 12 while wall clock barely moves. Three things the numbers depend on, all learned the expensive way: - Both clocks, every row. The hardware rows differ by ~2x between them because the driver sleeps on its completion interrupt and hands the core back; quoting either alone hides the point. cpu_wall near 1.00 on a hardware row means the run was contending for CPU. - Nothing is printed between timed blocks. A single printf to a serial console or an ssh pipe between two identical runs made the second one read twice as slow. - Every primitive is checked against a published vector before it is timed -- GCM test cases 2 and 14, RFC 8439 2.8.2, and for the engine NIST SP 800-38A F.5.1 plus a vector spanning a 32-bit counter wrap. A row that fails is reported unverified with no numbers rather than as a fast wrong answer. The wrap vector is not optional: an engine carrying only the low 32 bits of the counter agrees with software for every IV that is not near a wrap, so it would pass F.5.1, ship, and then disagree on one packet in millions. The /dev/cipher ABI is transcribed rather than linked, as majestic does it: libhi_cipher.so is a thin ioctl wrapper and most images do not ship it, so a DT_NEEDED on it would stop ipctool starting on the very cameras it is meant to inspect. Commands are derived from struct sizes with the vendor's own macro, so a struct that does not match the driver's yields a command it does not recognise rather than one it misreads. Degradation was tested on the board, not just written: no /dev/cipher reports `hardware: absent` and the software half still runs; a driver that rejects the batched command reports `batched: unsupported` and still measures the single-packet and floor rows. Cross-builds clean and still statically linked on all three CI targets, for 17-24 KB of .text (arm32 +17636, arm64 +20328, mips32 +24432).

qodo-free-for-open-source-projects · 2026-09-02T09:51:07Z

PR Summary by Qodo

Add per-SoC AEAD and cipher-engine cryptobench

✨ Enhancement 🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

AI Description

• Adds packet-sized software AEAD benchmarks with vector verification and dual-clock metrics.
• Probes cipher-engine GCM support and benchmarks single, floor, and batched AES-CTR paths.
• Fixes UTF-8 YAML serialization and makes YAML tests propagate failures.

Diagram

graph TD
  CLI["ipctool CLI"] --> Runner["Benchmark runner"] --> Vectors["Known-answer checks"] --> Software["Software AEAD"] --> Results["Result model"] --> Output["YAML or JSON"]
  Runner --> Hardware["Cipher engine"] --> Results

High-Level Assessment

The following are alternative approaches to this PR:

1. Link an established crypto library

➕ Reduces custom cryptographic code requiring security and correctness review.
➕ Measures an implementation closer to application TLS stacks.
➖ Available cross-toolchains provide shared mbedcrypto libraries but no static archives.
➖ Static musl deployment cannot reliably load the platform library at runtime.
➖ Library configuration differences would reduce cross-device benchmark reproducibility.

2. Ship a dynamically linked benchmark helper

➕ Keeps custom primitives out of the main ipctool binary.
➕ Could use vendor or platform libraries when present.
➖ Many target images omit the required shared libraries.
➖ Creates a second deployment artifact and weakens the existing single-binary probe model.
➖ Cannot consistently run on the devices whose capabilities need inspection.

3. Use a standard kernel crypto interface

➕ Avoids maintaining a vendor-specific ioctl transcription.
➕ Could provide a common hardware benchmarking path across SoCs.
➖ The deployed vendor engine is exposed through /dev/cipher rather than a standard interface.
➖ Would not directly test GCM acceptance or batched behavior of the actual field driver.

Recommendation: Keep the in-tree, seal-only implementations and direct /dev/cipher probing because they preserve static deployment and measure the hardware actually present. The published known-answer checks substantially mitigate correctness risk; differential tests against a reference library in host CI would further strengthen this approach without adding a target dependency.

Files changed (14) +1602 / -8

Enhancement (11) +1551 / -0

aes.cImplement table-driven AES encryption and CTR +185/-0

Implement table-driven AES encryption and CTR

• Implements runtime-generated AES T-tables, AES-128/256 key expansion, forward block encryption, and in-place CTR processing. The implementation targets representative performance on CPUs without AES instructions.

src/crypto/aes.c

aes.hDefine the benchmark AES interface +43/-0

Define the benchmark AES interface

• Declares the AES context, supported key sizes, block encryption API, and full-width counter-mode API used by GCM and hardware verification.

src/crypto/aes.h

chachapoly.cImplement ChaCha20-Poly1305 packet sealing +278/-0

Implement ChaCha20-Poly1305 packet sealing

• Adds one-shot RFC 8439 sealing using ChaCha20 and a five-limb Poly1305 implementation. Correctly applies AEAD pad16 handling for partial AAD and ciphertext blocks.

src/crypto/chachapoly.c

chachapoly.hExpose one-shot ChaCha20-Poly1305 sealing +24/-0

Expose one-shot ChaCha20-Poly1305 sealing
• Declares the packet-oriented ChaCha20-Poly1305 seal API used by cryptobench.
src/crypto/chachapoly.h

gcm.cImplement AES-GCM sealing with table-based GHASH +131/-0

Implement AES-GCM sealing with table-based GHASH

• Adds AES-GCM key setup and one-shot sealing for 96-bit nonces. Uses a 4-bit GHASH table strategy representative of software TLS implementations on the target CPUs.

src/crypto/gcm.c

gcm.hDefine the AES-GCM benchmark interface +36/-0

Define the AES-GCM benchmark interface
• Declares the GCM context, AES-128/256 setup, and packet-sealing API for 96-bit nonces.
src/crypto/gcm.h

hisi_cipher.cAccess and probe the SoC cipher engine +245/-0

Access and probe the SoC cipher engine

• Transcribes the /dev/cipher ABI to create channels, probe AES-GCM support, and execute AES-CTR operations directly. Supports single-packet and optional batched ioctls, handles absent devices, and accommodates musl and glibc ioctl request types.

src/crypto/hisi_cipher.c

hisi_cipher.hDefine the HiSilicon cipher-engine adapter +74/-0

Define the HiSilicon cipher-engine adapter

• Defines cipher-engine limits, channel state, batch jobs, capability probing, and single or batched AES-CTR APIs without requiring the vendor shared library.

src/crypto/hisi_cipher.h

cryptobench.cAdd verified packet-sized crypto benchmarking +523/-0

Add verified packet-sized crypto benchmarking

• Implements the cryptobench command with configurable packet size, iterations, JSON output, and hardware opt-out. Verifies software AEAD and hardware CTR against published vectors, records wall and thread CPU time, probes hardware AEAD support, and reports single, batched, and ioctl-floor metrics.

src/cryptobench.c

cryptobench.hDeclare the cryptobench command entry point +6/-0

Declare the cryptobench command entry point
• Exposes the command handler for registration with the main ipctool dispatcher.
src/cryptobench.h

main.cRegister the cryptobench CLI command +6/-0

Register the cryptobench CLI command
• Adds cryptobench to global help output and dispatches the subcommand to its dedicated handler.
src/main.c

Bug fix (1) +12 / -4

cYAML.cPreserve UTF-8 bytes during YAML serialization +12/-4

Preserve UTF-8 bytes during YAML serialization

• Treats bytes as unsigned when identifying control characters, preventing non-ASCII UTF-8 from entering the escape path and causing the entire YAML print to fail. Control bytes remain escaped as Unicode sequences.

src/cjson/cYAML.c

Tests (1) +29 / -4

cYAML_test.cTest UTF-8 output and propagate YAML test failures +29/-4

Test UTF-8 output and propagate YAML test failures
• Adds coverage for UTF-8 pass-through and control-character escaping. Aggregates test results so the executable exits non-zero when any case fails.
src/cjson/cYAML_test.c

Other (1) +10 / -0

CMakeLists.txtCompile cryptobench and its crypto backends +10/-0

Compile cryptobench and its crypto backends
• Adds the benchmark command, software crypto primitives, and HiSilicon cipher adapter to the ipctool source list.
CMakeLists.txt

qodo-free-for-open-source-projects · 2026-09-02T09:53:24Z

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (2) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. --bytes permits non-packet AEAD 📎 Requirement gap ≡ Correctness

Description

The new option accepts payloads as small as 16 bytes and reports only that selected size, so a run
can omit the required approximately 1200-byte AEAD measurements. Although packet_bytes discloses
the size, materially different measurements are not accompanied by a packet-sized baseline.

Code

src/cryptobench.c[R482-483]

+            bytes = (size_t)strtoul(optarg, NULL, 10);
+            if (bytes < 16 || bytes > MAX_PACKET) {

Evidence
PR Compliance ID 2 requires packet-sized AEAD results and treats materially different sizes without
packet-sized results as a failure. The parser accepts 16–2048 bytes, while the output builder runs
each AEAD only with the selected bytes value.
Use Packet-Sized Buffers for Throughput Measurements
src/cryptobench.c[417-429]
src/cryptobench.c[482-489]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Custom `--bytes` values can replace the required approximately 1200-byte AEAD measurements entirely.
## Issue Context
PR Compliance ID 2 permits additional sizes only when packet-sized results remain available and disclosed.
## Fix Focus Areas
- src/cryptobench.c[408-430]
- src/cryptobench.c[460-510]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. cryptobench omits SoC identity ✓ Resolved 📎 Requirement gap ≡ Correctness

Description

The benchmark output contains measurements and parameters but no detected chip identifier, so saved
YAML or JSON results cannot be associated with the tested SoC. The repository already exposes
getchipname() and uses it to tag analogous membw results.

Code

src/cryptobench.c[R510-512]

+    cJSON *bench = build_cryptobench_json(bytes, iters, want_hw);
+    cJSON *root = cJSON_CreateObject();
+    cJSON_AddItemToObject(root, "cryptobench", bench);

Evidence
PR Compliance ID 3 requires results to identify or otherwise associate themselves with the tested
SoC. cryptobench constructs its output without a chip field, whereas membw demonstrates the
available getchipname() integration.
Report Results for the Detected SoC
src/cryptobench.c[421-430]
src/cryptobench.c[510-512]
src/membw.c[263-268]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`cryptobench` results do not identify the SoC on which the measurements were collected.
## Issue Context
Use the existing `getchipname()` facility, following the pattern used by `membw`, and include the detected chip in both YAML and JSON output.
## Fix Focus Areas
- src/cryptobench.c[48-54]
- src/cryptobench.c[510-512]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

3. Supported GCM remains unmeasured 📎 Requirement gap ≡ Correctness

Description

When the driver accepts AES-GCM, the code reports aead: supported but still benchmarks and emits
only AES-128-CTR hardware rows. Such a supported engine therefore receives no packet-sized AEAD seal
measurement comparable with the software AES-GCM workload.

Code

src/cryptobench.c[349]

+    const bool aead = hisi_cipher_supports_gcm(&hw);

Evidence
PR Compliance ID 4 requires comparable packet-sized AEAD measurements when an engine supports them.
The code records the GCM capability result but unconditionally builds only an aes_128_ctr hardware
result, with no GCM seal path.
Provide Comparable Crypto-Engine Measurements When Supported
src/cryptobench.c[349-357]
src/cryptobench.c[367-387]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The hardware capability probe can detect AES-GCM support, but the supported path does not execute an AES-GCM sealing benchmark.
## Issue Context
Unsupported engines may continue reporting the explanatory AES-CTR measurements, but engines that accept GCM must run the same packet-sized AES-GCM workload and report it on the same per-packet basis as software.
## Fix Focus Areas
- src/crypto/hisi_cipher.c[179-200]
- src/cryptobench.c[339-390]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

View high (1)

4. ~~Batch IV handling unverified~~ ✓ Resolved 🐞 Bug ≡ Correctness

Description

The batch probe submits identical zero IVs and plaintexts and checks only ioctl success, so it
cannot detect a driver that ignores per-package IVs. Such a driver can be enabled for benchmarking
with distinct IVs, while the batch row is incorrectly reported as verified based solely on the
single-packet result.

Code

src/crypto/hisi_cipher.c[R225-228]

+    uint8_t key[16] = {0}, iv[16] = {0}, a[16] = {0}, b[16] = {0};
+    hisi_cipher_job probe[2] = {
+        {iv, a, sizeof(a)},
+        {iv, b, sizeof(b)},

Evidence
Although the probe comment says two packets are intended to detect drivers that ignore per-package
IVs, both entries reference the same zero IV and use zero-filled buffers, and batching is enabled
solely when the ioctl succeeds. The actual benchmark initializes a distinct IV for each job, but it
does not validate the batch output and instead copies the verification flag from the unrelated
single-packet test.
src/crypto/hisi_cipher.c[225-231]
src/crypto/hisi_cipher.c[156-176]
src/cryptobench.c[316-327]
src/cryptobench.c[359-377]
src/crypto/hisi_cipher.c[221-231]
src/cryptobench.c[316-336]
src/cryptobench.c[363-386]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The batch capability probe cannot validate per-package IV handling because both jobs use the same IV and the output is never checked. Add batch-specific known-answer verification using distinct IVs, comparing every output against software AES-CTR or fixed expected ciphertext before enabling batching or marking the batch row as verified.
## Issue Context
A successful probe sets `batch_max`, enabling a batched benchmark whose real jobs use distinct IVs. The resulting row currently inherits `verified` from the single-packet CTR check, so a driver that accepts the ioctl but ignores per-package IVs can be presented as usable and verified despite producing incorrect batch results.
## Fix Focus Areas
- src/crypto/hisi_cipher.c[225-231]
- src/cryptobench.c[316-336]
- src/cryptobench.c[363-386]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

5. ~~Invalid UTF-8 emitted raw~~ ✓ Resolved 🐞 Bug ≡ Correctness

Description

The YAML printer now assumes every byte at least 128 belongs to valid UTF-8 and emits it unchanged.
U-Boot values are copied directly from raw flash without encoding validation, so invalid or
legacy-encoded bytes can produce an unparsable YAML stream.

Code

src/cjson/cYAML.c[R113-115]

+        if ((unsigned char)*c < 32) {
+            /* Expand non-printable characters. Bytes >= 128 are left alone:
+             * they are UTF-8, which YAML takes as-is. */

Evidence

The changed condition excludes every byte at least 128 from escaping and the following path copies
each such byte directly. Repository data flow shows raw U-Boot environment bytes reaching firmware
fields and then normal YAML serialization without any encoding check.

src/cjson/cYAML.c[113-125]
src/uboot.c[102-124]
src/firmware.c[110-115]
src/main.c[148-169]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Do not treat every high byte as valid UTF-8. Validate complete UTF-8 sequences and define safe handling for invalid bytes, such as replacement or an explicitly encoded representation, while preserving valid Unicode unchanged.
## Issue Context
U-Boot environment values originate as raw bytes and can reach cYAML without transcoding or UTF-8 validation.
## Fix Focus Areas
- src/cjson/cYAML.c[84-125]
- src/uboot.c[102-124]
- src/firmware.c[110-115]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

6. ~~Partial batch packets are unhandled~~ ✓ Resolved 🐞 Bug ☼ Reliability

Description

hisi_cipher_ctr_batch() submits a non-block-aligned job length directly, although the
single-packet path pads it because the engine takes whole blocks. Since --bytes accepts values
such as 17 and passes them into the batch benchmark, valid command input can fail or use different
CTR semantics in the batch row.

Code

src/crypto/hisi_cipher.c[R156-162]

+    for (size_t i = 0; i < count; i++) {
+        if (jobs[i].len == 0 || jobs[i].len > HISI_CIPHER_MAX_LEN)
+            return false;
+        pkg[i].src.cp = jobs[i].buf;
+        pkg[i].dst.p = jobs[i].buf;
+        pkg[i].length = (uint32_t)jobs[i].len;
+        memcpy(pkg[i].iv, jobs[i].iv, 16);

Evidence

The implementation explicitly states that the engine takes whole blocks, and the single-packet
implementation rounds its submitted length up before copying only the requested bytes back. The
batch implementation performs neither operation and directly assigns the caller's arbitrary length;
the command-line parser permits those lengths.

src/crypto/hisi_cipher.c[81-85]
src/crypto/hisi_cipher.c[122-146]
src/crypto/hisi_cipher.c[149-176]
src/cryptobench.c[481-488]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The batch path accepts arbitrary job sizes but forwards each size to the engine unchanged. The single-job CTR path pads short final blocks because the cipher engine consumes whole blocks, so the two benchmark paths do not handle the documented `--bytes` range consistently.
## Issue Context
`cryptobench` permits every size from 16 through 2048 and invokes the batch benchmark when the driver advertises support. In particular, packet lengths not divisible by 16 reach this path.
## Fix Focus Areas
- src/crypto/hisi_cipher.c[149-176]
- src/crypto/hisi_cipher.c[81-85]
- src/crypto/hisi_cipher.c[122-146]
- src/cryptobench.c[367-377]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

7. ~~Iterations can wrap enormous~~ ✓ Resolved 🐞 Bug ☼ Reliability

Description

Both numeric options use strtoul() without checking errors or full input consumption and narrow
the result to unsigned before validating only minimum bounds. Consequently, inputs such as
--iters -1, --iters 100junk, or overflowing values can silently become unintended iteration
counts—including UINT_MAX—and make the diagnostic command execute benchmark loops billions of
times instead of rejecting invalid input.

Code

src/cryptobench.c[R491-496]

+        case 'i':
+            iters = (unsigned)strtoul(optarg, NULL, 10);
+            if (iters < 100) {
+                fprintf(stderr, "cryptobench: --iters must be >= 100\n");
+                return EXIT_FAILURE;
+            }

Evidence

The parser passes a null end pointer to both strtoul() calls, narrows each result before applying
post-cast range checks, and therefore fails to detect trailing characters, conversion errors, and
some narrowing overflows. The unchecked iteration value is then passed to build_cryptobench_json
and used directly as the loop bound by software and hardware benchmark loops, showing how malformed
input can cause an unexpectedly long run.

src/cryptobench.c[491-510]
src/cryptobench.c[257-263]
src/cryptobench.c[277-282]
src/cryptobench.c[417-429]
src/cryptobench.c[481-496]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Numeric option parsing uses `strtoul()` without checking conversion errors or requiring full argument consumption, then narrows the result to `unsigned` before validating its range. Fix both `--bytes` and `--iters` by using an end pointer, checking `errno`, requiring the entire argument to be consumed, validating bounds before casting, and rejecting negative, trailing-character, or overflowing inputs rather than silently using a different value.
## Issue Context
The same unsafe parsing pattern is used for both numeric options. In particular, `strtoul()` accepts an optional leading minus sign, so `--iters -1` can become `UINT_MAX` after conversion and narrowing; the parsed iteration value is passed to all software and hardware benchmark loops and can therefore cause unexpectedly long benchmark runs.
## Fix Focus Areas
- src/cryptobench.c[481-496]
- src/cryptobench.c[510-510]
- src/cryptobench.c[257-260]
- src/cryptobench.c[277-280]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Tip of the day

💡 Did you know, you can turn on the rule miner and Qodo learns your standards from review history

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

qodo-free-for-open-source-projects · 2026-09-02T09:53:25Z

+            bytes = (size_t)strtoul(optarg, NULL, 10);
+            if (bytes < 16 || bytes > MAX_PACKET) {


1. --bytes permits non-packet aead 📎 Requirement gap ≡ Correctness

The new option accepts payloads as small as 16 bytes and reports only that selected size, so a run can omit the required approximately 1200-byte AEAD measurements. Although packet_bytes discloses the size, materially different measurements are not accompanied by a packet-sized baseline.

Agent Prompt

## Issue description Custom `--bytes` values can replace the required approximately 1200-byte AEAD measurements entirely. ## Issue Context PR Compliance ID 2 permits additional sizes only when packet-sized results remain available and disclosed. ## Fix Focus Areas - src/cryptobench.c[408-430] - src/cryptobench.c[460-510]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

qodo-free-for-open-source-projects · 2026-09-02T09:53:25Z

+        return j_inner;
+    }
+
+    const bool aead = hisi_cipher_supports_gcm(&hw);


3. Supported gcm remains unmeasured 📎 Requirement gap ≡ Correctness

When the driver accepts AES-GCM, the code reports aead: supported but still benchmarks and emits only AES-128-CTR hardware rows. Such a supported engine therefore receives no packet-sized AEAD seal measurement comparable with the software AES-GCM workload.

Agent Prompt

## Issue description The hardware capability probe can detect AES-GCM support, but the supported path does not execute an AES-GCM sealing benchmark. ## Issue Context Unsupported engines may continue reporting the explanatory AES-CTR measurements, but engines that accept GCM must run the same packet-sized AES-GCM workload and report it on the same per-packet basis as software. ## Fix Focus Areas - src/crypto/hisi_cipher.c[179-200] - src/cryptobench.c[339-390]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

The batch probe was the real defect. Its own comment claimed it used "two packets rather than one, because one would also succeed on a driver that ignored the per-package IV entirely" -- and then submitted two packets with the SAME zero IV and never looked at the output. It checked that the ioctl existed and nothing else, while the row it enabled inherited `verified` from the single-packet vector, so a driver that accepted the burst and applied the channel IV to all fifteen packages would have been reported as a verified 21 MB/s. There is now a known-answer test for the batched command specifically: fifteen packages, each with a distinct IV and distinct plaintext, every one compared against the software AES-CTR in this same binary, which F.5.1 has already pinned to the standard. `batched.verified` stands on that vector rather than inheriting, and the row is not measured unless it passes. On a gk7205v200 all fifteen match, so openhisilicon#217 does honour per-package IVs -- previously assumed, now checked. That test also settles a question raised in review about the batch path handing unrounded lengths to the driver while the single-packet path pads: it runs at the benchmark's own length, which is 1100 bytes by default on that board and not a multiple of the block. It passes, so the driver rounds the descriptor itself and the two paths agree. Padding in the batch path would need a bounce buffer per package and would make the benchmark measure that copy instead of the engine; the asymmetry is now documented as deliberate rather than left to look like an oversight. Also: - Tag the output with `chip:` from getchipname(), as membw does. #186 asks for a table with a row per SoC, and a row of numbers that does not say which SoC is not a row. - `--bytes`/`--iters` went through strtoul() with no endptr and no errno check, then narrowed to unsigned before a minimum-only bound. `--iters -1` became UINT_MAX and `--iters 100junk` silently became 100. Require the whole argument to parse and range-check before narrowing. - A part that does accept AES-GCM reported `aead: supported` and then measured only AES-CTR. No such part was available to test against -- CHIP_AES_CCM_GCM_SUPPORT is hi3569v100 alone -- so rather than ship a sealing path that has never run, it now reports "supported, not measured" and asks for an issue. An unverifiable number is worse than a missing one. - cYAML: validate UTF-8 rather than assuming every byte >= 128 is part of a well-formed sequence. U-Boot environments reach the printer as raw flash bytes, so latin-1 or truncated input could produce a stream that no parser accepts. Valid sequences still pass through unescaped; invalid bytes, overlong forms, surrogates and out-of-range code points become \u00XX, which round-trips losslessly. One more test. Not changed: review also asked that `--bytes` be constrained so a run cannot report a size other than ~1200. Choosing the size is the feature -- the default is 1200, every run states its `packet_bytes`, and the 16-byte row exists precisely to show how much of the cost is per-call. A benchmark that refuses to measure what it is asked to measure would answer a narrower question than #186 poses.

widgetii · 2026-09-02T10:02:30Z

Thanks — finding 4 was a real defect and a well-aimed one. Dispositions below; fixes in ecca9ce.

4. Batch IV handling unverified — fixed, and it was worse than described. The probe's own comment claimed it used "two packets rather than one, because one would also succeed on a driver that ignored the per-package IV entirely", and then submitted two packets with the same zero IV and never looked at the output. So the comment described a check that did not exist, and the row it enabled inherited verified from the single-packet vector on top of that.

There is now a known-answer test for the batched command specifically: fifteen packages, each with a distinct IV and distinct plaintext, every one compared against the software AES-CTR in this same binary (which F.5.1 has already pinned to the standard). batched.verified stands on that vector instead of inheriting, and the row is not measured unless it passes. All fifteen match on a gk7205v200, so OpenIPC/openhisilicon#217 does honour per-package IVs — previously assumed, now checked.

6. Partial batch packets — answered by that same test, no padding added. The new vector runs at the benchmark's own length, which on that board is 1100 bytes by default and is not a multiple of the block. It passes, so the driver rounds the descriptor itself and the two paths agree. Padding in the batch path would need a bounce buffer per package and would make the benchmark measure that copy rather than the engine, so the asymmetry stays — now documented as deliberate and verified, rather than left looking like an oversight.

2. SoC identity — fixed. Correct, and it undercut the point of the PR: #186 wants a table with a row per SoC, and a row that does not say which SoC is not a row. Now tagged with getchipname() following membw.

7. Iterations can wrap — fixed. --iters -1 became UINT_MAX and --iters 100junk silently became 100. Both options now require the whole argument to parse and are range-checked before narrowing.

3. Supported GCM remains unmeasured — reported honestly rather than implemented. No part that accepts AES-GCM exists to test against: CHIP_AES_CCM_GCM_SUPPORT is defined for hi3569v100 alone, and nothing OpenIPC ships is that part. Writing a sealing path that has never executed and cannot be checked against a vector would produce exactly the failure mode this tool is built to avoid — a fast number nobody can trust. That path now reports aead: supported, not measured and asks for an issue to be opened, which is a lead to hardware rather than a guess about it.

5. Invalid UTF-8 emitted raw — fixed properly. Fair: my change traded a total-output failure for a possibly-unparsable one, which is better but not right. cYAML now validates sequences instead of assuming every byte ≥ 128 is well-formed. Valid sequences pass through unescaped; invalid bytes, overlong forms, surrogates and out-of-range code points become \u00XX, which round-trips losslessly and keeps the document parsable. Test added.

1. --bytes permits non-packet AEAD — declining. Choosing the size is the feature, not a gap. The default is 1200, every run states its own packet_bytes, and the 16-byte row exists specifically to expose how much of the cost is per-call rather than per-byte — that row is only obtainable because the size is selectable. A benchmark that refused to measure what it was asked to measure would answer a narrower question than #186 poses, and would not have produced the finding that half the engine's per-packet cost is the trip.

widgetii added 2 commits September 2, 2026 12:47

qodo-free-for-open-source-projects Bot reviewed Sep 2, 2026

View reviewed changes

widgetii merged commit 76bf277 into master Sep 2, 2026
4 checks passed

widgetii deleted the feat/cryptobench branch September 2, 2026 10:07

widgetii mentioned this pull request Sep 2, 2026

Proposal: per-SoC AEAD throughput probe (ChaCha20 is 2.3x AES-GCM on GK7202V300) #186

Closed

Navigation Menu

Uh oh!

Uh oh!

cryptobench: per-SoC AEAD seal rate, in software and on the cipher engine - #187

cryptobench: per-SoC AEAD seal rate, in software and on the cipher engine#187
widgetii merged 3 commits into
masterfrom
feat/cryptobench

widgetii commented Sep 2, 2026 •

edited

Loading

Uh oh!

qodo-free-for-open-source-projects Bot commented Sep 2, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Sep 2, 2026 •

edited

Loading

Uh oh!

qodo-free-for-open-source-projects Bot Sep 2, 2026

Uh oh!

Uh oh!

qodo-free-for-open-source-projects Bot Sep 2, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

widgetii commented Sep 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		bytes = (size_t)strtoul(optarg, NULL, 10);
		if (bytes < 16 \|\| bytes > MAX_PACKET) {

Uh oh!

Uh oh!

Conversation

widgetii commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The finding that reframes the proposal

Numbers

Your three questions

Correctness before numbers

Measurement discipline

Degradation, tested on the board rather than written

Second commit, unrelated but found by this work

Uh oh!

qodo-free-for-open-source-projects Bot commented Sep 2, 2026

PR Summary by Qodo

Uh oh!

qodo-free-for-open-source-projects Bot commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

qodo-free-for-open-source-projects Bot Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

qodo-free-for-open-source-projects Bot Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

widgetii commented Sep 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

widgetii commented Sep 2, 2026 •

edited

Loading

qodo-free-for-open-source-projects Bot commented Sep 2, 2026 •

edited

Loading