Aspeed next

[legoater]

Summary by CodeRabbit

• New Features

  • Added support for 64-bit DMA addressing in the ASPEED Hash and Crypto Engine (HACE) emulation, specifically for the AST2700 platform.
  • Introduced new QTest test suites for ASPEED HACE on AST2700, covering various hash algorithms and modes.

• Bug Fixes

  • Adjusted memory region sizes and addresses for improved accuracy in memory mapping and device initialization.
  • Tightened access size restrictions for certain memory regions to ensure correct operation.

• Tests

  • Added comprehensive test coverage for HACE functionality, including direct, scatter-gather, and accumulative hashing modes on multiple platforms.
  • Improved memory management in test suites by ensuring temporary files and memory are properly freed.

• Documentation

  • Updated documentation to reflect changes in supported board listings.

• Chores

  • Enhanced trace logging for debugging and monitoring of HACE operations.
  • Reorganized and extended test source file associations for Aspeed and Ast2700 test suites.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

The changes introduce 64-bit DMA support and major refactoring to the ASPEED HACE (hash and crypto engine) emulation, including dynamic register allocation, new register masks, and improved memory mapping. Extensive QTest-based test suites for HACE on multiple platforms, including AST2700, are added. Documentation and test infrastructure are updated accordingly.

Changes

File(s)	Change Summary
docs/system/arm/aspeed.rst	Removed ast2700fc from supported boards list.
hw/arm/aspeed_ast27x0-fc.c, hw/arm/aspeed_ast27x0.c, hw/arm/fby35.c	Adjusted memory region sizes and mappings; updated NIC configuration and memory subregion registration.
hw/intc/aspeed_intc.c	Set .impl.min_access_size = 4 in MemoryRegionOps for all INTC variants; reformatted function signatures.
hw/misc/aspeed_hace.c	Added 64-bit DMA support for AST2700; refactored hash operation logic; dynamic register allocation; improved tracing and error handling.
hw/misc/trace-events	Added new trace events for HACE operations, including hexdump and hash execution tracing.
include/hw/misc/aspeed_hace.h	Refactored HACE state struct: dynamic register array, new mask fields, and DMA64 capability flag.
tests/qtest/aspeed-hace-utils.c, tests/qtest/aspeed-hace-utils.h	Introduced utility module with test vectors, helpers, and prototypes for HACE QTest suites.
tests/qtest/aspeed_hace-test.c	Refactored to use utility helpers for concise test registration and execution across platforms.
tests/qtest/ast2700-hace-test.c	New QTest suite for AST2700 HACE: tests for multiple hash algorithms and address masking.
tests/qtest/aspeed_smc-test.c, tests/qtest/ast2700-smc-test.c	Added g_free() to free temporary path strings after unlinking files.
tests/qtest/meson.build	Reorganized and extended test source file associations; added AST2700 HACE test to build.

Sequence Diagram(s)

sequenceDiagram
    participant Guest
    participant HACE_Device
    participant SystemMemory

    Note over Guest, HACE_Device: Hash Operation (Direct or SG Mode)
    Guest->>HACE_Device: Write registers to configure hash
    HACE_Device->>SystemMemory: Map source buffer(s) (direct or SG)
    HACE_Device->>HACE_Device: Prepare iovec(s) for hash
    HACE_Device->>HACE_Device: Execute hash (accumulated or non-accumulated)
    HACE_Device->>SystemMemory: Write digest result
    HACE_Device->>Guest: Set IRQ/status, signal completion

Loading

sequenceDiagram
    participant QTest
    participant QEMU
    participant HACE_Device

    QTest->>QEMU: Start VM with test image
    QTest->>QEMU: Write test vector to guest memory
    QTest->>QEMU: Write HACE config registers
    QEMU->>HACE_Device: Emulate hash operation
    HACE_Device->>QEMU: Set status/IRQ
    QTest->>QEMU: Read digest result
    QTest->>QEMU: Assert correctness

Loading

Poem

In the warren of code, a new path we trace,
HACE now hops with 64-bit grace!
Tests abound for every hash,
Dynamic regs—no more static stash.
AST2700 joins the fun,
With QEMU’s engine, hashes run.
🐰✨—another leap, another race!

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Explain this complex logic.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai explain this code block.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and explain its main purpose.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR.
• @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[legoater]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 5

