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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 10/8/19 1:32 PM, Alex Bennée wrote:
> 
> Philippe Mathieu-Daudé <f4bug@amsat.org> 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é <f4bug@amsat.org>
>> ---
>>   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 <alex.bennee@linaro.org>

Thanks!