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.
Thanks-Jamin
>
>
> Thanks,
>
> C.
>
>
>
> > plen = len & SG_LIST_LEN_MASK;
> > haddr = address_space_map(&s->dram_as, sg_addr, &plen,
> > false, @@ -218,7 +237,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]);
> > -
> > + if (ahc->has_dma64) {
> > + src = deposit64(src, 32, 32, s->regs[R_HASH_SRC_HI]);
> > + }
> > haddr = address_space_map(&s->dram_as, src,
> > &plen, false,
> MEMTXATTRS_UNSPECIFIED);
> > if (haddr == NULL) {
> > @@ -275,6 +296,9 @@ static void do_hash_operation(AspeedHACEState *s,
> int algo, bool sg_mode,
> > }
> >
> > digest_addr = deposit64(digest_addr, 0, 32,
> > s->regs[R_HASH_DEST]);
> > + if (ahc->has_dma64) {
> > + digest_addr = deposit64(digest_addr, 32, 32,
> s->regs[R_HASH_DEST_HI]);
> > + }
> > if (address_space_write(&s->dram_as, digest_addr,
> > MEMTXATTRS_UNSPECIFIED,
> > digest_buf, digest_len)) { @@ -601,6
> > +625,7 @@ static void aspeed_ast2700_hace_class_init(ObjectClass *klass,
> void *data)
> > * has completed. It is a temporary workaround.
> > */
> > ahc->raise_crypt_interrupt_workaround = true;
> > + ahc->has_dma64 = true;
> > }
> >
> > static const TypeInfo aspeed_ast2700_hace_info = {