🧹 Nitpick comments (7)

hw/arm/aspeed_ast27x0-fc.c (1)

91-96: Improve error handling in NIC configuration loop.

The current loop breaks on the first configuration failure, which may leave some NICs unconfigured. Consider whether this is the intended behavior or if error handling should be improved.

    for (int i = 0; i < sc->macs_num; i++) {
-        if (!qemu_configure_nic_device(DEVICE(&soc->ftgmac100[i]),
-                                       true, NULL)) {
-            break;
-        }
+        bool success = qemu_configure_nic_device(DEVICE(&soc->ftgmac100[i]),
+                                                 true, NULL);
+        if (!success) {
+            warn_report("Failed to configure NIC device %d", i);
+            // Continue configuring remaining NICs or return error based on requirements
+        }
    }

tests/qtest/ast2700-hace-test.c (2)

24-72: Cut repetition – use a helper macro or table-driven loop

Each test wrapper only differs by the helper function invoked. This creates 11 near-identical functions.

A compact pattern keeps the file shorter and reduces the chance of forgetting to add a new variant later:

typedef void (*AlgoFn)(const char *mach, uint64_t base, uint64_t mem);

static struct {
    const char *name;
    AlgoFn fn;
} ast2700_tests[] = {
    { "md5",             aspeed_test_md5 },
    { "sha256",          aspeed_test_sha256 },
    { "sha256_sg",       aspeed_test_sha256_sg },
    ...
};

static void run_algo(gconstpointer user_data)
{
    const AlgoFn fn = user_data;
    fn("-machine ast2700a1-evb", 0x12070000, 0x400000000ULL);
}

and register with:

for (size_t i = 0; i < ARRAY_SIZE(ast2700_tests); ++i) {
    qtest_add_data_func(g_strdup_printf("ast2700/hace/%s", ast2700_tests[i].name),
                        ast2700_tests[i].fn,
                        run_algo);
}

This is optional, yet it noticeably improves maintainability.

---

79-98: Registration order – non-issue, but mind alphabetical grouping

Tests are currently listed in a quasi-reverse cryptographic-strength order (sha512→sha256→md5). Grouping by algorithm or mode (direct/sg/accum) makes qtest output easier to scan, though this is purely cosmetic.

tests/qtest/aspeed-hace-utils.h (1)

45-48: Avoid unnecessary __packed__ on naturally-aligned structure

AspeedSgList only contains two 32-bit words, which are already naturally aligned on all QEMU host targets. Adding __packed__ disables the compiler’s normal alignment and can generate sub-optimal code on some architectures.

-} __attribute__ ((__packed__));
+};

tests/qtest/aspeed-hace-utils.c (1)

99-107: Minor: avoid heap buffer when tracing large iovecs

hace_iov_hexdump() copies the whole iovec into a temporary buffer even
when tracing is disabled or when the payload is huge. Walking the iovec and
calling hace_hexdump() chunk-by-chunk removes the extra allocation and
keeps memory usage bounded.

Not urgent, but worth streamlining if very large scatter-gather requests are
expected.

tests/qtest/aspeed_hace-test.c (1)

195-236: Register tests via data-driven table to DRY up boiler-plate

The qtest_add_func() calls repeat the same pattern dozens of times,
making future maintenance error-prone. A small table and a tiny helper
would cut ~150 lines:

typedef void (*TestFn)(void);
static struct {
    const char *name;
    TestFn      fn;
} tests[] = {
    { "ast1030/hace/sha512",        test_sha512_ast1030 },
    { "ast1030/hace/sha512_sg",     test_sha512_sg_ast1030 },
    /* … */
};

for (size_t i = 0; i < G_N_ELEMENTS(tests); ++i) {
    qtest_add_func(tests[i].name, tests[i].fn);
}

This keeps the list declarative, helps spot duplicates, and eases adding
new variants.

hw/misc/aspeed_hace.c (1)

401-417: Skip digest write when no digest is produced

In accumulation mode, intermediate chunks call
hash_write_digest_and_unmap_iov() with digest_len == 0. Although the
current address_space_write() bails out early, an explicit guard would
avoid passing a NULL pointer and makes the intent clearer:

