Re: [Qemu-devel] [PATCH 3/5] hw: arm_gic: Keep track of SGI sources

[Christoffer Dall <christoffer.dall@linaro.org>]

On Fri, Sep 06, 2013 at 03:08:16PM +0100, Peter Maydell wrote:
> On 23 August 2013 21:10, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> > Right now the arm gic emulation doesn't keep track of the source of an
> > SGI (which apparently Linux guests don't use, or they're fine with
> > assuming CPU 0 always).
> >
> > Add the necessary matrix on the GICState structure and maintain the data
> > when setting and clearing the pending state of an IRQ.
> >
> > Note that we always choose to present the source as the lowest-numbered
> > CPU in case multiple cores have signalled the same SGI number to a core
> > on the system.
> 
> > @@ -525,6 +538,11 @@ static void gic_dist_writel(void *opaque, hwaddr offset,
> >              break;
> >          }
> >          GIC_SET_PENDING(irq, mask);
> > +        target_cpu = (unsigned)ffs(mask) - 1;
> > +        while (target_cpu < NCPU) {
> > +            s->sgi_source[irq][target_cpu] |= (1 << cpu);
> > +            target_cpu = (unsigned)ffs(mask) - 1;
> > +        }
> 
> This is an infinite loop, because you don't do anything
> with mask inside the loop, so target_cpu is always
> the same each time round. gcc with optimization is
> smart enough to notice this:
> 
> => 0x00005555556c1625 <+229>:   jmp    0x5555556c1625 <gic_dist_writel+229>
> 
> :-)
> 
> Unsurprisingly, my test vexpress-a9 image hangs on startup.
> 

that's unfortunate, I think I meant to use find_next_bit and pass
target_cpu, but ended up deciding to just clear the bit in the mask, but
forgot to actually clear the bit. Whoops.

-Christoffer

> > --- a/hw/intc/arm_gic_common.c
> > +++ b/hw/intc/arm_gic_common.c
> > @@ -71,6 +71,7 @@ static const VMStateDescription vmstate_gic = {
> >          VMSTATE_UINT8_2DARRAY(priority1, GICState, GIC_INTERNAL, NCPU),
> >          VMSTATE_UINT8_ARRAY(priority2, GICState, GIC_MAXIRQ - GIC_INTERNAL),
> >          VMSTATE_UINT16_2DARRAY(last_active, GICState, GIC_MAXIRQ, NCPU),
> > +        VMSTATE_UINT8_2DARRAY(sgi_source, GICState, GIC_NR_SGIS, NCPU),
> >          VMSTATE_UINT16_ARRAY(priority_mask, GICState, NCPU),
> >          VMSTATE_UINT16_ARRAY(running_irq, GICState, NCPU),
> >          VMSTATE_UINT16_ARRAY(running_priority, GICState, NCPU),
> 
> You need to bump the version_id and minimum_version_id
> if you add a new field here.
>