Re: [Qemu-devel] [PATCH 04/11] heathrow: convert to trace-events

[Mark Cave-Ayland]

On 20/02/18 04:20, David Gibson wrote:

> On Mon, Feb 19, 2018 at 06:19:15PM +0000, Mark Cave-Ayland wrote:
> > Signed-off-by: Mark Cave-Ayland <address@hidden>
> > ---
> >   hw/intc/heathrow_pic.c         | 31 ++++++++++++-------------------
> >   hw/intc/trace-events           |  5 +++++
> >   include/hw/intc/heathrow_pic.h |  1 +
> >   3 files changed, 18 insertions(+), 19 deletions(-)
> >
> > diff --git a/hw/intc/heathrow_pic.c b/hw/intc/heathrow_pic.c
> > index 7bf44e0d86..afe0b08cbb 100644
> > --- a/hw/intc/heathrow_pic.c
> > +++ b/hw/intc/heathrow_pic.c
> > @@ -26,16 +26,7 @@
> >   #include "hw/hw.h"
> >   #include "hw/ppc/mac.h"
> >   #include "hw/intc/heathrow_pic.h"
> > -
> > -/* debug PIC */
> > -//#define DEBUG_PIC
> > -
> > -#ifdef DEBUG_PIC
> > -#define PIC_DPRINTF(fmt, ...)                                   \
> > -    do { printf("PIC: " fmt , ## __VA_ARGS__); } while (0)
> > -#else
> > -#define PIC_DPRINTF(fmt, ...)
> > -#endif
> > +#include "trace.h"
> >   
> >   static inline int heathrow_check_irq(HeathrowPICState *pic)
> >   {
> > @@ -61,7 +52,7 @@ static void heathrow_write(void *opaque, hwaddr addr,
> >       unsigned int n;
> >   
> >       n = ((addr & 0xfff) - 0x10) >> 4;
> > -    PIC_DPRINTF("writel: " TARGET_FMT_plx " %u: %08x\n", addr, n, value);
> > +    trace_heathrow_write(addr, n, value);
> >       if (n >= 2)
> >           return;
> >       pic = &s->pics[n];
> > @@ -109,7 +100,7 @@ static uint64_t heathrow_read(void *opaque, hwaddr addr,
> >               break;
> >           }
> >       }
> > -    PIC_DPRINTF("readl: " TARGET_FMT_plx " %u: %08x\n", addr, n, value);
> > +    trace_heathrow_read(addr, n, value);
> >       return value;
> >   }
> >   
> > @@ -124,16 +115,18 @@ static void heathrow_set_irq(void *opaque, int num, int level)
> >       HeathrowState *s = opaque;
> >       HeathrowPICState *pic;
> >       unsigned int irq_bit;
> > +    int last_level = (s->last_levels & (num << 1UL)) ? 1 : 0;
> >   
> > -#if defined(DEBUG)
> > -    {
> > -        static int last_level[64];
> > -        if (last_level[num] != level) {
> > -            PIC_DPRINTF("set_irq: num=0x%02x level=%d\n", num, level);
> > -            last_level[num] = level;
> > +    if (last_level != level) {
> > +        trace_heathrow_set_irq(num, level);
> > +
> > +        if (level) {
> > +            s->last_levels |= (num << 1UL);
> > +        } else {
> > +            s->last_levels &= ~(num << 1UL);
>
> Cleaning up this last_level stuff is a good idea, but is a bit more
> involved than merely converting to trace-events.
>
> It's also not really the "last" level - it's effectively maintaining
> the current level of the input irqs.  .. and isn't that information
> already within the HeathrowPICState structure?

Yeah actually you're right here - I was so focused on converting the 
code as-is that I didn't really look at what was happening at the end of 
heathrow_set_irq(). The motivation was that without this hack the trace 
output is always outputting the IRQ status, presumably because we have a 
level-triggered interrupt being asserted.

Let me rework this patch to use the last state information from 
HeathrowPICState instead.


ATB,

Mark.