-    hash_write_digest_and_unmap_iov(s, iov, iov_idx, digest_buf, digest_len);
+    if (digest_len) {
+        hash_write_digest_and_unmap_iov(s, iov, iov_idx,
+                                        digest_buf, digest_len);
+    } else {
+        /* Still need to un-map the iov */
+        for (; iov_idx > 0; iov_idx--) {
+            address_space_unmap(&s->dram_as, iov[iov_idx - 1].iov_base,
+                                iov[iov_idx - 1].iov_len, false,
+                                iov[iov_idx - 1].iov_len);
+        }
+    }

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b9aa4e3 and 8eaea40.

📒 Files selected for processing (15)

• docs/system/arm/aspeed.rst (1 hunks)
• hw/arm/aspeed_ast27x0-fc.c (3 hunks)
• hw/arm/aspeed_ast27x0.c (3 hunks)
• hw/arm/fby35.c (1 hunks)
• hw/intc/aspeed_intc.c (8 hunks)
• hw/misc/aspeed_hace.c (16 hunks)
• hw/misc/trace-events (1 hunks)
• include/hw/misc/aspeed_hace.h (2 hunks)
• tests/qtest/aspeed-hace-utils.c (1 hunks)
• tests/qtest/aspeed-hace-utils.h (1 hunks)
• tests/qtest/aspeed_hace-test.c (1 hunks)
• tests/qtest/aspeed_smc-test.c (1 hunks)
• tests/qtest/ast2700-hace-test.c (1 hunks)
• tests/qtest/ast2700-smc-test.c (1 hunks)
• tests/qtest/meson.build (2 hunks)

🧰 Additional context used

🧬 Code Graph Analysis (5)

hw/arm/fby35.c (2)

system/memory.c (1)

• memory_region_add_subregion (2668-2674)

system/physmem.c (1)

• get_system_memory (2808-2811)

tests/qtest/ast2700-smc-test.c (1)

rust/qemu-api/src/lib.rs (1)

• g_free (55-55)

tests/qtest/aspeed_smc-test.c (1)

rust/qemu-api/src/lib.rs (1)

• g_free (55-55)

hw/arm/aspeed_ast27x0-fc.c (2)

system/memory.c (1)

• memory_region_add_subregion (2668-2674)

system/physmem.c (1)

• get_system_memory (2808-2811)

hw/misc/aspeed_hace.c (2)

rust/qemu-api/src/lib.rs (1)

• g_free (55-55)

system/physmem.c (3)

• address_space_map (3398-3460)
• address_space_write (3128-3142)
• address_space_unmap (3466-3500)

🔇 Additional comments (17)

tests/qtest/aspeed_smc-test.c (1)

231-234: LGTM! Proper memory management for temporary paths.

The addition of g_free() calls for the tmp_path members ensures proper cleanup of memory allocated by g_file_open_tmp(). The placement after the unlink() calls is correct and prevents memory leaks.

tests/qtest/ast2700-smc-test.c (1)

70-70: LGTM! Consistent memory management improvement.

The addition of g_free() for the temporary path ensures proper cleanup of memory allocated by g_file_open_tmp(). This change is consistent with similar improvements made in tests/qtest/aspeed_smc-test.c.

hw/intc/aspeed_intc.c (2)

740-740: LGTM! Consistent implementation access size enforcement.

The addition of .impl.min_access_size = 4 to all ASPEED INTC MemoryRegionOps structures ensures consistent 4-byte minimum access size enforcement at the implementation level. This aligns well with the existing .valid.min_access_size = 4 settings and improves hardware emulation accuracy.

Also applies to: 751-751, 762-762, 773-773, 784-784, 795-795

---

1004-1005: LGTM! Improved function signature formatting.

The function signature formatting changes enhance code readability by splitting long parameter lists across multiple lines. This follows good coding style practices without changing functionality.

Also applies to: 1073-1074

hw/arm/fby35.c (1)

80-80:

✅ Verification successful

LGTM! Memory region integration follows established pattern.

The addition of the BMC memory region to system memory at address 0 aligns with similar changes in other Aspeed ARM board files and properly integrates the BMC memory into the global system memory map.

Verify that the BMC memory region with size UINT64_MAX doesn't overlap with other memory regions in the system memory map:

---

🏁 Script executed:

