Hi Cédric
> Subject: Re: [PATCH v1 08/22] hw/misc/aspeed_hace: Support DMA 64 bits
> dram address.
>
> On 5/9/25 09:04, Jamin Lin wrote:
> > Hi Cédric
> >
> >> Subject: Re: [PATCH v1 08/22] hw/misc/aspeed_hace: Support DMA 64
> >> bits dram address.
> >>
> >> On 3/21/25 10:26, Jamin Lin wrote:
> >>> According to the AST2700 design, the data source address is 64-bit,
> >>> with R_HASH_SRC_HI storing bits [63:32] and R_HASH_SRC storing bits
> >> [31:0].
> >>>
> >>> Similarly, the digest address is 64-bit, with R_HASH_DEST_HI storing
> >>> bits [63:32] and R_HASH_DEST storing bits [31:0].
> >>>
> >>> Ideally, sg_addr should be 64-bit for the AST2700, using the
> >>> following program to obtain the 64-bit sg_addr and convert it to a DRAM
> offset:
> >>>
> >>> ```
> >>> sg_addr = deposit64(sg_addr, 32, 32,
> >>> address_space_ldl_le(&s->dram_as, src +
> >> SG_LIST_ADDR_SIZE,
> >>>
> >> MEMTXATTRS_UNSPECIFIED,
> >>> NULL); sg_addr -= 0x400000000; ```
> >>
> >> I don't think the code extract above is useful.
> >>
> >>> To maintain compatibility with older SoCs such as the AST2600, the
> >>> AST2700 HW HACE controllers automatically set bit 34 of the 64-bit
> sg_addr.
> >>
> >> I suppose that's what bits [30:28] of the first word of the
> >> scatter-gather entry are for ?
> >>
> >>> As a result,
> >>> the firmware only needs to provide a 32-bit sg_addr containing bits
> >>> [31:0].
> >>> This is sufficient for the AST2700, as it uses a DRAM offset rather
> >>> than a DRAM address.
> >>
> >> yes the HACE model can use a relative address because the DRAM memory
> >> region is directly available. There is no need to construct a physical
> >> address.
> >>
> >>> Introduce a has_dma64 class attribute and set it to true for the AST2700.
> >>>
> >>> Signed-off-by: Jamin Lin <jamin_...@aspeedtech.com>
> >>> ---
> >>> include/hw/misc/aspeed_hace.h | 1 +
> >>> hw/misc/aspeed_hace.c | 27
> ++++++++++++++++++++++++++-
> >>> 2 files changed, 27 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/include/hw/misc/aspeed_hace.h
> >>> b/include/hw/misc/aspeed_hace.h index a4479bd383..58fb66009a 100644
> >>> --- a/include/hw/misc/aspeed_hace.h
> >>> +++ b/include/hw/misc/aspeed_hace.h
> >>> @@ -52,6 +52,7 @@ struct AspeedHACEClass {
> >>> uint32_t src_hi_mask;
> >>> uint32_t dest_hi_mask;
> >>> uint32_t key_hi_mask;
> >>> + bool has_dma64;
> >>> };
> >>>
> >>> #endif /* ASPEED_HACE_H */
> >>> diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c index
> >>> 51c6523fab..8f333fc97e 100644
> >>> --- a/hw/misc/aspeed_hace.c
> >>> +++ b/hw/misc/aspeed_hace.c
> >>> @@ -148,6 +148,7 @@ static bool has_padding(AspeedHACEState *s,
> >>> struct
> >> iovec *iov,
> >>> static void do_hash_operation(AspeedHACEState *s, int algo, bool
> >> sg_mode,
> >>> bool acc_mode)
> >>> {
> >>> + AspeedHACEClass *ahc = ASPEED_HACE_GET_CLASS(s);
> >>> bool sg_acc_mode_final_request = false;
> >>> g_autofree uint8_t *digest_buf = NULL;
> >>> struct iovec iov[ASPEED_HACE_MAX_SG]; @@ -182,6 +183,9
> @@
> >> static
> >>> void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode,
> >>> }
> >>>
> >>> src = deposit64(src, 0, 32, s->regs[R_HASH_SRC]);
> >>> + if (ahc->has_dma64) {
> >>> + src = deposit64(src, 32, 32, s->regs[R_HASH_SRC_HI]);
> >>> + }
> >>
> >> That's where a little helper would be nice to have.
> >>
> > I add hash_get_source_addr help function.
> > Please see patch 6 comments.
> >
> >>> src += i * SG_LIST_ENTRY_SIZE;
> >>>
> >>> len = address_space_ldl_le(&s->dram_as, src, @@
> >>> -190,6
> >>> +194,21 @@ static void do_hash_operation(AspeedHACEState *s, int
> >>> +algo,
> >> bool sg_mode,
> >>> sg_addr = address_space_ldl_le(&s->dram_as, src +
> >> SG_LIST_LEN_SIZE,
> >>>
> >> MEMTXATTRS_UNSPECIFIED, NULL);
> >>> sg_addr &= SG_LIST_ADDR_MASK;
> >>> + /*
> >>> + * Ideally, sg_addr should be 64-bit for the AST2700,
> >>> + using
> >> the
> >>> + * following program to obtain the 64-bit sg_addr and
> >> convert it
> >>> + * to a DRAM offset:
> >>> + * sg_addr = deposit64(sg_addr, 32, 32,
> >>> + * address_space_ldl_le(&s->dram_as, src +
> >> SG_ADDR_LEN_SIZE,
> >>> + *
> >> MEMTXATTRS_UNSPECIFIED, NULL);
> >>> + * sg_addr -= 0x400000000;
> >>> + *
> >>
> >> I don't think the above comment is useful. Please keep the one below.
> >>
> >>> + * To maintain compatibility with older SoCs such as
> >>> + the
> >> AST2600,
> >>> + * the AST2700 HW automatically set bit 34 of the
> >>> + 64-bit
> >> sg_addr.
> >>> + * As a result, the firmware only needs to provide a
> >>> + 32-bit
> >> sg_addr
> >>> + * containing bits [31:0]. This is sufficient for the
> >>> + AST2700,
> >> as
> >>> + * it uses a DRAM offset rather than a DRAM address.
> >>> + */
> >>
> >
> > Thanks for suggestion will update them.
> >
> >> The SG_LIST_ADDR_MASK needs an update though. AFAICT, it's bigger on
> >> AST2700.
> >
> > The value of SG_LIST_ADDR_MASK was wrong for AST2700, AST2600 and
> AST1030.
> > The correct value should be 0x7FFFFFF8.
> > Will create a new patch to fix it.
> > Please see patch 4 comments.
> > By the way, AST2500 do not support SG mode.
>
> Should we introduce a class attribute then ?
>
Can I modify SG_LIST_ADDR_MASK directly?
In this model, hash_mask is set to 0x000003ff for AST2500, which disables
support for SG mode and SHA512:
As a result, the model does not handle SG mode for AST2500.
https://github.com/qemu/qemu/blob/master/hw/misc/aspeed_hace.c#L529
ahc->hash_mask = 0x000003ff; /* No SG or SHA512 modes */
Thanks-Jamin
> Thanks,
>
> C.
