Hi Cédric > Subject: Re: [PATCH v1 04/22] hw/misc/aspeed_hace: Update hash source > address handling to 64-bit for AST2700 > > On 3/21/25 10:26, Jamin Lin wrote: > > The AST2700 CPU, based on the Cortex-A35, is a 64-bit processor, and > > its DRAM address space is also 64-bit. To support future AST2700 > > updates, the source hash buffer address data type is being updated to > > 64-bit. > > > > Signed-off-by: Jamin Lin <jamin_...@aspeedtech.com> > > --- > > hw/misc/aspeed_hace.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c index > > 4bcf6ed074..9771d6e490 100644 > > --- a/hw/misc/aspeed_hace.c > > +++ b/hw/misc/aspeed_hace.c > > @@ -154,7 +154,7 @@ static void do_hash_operation(AspeedHACEState *s, > int algo, bool sg_mode, > > uint32_t sg_addr = 0; > > uint32_t pad_offset; > > uint32_t len = 0; > > - uint32_t src = 0; > > + uint64_t src = 0; > > void *haddr; > > hwaddr plen; > > int i; > > @@ -177,7 +177,8 @@ static void do_hash_operation(AspeedHACEState *s, > int algo, bool sg_mode, > > break; > > } > > > > - src = s->regs[R_HASH_SRC] + (i * SG_LIST_ENTRY_SIZE); > > + src = deposit64(src, 0, 32, s->regs[R_HASH_SRC]); > > I would introduce an helper routine to return the hash 'src' address. > More changes are expected in the following patches. >
Thanks for the review and suggestion. I will add "hash_get_source_addr" helper function to get the source address. > Why is the initial hash 'src' address assigned in the loop ? This could done > before looping on the scatter-gather list elements. Yes, it can be moved before the "for loop" and only need to assign one time. > > Also should this address be aligned to some size ? The HACE hash engine supports two modes for the source buffer: 1. Direct Mode Byte-aligned source data Bit 31: Reserved Bit 30:0 Base address of the hash source data buffer To ensure byte alignment, the "src_mask" is set to 0x7FFFFFFF Reference: https://github.com/qemu/qemu/blob/master/hw/misc/aspeed_hace.c#L583 ahc->src_mask = 0x7FFFFFFF; https://github.com/qemu/qemu/blob/master/hw/misc/aspeed_hace.c#L362 case R_HASH_SRC: data &= ahc->src_mask; 2. Scatter-Gather (SG) Mode Bit 31: Reserved Bit 30:3 Base address of the SG list (must be 8-byte aligned) Bit 2:0 Reserved In SG mode: The R_HASH_SRC value is first masked using ahc->src_mask as in Direct Mode. Then, while parsing SG entries, each SG list entry's address is additionally masked using SG_LIST_ADDR_MASK. https://github.com/qemu/qemu/blob/master/hw/misc/aspeed_hace.c#L207 ``` src = s->regs[R_HASH_SRC] + (i * SG_LIST_ENTRY_SIZE); 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; ``` However, the value of SG_LIST_ADDR_MASK is incorrect. It should be updated to "0x7FFFFFF8" to correctly enforce the 8-byte alignment requirement. Please refer to the HACE and CRYPTO chapters in the datasheet for more details. The SG mode address format (HACE24) is identical between the AST2600 and AST2700. Thanks-Jamin > > > Thanks, > > C. > > > > > > + src += i * SG_LIST_ENTRY_SIZE; > > > > len = address_space_ldl_le(&s->dram_as, src, > > > MEMTXATTRS_UNSPECIFIED, > > NULL); @@ -212,8 +213,9 @@ static void > do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode, > > } > > } else { > > plen = s->regs[R_HASH_SRC_LEN]; > > + src = deposit64(src, 0, 32, s->regs[R_HASH_SRC]); > > > > - haddr = address_space_map(&s->dram_as, s->regs[R_HASH_SRC], > > + haddr = address_space_map(&s->dram_as, src, > > &plen, false, > MEMTXATTRS_UNSPECIFIED); > > if (haddr == NULL) { > > qemu_log_mask(LOG_GUEST_ERROR, "%s: qcrypto > failed\n", > > __func__);