#!/bin/bash
# Description: Check for potential memory region overlaps in FBY35 board
# Expected: No overlapping memory regions should be found

# Search for other memory_region_add_subregion calls in the same file
rg -A 2 -B 2 "memory_region_add_subregion" hw/arm/fby35.c

# Search for similar memory map patterns in other Aspeed boards
rg -A 2 -B 2 "memory_region_add_subregion.*get_system_memory.*0" hw/arm/aspeed*.c

Length of output: 1366

---

Approve BMC memory region addition—pattern verified

We searched other Aspeed ARM boards (e.g., hw/arm/aspeed_ast27x0-fc.c) and found the same

memory_region_add_subregion(get_system_memory(), 0, &…_memory);

pattern with UINT64_MAX sizing. There are no indications of overlapping regions. LGTM!

hw/arm/aspeed_ast27x0-fc.c (2)

71-71: LGTM! Memory region integration follows established pattern.

The addition of the CA35 memory region to system memory at address 0 is consistent with the approach used in other Aspeed board files.

---

51-51:

✅ Verification successful

Verify the impact of BMC RAM size reduction.

The BMC RAM size has been reduced from 2 GiB to 1 GiB. Ensure this change doesn't break existing firmware or software that expects the larger memory size.

---

🏁 Script executed:

#!/bin/bash
# Description: Check for references to the old BMC RAM size across the codebase
# Expected: No hard-coded references to 2 GiB BMC RAM size should remain

# Search for references to 2 GiB in Aspeed-related files
rg -i "2.*gib|2\s*\*\s*gib" hw/arm/aspeed*.c

# Search for AST2700FC_BMC_RAM_SIZE usage
rg "AST2700FC_BMC_RAM_SIZE" hw/arm/

Length of output: 786

---

No remaining hard-coded 2 GiB references for AST2700FC

I verified that AST2700FC_BMC_RAM_SIZE is only defined and used within hw/arm/aspeed_ast27x0-fc.c and there are no leftover hard-coded “2 GiB” values for this variant. Other BMC variants (Fuji, Bletchley) still correctly use 2 GiB.

– hw/arm/aspeed_ast27x0-fc.c: #define AST2700FC_BMC_RAM_SIZE (1 * GiB) and its uses
– hw/arm/aspeed.c: Fuji/Bletchley variants remain at 2 GiB as expected

No further changes required.

hw/arm/aspeed_ast27x0.c (4)

349-351: LGTM! Improved memory write implementation.

The update to use address_space_stl_le with explicit result handling is correct and improves the robustness of the RAM capacity write operation.

---

26-26:

✅ Verification successful

Verify the impact of IO size reduction.

The AST2700_SOC_IO_SIZE has been reduced. Ensure this change doesn't affect existing device mappings or cause memory layout conflicts.

---

🏁 Script executed:

#!/bin/bash
# Description: Check the usage of AST2700_SOC_IO_SIZE and verify no memory layout conflicts
# Expected: No conflicts with the reduced IO size

# Find all usages of AST2700_SOC_IO_SIZE
rg "AST2700_SOC_IO_SIZE" hw/arm/

# Check for any hard-coded references to the old size
rg "0x00FE0000|0x01000000" hw/arm/aspeed*.c

Length of output: 674

---

AST2700_SOC_IO_SIZE reduction verified – no conflicts found

A search for AST2700_SOC_IO_SIZE and hard-coded old sizes across hw/arm/ only returned usages within hw/arm/aspeed_ast27x0.c itself. There are no references to the prior size in other files, so the reduced IO region does not collide with existing mappings.

---

33-33:

❓ Verification inconclusive

Verify the impact of IOMEM device relocation.

The ASPEED_DEV_IOMEM device has been moved from address 0x00000000 to 0x00020000. This is a significant memory map change that could break existing firmware or software compatibility.

---

🏁 Script executed:

#!/bin/bash
# Description: Verify the impact of IOMEM device relocation
# Expected: No references to the old address should remain

# Search for references to the old IOMEM address
rg "0x00000000" hw/arm/aspeed*.c

# Check for any firmware or documentation that might reference the old address
rg -i "iomem.*0x00000000|0x00000000.*iomem" docs/ hw/arm/

Length of output: 965

---

Verify relocation of ASPEED_DEV_IOMEM and remaining 0x00000000 mappings

