On 3/21/25 10:25, Jamin Lin wrote:
> Currently, users define multiple local variables within different if-statements.
> To improve readability and maintain consistency in variable naming, rename the
> variables accordingly.
> Introduced "sg_addr" to clearly indicate the scatter-gather mode buffer address.
>
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
The change look OK. do_hash_operation() is a big routine, difficult
to read. It does stuff like :
if (sg_mode) {
...
} else {
...
}
if (acc_mode) {
...
} else {
...
}
...
I think we should also split it in multiple routines to reduce the
complexity, even if some part are redundant.
Thanks,
C.
> ---> hw/misc/aspeed_hace.c | 33 ++++++++++++++++-----------------
> 1 file changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c
> index d8b5f048bb..4bcf6ed074 100644
> --- a/hw/misc/aspeed_hace.c
> +++ b/hw/misc/aspeed_hace.c
> @@ -145,15 +145,19 @@ static bool has_padding(AspeedHACEState *s, struct iovec *iov,
> static void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode,
> bool acc_mode)
> {
> + bool sg_acc_mode_final_request = false;
> + g_autofree uint8_t *digest_buf = NULL;
> struct iovec iov[ASPEED_HACE_MAX_SG];
> + Error *local_err = NULL;
> uint32_t total_msg_len;
> - uint32_t pad_offset;
> - g_autofree uint8_t *digest_buf = NULL;
> size_t digest_len = 0;
> - bool sg_acc_mode_final_request = false;
> - int i;
> + uint32_t sg_addr = 0;
> + uint32_t pad_offset;
> + uint32_t len = 0;
> + uint32_t src = 0;
> void *haddr;
> - Error *local_err = NULL;
> + hwaddr plen;
> + int i;
>
> if (acc_mode && s->hash_ctx == NULL) {
> s->hash_ctx = qcrypto_hash_new(algo, &local_err);
> @@ -166,12 +170,7 @@ static void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode,
> }
>
> if (sg_mode) {
> - uint32_t len = 0;
> -
> for (i = 0; !(len & SG_LIST_LEN_LAST); i++) {
> - uint32_t addr, src;
> - hwaddr plen;
> -
> if (i == ASPEED_HACE_MAX_SG) {
> qemu_log_mask(LOG_GUEST_ERROR,
> "aspeed_hace: guest failed to set end of sg list marker\n");
> @@ -183,12 +182,12 @@ static void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode,
> len = address_space_ldl_le(&s->dram_as, src,
> MEMTXATTRS_UNSPECIFIED, NULL);
>
> - addr = address_space_ldl_le(&s->dram_as, src + SG_LIST_LEN_SIZE,
> - MEMTXATTRS_UNSPECIFIED, NULL);
> - addr &= SG_LIST_ADDR_MASK;
> + sg_addr = address_space_ldl_le(&s->dram_as, src + SG_LIST_LEN_SIZE,
> + MEMTXATTRS_UNSPECIFIED, NULL);
> + sg_addr &= SG_LIST_ADDR_MASK;
>
> plen = len & SG_LIST_LEN_MASK;
> - haddr = address_space_map(&s->dram_as, addr, &plen, false,
> + haddr = address_space_map(&s->dram_as, sg_addr, &plen, false,
> MEMTXATTRS_UNSPECIFIED);
> if (haddr == NULL) {
> qemu_log_mask(LOG_GUEST_ERROR,
> @@ -212,16 +211,16 @@ static void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode,
> }
> }
> } else {
> - hwaddr len = s->regs[R_HASH_SRC_LEN];
> + plen = s->regs[R_HASH_SRC_LEN];
>
> haddr = address_space_map(&s->dram_as, s->regs[R_HASH_SRC],
> - &len, false, MEMTXATTRS_UNSPECIFIED);
> + &plen, false, MEMTXATTRS_UNSPECIFIED);
> if (haddr == NULL) {
> qemu_log_mask(LOG_GUEST_ERROR, "%s: qcrypto failed\n", __func__);
> return;
> }
> iov[0].iov_base = haddr;
> - iov[0].iov_len = len;
> + iov[0].iov_len = plen;
> i = 1;
> }
>