qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Cédric Le Goater
Subject: Re: [PATCH v1 01/22] hw/misc/aspeed_hace: Remove unused code for better readability
Date: Tue, 6 May 2025 11:01:20 +0200
User-agent: Mozilla Thunderbird 
On 5/5/25 05:28, Jamin Lin wrote:

Hi Cédric


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

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_lin@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.



How about the following commit log:

```
hw/misc/aspeed_hace: Remove unused code for better readability

In the previous design of the hash framework, accumulative hashing was not
supported. To work around this limitation, commit 5cd7d85 introduced an
iov_cache array to store all the hash data from firmware.
Once the ASPEED HACE model collected all the data, it passed the iov_cache to
the hash API to calculate the final digest.

However, with commit e3c0752, the hash framework now supports accumulative
hashing. This allows us to refactor the ASPEED HACE model, removing redundant
logic and simplifying the implementation for better readability and
maintainability.

As a result, the iov_count variable is no longer needed—it was previously used
to track how many cached entries were used for hashing.
To maintain VMSTATE compatibility after removing this field, the VMSTATE_VERSION
is bumped to 2

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

- Deleted "iov_cache" and "iov_count" from "AspeedHACEState".
- Removed "reconstruct_iov" function and related logic.
- Simplified "do_hash_operation" by eliminating redundant checks.
```



OK Let's see in v2.


Thanks,

C.



Thans-Jamin


Thanks,

C.

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


           VMSTATE_END_OF_LIST(),
       }
   };
reply via email to
[Prev in Thread] Current Thread [Next in Thread]