Re: [PATCH v1 01/22] hw/misc/aspeed_hace: Remove unused code for better readability

[Cédric Le Goater]

On 3/21/25 10:25, Jamin Lin wrote:

This cleanup follows significant changes in commit 4c1d0af4a28d, making the
model more readable.

- Deleted "iov_cache" and "iov_count" from "AspeedHACEState".

It would be good to say why we can remove these. I think this is because
s->iov_count is always zero, right ?

- Removed "reconstruct_iov" function and related logic.
- Simplified "do_hash_operation" by eliminating redundant checks.

Signed-off-by: Jamin Lin <jamin_...@aspeedtech.com>
---
  include/hw/misc/aspeed_hace.h |  2 --
  hw/misc/aspeed_hace.c         | 35 -----------------------------------
  2 files changed, 37 deletions(-)

diff --git a/include/hw/misc/aspeed_hace.h b/include/hw/misc/aspeed_hace.h
index 5d4aa19cfe..b69a038d35 100644
--- a/include/hw/misc/aspeed_hace.h
+++ b/include/hw/misc/aspeed_hace.h
@@ -31,10 +31,8 @@ struct AspeedHACEState {
      MemoryRegion iomem;
      qemu_irq irq;

- struct iovec iov_cache[ASPEED_HACE_MAX_SG];

      uint32_t regs[ASPEED_HACE_NR_REGS];
      uint32_t total_req_len;
-    uint32_t iov_count;

MemoryRegion *dram_mr;

      AddressSpace dram_as;
diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c
index 32a5dbded3..8e7e8113a5 100644
--- a/hw/misc/aspeed_hace.c
+++ b/hw/misc/aspeed_hace.c
@@ -137,25 +137,6 @@ static bool has_padding(AspeedHACEState *s, struct iovec 
*iov,
      return false;
  }

-static int reconstruct_iov(AspeedHACEState *s, struct iovec *iov, int id,

-                           uint32_t *pad_offset)
-{
-    int i, iov_count;
-    if (*pad_offset != 0) {
-        s->iov_cache[s->iov_count].iov_base = iov[id].iov_base;
-        s->iov_cache[s->iov_count].iov_len = *pad_offset;
-        ++s->iov_count;
-    }
-    for (i = 0; i < s->iov_count; i++) {
-        iov[i].iov_base = s->iov_cache[i].iov_base;
-        iov[i].iov_len = s->iov_cache[i].iov_len;
-    }
-    iov_count = s->iov_count;
-    s->iov_count = 0;
-    s->total_req_len = 0;
-    return iov_count;
-}
-
  static void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode,
                                bool acc_mode)
  {
@@ -237,19 +218,6 @@ static void do_hash_operation(AspeedHACEState *s, int 
algo, bool sg_mode,
          iov[0].iov_base = haddr;
          iov[0].iov_len = len;
          i = 1;
-
-        if (s->iov_count) {
-            /*
-             * In aspeed sdk kernel driver, sg_mode is disabled in 
hash_final().
-             * Thus if we received a request with sg_mode disabled, it is
-             * required to check whether cache is empty. If no, we should
-             * combine cached iov and the current iov.
-             */
-            s->total_req_len += len;
-            if (has_padding(s, iov, len, &total_msg_len, &pad_offset)) {
-                i = reconstruct_iov(s, iov, 0, &pad_offset);
-            }
-        }
      }

if (acc_mode) {

@@ -273,7 +241,6 @@ static void do_hash_operation(AspeedHACEState *s, int algo, 
bool sg_mode,
              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,
@@ -432,7 +399,6 @@ static void aspeed_hace_reset(DeviceState *dev)
      }

memset(s->regs, 0, sizeof(s->regs));

-    s->iov_count = 0;
      s->total_req_len = 0;
  }

@@ -469,7 +435,6 @@ static const VMStateDescription vmstate_aspeed_hace = {

      .fields = (const VMStateField[]) {
          VMSTATE_UINT32_ARRAY(regs, AspeedHACEState, ASPEED_HACE_NR_REGS),
          VMSTATE_UINT32(total_req_len, AspeedHACEState),
-        VMSTATE_UINT32(iov_count, AspeedHACEState),

This is a vmstate change which is breaking migration compatibility.
We could preserve compatibility [1] but I think this is overkill.
However, we should say so. Please add a comment in the commit log.

Thanks,

C.

[1] https://qemu.readthedocs.io/en/v9.2.0/devel/migration/main.html#vmstate

          VMSTATE_END_OF_LIST(),
      }
  };