Hi Cédric
> From: Cédric Le Goater <c...@kaod.org>
>
> Subject: Re: [PATCH v1 11/22] hw/misc/aspeed_hace: Add trace-events for
> better debugging
>
> On 3/21/25 10:26, Jamin Lin wrote:
> > Introduced "trace_aspeed_hace_addr", "trace_aspeed_hace_sg",
> > "trace_aspeed_hace_read", and "trace_aspeed_hace_write" trace events.
> >
> > Signed-off-by: Jamin Lin <jamin_...@aspeedtech.com>
> > ---
> > hw/misc/aspeed_hace.c | 8 ++++++++
> > hw/misc/trace-events | 6 ++++++
> > 2 files changed, 14 insertions(+)
> >
> > diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c index
> > 53b3b390e3..b8e473ee3f 100644
> > --- a/hw/misc/aspeed_hace.c
> > +++ b/hw/misc/aspeed_hace.c
> > @@ -18,6 +18,7 @@
> > #include "crypto/hash.h"
> > #include "hw/qdev-properties.h"
> > #include "hw/irq.h"
> > +#include "trace.h"
> >
> > #define R_CRYPT_CMD (0x10 / 4)
> >
> > @@ -186,6 +187,7 @@ static void do_hash_operation(AspeedHACEState *s,
> int algo, bool sg_mode,
> > if (ahc->has_dma64) {
> > src = deposit64(src, 32, 32, s->regs[R_HASH_SRC_HI]);
> > }
> > + trace_aspeed_hace_addr("src", src);
> > src += i * SG_LIST_ENTRY_SIZE;
> >
> > len = address_space_ldl_le(&s->dram_as, src, @@ -194,6
> > +196,7 @@ 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;
> > + trace_aspeed_hace_sg(i, sg_addr, len);
> > /*
> > * Ideally, sg_addr should be 64-bit for the AST2700, using
> the
> > * following program to obtain the 64-bit sg_addr and
> > convert it @@ -237,6 +240,7 @@ 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]);
> > + trace_aspeed_hace_addr("src", src);
> > if (ahc->has_dma64) {
> > src = deposit64(src, 32, 32, s->regs[R_HASH_SRC_HI]);
> > }
> > @@ -299,6 +303,7 @@ static void do_hash_operation(AspeedHACEState *s,
> int algo, bool sg_mode,
> > if (ahc->has_dma64) {
> > digest_addr = deposit64(digest_addr, 32, 32,
> s->regs[R_HASH_DEST_HI]);
> > }
> > + trace_aspeed_hace_addr("digest", digest_addr);
> > if (address_space_write(&s->dram_as, digest_addr,
> > MEMTXATTRS_UNSPECIFIED,
> > digest_buf, digest_len)) {
>
> I would use the prefix 'trace_aspeed_hace_hash_addr' since the trace events
> are called from do_hash_operation()
>
>
Will add "aspeed_hace_hash" prefix to make user know trace event for hash
command.
Thanks-Jamin
> Thanks,
>
> C.
>
>
>
> > @@ -326,6 +331,7 @@ static uint64_t aspeed_hace_read(void *opaque,
> hwaddr addr, unsigned int size)
> > return 0;
> > }
> >
> > + trace_aspeed_hace_read(addr << 2, s->regs[addr]);
> > return s->regs[addr];
> > }
> >
> > @@ -344,6 +350,8 @@ static void aspeed_hace_write(void *opaque,
> hwaddr addr, uint64_t data,
> > return;
> > }
> >
> > + trace_aspeed_hace_write(addr << 2, data);
> > +
> > switch (addr) {
> > case R_STATUS:
> > if (data & HASH_IRQ) {
> > diff --git a/hw/misc/trace-events b/hw/misc/trace-events index
> > 4383808d7a..cf96e68cfa 100644
> > --- a/hw/misc/trace-events
> > +++ b/hw/misc/trace-events
> > @@ -302,6 +302,12 @@ aspeed_peci_read(uint64_t offset, uint64_t data)
> "offset 0x%" PRIx64 " data 0x%"
> > aspeed_peci_write(uint64_t offset, uint64_t data) "offset 0x%" PRIx64 "
> data 0x%" PRIx64
> > aspeed_peci_raise_interrupt(uint32_t ctrl, uint32_t status) "ctrl
> > 0x%" PRIx32 " status 0x%" PRIx32
> >
> > +# aspeed_hace.c
> > +aspeed_hace_read(uint64_t offset, uint64_t data) "offset 0x%" PRIx64
> > +" data 0x%" PRIx64 aspeed_hace_write(uint64_t offset, uint64_t data)
> > +"offset 0x%" PRIx64 " data 0x%" PRIx64 aspeed_hace_sg(int index,
> > +uint64_t addr, uint32_t len) "%d: addr 0x%" PRIx64 " len 0x%" PRIx32
> > +aspeed_hace_addr(const char *s, uint64_t addr) "%s: 0x%" PRIx64
> > +
> > # bcm2835_property.c
> > bcm2835_mbox_property(uint32_t tag, uint32_t bufsize, size_t resplen)
> "mbox property tag:0x%08x in_sz:%u out_sz:%zu"
> >
