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.thomp...@linaro.org>
wrote:
> On 30/10/14 22:12, Greg Bellows wrote:
> > From: Fabian Aggeler <aggel...@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 <aggel...@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);
> >
>
>
