[v2,06/16] hw/intc/arm_gic: Add Interrupt Group Registers

From: Fabian Aggeler <aggelerf@ethz.ch>

Interrupt Group Registers (previously called Interrupt Security
Registers) as defined in GICv1 with Security Extensions or GICv2 allow
to configure interrupts as Secure (Group0) or Non-secure (Group1).
In GICv2 these registers are implemented independent of the existence of
Security Extensions.

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>

---

v1 -> v2
- Add clarifying comments to gic_dist_readb/writeb on interrupt group register
  update
- Swap GIC_SET_GROUP0/1 macro logic.  Setting the irq_state.group field for
  group 0 should clear the bit not set it.  Similarly, setting the field for
  group 1 should set the bit not clear it.
---
 hw/intc/arm_gic.c                | 49 +++++++++++++++++++++++++++++++++++++---
 hw/intc/arm_gic_common.c         |  1 +
 hw/intc/gic_internal.h           |  4 ++++
 include/hw/intc/arm_gic_common.h |  1 +
 4 files changed, 52 insertions(+), 3 deletions(-)

On 30 October 2014 at 22:12, Greg Bellows <greg.bellows@linaro.org> wrote:
> From: Fabian Aggeler <aggelerf@ethz.ch>
>
> Interrupt Group Registers (previously called Interrupt Security
> Registers) as defined in GICv1 with Security Extensions or GICv2 allow
> to configure interrupts as Secure (Group0) or Non-secure (Group1).
> In GICv2 these registers are implemented independent of the existence of
> Security Extensions.

Worth mentioning in the commit that this patch only
implements the register accessors, not the functionality
that the bits control.

> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
>
> ---
>
> v1 -> v2
> - Add clarifying comments to gic_dist_readb/writeb on interrupt group register
>   update
> - Swap GIC_SET_GROUP0/1 macro logic.  Setting the irq_state.group field for
>   group 0 should clear the bit not set it.  Similarly, setting the field for
>   group 1 should set the bit not clear it.
> ---
>  hw/intc/arm_gic.c                | 49 +++++++++++++++++++++++++++++++++++++---
>  hw/intc/arm_gic_common.c         |  1 +
>  hw/intc/gic_internal.h           |  4 ++++
>  include/hw/intc/arm_gic_common.h |  1 +
>  4 files changed, 52 insertions(+), 3 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index bee71a1..36ac188 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -312,8 +312,27 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset)
>          if (offset < 0x08)
>              return 0;
>          if (offset >= 0x80) {
> -            /* Interrupt Security , RAZ/WI */
> -            return 0;
> +            /* Interrupt Group Registers
> +             *
> +             * For GIC with Security Extn and Non-secure access RAZ/WI
> +             * For GICv1 without Security Extn RAZ/WI
> +             */
> +            res = 0;
> +            if (!(s->security_extn && ns_access()) &&
> +                    ((s->revision == 1 && s->security_extn)
> +                            || s->revision == 2)) {

It would probably be clearer to write this as
   if (whatever) {
       return 0;
   }
   if (whatever) {
       return 0;
   }
   logic for registers;

rather than inverting the conditions from their more
natural and readable sense.

Also I suspect we will want some utility functions. One
that springs to mind here would be a gic_has_groups()
which returns (gic is v2 || (gic is v1 && has security extns)).

> +                /* Every byte offset holds 8 group status bits */
> +                irq = (offset - 0x080) * 8 + GIC_BASE_IRQ;

Better written as 0x80 I think.

> +                if (irq >= s->num_irq) {
> +                    goto bad_reg;
> +                }
> +                for (i = 0; i < 8; i++) {
> +                    if (!GIC_TEST_GROUP0(irq + i, cm)) {
> +                        res |= (1 << i);
> +                    }
> +                }
> +            }
> +            return res;
>          }
>          goto bad_reg;
>      } else if (offset < 0x200) {
> @@ -457,7 +476,31 @@ 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
> +             *
> +             * For GIC with Security Extn and Non-secure access RAZ/WI
> +             * For GICv1 without Security Extn RAZ/WI
> +             */
> +            if (!(s->security_extn && ns_access()) &&
> +                    ((s->revision == 1 && s->security_extn)
> +                            || s->revision == 2)) {
> +                /* 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++) {
> +                    /* Group bits are banked for private interrupts (internal)*/

Missing trailing space before */.

> +                    int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
> +                    if (value & (1 << i)) {
> +                        /* Group1 (Non-secure) */
> +                        GIC_SET_GROUP1(irq + i, cm);
> +                    } else {
> +                        /* Group0 (Secure) */
> +                        GIC_SET_GROUP0(irq + i, cm);
> +                    }
> +                }
> +            }
>          } else {
>              goto bad_reg;
>          }
> diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
> index e35049d..28f3b2a 100644
> --- a/hw/intc/arm_gic_common.c
> +++ b/hw/intc/arm_gic_common.c
> @@ -52,6 +52,7 @@ 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),

We want to bump the vmstate version at some point in this
series, but if we're making several changes to the structure
I guess we can do that last.

>          VMSTATE_END_OF_LIST()
>      }
>  };
> diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
> index e87ef36..f01955a 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_SET_GROUP0(irq, cm) (s->irq_state[irq].group &= ~(cm))
> +#define GIC_SET_GROUP1(irq, cm) (s->irq_state[irq].group |= (cm))
> +#define GIC_TEST_GROUP0(irq, cm) ((s->irq_state[irq].group & (cm)) == 0)

This is confusing (especially the way the TEST macro
is doing an == 0 comparison). I think we should just have
GIC_SET_GROUP/GIC_CLEAR_GROUP/GIC_TEST_GROUP macros,
and not try to include the idea of "bit set means
group 1" in the macro names.

> +
>
>  /* 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.8.3.2
>

-- PMM

On 14 April 2015 at 20:13, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 30 October 2014 at 22:12, Greg Bellows <greg.bellows@linaro.org> wrote:
>> From: Fabian Aggeler <aggelerf@ethz.ch>
>>
>> Interrupt Group Registers (previously called Interrupt Security
>> Registers) as defined in GICv1 with Security Extensions or GICv2 allow
>> to configure interrupts as Secure (Group0) or Non-secure (Group1).
>> In GICv2 these registers are implemented independent of the existence of
>> Security Extensions.

I've just noticed that these are actually already implemented
in KVM's in-kernel GICv2 model and exposed to userspace, so
we should have another patch which makes them be properly
handled in kvm_arm_gic_get/put.

Other than that I think the only KVM-related change we need
is going to be to give the arm_gic_kvm a 'secure' property
(which gives a realize error if it's set to 'true'), purely
to maintain consistency of interface between it and the
fully-emulated GICv2. (The view you get from a KVM vGIC
is basically a v2 GIC with grouping but no TZ support.)

-- PMM