On 26 August 2014 06:47, Sergey Fedorov <serge.fdrv@gmail.com> wrote:

> 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 <aggelerf@ethz.ch>
> > ---
> >  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.
>

I agree, this needs to be fixed.  I'll fix in the next version.


>
> > +            } 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.
>
>
I don't believe this is properly handling banking.  There should be two
copies of the mask that get decided upon based on the condition criteria,
but their only appears to be a single bank.


> 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
> */
>

I'm confused why this is an array based on the group number?  The register
is banked so I can see why we need two copies, but this appears to be
separate registers for the different groups.  I think it would be cleaner
to have distinct s/ns copies where the content/definition depends on the
condition.

> +    };
> >      bool cpu_enabled[GIC_NCPU];
> >
> >      gic_irq_state irq_state[GIC_MAXIRQ];
>
>