Hi Cédric > Subject: Re: [PATCH v2 05/25] hw/misc/aspeed_hace: Split hash execution into > helper functions for clarity > > Hello Jamin > > On 5/13/25 08:28, Jamin Lin wrote: > > Refactor "do_hash_operation()" by extracting hash execution and result > > handling into dedicated helper functions: > > > > - "hash_write_digest_and_unmap_iov()": Writes the digest result to memory > and > > unmaps IOVs after processing. > > - "hash_execute_non_acc_mode()": Handles one-shot (non-accumulated) > hash > > operations. > > - "hash_execute_acc_mode()": Handles accumulated hashing, including > update and > > finalize logic. > > > > No functional changes introduced. > > This patch is breaking 'check-qtest-arm' > > > stderr: > ** > ERROR:../tests/qtest/aspeed_hace-test.c:254:test_sha512: assertion failed > (digest == test_result_sha512) >
I cannot reproduce it and my test steps as following. Download this patch series wget https://patchew.org/QEMU/20250513062901.2256865-1-jamin._5f...@aspeedtech.com/mbox Git clone QEMU, Build and Test version: 69ee0189d7977cfbb1b2c7a27393d8b9fb661b20 1. git clone https://github.com/qemu/qemu.git 2. cd qemu 3. git am ../mbox 4. git log --oneline ef01d966fb hw/misc/aspeed_hace: Split hash execution into helper functions for clarity 5. git rebase --interactive ef01d966fb^ 6. git log | head commit ef01d966fb27e9f9f7da16900f2d7b25de06fb32 Author: Jamin Lin via <qemu-devel@nongnu.org> Date: Tue May 13 14:28:35 2025 +0800 hw/misc/aspeed_hace: Split hash execution into helper functions for clarity 7. cd .. 8. mkdir build 9. cd build ../qemu/configure --target-list=arm-linux-user,arm-softmmu,aarch64-softmmu,aarch64-linux-user --enable-slirp --disable-gcrypt --disable-nettle Crypto TLS priority : NORMAL GNUTLS support : NO libgcrypt : NO nettle : NO SM4 ALG support : NO SM3 ALG support : NO AF_ALG support : NO rng-none : NO Linux keyring : YES Linux keyutils : NO 10. Build make -j32 11. Run check-qtest-arm make check-qtest-arm jamin_lin@aspeed-fw02:~/commit/build$ make check-qtest-arm [1/8] Generating qemu-version.h with a custom command (wrapped by meson to capture output) /home/jamin_lin/commit/build/pyvenv/bin/meson test --no-rebuild -t 1 --num-processes 1 --print-errorlogs --suite qtest-arm 1/41 qemu:qtest+qtest-arm / qtest-arm/qom-test OK 232.04s 79 subtests passed 2/41 qemu:qtest+qtest-arm / qtest-arm/device-introspect-test OK 5.39s 6 subtests passed 3/41 qemu:qtest+qtest-arm / qtest-arm/cdrom-test SKIP 0.01s 4/41 qemu:qtest+qtest-arm / qtest-arm/stm32l4x5_usart-test OK 1.00s 7 subtests passed 5/41 qemu:qtest+qtest-arm / qtest-arm/aspeed_smc-test OK 89.73s 34 subtests passed 6/41 qemu:qtest+qtest-arm / qtest-arm/boot-serial-test OK 0.72s 3 subtests passed 7/41 qemu:qtest+qtest-arm / qtest-arm/npcm7xx_pwm-test OK 5.44s 3 subtests passed 8/41 qemu:qtest+qtest-arm / qtest-arm/test-hmp OK 47.27s 80 subtests passed 9/41 qemu:qtest+qtest-arm / qtest-arm/npcm7xx_watchdog_timer-test OK 5.16s 15 subtests passed 10/41 qemu:qtest+qtest-arm / qtest-arm/qmp-cmd-test OK 8.01s 62 subtests passed 11/41 qemu:qtest+qtest-arm / qtest-arm/qos-test OK 21.41s 97 subtests passed 12/41 qemu:qtest+qtest-arm / qtest-arm/sse-timer-test OK 0.24s 3 subtests passed 13/41 qemu:qtest+qtest-arm / qtest-arm/cmsdk-apb-dualtimer-test OK 0.14s 2 subtests passed 14/41 qemu:qtest+qtest-arm / qtest-arm/cmsdk-apb-timer-test OK 0.14s 1 subtests passed 15/41 qemu:qtest+qtest-arm / qtest-arm/cmsdk-apb-watchdog-test OK 1.02s 7 subtests passed 16/41 qemu:qtest+qtest-arm / qtest-arm/pflash-cfi02-test OK 1.62s 4 subtests passed 17/41 qemu:qtest+qtest-arm / qtest-arm/aspeed_hace-test OK 5.18s 16 subtests passed 18/41 qemu:qtest+qtest-arm / qtest-arm/aspeed_gpio-test OK 0.35s 2 subtests passed 19/41 qemu:qtest+qtest-arm / qtest-arm/npcm7xx_adc-test OK 2.13s 6 subtests passed 20/41 qemu:qtest+qtest-arm / qtest-arm/npcm7xx_gpio-test OK 0.19s 18 subtests passed 21/41 qemu:qtest+qtest-arm / qtest-arm/npcm7xx_rng-test OK 0.18s 2 subtests passed 22/41 qemu:qtest+qtest-arm / qtest-arm/npcm7xx_sdhci-test OK 0.74s 3 subtests passed 23/41 qemu:qtest+qtest-arm / qtest-arm/npcm7xx_smbus-test OK 6.85s 40 subtests passed 24/41 qemu:qtest+qtest-arm / qtest-arm/npcm7xx_timer-test OK 0.31s 180 subtests passed 25/41 qemu:qtest+qtest-arm / qtest-arm/npcm_gmac-test OK 0.35s 2 subtests passed 26/41 qemu:qtest+qtest-arm / qtest-arm/npcm7xx_emc-test OK 1.21s 6 subtests passed 27/41 qemu:qtest+qtest-arm / qtest-arm/hexloader-test OK 0.22s 1 subtests passed 28/41 qemu:qtest+qtest-arm / qtest-arm/tpm-tis-i2c-test OK 0.58s 6 subtests passed 29/41 qemu:qtest+qtest-arm / qtest-arm/test-arm-mptimer OK 0.16s 61 subtests passed 30/41 qemu:qtest+qtest-arm / qtest-arm/microbit-test OK 3.87s 6 subtests passed 31/41 qemu:qtest+qtest-arm / qtest-arm/stm32l4x5_exti-test OK 0.15s 9 subtests passed 32/41 qemu:qtest+qtest-arm / qtest-arm/stm32l4x5_syscfg-test OK 0.15s 10 subtests passed 33/41 qemu:qtest+qtest-arm / qtest-arm/stm32l4x5_rcc-test OK 0.14s 5 subtests passed 34/41 qemu:qtest+qtest-arm / qtest-arm/stm32l4x5_gpio-test OK 0.17s 14 subtests passed 35/41 qemu:qtest+qtest-arm / qtest-arm/aspeed_fsi-test OK 0.30s 4 subtests passed 36/41 qemu:qtest+qtest-arm / qtest-arm/dm163-test OK 0.45s 3 subtests passed 37/41 qemu:qtest+qtest-arm / qtest-arm/arm-cpu-features OK 0.65s 1 subtests passed 38/41 qemu:qtest+qtest-arm / qtest-arm/machine-none-test OK 0.13s 1 subtests passed 39/41 qemu:qtest+qtest-arm / qtest-arm/qmp-test OK 0.51s 4 subtests passed 40/41 qemu:qtest+qtest-arm / qtest-arm/readconfig-test OK 0.13s 1 subtests passed 41/41 qemu:qtest+qtest-arm / qtest-arm/netdev-socket OK 3.03s 9 subtests passed Ok: 40 Expected Fail: 0 Fail: 0 Unexpected Pass: 0 Skipped: 1 Timeout: 0 12. Run aspeed_hace-test $ QTEST_QEMU_BINARY=${PWD}/qemu-system-arm tests/qtest/aspeed_hace-test TAP version 13 # random seed: R02Sc6431b2cd4efa9bf268be49b109f1240 1..16 # Start of arm tests # Start of ast2600 tests # Start of hace tests # starting QEMU: exec /home/jamin_lin/commit/build/qemu-system-arm -qtest unix:/tmp/qtest-3741778.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3741778.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine ast2600-evb -accel qtest ok 1 /arm/ast2600/hace/addresses # starting QEMU: exec /home/jamin_lin/commit/build/qemu-system-arm -qtest unix:/tmp/qtest-3741778.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3741778.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine ast2600-evb -accel qtest ok 2 /arm/ast2600/hace/sha512 # starting QEMU: exec /home/jamin_lin/commit/build/qemu-system-arm -qtest unix:/tmp/qtest-3741778.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3741778.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine ast2600-evb -accel qtest ok 3 /arm/ast2600/hace/sha256 # starting QEMU: exec /home/jamin_lin/commit/build/qemu-system-arm -qtest unix:/tmp/qtest-3741778.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3741778.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine ast2600-evb -accel qtest ok 4 /arm/ast2600/hace/md5 # starting QEMU: exec /home/jamin_lin/commit/build/qemu-system-arm -qtest unix:/tmp/qtest-3741778.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3741778.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine ast2600-evb -accel qtest ok 5 /arm/ast2600/hace/sha512_sg # slow test /arm/ast2600/hace/sha512_sg executed in 0.58 secs # starting QEMU: exec /home/jamin_lin/commit/build/qemu-system-arm -qtest unix:/tmp/qtest-3741778.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3741778.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine ast2600-evb -accel qtest ok 6 /arm/ast2600/hace/sha256_sg # slow test /arm/ast2600/hace/sha256_sg executed in 0.58 secs # starting QEMU: exec /home/jamin_lin/commit/build/qemu-system-arm -qtest unix:/tmp/qtest-3741778.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3741778.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine ast2600-evb -accel qtest ok 7 /arm/ast2600/hace/sha512_accum # starting QEMU: exec /home/jamin_lin/commit/build/qemu-system-arm -qtest unix:/tmp/qtest-3741778.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3741778.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine ast2600-evb -accel qtest ok 8 /arm/ast2600/hace/sha256_accum # End of hace tests # End of ast2600 tests # Start of ast2500 tests # Start of hace tests # starting QEMU: exec /home/jamin_lin/commit/build/qemu-system-arm -qtest unix:/tmp/qtest-3741778.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3741778.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine ast2500-evb -accel qtest ok 9 /arm/ast2500/hace/addresses # starting QEMU: exec /home/jamin_lin/commit/build/qemu-system-arm -qtest unix:/tmp/qtest-3741778.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3741778.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine ast2500-evb -accel qtest ok 10 /arm/ast2500/hace/sha512 # starting QEMU: exec /home/jamin_lin/commit/build/qemu-system-arm -qtest unix:/tmp/qtest-3741778.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3741778.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine ast2500-evb -accel qtest ok 11 /arm/ast2500/hace/sha256 # starting QEMU: exec /home/jamin_lin/commit/build/qemu-system-arm -qtest unix:/tmp/qtest-3741778.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3741778.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine ast2500-evb -accel qtest ok 12 /arm/ast2500/hace/md5 # End of hace tests # End of ast2500 tests # Start of ast2400 tests # Start of hace tests # starting QEMU: exec /home/jamin_lin/commit/build/qemu-system-arm -qtest unix:/tmp/qtest-3741778.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3741778.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine palmetto-bmc -accel qtest ok 13 /arm/ast2400/hace/addresses # starting QEMU: exec /home/jamin_lin/commit/build/qemu-system-arm -qtest unix:/tmp/qtest-3741778.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3741778.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine palmetto-bmc -accel qtest ok 14 /arm/ast2400/hace/sha512 # starting QEMU: exec /home/jamin_lin/commit/build/qemu-system-arm -qtest unix:/tmp/qtest-3741778.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3741778.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine palmetto-bmc -accel qtest ok 15 /arm/ast2400/hace/sha256 # starting QEMU: exec /home/jamin_lin/commit/build/qemu-system-arm -qtest unix:/tmp/qtest-3741778.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3741778.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine palmetto-bmc -accel qtest ok 16 /arm/ast2400/hace/md5 # End of hace tests # End of ast2400 tests # End of arm tests Thanks-Jamin > > Thanks, > > C. > > > > > > > Signed-off-by: Jamin Lin <jamin_...@aspeedtech.com> > > --- > > hw/misc/aspeed_hace.c | 133 > ++++++++++++++++++++++++++---------------- > > 1 file changed, 83 insertions(+), 50 deletions(-) > > > > diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c index > > 22eea62693..eb513ba00f 100644 > > --- a/hw/misc/aspeed_hace.c > > +++ b/hw/misc/aspeed_hace.c > > @@ -228,26 +228,96 @@ static int hash_prepare_sg_iov(AspeedHACEState > *s, struct iovec *iov, > > return iov_idx; > > } > > > > -static void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode, > > - bool acc_mode) > > +static void hash_write_digest_and_unmap_iov(AspeedHACEState *s, > > + struct iovec *iov, > > + int iov_idx, > > + uint8_t *digest_buf, > > + size_t digest_len) { > > + if (address_space_write(&s->dram_as, s->regs[R_HASH_DEST], > > + MEMTXATTRS_UNSPECIFIED, digest_buf, > digest_len)) { > > + qemu_log_mask(LOG_GUEST_ERROR, > > + "%s: Failed to write digest to 0x%x\n", > > + __func__, s->regs[R_HASH_DEST]); > > + } > > + > > + 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); > > + } > > +} > > + > > +static void hash_execute_non_acc_mode(AspeedHACEState *s, int algo, > > + struct iovec *iov, int iov_idx) > > { > > g_autofree uint8_t *digest_buf = NULL; > > - struct iovec iov[ASPEED_HACE_MAX_SG]; > > - bool acc_final_request = false; > > Error *local_err = NULL; > > - size_t digest_len = 0; > > - int iov_idx = -1; > > + size_t digest_len; > > + > > + if (qcrypto_hash_bytesv(algo, iov, iov_idx, &digest_buf, > > + &digest_len, &local_err) < 0) { > > + qemu_log_mask(LOG_GUEST_ERROR, > > + "%s: qcrypto hash bytesv failed : %s", > > + __func__, error_get_pretty(local_err)); > > + error_free(local_err); > > + return; > > + } > > + > > + hash_write_digest_and_unmap_iov(s, iov, iov_idx, digest_buf, > > +digest_len); } > > + > > +static void hash_execute_acc_mode(AspeedHACEState *s, int algo, > > + struct iovec *iov, int iov_idx, > > + bool final_request) { > > + g_autofree uint8_t *digest_buf = NULL; > > + Error *local_err = NULL; > > + size_t digest_len; > > > > - if (acc_mode && s->hash_ctx == NULL) { > > + if (s->hash_ctx == NULL) { > > s->hash_ctx = qcrypto_hash_new(algo, &local_err); > > if (s->hash_ctx == NULL) { > > - qemu_log_mask(LOG_GUEST_ERROR, "qcrypto hash failed : > %s", > > - error_get_pretty(local_err)); > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: qcrypto hash new > failed : %s", > > + __func__, error_get_pretty(local_err)); > > error_free(local_err); > > return; > > } > > } > > > > + if (qcrypto_hash_updatev(s->hash_ctx, iov, iov_idx, &local_err) < 0) { > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: qcrypto hash updatev > failed : %s", > > + __func__, error_get_pretty(local_err)); > > + error_free(local_err); > > + return; > > + } > > + > > + if (final_request) { > > + if (qcrypto_hash_finalize_bytes(s->hash_ctx, &digest_buf, > > + &digest_len, &local_err)) > { > > + qemu_log_mask(LOG_GUEST_ERROR, > > + "%s: qcrypto hash finalize bytes failed : %s", > > + __func__, error_get_pretty(local_err)); > > + error_free(local_err); > > + local_err = NULL; > > + } > > + > > + qcrypto_hash_free(s->hash_ctx); > > + > > + s->hash_ctx = NULL; > > + s->total_req_len = 0; > > + } > > + > > + hash_write_digest_and_unmap_iov(s, iov, iov_idx, digest_buf, > > +digest_len); } > > + > > +static void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode, > > + bool acc_mode) { > > + struct iovec iov[ASPEED_HACE_MAX_SG]; > > + bool acc_final_request = false; > > + int iov_idx = -1; > > + > > /* Prepares the iov for hashing operations based on the selected > mode */ > > if (sg_mode) { > > iov_idx = hash_prepare_sg_iov(s, iov, acc_mode, > > &acc_final_request); @@ -261,48 +331,11 @@ static void > do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode, > > return; > > } > > > > + /* Executes the hash operation */ > > if (acc_mode) { > > - if (qcrypto_hash_updatev(s->hash_ctx, iov, iov_idx, &local_err) < > 0) { > > - qemu_log_mask(LOG_GUEST_ERROR, "qcrypto hash update > failed : %s", > > - error_get_pretty(local_err)); > > - error_free(local_err); > > - return; > > - } > > - > > - if (acc_final_request) { > > - if (qcrypto_hash_finalize_bytes(s->hash_ctx, &digest_buf, > > - &digest_len, > &local_err)) { > > - qemu_log_mask(LOG_GUEST_ERROR, > > - "qcrypto hash finalize failed : %s", > > - error_get_pretty(local_err)); > > - error_free(local_err); > > - local_err = NULL; > > - } > > - > > - qcrypto_hash_free(s->hash_ctx); > > - > > - s->hash_ctx = NULL; > > - s->total_req_len = 0; > > - } > > - } else if (qcrypto_hash_bytesv(algo, iov, iov_idx, &digest_buf, > > - &digest_len, &local_err) < 0) { > > - qemu_log_mask(LOG_GUEST_ERROR, "qcrypto hash bytesv failed : > %s", > > - error_get_pretty(local_err)); > > - error_free(local_err); > > - return; > > - } > > - > > - if (address_space_write(&s->dram_as, s->regs[R_HASH_DEST], > > - MEMTXATTRS_UNSPECIFIED, > > - digest_buf, digest_len)) { > > - qemu_log_mask(LOG_GUEST_ERROR, > > - "aspeed_hace: address space write failed\n"); > > - } > > - > > - 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); > > + hash_execute_acc_mode(s, algo, iov, iov_idx, acc_final_request); > > + } else { > > + hash_execute_non_acc_mode(s, algo, iov, iov_idx); > > } > > } > >
