Thanks Daniel, I'll address this in the next version.
On 4 November 2014 08:46, Daniel Thompson <daniel.thomp...@linaro.org>
wrote:
> On 30/10/14 22:12, Greg Bellows wrote:
> > From: Fabian Aggeler <aggel...@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 <aggel...@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];
> >
>
>
