On Mon, Oct 14, 2013 at 04:36:24PM +0100, Peter Maydell wrote:
> On 26 September 2013 22:03, Christoffer Dall
> <christoffer.dall@xxxxxxxxxx> 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@xxxxxxxxxx>
> >
> > ---
> >
> > 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
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
