| Message ID | 1414707132-24588-8-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> > > 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 <aggelerf@ethz.ch> > > --- > > v1 -> v2 > - Fix gic_dist_writeb() update of GICD_CTRL to only use bit[0] of the > EnableGrp1 field not bit[1]. > - Add clarifying comments > --- > hw/intc/arm_gic.c | 49 ++++++++++++++++++++++++++++++++++++---- > hw/intc/arm_gic_common.c | 2 +- > include/hw/intc/arm_gic_common.h | 7 +++++- > 3 files changed, 52 insertions(+), 6 deletions(-) > > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c > index 36ac188..1db15aa 100644 > --- a/hw/intc/arm_gic.c > +++ b/hw/intc/arm_gic.c > @@ -302,8 +302,25 @@ 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) { /* GICD_CTLR */ > + res = 0; > + if ((s->revision == 2 && !s->security_extn) > + || (s->security_extn && !ns_access())) { > + /* In this case the GICD_CTRL contains both a group0 and group1 > + * enable bit, so we create the resuling value by aggregating > + * the bits from the two enable values. > + * The group0 enable bit is only visible to secure accesses. > + * The group1 enable bit (bit[1]) is an alias of bit[0] in > + * the non-secure copy (enabled_grp[1]). > + */ > + res = (s->enabled_grp[1] << 1) | s->enabled_grp[0]; > + } else if (s->security_extn && ns_access()) { > + res = s->enabled_grp[1]; > + } else { > + /* Neither GICv2 nor Security Extensions present */ > + res = s->enabled; > + } return res; is missing here. > + } > if (offset == 4) > /* Interrupt Controller Type Register */ > return ((s->num_irq / 32) - 1) > @@ -471,8 +488,32 @@ 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. */ > + /* We only use the first bit of the enabled_grp vars to > + * indicate enabled or disabled. In this case we have to shift > + * the incoming value down to the low bit because the group1 > + * enabled bit is bit[1] in the secure/GICv2 GICD_CTLR.. > + */ > + s->enabled_grp[1] = (value >> 1) & 0x1; /* EnableGrp1 */ > + 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()) { > + /* If we are non-secure only the group1 enable bit is visible > + * as bit[0] in the GICD_CTLR. > + */ > + s->enabled_grp[1] = (value & 0x1); > + DPRINTF("Group1 distribution %sabled\n", > + s->enabled_grp[1] ? "En" : "Dis"); > + } else { > + /* Neither GICv2 nor Security Extensions present */ > + s->enabled = (value & 0x1); > + 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 28f3b2a..c44050d 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 b78981e..16e193d 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]; >
Thanks Daniel, I'll address this in the next version. On 4 November 2014 08:46, Daniel Thompson <daniel.thompson@linaro.org> wrote: > On 30/10/14 22:12, Greg Bellows wrote: > > From: Fabian Aggeler <aggelerf@ethz.ch> > > > > 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 <aggelerf@ethz.ch> > > > > --- > > > > v1 -> v2 > > - Fix gic_dist_writeb() update of GICD_CTRL to only use bit[0] of the > > EnableGrp1 field not bit[1]. > > - Add clarifying comments > > --- > > hw/intc/arm_gic.c | 49 > ++++++++++++++++++++++++++++++++++++---- > > hw/intc/arm_gic_common.c | 2 +- > > include/hw/intc/arm_gic_common.h | 7 +++++- > > 3 files changed, 52 insertions(+), 6 deletions(-) > > > > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c > > index 36ac188..1db15aa 100644 > > --- a/hw/intc/arm_gic.c > > +++ b/hw/intc/arm_gic.c > > @@ -302,8 +302,25 @@ 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) { /* GICD_CTLR */ > > + res = 0; > > + if ((s->revision == 2 && !s->security_extn) > > + || (s->security_extn && !ns_access())) { > > + /* In this case the GICD_CTRL contains both a group0 > and group1 > > + * enable bit, so we create the resuling value by > aggregating > > + * the bits from the two enable values. > > + * The group0 enable bit is only visible to secure > accesses. > > + * The group1 enable bit (bit[1]) is an alias of bit[0] > in > > + * the non-secure copy (enabled_grp[1]). > > + */ > > + res = (s->enabled_grp[1] << 1) | s->enabled_grp[0]; > > + } else if (s->security_extn && ns_access()) { > > + res = s->enabled_grp[1]; > > + } else { > > + /* Neither GICv2 nor Security Extensions present */ > > + res = s->enabled; > > + } > > return res; is missing here. > > > > + } > > if (offset == 4) > > /* Interrupt Controller Type Register */ > > return ((s->num_irq / 32) - 1) > > @@ -471,8 +488,32 @@ 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. */ > > + /* We only use the first bit of the enabled_grp vars to > > + * indicate enabled or disabled. In this case we have > to shift > > + * the incoming value down to the low bit because the > group1 > > + * enabled bit is bit[1] in the secure/GICv2 GICD_CTLR.. > > + */ > > + s->enabled_grp[1] = (value >> 1) & 0x1; /* EnableGrp1 */ > > + 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()) { > > + /* If we are non-secure only the group1 enable bit is > visible > > + * as bit[0] in the GICD_CTLR. > > + */ > > + s->enabled_grp[1] = (value & 0x1); > > + DPRINTF("Group1 distribution %sabled\n", > > + s->enabled_grp[1] ? "En" : "Dis"); > > + } else { > > + /* Neither GICv2 nor Security Extensions present */ > > + s->enabled = (value & 0x1); > > + 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 28f3b2a..c44050d 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 b78981e..16e193d 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]; > > > >
On 30 October 2014 at 22:12, Greg Bellows <greg.bellows@linaro.org> wrote: > From: Fabian Aggeler <aggelerf@ethz.ch> > > 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 <aggelerf@ethz.ch> > > --- > > v1 -> v2 > - Fix gic_dist_writeb() update of GICD_CTRL to only use bit[0] of the > EnableGrp1 field not bit[1]. > - Add clarifying comments > --- > hw/intc/arm_gic.c | 49 ++++++++++++++++++++++++++++++++++++---- > hw/intc/arm_gic_common.c | 2 +- > include/hw/intc/arm_gic_common.h | 7 +++++- > 3 files changed, 52 insertions(+), 6 deletions(-) > > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c > index 36ac188..1db15aa 100644 > --- a/hw/intc/arm_gic.c > +++ b/hw/intc/arm_gic.c > @@ -302,8 +302,25 @@ 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) { /* GICD_CTLR */ > + res = 0; > + if ((s->revision == 2 && !s->security_extn) > + || (s->security_extn && !ns_access())) { > + /* In this case the GICD_CTRL contains both a group0 and group1 Typo: should be _CTLR. > + * enable bit, so we create the resuling value by aggregating > + * the bits from the two enable values. > + * The group0 enable bit is only visible to secure accesses. > + * The group1 enable bit (bit[1]) is an alias of bit[0] in > + * the non-secure copy (enabled_grp[1]). > + */ > + res = (s->enabled_grp[1] << 1) | s->enabled_grp[0]; > + } 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) > @@ -471,8 +488,32 @@ 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. */ > + /* We only use the first bit of the enabled_grp vars to > + * indicate enabled or disabled. In this case we have to shift > + * the incoming value down to the low bit because the group1 > + * enabled bit is bit[1] in the secure/GICv2 GICD_CTLR.. Typo: repeated '.'. > + */ > + s->enabled_grp[1] = (value >> 1) & 0x1; /* EnableGrp1 */ > + 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()) { > + /* If we are non-secure only the group1 enable bit is visible > + * as bit[0] in the GICD_CTLR. > + */ > + s->enabled_grp[1] = (value & 0x1); > + DPRINTF("Group1 distribution %sabled\n", > + s->enabled_grp[1] ? "En" : "Dis"); > + } else { > + /* Neither GICv2 nor Security Extensions present */ > + s->enabled = (value & 0x1); > + 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 28f3b2a..c44050d 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 b78981e..16e193d 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 */ > + }; Rather than a union, we should just make the code use enabled_grp[0] when there's no grouping enabled. > bool cpu_enabled[GIC_NCPU]; > > gic_irq_state irq_state[GIC_MAXIRQ]; > -- > 1.8.3.2 > -- PMM
diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c index 36ac188..1db15aa 100644 --- a/hw/intc/arm_gic.c +++ b/hw/intc/arm_gic.c @@ -302,8 +302,25 @@ 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) { /* GICD_CTLR */ + res = 0; + if ((s->revision == 2 && !s->security_extn) + || (s->security_extn && !ns_access())) { + /* In this case the GICD_CTRL contains both a group0 and group1 + * enable bit, so we create the resuling value by aggregating + * the bits from the two enable values. + * The group0 enable bit is only visible to secure accesses. + * The group1 enable bit (bit[1]) is an alias of bit[0] in + * the non-secure copy (enabled_grp[1]). + */ + res = (s->enabled_grp[1] << 1) | s->enabled_grp[0]; + } 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) @@ -471,8 +488,32 @@ 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. */ + /* We only use the first bit of the enabled_grp vars to + * indicate enabled or disabled. In this case we have to shift + * the incoming value down to the low bit because the group1 + * enabled bit is bit[1] in the secure/GICv2 GICD_CTLR.. + */ + s->enabled_grp[1] = (value >> 1) & 0x1; /* EnableGrp1 */ + 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()) { + /* If we are non-secure only the group1 enable bit is visible + * as bit[0] in the GICD_CTLR. + */ + s->enabled_grp[1] = (value & 0x1); + DPRINTF("Group1 distribution %sabled\n", + s->enabled_grp[1] ? "En" : "Dis"); + } else { + /* Neither GICv2 nor Security Extensions present */ + s->enabled = (value & 0x1); + 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 28f3b2a..c44050d 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 b78981e..16e193d 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];