| Message ID | 1414707132-24588-17-git-send-email-greg.bellows@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 30/10/14 22:12, Greg Bellows wrote: > From: Fabian Aggeler <aggelerf@ethz.ch> > > GICs with grouping (GICv2 or GICv1 with Security Extensions) have a > different exception generation model which is more complicated than > without interrupt grouping. We add a new function to handle this model. > > Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch> > > --- > > v1 -> v2 > - Fix issue in gic_update_with_grouping() using the wrong combination of > flag and CPU control bank for checking if group 1 interrupts are enabled. > --- > hw/intc/arm_gic.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++- > hw/intc/gic_internal.h | 1 + > 2 files changed, 87 insertions(+), 1 deletion(-) > > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c > index 808aa18..e33c470 100644 > --- a/hw/intc/arm_gic.c > +++ b/hw/intc/arm_gic.c > @@ -52,6 +52,87 @@ static inline bool ns_access(void) > return true; > } > > +inline void gic_update_with_grouping(GICState *s) > +{ > + int best_irq; > + int best_prio; > + int irq; > + int irq_level; > + int fiq_level; > + int cpu; > + int cm; > + bool next_int; > + bool next_grp0; > + bool gicc_grp0_enabled; > + bool gicc_grp1_enabled; > + > + for (cpu = 0; cpu < NUM_CPU(s); cpu++) { > + cm = 1 << cpu; > + gicc_grp0_enabled = s->cpu_control[cpu][0] & GICC_CTLR_S_EN_GRP0; > + gicc_grp1_enabled = s->cpu_control[cpu][1] & GICC_CTLR_NS_EN_GRP1; > + next_int = 0; > + next_grp0 = 0; > + > + s->current_pending[cpu] = 1023; > + if ((!s->enabled_grp[0] && !s->enabled_grp[1]) > + || (!gicc_grp0_enabled && !gicc_grp1_enabled)) { > + qemu_irq_lower(s->parent_irq[cpu]); > + qemu_irq_lower(s->parent_fiq[cpu]); > + return; Shouldn't that be continue? Otherwise the state of CPU[N-1] influences whether interrupts can be delivered to CPU[N]. > + } > + > + /* Determine highest priority pending interrupt */ > + best_prio = 0x100; > + best_irq = 1023; > + for (irq = 0; irq < s->num_irq; irq++) { > + if (GIC_TEST_ENABLED(irq, cm) && gic_test_pending(s, irq, cm)) { > + if (GIC_GET_PRIORITY(irq, cpu) < best_prio) { > + best_prio = GIC_GET_PRIORITY(irq, cpu); > + best_irq = irq; > + } > + } > + } > + > + /* Priority of IRQ higher than priority mask? */ > + if (best_prio < s->priority_mask[cpu]) { > + s->current_pending[cpu] = best_irq; > + if (GIC_TEST_GROUP0(best_irq, cm) && s->enabled_grp[0]) { > + /* TODO: Add subpriority handling (binary point register) */ > + if (best_prio < s->running_priority[cpu]) { > + next_int = true; > + next_grp0 = true; > + } > + } else if (!GIC_TEST_GROUP0(best_irq, cm) && s->enabled_grp[1]) { > + /* TODO: Add subpriority handling (binary point register) */ > + if (best_prio < s->running_priority[cpu]) { > + next_int = true; > + next_grp0 = false; > + } > + } > + } > + > + fiq_level = 0; > + irq_level = 0; > + if (next_int) { > + if (next_gr && (s->cpu_control[cpu][0] & GICC_CTLR_S_FIQ_EN)) { > + if (gicc_grp0_enabled) { > + fiq_level = 1; > + DPRINTF("Raised pending FIQ %d (cpu %d)\n", best_irq, cpu); > + } > + } else { > + if ((next_grp0 && gicc_grp0_enabled) > + || (!next_grp0 && gicc_grp1_enabled)) { > + irq_level = 1; > + DPRINTF("Raised pending IRQ %d (cpu %d)\n", best_irq, cpu); > + } > + } > + } > + /* Set IRQ/FIQ signal */ > + qemu_set_irq(s->parent_irq[cpu], irq_level); > + qemu_set_irq(s->parent_fiq[cpu], fiq_level); > + } > +} > + > inline void gic_update_no_grouping(GICState *s) > { > int best_irq; > @@ -95,7 +176,11 @@ inline void gic_update_no_grouping(GICState *s) > /* Update interrupt status after enabled or pending bits have been changed. */ > void gic_update(GICState *s) > { > - gic_update_no_grouping(s); > + if (s->revision >= 2 || s->security_extn) { > + gic_update_with_grouping(s); > + } else { > + gic_update_no_grouping(s); > + } > } > > void gic_set_pending_private(GICState *s, int cpu, int irq) > diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h > index e16a7e5..01859ed 100644 > --- a/hw/intc/gic_internal.h > +++ b/hw/intc/gic_internal.h > @@ -73,6 +73,7 @@ > void gic_set_pending_private(GICState *s, int cpu, int irq); > uint32_t gic_acknowledge_irq(GICState *s, int cpu); > void gic_complete_irq(GICState *s, int cpu, int irq); > +inline void gic_update_with_grouping(GICState *s); > inline void gic_update_no_grouping(GICState *s); > void gic_update(GICState *s); > void gic_init_irqs_and_distributor(GICState *s); >
Thanks Daniel. I see what you are saying, but historically the code looks like it has always returned so I'd have to investigate it more as I am still learning the code myself. If this is a regression, it would be one inherited from the previous gic_update() function. I'll look further into it to see if this is an issue. If anyone else familiar with the code has any input it would be appreciated. Regards, Greg On 7 November 2014 06:44, Daniel Thompson <daniel.thompson@linaro.org> wrote: > On 30/10/14 22:12, Greg Bellows wrote: > > From: Fabian Aggeler <aggelerf@ethz.ch> > > > > GICs with grouping (GICv2 or GICv1 with Security Extensions) have a > > different exception generation model which is more complicated than > > without interrupt grouping. We add a new function to handle this model. > > > > Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch> > > > > --- > > > > v1 -> v2 > > - Fix issue in gic_update_with_grouping() using the wrong combination of > > flag and CPU control bank for checking if group 1 interrupts are > enabled. > > --- > > hw/intc/arm_gic.c | 87 > +++++++++++++++++++++++++++++++++++++++++++++++++- > > hw/intc/gic_internal.h | 1 + > > 2 files changed, 87 insertions(+), 1 deletion(-) > > > > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c > > index 808aa18..e33c470 100644 > > --- a/hw/intc/arm_gic.c > > +++ b/hw/intc/arm_gic.c > > @@ -52,6 +52,87 @@ static inline bool ns_access(void) > > return true; > > } > > > > +inline void gic_update_with_grouping(GICState *s) > > +{ > > + int best_irq; > > + int best_prio; > > + int irq; > > + int irq_level; > > + int fiq_level; > > + int cpu; > > + int cm; > > + bool next_int; > > + bool next_grp0; > > + bool gicc_grp0_enabled; > > + bool gicc_grp1_enabled; > > + > > + for (cpu = 0; cpu < NUM_CPU(s); cpu++) { > > + cm = 1 << cpu; > > + gicc_grp0_enabled = s->cpu_control[cpu][0] & > GICC_CTLR_S_EN_GRP0; > > + gicc_grp1_enabled = s->cpu_control[cpu][1] & > GICC_CTLR_NS_EN_GRP1; > > + next_int = 0; > > + next_grp0 = 0; > > + > > + s->current_pending[cpu] = 1023; > > + if ((!s->enabled_grp[0] && !s->enabled_grp[1]) > > + || (!gicc_grp0_enabled && !gicc_grp1_enabled)) { > > + qemu_irq_lower(s->parent_irq[cpu]); > > + qemu_irq_lower(s->parent_fiq[cpu]); > > + return; > > Shouldn't that be continue? Otherwise the state of CPU[N-1] influences > whether interrupts can be delivered to CPU[N]. > > > > + } > > + > > + /* Determine highest priority pending interrupt */ > > + best_prio = 0x100; > > + best_irq = 1023; > > + for (irq = 0; irq < s->num_irq; irq++) { > > + if (GIC_TEST_ENABLED(irq, cm) && gic_test_pending(s, irq, > cm)) { > > + if (GIC_GET_PRIORITY(irq, cpu) < best_prio) { > > + best_prio = GIC_GET_PRIORITY(irq, cpu); > > + best_irq = irq; > > + } > > + } > > + } > > + > > + /* Priority of IRQ higher than priority mask? */ > > + if (best_prio < s->priority_mask[cpu]) { > > + s->current_pending[cpu] = best_irq; > > + if (GIC_TEST_GROUP0(best_irq, cm) && s->enabled_grp[0]) { > > + /* TODO: Add subpriority handling (binary point > register) */ > > + if (best_prio < s->running_priority[cpu]) { > > + next_int = true; > > + next_grp0 = true; > > + } > > + } else if (!GIC_TEST_GROUP0(best_irq, cm) && > s->enabled_grp[1]) { > > + /* TODO: Add subpriority handling (binary point > register) */ > > + if (best_prio < s->running_priority[cpu]) { > > + next_int = true; > > + next_grp0 = false; > > + } > > + } > > + } > > + > > + fiq_level = 0; > > + irq_level = 0; > > + if (next_int) { > > + if (next_gr && (s->cpu_control[cpu][0] & > GICC_CTLR_S_FIQ_EN)) { > > + if (gicc_grp0_enabled) { > > + fiq_level = 1; > > + DPRINTF("Raised pending FIQ %d (cpu %d)\n", > best_irq, cpu); > > + } > > + } else { > > + if ((next_grp0 && gicc_grp0_enabled) > > + || (!next_grp0 && gicc_grp1_enabled)) { > > + irq_level = 1; > > + DPRINTF("Raised pending IRQ %d (cpu %d)\n", > best_irq, cpu); > > + } > > + } > > + } > > + /* Set IRQ/FIQ signal */ > > + qemu_set_irq(s->parent_irq[cpu], irq_level); > > + qemu_set_irq(s->parent_fiq[cpu], fiq_level); > > + } > > +} > > + > > inline void gic_update_no_grouping(GICState *s) > > { > > int best_irq; > > @@ -95,7 +176,11 @@ inline void gic_update_no_grouping(GICState *s) > > /* Update interrupt status after enabled or pending bits have been > changed. */ > > void gic_update(GICState *s) > > { > > - gic_update_no_grouping(s); > > + if (s->revision >= 2 || s->security_extn) { > > + gic_update_with_grouping(s); > > + } else { > > + gic_update_no_grouping(s); > > + } > > } > > > > void gic_set_pending_private(GICState *s, int cpu, int irq) > > diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h > > index e16a7e5..01859ed 100644 > > --- a/hw/intc/gic_internal.h > > +++ b/hw/intc/gic_internal.h > > @@ -73,6 +73,7 @@ > > void gic_set_pending_private(GICState *s, int cpu, int irq); > > uint32_t gic_acknowledge_irq(GICState *s, int cpu); > > void gic_complete_irq(GICState *s, int cpu, int irq); > > +inline void gic_update_with_grouping(GICState *s); > > inline void gic_update_no_grouping(GICState *s); > > void gic_update(GICState *s); > > void gic_init_irqs_and_distributor(GICState *s); > > > >
On Mon, Nov 10, 2014 at 3:43 PM, Greg Bellows <greg.bellows@linaro.org> wrote: [...] > On 7 November 2014 06:44, Daniel Thompson <daniel.thompson@linaro.org> > wrote: >> >> On 30/10/14 22:12, Greg Bellows wrote: >> > From: Fabian Aggeler <aggelerf@ethz.ch> >> > >> > GICs with grouping (GICv2 or GICv1 with Security Extensions) have a >> > different exception generation model which is more complicated than >> > without interrupt grouping. We add a new function to handle this model. >> > >> > Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch> >> > >> > --- >> > >> > v1 -> v2 >> > - Fix issue in gic_update_with_grouping() using the wrong combination of >> > flag and CPU control bank for checking if group 1 interrupts are >> > enabled. >> > --- >> > hw/intc/arm_gic.c | 87 >> > +++++++++++++++++++++++++++++++++++++++++++++++++- >> > hw/intc/gic_internal.h | 1 + >> > 2 files changed, 87 insertions(+), 1 deletion(-) >> > >> > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c >> > index 808aa18..e33c470 100644 >> > --- a/hw/intc/arm_gic.c >> > +++ b/hw/intc/arm_gic.c >> > @@ -52,6 +52,87 @@ static inline bool ns_access(void) >> > return true; >> > } >> > >> > +inline void gic_update_with_grouping(GICState *s) >> > +{ >> > + int best_irq; >> > + int best_prio; >> > + int irq; >> > + int irq_level; >> > + int fiq_level; >> > + int cpu; >> > + int cm; >> > + bool next_int; >> > + bool next_grp0; >> > + bool gicc_grp0_enabled; >> > + bool gicc_grp1_enabled; >> > + >> > + for (cpu = 0; cpu < NUM_CPU(s); cpu++) { >> > + cm = 1 << cpu; >> > + gicc_grp0_enabled = s->cpu_control[cpu][0] & >> > GICC_CTLR_S_EN_GRP0; >> > + gicc_grp1_enabled = s->cpu_control[cpu][1] & >> > GICC_CTLR_NS_EN_GRP1; >> > + next_int = 0; >> > + next_grp0 = 0; >> > + >> > + s->current_pending[cpu] = 1023; >> > + if ((!s->enabled_grp[0] && !s->enabled_grp[1]) >> > + || (!gicc_grp0_enabled && !gicc_grp1_enabled)) { >> > + qemu_irq_lower(s->parent_irq[cpu]); >> > + qemu_irq_lower(s->parent_fiq[cpu]); >> > + return; >> >> Shouldn't that be continue? Otherwise the state of CPU[N-1] influences >> whether interrupts can be delivered to CPU[N]. >> >> > I see what you are saying, but historically the code looks like it has > always returned so I'd have to investigate it more as I am still learning > the code myself. If this is a regression, it would be one inherited from > the previous gic_update() function. This does look like an existing bug, and I agree it should be continue (you could optimize the disable IRQ state by using return if the distributor disable all IRQs, but the code complexity cost is probably not worth the optimization for the unusual situation. Thanks, -Christoffer
On 30 October 2014 at 22:12, Greg Bellows <greg.bellows@linaro.org> wrote: > From: Fabian Aggeler <aggelerf@ethz.ch> > > GICs with grouping (GICv2 or GICv1 with Security Extensions) have a > different exception generation model which is more complicated than > without interrupt grouping. We add a new function to handle this model. > > Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch> > > --- > > v1 -> v2 > - Fix issue in gic_update_with_grouping() using the wrong combination of > flag and CPU control bank for checking if group 1 interrupts are enabled. > --- > hw/intc/arm_gic.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++- > hw/intc/gic_internal.h | 1 + > 2 files changed, 87 insertions(+), 1 deletion(-) > > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c > index 808aa18..e33c470 100644 > --- a/hw/intc/arm_gic.c > +++ b/hw/intc/arm_gic.c > @@ -52,6 +52,87 @@ static inline bool ns_access(void) > return true; > } > > +inline void gic_update_with_grouping(GICState *s) > +{ > + int best_irq; > + int best_prio; > + int irq; > + int irq_level; > + int fiq_level; > + int cpu; > + int cm; > + bool next_int; > + bool next_grp0; > + bool gicc_grp0_enabled; > + bool gicc_grp1_enabled; > + > + for (cpu = 0; cpu < NUM_CPU(s); cpu++) { > + cm = 1 << cpu; > + gicc_grp0_enabled = s->cpu_control[cpu][0] & GICC_CTLR_S_EN_GRP0; > + gicc_grp1_enabled = s->cpu_control[cpu][1] & GICC_CTLR_NS_EN_GRP1; > + next_int = 0; > + next_grp0 = 0; > + > + s->current_pending[cpu] = 1023; > + if ((!s->enabled_grp[0] && !s->enabled_grp[1]) > + || (!gicc_grp0_enabled && !gicc_grp1_enabled)) { > + qemu_irq_lower(s->parent_irq[cpu]); > + qemu_irq_lower(s->parent_fiq[cpu]); > + return; > + } > + > + /* Determine highest priority pending interrupt */ > + best_prio = 0x100; > + best_irq = 1023; > + for (irq = 0; irq < s->num_irq; irq++) { > + if (GIC_TEST_ENABLED(irq, cm) && gic_test_pending(s, irq, cm)) { > + if (GIC_GET_PRIORITY(irq, cpu) < best_prio) { > + best_prio = GIC_GET_PRIORITY(irq, cpu); > + best_irq = irq; > + } > + } > + } > + > + /* Priority of IRQ higher than priority mask? */ > + if (best_prio < s->priority_mask[cpu]) { > + s->current_pending[cpu] = best_irq; > + if (GIC_TEST_GROUP0(best_irq, cm) && s->enabled_grp[0]) { > + /* TODO: Add subpriority handling (binary point register) */ > + if (best_prio < s->running_priority[cpu]) { > + next_int = true; > + next_grp0 = true; > + } > + } else if (!GIC_TEST_GROUP0(best_irq, cm) && s->enabled_grp[1]) { > + /* TODO: Add subpriority handling (binary point register) */ > + if (best_prio < s->running_priority[cpu]) { > + next_int = true; > + next_grp0 = false; > + } > + } > + } > + > + fiq_level = 0; > + irq_level = 0; > + if (next_int) { > + if (next_grp0 && (s->cpu_control[cpu][0] & GICC_CTLR_S_FIQ_EN)) { > + if (gicc_grp0_enabled) { > + fiq_level = 1; > + DPRINTF("Raised pending FIQ %d (cpu %d)\n", best_irq, cpu); > + } > + } else { > + if ((next_grp0 && gicc_grp0_enabled) > + || (!next_grp0 && gicc_grp1_enabled)) { > + irq_level = 1; > + DPRINTF("Raised pending IRQ %d (cpu %d)\n", best_irq, cpu); > + } > + } > + } > + /* Set IRQ/FIQ signal */ > + qemu_set_irq(s->parent_irq[cpu], irq_level); > + qemu_set_irq(s->parent_fiq[cpu], fiq_level); > + } > +} I'm not 100% convinced of the benefit of splitting out the "no grouping" and "grouping" code paths (for instance it means this function doesn't have the bugfix from commit b52b81e44f7 to honour the cpu-target-mask). I'll see how I feel when I get to this patch in rework :-) > inline void gic_update_no_grouping(GICState *s) > { > int best_irq; > @@ -95,7 +176,11 @@ inline void gic_update_no_grouping(GICState *s) > /* Update interrupt status after enabled or pending bits have been changed. */ > void gic_update(GICState *s) > { > - gic_update_no_grouping(s); > + if (s->revision >= 2 || s->security_extn) { > + gic_update_with_grouping(s); > + } else { > + gic_update_no_grouping(s); > + } > } > > void gic_set_pending_private(GICState *s, int cpu, int irq) > diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h > index e16a7e5..01859ed 100644 > --- a/hw/intc/gic_internal.h > +++ b/hw/intc/gic_internal.h > @@ -73,6 +73,7 @@ > void gic_set_pending_private(GICState *s, int cpu, int irq); > uint32_t gic_acknowledge_irq(GICState *s, int cpu); > void gic_complete_irq(GICState *s, int cpu, int irq); > +inline void gic_update_with_grouping(GICState *s); static inline, no proto. -- PMM
diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c index 808aa18..e33c470 100644 --- a/hw/intc/arm_gic.c +++ b/hw/intc/arm_gic.c @@ -52,6 +52,87 @@ static inline bool ns_access(void) return true; } +inline void gic_update_with_grouping(GICState *s) +{ + int best_irq; + int best_prio; + int irq; + int irq_level; + int fiq_level; + int cpu; + int cm; + bool next_int; + bool next_grp0; + bool gicc_grp0_enabled; + bool gicc_grp1_enabled; + + for (cpu = 0; cpu < NUM_CPU(s); cpu++) { + cm = 1 << cpu; + gicc_grp0_enabled = s->cpu_control[cpu][0] & GICC_CTLR_S_EN_GRP0; + gicc_grp1_enabled = s->cpu_control[cpu][1] & GICC_CTLR_NS_EN_GRP1; + next_int = 0; + next_grp0 = 0; + + s->current_pending[cpu] = 1023; + if ((!s->enabled_grp[0] && !s->enabled_grp[1]) + || (!gicc_grp0_enabled && !gicc_grp1_enabled)) { + qemu_irq_lower(s->parent_irq[cpu]); + qemu_irq_lower(s->parent_fiq[cpu]); + return; + } + + /* Determine highest priority pending interrupt */ + best_prio = 0x100; + best_irq = 1023; + for (irq = 0; irq < s->num_irq; irq++) { + if (GIC_TEST_ENABLED(irq, cm) && gic_test_pending(s, irq, cm)) { + if (GIC_GET_PRIORITY(irq, cpu) < best_prio) { + best_prio = GIC_GET_PRIORITY(irq, cpu); + best_irq = irq; + } + } + } + + /* Priority of IRQ higher than priority mask? */ + if (best_prio < s->priority_mask[cpu]) { + s->current_pending[cpu] = best_irq; + if (GIC_TEST_GROUP0(best_irq, cm) && s->enabled_grp[0]) { + /* TODO: Add subpriority handling (binary point register) */ + if (best_prio < s->running_priority[cpu]) { + next_int = true; + next_grp0 = true; + } + } else if (!GIC_TEST_GROUP0(best_irq, cm) && s->enabled_grp[1]) { + /* TODO: Add subpriority handling (binary point register) */ + if (best_prio < s->running_priority[cpu]) { + next_int = true; + next_grp0 = false; + } + } + } + + fiq_level = 0; + irq_level = 0; + if (next_int) { + if (next_grp0 && (s->cpu_control[cpu][0] & GICC_CTLR_S_FIQ_EN)) { + if (gicc_grp0_enabled) { + fiq_level = 1; + DPRINTF("Raised pending FIQ %d (cpu %d)\n", best_irq, cpu); + } + } else { + if ((next_grp0 && gicc_grp0_enabled) + || (!next_grp0 && gicc_grp1_enabled)) { + irq_level = 1; + DPRINTF("Raised pending IRQ %d (cpu %d)\n", best_irq, cpu); + } + } + } + /* Set IRQ/FIQ signal */ + qemu_set_irq(s->parent_irq[cpu], irq_level); + qemu_set_irq(s->parent_fiq[cpu], fiq_level); + } +} + inline void gic_update_no_grouping(GICState *s) { int best_irq; @@ -95,7 +176,11 @@ inline void gic_update_no_grouping(GICState *s) /* Update interrupt status after enabled or pending bits have been changed. */ void gic_update(GICState *s) { - gic_update_no_grouping(s); + if (s->revision >= 2 || s->security_extn) { + gic_update_with_grouping(s); + } else { + gic_update_no_grouping(s); + } } void gic_set_pending_private(GICState *s, int cpu, int irq) diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h index e16a7e5..01859ed 100644 --- a/hw/intc/gic_internal.h +++ b/hw/intc/gic_internal.h @@ -73,6 +73,7 @@ void gic_set_pending_private(GICState *s, int cpu, int irq); uint32_t gic_acknowledge_irq(GICState *s, int cpu); void gic_complete_irq(GICState *s, int cpu, int irq); +inline void gic_update_with_grouping(GICState *s); inline void gic_update_no_grouping(GICState *s); void gic_update(GICState *s); void gic_init_irqs_and_distributor(GICState *s);