From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52064) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YpRCN-0003ME-6E for qemu-devel@nongnu.org; Mon, 04 May 2015 20:59:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YpRCJ-0008BK-2e for qemu-devel@nongnu.org; Mon, 04 May 2015 20:58:59 -0400 Received: from mail-ob0-x232.google.com ([2607:f8b0:4003:c01::232]:32956) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YpRCI-0008B1-TS for qemu-devel@nongnu.org; Mon, 04 May 2015 20:58:55 -0400 Received: by obblk2 with SMTP id lk2so80250689obb.0 for ; Mon, 04 May 2015 17:58:54 -0700 (PDT) Date: Tue, 5 May 2015 10:55:27 +1000 From: "Edgar E. Iglesias" Message-ID: <20150505005527.GG10142@toto> References: <1430502643-25909-1-git-send-email-peter.maydell@linaro.org> <1430502643-25909-5-git-send-email-peter.maydell@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1430502643-25909-5-git-send-email-peter.maydell@linaro.org> Subject: Re: [Qemu-devel] [PATCH v4 04/17] hw/intc/arm_gic: Add Interrupt Group Registers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Greg Bellows , qemu-devel@nongnu.org, patches@linaro.org On Fri, May 01, 2015 at 06:50:30PM +0100, Peter Maydell wrote: > From: Fabian Aggeler > > The Interrupt Group Registers allow the guest to configure interrupts > into one of two groups, where Group0 are higher priority and may > be routed to IRQ or FIQ, and Group1 are lower priority and always > routed to IRQ. (In a GIC with the security extensions Group0 is > Secure interrupts and Group 1 is NonSecure.) > The GICv2 always supports interrupt grouping; the GICv1 does only > if it implements the security extensions. > > This patch implements the ability to read and write the registers; > the actual functionality the bits control will be added in a > subsequent patch. > > Signed-off-by: Fabian Aggeler > Signed-off-by: Greg Bellows > Message-id: 1429113742-8371-7-git-send-email-greg.bellows@linaro.org > [PMM: bring GIC_*_GROUP macros into line with the others, ie a > simple SET/CLEAR/TEST rather than GROUP0/GROUP1; > utility gic_has_groups() function; > minor style fixes; > bump vmstate version] > Signed-off-by: Peter Maydell I see now why we are doing the mask thing on the group bits to support the banking of private interrupts... The comment, /* Group bits are banked for private interrupts */ would have been more useful to me close to the definition of the GIC_SET_GROUP macros but that may just be me... Reviewed-by: Edgar E. Iglesias > --- > hw/intc/arm_gic.c | 50 +++++++++++++++++++++++++++++++++++++--- > hw/intc/arm_gic_common.c | 5 ++-- > hw/intc/gic_internal.h | 4 ++++ > include/hw/intc/arm_gic_common.h | 1 + > 4 files changed, 55 insertions(+), 5 deletions(-) > > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c > index 29015c2..1aa4520 100644 > --- a/hw/intc/arm_gic.c > +++ b/hw/intc/arm_gic.c > @@ -45,6 +45,14 @@ static inline int gic_get_current_cpu(GICState *s) > return 0; > } > > +/* Return true if this GIC config has interrupt groups, which is > + * true if we're a GICv2, or a GICv1 with the security extensions. > + */ > +static inline bool gic_has_groups(GICState *s) > +{ > + return s->revision == 2 || s->security_extn; > +} > + > /* TODO: Many places that call this routine could be optimized. */ > /* Update interrupt status after enabled or pending bits have been changed. */ > void gic_update(GICState *s) > @@ -305,8 +313,24 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs) > if (offset < 0x08) > return 0; > if (offset >= 0x80) { > - /* Interrupt Security , RAZ/WI */ > - return 0; > + /* Interrupt Group Registers: these RAZ/WI if this is an NS > + * access to a GIC with the security extensions, or if the GIC > + * doesn't have groups at all. > + */ > + res = 0; > + if (!(s->security_extn && !attrs.secure) && gic_has_groups(s)) { > + /* Every byte offset holds 8 group status bits */ > + irq = (offset - 0x080) * 8 + GIC_BASE_IRQ; > + if (irq >= s->num_irq) { > + goto bad_reg; > + } > + for (i = 0; i < 8; i++) { > + if (GIC_TEST_GROUP(irq + i, cm)) { > + res |= (1 << i); > + } > + } > + } > + return res; > } > goto bad_reg; > } else if (offset < 0x200) { > @@ -456,7 +480,27 @@ static void gic_dist_writeb(void *opaque, hwaddr offset, > } else if (offset < 4) { > /* ignored. */ > } else if (offset >= 0x80) { > - /* Interrupt Security Registers, RAZ/WI */ > + /* Interrupt Group Registers: RAZ/WI for NS access to secure > + * GIC, or for GICs without groups. > + */ > + if (!(s->security_extn && !attrs.secure) && gic_has_groups(s)) { > + /* Every byte offset holds 8 group status bits */ > + irq = (offset - 0x80) * 8 + GIC_BASE_IRQ; > + if (irq >= s->num_irq) { > + goto bad_reg; > + } > + for (i = 0; i < 8; i++) { > + /* Group bits are banked for private interrupts */ > + int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK; > + if (value & (1 << i)) { > + /* Group1 (Non-secure) */ > + GIC_SET_GROUP(irq + i, cm); > + } else { > + /* Group0 (Secure) */ > + GIC_CLEAR_GROUP(irq + i, cm); > + } > + } > + } > } else { > goto bad_reg; > } > diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c > index 5ed21f1..b5a85e5 100644 > --- a/hw/intc/arm_gic_common.c > +++ b/hw/intc/arm_gic_common.c > @@ -52,14 +52,15 @@ static const VMStateDescription vmstate_gic_irq_state = { > VMSTATE_UINT8(level, gic_irq_state), > VMSTATE_BOOL(model, gic_irq_state), > VMSTATE_BOOL(edge_trigger, gic_irq_state), > + VMSTATE_UINT8(group, gic_irq_state), > VMSTATE_END_OF_LIST() > } > }; > > static const VMStateDescription vmstate_gic = { > .name = "arm_gic", > - .version_id = 7, > - .minimum_version_id = 7, > + .version_id = 8, > + .minimum_version_id = 8, > .pre_save = gic_pre_save, > .post_load = gic_post_load, > .fields = (VMStateField[]) { > diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h > index e87ef36..e8cf773 100644 > --- a/hw/intc/gic_internal.h > +++ b/hw/intc/gic_internal.h > @@ -50,6 +50,10 @@ > s->priority1[irq][cpu] : \ > s->priority2[(irq) - GIC_INTERNAL]) > #define GIC_TARGET(irq) s->irq_target[irq] > +#define GIC_CLEAR_GROUP(irq, cm) (s->irq_state[irq].group &= ~(cm)) > +#define GIC_SET_GROUP(irq, cm) (s->irq_state[irq].group |= (cm)) > +#define GIC_TEST_GROUP(irq, cm) ((s->irq_state[irq].group & (cm)) != 0) > + > > /* The special cases for the revision property: */ > #define REV_11MPCORE 0 > diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h > index 7825134..b78981e 100644 > --- a/include/hw/intc/arm_gic_common.h > +++ b/include/hw/intc/arm_gic_common.h > @@ -42,6 +42,7 @@ typedef struct gic_irq_state { > uint8_t level; > bool model; /* 0 = N:N, 1 = 1:N */ > bool edge_trigger; /* true: edge-triggered, false: level-triggered */ > + uint8_t group; > } gic_irq_state; > > typedef struct GICState { > -- > 1.9.1 >