Hi Cédric
> Subject: Re: [PATCH v1 03/22] hw/misc/aspeed_hace: Improve readability and
> consistency in variable naming
>
> 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_...@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 for the review and suggestion.
I refactor "do_hash_operation". Please see patch 6 comments.
Thanks-Jamin
> 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;
> > }
> >
