From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60312) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yi6gC-0004Gq-QH for qemu-devel@nongnu.org; Tue, 14 Apr 2015 15:39:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yi6g9-00034E-GO for qemu-devel@nongnu.org; Tue, 14 Apr 2015 15:39:28 -0400 Received: from mail-ig0-f178.google.com ([209.85.213.178]:38056) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yi6g9-000349-Au for qemu-devel@nongnu.org; Tue, 14 Apr 2015 15:39:25 -0400 Received: by igbqf9 with SMTP id qf9so23892917igb.1 for ; Tue, 14 Apr 2015 12:39:24 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1414707132-24588-17-git-send-email-greg.bellows@linaro.org> References: <1414707132-24588-1-git-send-email-greg.bellows@linaro.org> <1414707132-24588-17-git-send-email-greg.bellows@linaro.org> From: Peter Maydell Date: Tue, 14 Apr 2015 20:39:04 +0100 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH v2 16/16] hw/intc/arm_gic: add gic_update() for grouping List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Greg Bellows Cc: Sergey Fedorov , Daniel Thompson , QEMU Developers , Christoffer Dall , "Edgar E. Iglesias" On 30 October 2014 at 22:12, Greg Bellows wrote: > From: Fabian Aggeler > > 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 > > --- > > 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