Re: [PATCH v1 03/22] hw/misc/aspeed_hace: Improve readability and consistency in variable naming

["Cédric Le Goater" <clg@kaod.org>]

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;
>       }
>