Hi Cedric, > Subject: Re: [PATCH v6] hw/misc/aspeed_hace: Fix SG Accumulative hashing > > + Aspeed reviewers. Sorry about that. > > C. > > > On 10/11/24 07:38, Cédric Le Goater wrote: > > From: Alejandro Zeise <alejandro.ze...@seagate.com> > > > > Make the Aspeed HACE module use the new qcrypto accumulative hashing > > functions when in scatter-gather accumulative mode. A hash context > > will maintain a "running-hash" as each scatter-gather chunk is received. > > > > Previously each scatter-gather "chunk" was cached so the hash could be > > computed once the final chunk was received. > > However, the cache was a shallow copy, so once the guest overwrote the > > memory provided to HACE the final hash would not be correct. > > > > Possibly related to: > > https://gitlab.com/qemu-project/qemu/-/issues/1121 > > Buglink: https://github.com/openbmc/qemu/issues/36 > > > > Signed-off-by: Alejandro Zeise <alejandro.ze...@seagate.com> [ clg: - > > Checkpatch fixes > > - Reworked qcrypto_hash*() error reports in > > do_hash_operation() ] > > Signed-off-by: Cédric Le Goater <c...@redhat.com> > > --- > > > > Changes in v6: > > - Reworked qcrypto_hash*() error reports in do_hash_operation() > > > > include/hw/misc/aspeed_hace.h | 4 ++ > > hw/misc/aspeed_hace.c | 104 > +++++++++++++++++++--------------- > > 2 files changed, 63 insertions(+), 45 deletions(-) > > > > diff --git a/include/hw/misc/aspeed_hace.h > > b/include/hw/misc/aspeed_hace.h index ecb1b67de816..4af99191955a > > 100644 > > --- a/include/hw/misc/aspeed_hace.h > > +++ b/include/hw/misc/aspeed_hace.h > > @@ -1,6 +1,7 @@ > > /* > > * ASPEED Hash and Crypto Engine > > * > > + * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates > > * Copyright (C) 2021 IBM Corp. > > * > > * SPDX-License-Identifier: GPL-2.0-or-later @@ -10,6 +11,7 @@ > > #define ASPEED_HACE_H > > > > #include "hw/sysbus.h" > > +#include "crypto/hash.h" > > > > #define TYPE_ASPEED_HACE "aspeed.hace" > > #define TYPE_ASPEED_AST2400_HACE TYPE_ASPEED_HACE "-ast2400" > > @@ -35,6 +37,8 @@ struct AspeedHACEState { > > > > MemoryRegion *dram_mr; > > AddressSpace dram_as; > > + > > + QCryptoHash *hash_ctx; > > }; > > > > > > diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c index > > b6f43f65b29a..bc1d66ad8064 100644 > > --- a/hw/misc/aspeed_hace.c > > +++ b/hw/misc/aspeed_hace.c > > @@ -1,6 +1,7 @@ > > /* > > * ASPEED Hash and Crypto Engine > > * > > + * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates > > * Copyright (C) 2021 IBM Corp. > > * > > * Joel Stanley <j...@jms.id.au> > > @@ -151,49 +152,28 @@ static int reconstruct_iov(AspeedHACEState *s, > struct iovec *iov, int id, > > return iov_count; > > } > > > > -/** > > - * Generate iov for accumulative mode. > > - * > > - * @param s aspeed hace state object > > - * @param iov iov of the current request > > - * @param id index of the current iov > > - * @param req_len length of the current request > > - * > > - * @return count of iov > > - */ > > -static int gen_acc_mode_iov(AspeedHACEState *s, struct iovec *iov, int id, > > - hwaddr *req_len) > > -{ > > - uint32_t pad_offset; > > - uint32_t total_msg_len; > > - s->total_req_len += *req_len; > > - > > - if (has_padding(s, &iov[id], *req_len, &total_msg_len, &pad_offset)) { > > - if (s->iov_count) { > > - return reconstruct_iov(s, iov, id, &pad_offset); > > - } > > - > > - *req_len -= s->total_req_len - total_msg_len; > > - s->total_req_len = 0; > > - iov[id].iov_len = *req_len; > > - } else { > > - s->iov_cache[s->iov_count].iov_base = iov->iov_base; > > - s->iov_cache[s->iov_count].iov_len = *req_len; > > - ++s->iov_count; > > - } > > - > > - return id + 1; > > -} > > - > > static void do_hash_operation(AspeedHACEState *s, int algo, bool > sg_mode, > > bool acc_mode) > > { > > struct iovec iov[ASPEED_HACE_MAX_SG]; > > + uint32_t total_msg_len; > > + uint32_t pad_offset; > > g_autofree uint8_t *digest_buf = NULL; > > size_t digest_len = 0; > > - int niov = 0; > > + bool sg_acc_mode_final_request = false; > > int i; > > void *haddr; > > + Error *local_err = NULL; > > + > > + if (acc_mode && 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)); > > + error_free(local_err); > > + return; > > + } > > + } > > > > if (sg_mode) { > > uint32_t len = 0; > > @@ -226,8 +206,16 @@ static void do_hash_operation(AspeedHACEState *s, > int algo, bool sg_mode, > > } > > iov[i].iov_base = haddr; > > if (acc_mode) { > > - niov = gen_acc_mode_iov(s, iov, i, &plen); > > - > > + s->total_req_len += plen; > > + > > + if (has_padding(s, &iov[i], plen, &total_msg_len, > > + &pad_offset)) { > > + /* Padding being present indicates the final > request */ > > + sg_acc_mode_final_request = true; > > + iov[i].iov_len = pad_offset; > > + } else { > > + iov[i].iov_len = plen; > > + } > > } else { > > iov[i].iov_len = plen; > > } > > @@ -252,21 +240,42 @@ static void do_hash_operation(AspeedHACEState > *s, int algo, bool sg_mode, > > * required to check whether cache is empty. If no, we > should > > * combine cached iov and the current iov. > > */ > > - uint32_t total_msg_len; > > - uint32_t pad_offset; > > s->total_req_len += len; > > if (has_padding(s, iov, len, &total_msg_len, &pad_offset)) { > > - niov = reconstruct_iov(s, iov, 0, &pad_offset); > > + i = reconstruct_iov(s, iov, 0, &pad_offset); > > } > > } > > } > > > > - if (niov) { > > - i = niov; > > - } > > + if (acc_mode) { > > + if (qcrypto_hash_updatev(s->hash_ctx, iov, i, &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 (sg_acc_mode_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; > > + } > > > > - if (qcrypto_hash_bytesv(algo, iov, i, &digest_buf, &digest_len, NULL) < > 0) { > > - qemu_log_mask(LOG_GUEST_ERROR, "%s: qcrypto failed\n", > __func__); > > + qcrypto_hash_free(s->hash_ctx); > > + > > + s->hash_ctx = NULL; > > + s->iov_count = 0; > > + s->total_req_len = 0; > > + } > > + } else if (qcrypto_hash_bytesv(algo, iov, i, &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; > > } > > > > @@ -397,6 +406,11 @@ static void aspeed_hace_reset(DeviceState *dev) > > { > > struct AspeedHACEState *s = ASPEED_HACE(dev); > > > > + if (s->hash_ctx != NULL) { > > + qcrypto_hash_free(s->hash_ctx); > > + s->hash_ctx = NULL; > > + } > > + > > memset(s->regs, 0, sizeof(s->regs)); > > s->iov_count = 0; > > s->total_req_len = 0;
Test Steps: 1. Create a random 32MB file dd if=/dev/random of=32MB count=32 bs=1M 2. Get the hash value in my host machine $ sha256sum 32MB 1ddcccdba742d762e2b8da0bceaf4778727c5eba54a24d7ae0c573c65414f736 32MB $ sha384sum 32MB 825d9b24bb797695545b3cbd2f373b9738627c7a1878e620415570a57c7faed77916d47084c954254f101fc0f10c0591 32MB $ sha512sum 32MB b5ae725b2dc1e521f48eae37dd82c3d5fc94f7acb5fff3dabf1caa4bb4b5bcfb498e7cc1fbaa97dda2664bff99f9f8e778f823e95afaf76fbf0985181522e478 32MB 3. Test HACE model with u-boot hash command a. load test file to address 83000000 via tftp ast# tftp 83000000 jamin_lin/32MB b. get sha256 ast# hash sha256 83000000 2000000 sha256 for 83000000 ... 84ffffff ==> 1ddcccdba742d762e2b8da0bceaf4778727c5eba54a24d7ae0c573c65414f736 c. get sha384 ast# hash sha384 83000000 2000000 sha384 for 83000000 ... 84ffffff ==> 825d9b24bb797695545b3cbd2f373b9738627c7a1878e620415570a57c7faed77916d47084c954254f101fc0f10c0591 d. get sha512 ast# hash sha512 83000000 2000000 sha512 for 83000000 ... 84ffffff ==> b5ae725b2dc1e521f48eae37dd82c3d5fc94f7acb5fff3dabf1caa4bb4b5bcfb498e7cc1fbaa97dda2664bff99f9f8e778f823e95afaf76fbf0985181522e478 Reviewed-by: Jamin Lin <jamin_...@aspeedtech.com> Thanks-Jamin