Moving ASPEED_DEV_IOMEM from 0x00000000 to 0x00020000 is a breaking change. In addition, the following device base addresses still resolve to 0x00000000:

• hw/arm/aspeed_ast27x0.c:
  [ASPEED_DEV_VBOOTROM] = 0x00000000
• hw/arm/aspeed_ast27x0-tsp.c & hw/arm/aspeed_ast27x0-ssp.c:
  [ASPEED_DEV_SRAM] = 0x00000000
• hw/arm/aspeed_ast2600.c & hw/arm/aspeed_ast2400.c:
  [ASPEED_DEV_SPI_BOOT] = 0x00000000
• hw/arm/aspeed_ast10x0.c:
  [ASPEED_DEV_SRAM] = 0x00000000
• hw/arm/aspeed.c:
  #define FUJI_BMC_HW_STRAP1 0x00000000
#define FUJI_BMC_HW_STRAP2 0x00000000
#define QCOM_DC_SCM_V1_BMC_HW_STRAP1 0x00000000

Please audit all device mappings, linker scripts, firmware blobs, and documentation to ensure no stale references remain and that firmware/software compatibility is maintained.

---

364-364:

✅ Verification successful

Verify the impact of 4-byte access size constraints.

The addition of .impl.min_access_size = 4 and the constraint of both .valid.min_access_size and .valid.max_access_size to 4 bytes enforces 4-byte aligned access. This could break existing code that uses non-4-byte accesses.

Also applies to: 366-367

---

🏁 Script executed:

#!/bin/bash
# Description: Check for potential compatibility issues with 4-byte access constraints
# Expected: Verify that all existing access patterns are 4-byte aligned

# Search for other memory region operations that might use different access sizes
rg -A 5 -B 5 "MemoryRegionOps.*aspeed.*ram" hw/

# Check for similar access size constraints in other Aspeed devices
rg "min_access_size.*=.*4|max_access_size.*=.*4" hw/arm/aspeed*.c hw/*/aspeed*.c

Length of output: 4975

---

4-byte access constraint matches existing Aspeed drivers

A repository-wide search confirms that all Aspeed MemoryRegionOps implementations already restrict accesses to 4 bytes (both in .impl and .valid):

• hw/arm/aspeed_ast27x0.c (line 364): .impl.min_access_size = 4
• Similar constraints in hw/timer/aspeed_timer.c, hw/sd/aspeed_sdhci.c, hw/misc/aspeed_xdma.c, hw/misc/aspeed_scu.c, and many others

This addition is consistent with the established pattern and does not introduce new compatibility risks.

tests/qtest/meson.build (3)

215-217: LGTM! Improved test organization and coverage.

The reordering of qtests_aspeed list and addition of aspeed_hace-test and aspeed_smc-test enhances test coverage for ASPEED hardware emulation.

---

220-220: LGTM! AST2700 HACE test addition.

The addition of ast2700-hace-test to the qtests_aspeed64 list appropriately extends test coverage for the AST2700 platform.

---

365-368: LGTM! Proper test file mappings.

The addition of test file mappings for the new HACE and SMC tests is correctly structured, properly linking source files with their utility dependencies.

tests/qtest/ast2700-hace-test.c (1)

13-21: Mask constants – double-check high-word coverage

The src_hi, dest_hi, and key_hi masks are limited to 0x00000003 (i.e. two low-order address bits).
If the AST2700 HACE really decodes the full 34-bit address range (as suggested by the 0x400000000 test DMA buffer), you may need to allow the full width of the high dword (8 bits on AST2600, 18 bits here). Please confirm the reference manual and extend the masks accordingly; otherwise some illegal addresses will silently be accepted in the model.

hw/misc/trace-events (1)

305-312: Tracepoint argument/format mismatch guard

The new aspeed_hace_hexdump event passes a char * that presumably points to a pre-formatted hexdump buffer. Beware of large dumps overflowing trace backend line limits; consider truncating or splitting long strings.

Otherwise, argument counts and format specifiers look consistent (bool → %d, 64-bit → %" PRIx64 "). No action needed.

tests/qtest/aspeed-hace-utils.c (1)

223-236: ⚠️ Potential issue

iov_len truncation breaks later un-mapping

When padding is detected you overwrite iov[0].iov_len with pad_offset.
address_space_unmap() later receives this shortened length, although the
buffer was mapped with the original plen. For RAM pointers this only
skips cache maintenance, but for bounce-buffers the mismatch can leave part
of the allocation dirty or leaked.

-            iov[0].iov_len = pad_offset;
+            /*
+             * Keep the original mapping length for a correct un-map, but
+             * expose only the meaningful payload to the hash engine.
+             */
+            uint32_t payload_len       = pad_offset;
+            uint32_t mapping_len_full  = plen;
+
+            iov[0].iov_len             = payload_len;
+            /* store the full length in a side array or reuse `plen`
+             * when calling address_space_unmap()
+             */

