Re: [PATCH 07/19] hw/misc/bcm2835_mbox: Add trace events

[Philippe Mathieu-Daudé]

On 10/8/19 1:32 PM, Alex Bennée wrote:
> Philippe Mathieu-Daudé <address@hidden> writes:
>
> > Add trace events for read/write accesses and IRQ.
> >
> > Properties are structures used for the ARM particular MBOX.
> > Since one call in bcm2835_property.c concerns the mbox block,
> > name this trace event in the same bcm2835_mbox* namespace.
> >
> > Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> > ---
> >   hw/misc/bcm2835_mbox.c     | 5 +++++
> >   hw/misc/bcm2835_property.c | 2 ++
> >   hw/misc/trace-events       | 6 ++++++
> >   3 files changed, 13 insertions(+)
> >
> > diff --git a/hw/misc/bcm2835_mbox.c b/hw/misc/bcm2835_mbox.c
> > index 77285624c9..77d2d80706 100644
> > --- a/hw/misc/bcm2835_mbox.c
> > +++ b/hw/misc/bcm2835_mbox.c
> > @@ -15,6 +15,7 @@
> >   #include "migration/vmstate.h"
> >   #include "qemu/log.h"
> >   #include "qemu/module.h"
> > +#include "trace.h"
> >
> >   #define MAIL0_PEEK   0x90
> >   #define MAIL0_SENDER 0x94
> > @@ -123,6 +124,7 @@ static void bcm2835_mbox_update(BCM2835MboxState *s)
> >               set = true;
> >           }
> >       }
> > +    trace_bcm2835_mbox_irq(set);
> >       qemu_set_irq(s->arm_irq, set);
>
> I'm kind surprised we don't have a common trace point for all IRQs.

Trace event names are used for filtering. You usually don't want to 
display all the IRQs, but rather a filtered set of devices you are 
working on/analyzing.

> >   }
> >
> > @@ -178,8 +180,10 @@ static uint64_t bcm2835_mbox_read(void *opaque, hwaddr 
> > offset, unsigned size)
> >       default:
> >           qemu_log_mask(LOG_UNIMP, "%s: Unsupported offset 0x%"HWADDR_PRIx"\n",
> >                         __func__, offset);
> > +        trace_bcm2835_mbox_read(size, offset, res);
> >           return 0;
> >       }
> > +    trace_bcm2835_mbox_read(size, offset, res);
> >
> >       bcm2835_mbox_update(s);
> >
> > @@ -195,6 +199,7 @@ static void bcm2835_mbox_write(void *opaque, hwaddr offset,
> >
> >       offset &= 0xff;
> >
> > +    trace_bcm2835_mbox_write(size, offset, value);
> >       switch (offset) {
> >       case MAIL0_SENDER:
> >           break;
> > diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c
> > index 43a5465c5d..0eea2e20f7 100644
> > --- a/hw/misc/bcm2835_property.c
> > +++ b/hw/misc/bcm2835_property.c
> > @@ -13,6 +13,7 @@
> >   #include "sysemu/dma.h"
> >   #include "qemu/log.h"
> >   #include "qemu/module.h"
> > +#include "trace.h"
> >
> >   /* https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface */
> >
> > @@ -283,6 +284,7 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState 
> > *s, uint32_t value)
> >               break;
> >           }
> >
> > +        trace_bcm2835_mbox_property(tag, bufsize, resplen);
> >           if (tag == 0) {
> >               break;
> >           }
> > diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> > index 74276225f8..1deb1d08c1 100644
> > --- a/hw/misc/trace-events
> > +++ b/hw/misc/trace-events
> > @@ -143,3 +143,9 @@ armsse_mhu_write(uint64_t offset, uint64_t data, unsigned size) 
> > "SSE-200 MHU wri
> >
> >   # aspeed_xdma.c
> >   aspeed_xdma_write(uint64_t offset, uint64_t data) "XDMA write: offset 0x%" PRIx64 
> > " data 0x%" PRIx64
> > +
> > +# bcm2835_mbox.c
> > +bcm2835_mbox_write(unsigned int size, uint64_t addr, uint64_t value) "mbox write sz:%u 
> > addr:0x%"PRIx64" data:0x%"PRIx64
> > +bcm2835_mbox_read(unsigned int size, uint64_t addr, uint64_t value) "mbox read sz:%u 
> > addr:0x%"PRIx64" data:0x%"PRIx64
> > +bcm2835_mbox_irq(unsigned level) "mbox irq:ARM level:%u"
> > +bcm2835_mbox_property(uint32_t tag, uint32_t bufsize, size_t resplen) "mbox 
> > property tag:0x%08x in_sz:%u out_sz:%zu"
>
> Anyway:
>
> Reviewed-by: Alex Bennée <address@hidden>

Thanks!