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

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

On Mon, Oct 14, 2013 at 04:36:24PM +0100, Peter Maydell wrote:
> On 26 September 2013 22:03, 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.
> 
> Shouldn't the state you have in sgi_source[][] be surfaced as the
> GICD_CPENDSGIR/GICD_SPENDSGIR registers [for a v2 GIC; they don't
> exist in v1]? It might then be better to represent the state in
> our data structures in the same layout as the registers.
> 

Hmm, those registers actually don't represent which CPU is the *source*
of a given SGI.  I think the array I propose is quite reasonable...

> >
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> >
> > ---
> >
> > Changelog [v2]:
> >  - Fixed endless loop bug
> >  - Bump version_id and minimum_version_id on vmstate struct
> > ---
> >  hw/intc/arm_gic.c        |   41 ++++++++++++++++++++++++++++++++---------
> >  hw/intc/arm_gic_common.c |    5 +++--
> >  hw/intc/gic_internal.h   |    3 +++
> >  3 files changed, 38 insertions(+), 11 deletions(-)
> >
> > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> > index 7eaa55f..6470d37 100644
> > --- a/hw/intc/arm_gic.c
> > +++ b/hw/intc/arm_gic.c
> > @@ -97,6 +97,20 @@ void gic_set_pending_private(GICState *s, int cpu, int irq)
> >      gic_update(s);
> >  }
> >
> > +static void gic_clear_pending(GICState *s, int irq, int cm, uint8_t src)
> > +{
> 
> I think that in the cases where we pass in 0 for src that the
> irq can't be < GIC_NR_SGIS.
> 

not quite sure what you mean by this comment, we pass in 0 for src in
two cases: 1) when we are dealing with something else than an SGI
(irq >= GIC_NR_SGIS), but we could pass whatever, the value doesn't
make sense here 2) when we are dealing with an SGI, the source can be 0
up to s->num_cpu - 1.

> > +    unsigned cpu;
> > +
> > +    GIC_CLEAR_PENDING(irq, cm);
> > +    if (irq < GIC_NR_SGIS) {
> > +        cpu = (unsigned)ffs(cm) - 1;
> 
> If you used ctz32() rather than ffs() it would save you having to
> subtract one all the time. Also, those unsigned casts are pretty
> ugly: better to just make 'cpu' an int...
> 

yeah, that was quite ridiculous.

> > +        while (cpu < NCPU) {
> > +            s->sgi_source[irq][cpu] &= ~(1 << src);
> > +            cpu = (unsigned)ffs(cm) - 1;
> > +        }
> 
> ...this still seems to be an infinite loop: cm isn't modified
> inside the loop so cpu will always have the same value each time.
> 
>      610:       eb fe                   jmp    610 <gic_clear_pending+0x50>
> 

if only I had for_each_set_bit - does QEMU have something sane for this?
Anyway, I tried again to fix this.  Thanks for spotting my broken fix
for my broken code.

> > +    }
> 
> Are you sure the logic in this function is right? (ie that we
> should only clear the sgi_source[][] bit for this source, and
> not completely? If nothing else, I think the interrupt should
> stay pending if some other source cpu still wants it to be
> pending. That is, I think we need to track the pending status
> separately for each (irq,target-cpu,source-cpu) separately for
> SGIs. (I'm not totally sure I have this right though, the spec
> is quite confusing.)
> 

no, you're right, for SGIs we need to loop through the sgi_source array
and make sure the irq is not pending on any CPUs from any CPUs.

> > +}
> > +
> 
> >  /* Maximum number of possible CPU interfaces, determined by GIC architecture */
> >  #define NCPU 8
> >
> > @@ -58,6 +59,7 @@
> >                                      s->priority1[irq][cpu] :            \
> >                                      s->priority2[(irq) - GIC_INTERNAL])
> >  #define GIC_TARGET(irq) s->irq_target[irq]
> > +#define GIC_SGI_SRC(irq, cpu) (((irq) < GIC_NR_SGIS) ? ffs(s->sgi_source[irq][cpu]) - 1 : 0)
> 
> WARNING: line over 80 characters
> #161: FILE: hw/intc/gic_internal.h:62:
> +#define GIC_SGI_SRC(irq, cpu) (((irq) < GIC_NR_SGIS) ?
> ffs(s->sgi_source[irq][cpu]) - 1 : 0)
> 

Thans,
-Christoffer