Consider storing the mapping length in a parallel array or an auxiliary
field so the exact size passed to address_space_unmap() always matches the
one returned by address_space_map().

Likely an incorrect or invalid review comment.

[coderabbitai]

⚠️ Potential issue

Inconsistency between introduction and detailed documentation.

The ast2700fc board appears to have been removed from the introductory list of supported boards, but it's still documented later in the file at line 258. This creates an inconsistency that may confuse users.

Either:

1. Remove all references to ast2700fc if it's no longer supported, or
2. Add it back to the introductory list if it remains supported

-Aspeed family boards (``ast2500-evb``, ``ast2600-evb``, ``ast2700-evb``, ``bletchley-bmc``, ``fuji-bmc``, ``fby35-bmc``, ``fp5280g2-bmc``, ``g220a-bmc``, ``palmetto-bmc``, ``qcom-dc-scm-v1-bmc``, ``qcom-firework-bmc``, ``quanta-q71l-bmc``, ``rainier-bmc``, ``romulus-bmc``, ``sonorapass-bmc``, ``supermicrox11-bmc``, ``supermicrox11spi-bmc``, ``tiogapass-bmc``, ``witherspoon-bmc``, ``yosemitev2-bmc``)
+Aspeed family boards (``ast2500-evb``, ``ast2600-evb``, ``ast2700-evb``, ``ast2700fc``, ``bletchley-bmc``, ``fuji-bmc``, ``fby35-bmc``, ``fp5280g2-bmc``, ``g220a-bmc``, ``palmetto-bmc``, ``qcom-dc-scm-v1-bmc``, ``qcom-firework-bmc``, ``quanta-q71l-bmc``, ``rainier-bmc``, ``romulus-bmc``, ``sonorapass-bmc``, ``supermicrox11-bmc``, ``supermicrox11spi-bmc``, ``tiogapass-bmc``, ``witherspoon-bmc``, ``yosemitev2-bmc``)

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	Aspeed family boards (``ast2500-evb``, ``ast2600-evb``, ``ast2700-evb``, ``bletchley-bmc``, ``fuji-bmc``, ``fby35-bmc``, ``fp5280g2-bmc``, ``g220a-bmc``, ``palmetto-bmc``, ``qcom-dc-scm-v1-bmc``, ``qcom-firework-bmc``, ``quanta-q71l-bmc``, ``rainier-bmc``, ``romulus-bmc``, ``sonorapass-bmc``, ``supermicrox11-bmc``, ``supermicrox11spi-bmc``, ``tiogapass-bmc``, ``witherspoon-bmc``, ``yosemitev2-bmc``)
	-Aspeed family boards (``ast2500-evb``, ``ast2600-evb``, ``ast2700-evb``, ``bletchley-bmc``, ``fuji-bmc``, ``fby35-bmc``, ``fp5280g2-bmc``, ``g220a-bmc``, ``palmetto-bmc``, ``qcom-dc-scm-v1-bmc``, ``qcom-firework-bmc``, ``quanta-q71l-bmc``, ``rainier-bmc``, ``romulus-bmc``, ``sonorapass-bmc``, ``supermicrox11-bmc``, ``supermicrox11spi-bmc``, ``tiogapass-bmc``, ``witherspoon-bmc``, ``yosemitev2-bmc``)
	+Aspeed family boards (``ast2500-evb``, ``ast2600-evb``, ``ast2700-evb``, ``ast2700fc``, ``bletchley-bmc``, ``fuji-bmc``, ``fby35-bmc``, ``fp5280g2-bmc``, ``g220a-bmc``, ``palmetto-bmc``, ``qcom-dc-scm-v1-bmc``, ``qcom-firework-bmc``, ``quanta-q71l-bmc``, ``rainier-bmc``, ``romulus-bmc``, ``sonorapass-bmc``, ``supermicrox11-bmc``, ``supermicrox11spi-bmc``, ``tiogapass-bmc``, ``witherspoon-bmc``, ``yosemitev2-bmc``)

