| Message ID | 1380229386-24166-4-git-send-email-christoffer.dall@linaro.org |
|---|---|
| State | New |
| Headers | show |
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. > > 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. > + 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... > + 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> > + } 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.) > +} > + > /* 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) -- PMM
On 14 October 2013 16:36, Peter Maydell <peter.maydell@linaro.org> wrote: > 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.) Having spoken to Marc I'm now a bit less confused. For SGIs: * there is effectively a pending bit for each (irq-number, source-cpu, target-cpu) tuple * for a v2 GIC these are guest-visible in GICD_CPENDSGIRn and GICD_SPENDSGIRn; for a pre-v2 GIC the state still exists, it's just not guest-visible * the overall "pending state" bit in GICD_ISPENDRn and GICD_ICPENDRn is effectively the logical OR of the pending bits for each source-cpu; writes to this bit are ignored * when the guest reads from GICC_IAR, if the SGI is pending for multiple source-cpus for this target-cpu then we clear the pending bit for the (int, src, tgt) tuple in question, but it will still be set for the other (int,src,tgt) tuples for the other source-cpus (and so the GICD_ISPENDRn bit will still be set) Tangentially, I notice that we don't correctly handle the PENDING bit for level triggered interrupts, since we do: /* Clear pending flags for both level and edge triggered interrupts. Level triggered IRQs will be reasserted once they become inactive. */ gic_clear_pending(s, new_irq, GIC_TEST_MODEL(new_irq) ? ALL_CPU_MASK : cm, GIC_SGI_SRC(new_irq, cpu)); in gic_acknowledge_irq(). This is wrong, because section 3.2.4 is clear for a level triggered interrupt that if the interrupt signal remains asserted (which it usually will be) then we go from Pending to Active+Pending (whereas our current implementation goes from Pending to Active and then resets Pending later in gic_complete_irq()). -- PMM
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
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) +{ + unsigned cpu; + + GIC_CLEAR_PENDING(irq, cm); + if (irq < GIC_NR_SGIS) { + cpu = (unsigned)ffs(cm) - 1; + while (cpu < NCPU) { + s->sgi_source[irq][cpu] &= ~(1 << src); + cpu = (unsigned)ffs(cm) - 1; + } + } +} + /* Process a change in an external IRQ input. */ static void gic_set_irq(void *opaque, int irq, int level) { @@ -132,7 +146,7 @@ static void gic_set_irq(void *opaque, int irq, int level) GIC_SET_PENDING(irq, target); } else { if (!GIC_TEST_TRIGGER(irq)) { - GIC_CLEAR_PENDING(irq, target); + gic_clear_pending(s, irq, target, 0); } GIC_CLEAR_LEVEL(irq, cm); } @@ -163,7 +177,8 @@ uint32_t gic_acknowledge_irq(GICState *s, int cpu) s->last_active[new_irq][cpu] = s->running_irq[cpu]; /* Clear pending flags for both level and edge triggered interrupts. Level triggered IRQs will be reasserted once they become inactive. */ - GIC_CLEAR_PENDING(new_irq, GIC_TEST_MODEL(new_irq) ? ALL_CPU_MASK : cm); + gic_clear_pending(s, new_irq, GIC_TEST_MODEL(new_irq) ? ALL_CPU_MASK : cm, + GIC_SGI_SRC(new_irq, cpu)); gic_set_running_irq(s, cpu, new_irq); DPRINTF("ACK %d\n", new_irq); return new_irq; @@ -437,12 +452,9 @@ static void gic_dist_writeb(void *opaque, hwaddr offset, irq = (offset - 0x280) * 8 + GIC_BASE_IRQ; if (irq >= s->num_irq) goto bad_reg; - for (i = 0; i < 8; i++) { - /* ??? This currently clears the pending bit for all CPUs, even - for per-CPU interrupts. It's unclear whether this is the - corect behavior. */ - if (value & (1 << i)) { - GIC_CLEAR_PENDING(irq + i, ALL_CPU_MASK); + for (i = 0; i < 8; i++, irq++) { + if (irq > GIC_NR_SGIS && value & (1 << i)) { + gic_clear_pending(s, irq, 1 << cpu, 0); } } } else if (offset < 0x400) { @@ -515,6 +527,7 @@ static void gic_dist_writel(void *opaque, hwaddr offset, int cpu; int irq; int mask; + unsigned target_cpu; cpu = gic_get_current_cpu(s); irq = value & 0x3ff; @@ -534,6 +547,12 @@ 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); + mask &= ~(1 << target_cpu); + target_cpu = (unsigned)ffs(mask) - 1; + } gic_update(s); return; } @@ -551,6 +570,8 @@ static const MemoryRegionOps gic_dist_ops = { static uint32_t gic_cpu_read(GICState *s, int cpu, int offset) { + int value; + switch (offset) { case 0x00: /* Control */ return s->cpu_enabled[cpu]; @@ -560,7 +581,9 @@ static uint32_t gic_cpu_read(GICState *s, int cpu, int offset) /* ??? Not implemented. */ return 0; case 0x0c: /* Acknowledge */ - return gic_acknowledge_irq(s, cpu); + value = gic_acknowledge_irq(s, cpu); + value |= (GIC_SGI_SRC(value, cpu) & 0x7) << 10; + return value; case 0x14: /* Running Priority */ return s->running_priority[cpu]; case 0x18: /* Highest Pending Interrupt */ diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c index 709b5c2..0657e8b 100644 --- a/hw/intc/arm_gic_common.c +++ b/hw/intc/arm_gic_common.c @@ -58,8 +58,8 @@ static const VMStateDescription vmstate_gic_irq_state = { static const VMStateDescription vmstate_gic = { .name = "arm_gic", - .version_id = 4, - .minimum_version_id = 4, + .version_id = 5, + .minimum_version_id = 5, .pre_save = gic_pre_save, .post_load = gic_post_load, .fields = (VMStateField[]) { @@ -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), diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h index 09e7722..5b53242 100644 --- a/hw/intc/gic_internal.h +++ b/hw/intc/gic_internal.h @@ -27,6 +27,7 @@ #define GIC_MAXIRQ 1020 /* First 32 are private to each CPU (SGIs and PPIs). */ #define GIC_INTERNAL 32 +#define GIC_NR_SGIS 16 /* 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) typedef struct gic_irq_state { /* The enable bits are only banked for per-cpu interrupts. */ @@ -83,6 +85,7 @@ typedef struct GICState { uint8_t priority1[GIC_INTERNAL][NCPU]; uint8_t priority2[GIC_MAXIRQ - GIC_INTERNAL]; uint16_t last_active[GIC_MAXIRQ][NCPU]; + uint8_t sgi_source[GIC_NR_SGIS][NCPU]; uint16_t priority_mask[NCPU]; uint16_t running_irq[NCPU];
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. 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(-)