Re: [Qemu-devel] [PATCH v4 04/17] hw/intc/arm_gic: Add Interrupt Group Registers

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

On Fri, May 01, 2015 at 06:50:30PM +0100, Peter Maydell wrote:
> From: Fabian Aggeler <aggelerf@ethz.ch>
> 
> The Interrupt Group Registers allow the guest to configure interrupts
> into one of two groups, where Group0 are higher priority and may
> be routed to IRQ or FIQ, and Group1 are lower priority and always
> routed to IRQ. (In a GIC with the security extensions Group0 is
> Secure interrupts and Group 1 is NonSecure.)
> The GICv2 always supports interrupt grouping; the GICv1 does only
> if it implements the security extensions.
> 
> This patch implements the ability to read and write the registers;
> the actual functionality the bits control will be added in a
> subsequent patch.
> 
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> Message-id: 1429113742-8371-7-git-send-email-greg.bellows@linaro.org
> [PMM: bring GIC_*_GROUP macros into line with the others, ie a
>  simple SET/CLEAR/TEST rather than GROUP0/GROUP1;
>  utility gic_has_groups() function;
>  minor style fixes;
>  bump vmstate version]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

I see now why we are doing the mask thing on the group bits to support the
banking of private interrupts...
The comment,
/* Group bits are banked for private interrupts */
would have been more useful to me close to the definition of the GIC_SET_GROUP
macros but that may just be me...

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  hw/intc/arm_gic.c                | 50 +++++++++++++++++++++++++++++++++++++---
>  hw/intc/arm_gic_common.c         |  5 ++--
>  hw/intc/gic_internal.h           |  4 ++++
>  include/hw/intc/arm_gic_common.h |  1 +
>  4 files changed, 55 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 29015c2..1aa4520 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -45,6 +45,14 @@ static inline int gic_get_current_cpu(GICState *s)
>      return 0;
>  }
>  
> +/* Return true if this GIC config has interrupt groups, which is
> + * true if we're a GICv2, or a GICv1 with the security extensions.
> + */
> +static inline bool gic_has_groups(GICState *s)
> +{
> +    return s->revision == 2 || s->security_extn;
> +}
> +
>  /* TODO: Many places that call this routine could be optimized.  */
>  /* Update interrupt status after enabled or pending bits have been changed.  */
>  void gic_update(GICState *s)
> @@ -305,8 +313,24 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs)
>          if (offset < 0x08)
>              return 0;
>          if (offset >= 0x80) {
> -            /* Interrupt Security , RAZ/WI */
> -            return 0;
> +            /* Interrupt Group Registers: these RAZ/WI if this is an NS
> +             * access to a GIC with the security extensions, or if the GIC
> +             * doesn't have groups at all.
> +             */
> +            res = 0;
> +            if (!(s->security_extn && !attrs.secure) && gic_has_groups(s)) {
> +                /* 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++) {
> +                    if (GIC_TEST_GROUP(irq + i, cm)) {
> +                        res |= (1 << i);
> +                    }
> +                }
> +            }
> +            return res;
>          }
>          goto bad_reg;
>      } else if (offset < 0x200) {
> @@ -456,7 +480,27 @@ 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: RAZ/WI for NS access to secure
> +             * GIC, or for GICs without groups.
> +             */
> +            if (!(s->security_extn && !attrs.secure) && gic_has_groups(s)) {
> +                /* Every byte offset holds 8 group status bits */
> +                irq = (offset - 0x80) * 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 */
> +                    int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
> +                    if (value & (1 << i)) {
> +                        /* Group1 (Non-secure) */
> +                        GIC_SET_GROUP(irq + i, cm);
> +                    } else {
> +                        /* Group0 (Secure) */
> +                        GIC_CLEAR_GROUP(irq + i, cm);
> +                    }
> +                }
> +            }
>          } else {
>              goto bad_reg;
>          }
> diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
> index 5ed21f1..b5a85e5 100644
> --- a/hw/intc/arm_gic_common.c
> +++ b/hw/intc/arm_gic_common.c
> @@ -52,14 +52,15 @@ 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),
>          VMSTATE_END_OF_LIST()
>      }
>  };
>  
>  static const VMStateDescription vmstate_gic = {
>      .name = "arm_gic",
> -    .version_id = 7,
> -    .minimum_version_id = 7,
> +    .version_id = 8,
> +    .minimum_version_id = 8,
>      .pre_save = gic_pre_save,
>      .post_load = gic_post_load,
>      .fields = (VMStateField[]) {
> diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
> index e87ef36..e8cf773 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_CLEAR_GROUP(irq, cm) (s->irq_state[irq].group &= ~(cm))
> +#define GIC_SET_GROUP(irq, cm) (s->irq_state[irq].group |= (cm))
> +#define GIC_TEST_GROUP(irq, cm) ((s->irq_state[irq].group & (cm)) != 0)
> +
>  
>  /* 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.9.1
>