🤖 Prompt for AI Agents

In docs/system/arm/aspeed.rst at lines 1 and 258, there is an inconsistency
where the ast2700fc board is missing from the introductory list but still
documented later. To fix this, either remove all mentions of ast2700fc from the
entire file if it is no longer supported, or add ast2700fc back to the
introductory list at line 1 if it is still supported, ensuring consistency
throughout the document.

[coderabbitai]

🛠️ Refactor suggestion

Address-mask test – supply 64-bit DMA address above 4 GiB

aspeed_test_addresses is invoked with 0x12070000 (below 4 GiB).
To really validate 64-bit masking logic, consider also probing an address such as 0x12070000 + 0x1_0000_0000ULL, or add a second case exercising the high dword.
This catches regressions that only appear when bit > 31 is set.

🤖 Prompt for AI Agents

In tests/qtest/ast2700-hace-test.c around lines 74 to 78, the test uses a 32-bit
DMA address below 4 GiB, which does not fully validate 64-bit masking logic.
Modify the test to also call aspeed_test_addresses with an address above 4 GiB
by adding 0x1_0000_0000ULL to the current address or add a second test case
using such a high 64-bit address to ensure coverage of bits above 31.

[coderabbitai]

⚠️ Potential issue

Dynamic regs array – remember allocation & cleanup

uint32_t *regs; replaces the fixed array. Ensure that:

1. instance_init allocates nr_regs * sizeof(uint32_t) with g_new0() or similar.
2. instance_finalize/unrealize frees it with g_free().

Missing cleanup will leak on every VM reset.

🤖 Prompt for AI Agents

In include/hw/misc/aspeed_hace.h around lines 33 to 35, the pointer uint32_t
*regs replaces a fixed array but lacks memory management. Modify the
instance_init function to allocate memory for regs using g_new0() with size
nr_regs * sizeof(uint32_t), and update instance_finalize or unrealize to free
this memory using g_free() to prevent memory leaks on VM resets.

[coderabbitai]

🛠️ Refactor suggestion

Class fields – document invariants

The class now mixes run-time (masks, has_dma64) with “type” constants. Add doc-comments or move per-SoC values into the type registration helper so subclasses can never forget to initialise the new fields.

Example (in aspeed_hace_ast2400_class_init):

ahc->has_dma64 = false;
ahc->src_hi_mask = 0;
...

Without this, older SoCs may inherit uninitialised garbage and silently accept 64-bit transfers.

🤖 Prompt for AI Agents

In include/hw/misc/aspeed_hace.h around lines 46 to 57, the struct fields mix
runtime masks and type constants without clear documentation or initialization,
risking uninitialized values for older SoCs. Add doc-comments to clarify which
fields are runtime versus type constants, and move per-SoC constant
initializations into the type registration helper functions (e.g.,
aspeed_hace_ast2400_class_init) to ensure all fields like has_dma64 and
src_hi_mask are explicitly initialized for each SoC subclass.

[coderabbitai]

⚠️ Potential issue

Same length-mismatch issue in SG path

hash_prepare_sg_iov() also rewrites iov[i].iov_len after mapping (to
strip the padding). The buffer is later unmapped with the shortened length
in hash_write_digest_and_unmap_iov(), risking partial un-map of a bounce
buffer.

Reuse the fix proposed for the direct path: keep the mapping length for
un-map while presenting the truncated length to the hash engine.

🤖 Prompt for AI Agents

In hw/misc/aspeed_hace.c around lines 296 to 306, the code modifies
iov[iov_idx].iov_len to a truncated length after detecting padding, which causes
the unmap operation to use this shortened length and risks partially unmapping
the bounce buffer. To fix this, preserve the original mapping length for the
unmap operation by storing it separately before truncating iov[iov_idx].iov_len
for the hash engine, ensuring the full buffer is unmapped correctly while still
presenting the truncated length during hashing.