From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53963) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XMFE9-0004Sz-8t for qemu-devel@nongnu.org; Tue, 26 Aug 2014 07:47:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XMFE3-0006ek-LJ for qemu-devel@nongnu.org; Tue, 26 Aug 2014 07:47:53 -0400 Received: from mail-la0-x22a.google.com ([2a00:1450:4010:c03::22a]:62636) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XMFE3-0006eX-8W for qemu-devel@nongnu.org; Tue, 26 Aug 2014 07:47:47 -0400 Received: by mail-la0-f42.google.com with SMTP id pv20so14949319lab.15 for ; Tue, 26 Aug 2014 04:47:45 -0700 (PDT) Message-ID: <53FC73E0.6030905@gmail.com> Date: Tue, 26 Aug 2014 15:47:44 +0400 From: Sergey Fedorov MIME-Version: 1.0 References: <1408703392-23893-1-git-send-email-aggelerf@ethz.ch> <1408703392-23893-7-git-send-email-aggelerf@ethz.ch> In-Reply-To: <1408703392-23893-7-git-send-email-aggelerf@ethz.ch> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 06/15] hw/intc/arm_gic: Make ICDDCR/GICD_CTLR banked List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fabian Aggeler , qemu-devel@nongnu.org Cc: peter.maydell@linaro.org, edgar.iglesias@gmail.com, christoffer.dall@linaro.org, greg.bellows@linaro.org On 22.08.2014 14:29, Fabian Aggeler wrote: > ICDDCR/GICD_CTLR is banked in GICv1 implementations with Security > Extensions or in GICv2 in independent from Security Extensions. > This makes it possible to enable forwarding of interrupts from > Distributor to the CPU interfaces for Group0 and Group1. > > EnableGroup0 (Bit [1]) in GICv1 is IMPDEF. Since this bit (Enable > Non-secure) is present in the integrated IC of the Cortex-A9 MPCore, > which implements the GICv1 profile, we support this bit in GICv1 too. > > Signed-off-by: Fabian Aggeler > --- > hw/intc/arm_gic.c | 34 ++++++++++++++++++++++++++++++---- > hw/intc/arm_gic_common.c | 2 +- > include/hw/intc/arm_gic_common.h | 7 ++++++- > 3 files changed, 37 insertions(+), 6 deletions(-) > > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c > index a972942..c78b301 100644 > --- a/hw/intc/arm_gic.c > +++ b/hw/intc/arm_gic.c > @@ -301,8 +301,18 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset) > cpu = gic_get_current_cpu(s); > cm = 1 << cpu; > if (offset < 0x100) { > - if (offset == 0) > - return s->enabled; > + if (offset == 0) { > + res = 0; > + if ((s->revision == 2 && !s->security_extn) > + || (s->security_extn && !ns_access())) { > + res = (s->enabled_grp[1] << 1) | s->enabled_grp[0]; Seems that (s->enabled_grp[1] << 1) can give wrong value, namely 4 instead of 2. See below. > + } else if (s->security_extn && ns_access()) { > + res = s->enabled_grp[1]; > + } else { > + /* Neither GICv2 nor Security Extensions present */ > + res = s->enabled; > + } > + } > if (offset == 4) > /* Interrupt Controller Type Register */ > return ((s->num_irq / 32) - 1) > @@ -469,8 +479,24 @@ static void gic_dist_writeb(void *opaque, hwaddr offset, > cpu = gic_get_current_cpu(s); > if (offset < 0x100) { > if (offset == 0) { > - s->enabled = (value & 1); > - DPRINTF("Distribution %sabled\n", s->enabled ? "En" : "Dis"); > + if ((s->revision == 2 && !s->security_extn) > + || (s->security_extn && !ns_access())) { > + s->enabled_grp[0] = value & (1U << 0); /* EnableGrp0 */ > + /* For a GICv1 with Security Extn "EnableGrp1" is IMPDEF. */ > + s->enabled_grp[1] = value & (1U << 1); /* EnableGrp1 */ Here s->enabled_grp[1] can get value of 2. That can give wrong result as noted above. Best, Sergey > + DPRINTF("Group0 distribution %sabled\n" > + "Group1 distribution %sabled\n", > + s->enabled_grp[0] ? "En" : "Dis", > + s->enabled_grp[1] ? "En" : "Dis"); > + } else if (s->security_extn && ns_access()) { > + s->enabled_grp[1] = (value & 1U); > + DPRINTF("Group1 distribution %sabled\n", > + s->enabled_grp[1] ? "En" : "Dis"); > + } else { > + /* Neither GICv2 nor Security Extensions present */ > + s->enabled = (value & 1U); > + DPRINTF("Distribution %sabled\n", s->enabled ? "En" : "Dis"); > + } > } else if (offset < 4) { > /* ignored. */ > } else if (offset >= 0x80) { > diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c > index f74175d..7652754 100644 > --- a/hw/intc/arm_gic_common.c > +++ b/hw/intc/arm_gic_common.c > @@ -64,7 +64,7 @@ static const VMStateDescription vmstate_gic = { > .pre_save = gic_pre_save, > .post_load = gic_post_load, > .fields = (VMStateField[]) { > - VMSTATE_BOOL(enabled, GICState), > + VMSTATE_UINT8_ARRAY(enabled_grp, GICState, GIC_NR_GROUP), > VMSTATE_BOOL_ARRAY(cpu_enabled, GICState, GIC_NCPU), > VMSTATE_STRUCT_ARRAY(irq_state, GICState, GIC_MAXIRQ, 1, > vmstate_gic_irq_state, gic_irq_state), > diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h > index a61e52e..a39b066 100644 > --- a/include/hw/intc/arm_gic_common.h > +++ b/include/hw/intc/arm_gic_common.h > @@ -30,6 +30,8 @@ > #define GIC_NR_SGIS 16 > /* Maximum number of possible CPU interfaces, determined by GIC architecture */ > #define GIC_NCPU 8 > +/* Number of Groups (Group0 [Secure], Group1 [Non-secure]) */ > +#define GIC_NR_GROUP 2 > > #define MAX_NR_GROUP_PRIO 128 > #define GIC_NR_APRS (MAX_NR_GROUP_PRIO / 32) > @@ -52,7 +54,10 @@ typedef struct GICState { > > qemu_irq parent_irq[GIC_NCPU]; > qemu_irq parent_fiq[GIC_NCPU]; > - bool enabled; > + union { > + uint8_t enabled; > + uint8_t enabled_grp[GIC_NR_GROUP]; /* EnableGrp0 and EnableGrp1 */ > + }; > bool cpu_enabled[GIC_NCPU]; > > gic_irq_state irq_state[GIC_MAXIRQ];