Re: [Qemu-devel] [PATCH v3 06/16] hw/intc/arm_gic: Add Interrupt Group Registers

[Edgar E. Iglesias]

On Wed, Apr 15, 2015 at 11:02:12AM -0500, Greg Bellows wrote:
> From: Fabian Aggeler <address@hidden>
> 
> 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 <address@hidden>
> Signed-off-by: Greg Bellows <address@hidden>
> 
> ---
> 
> 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 e0bce6e..aa4402e 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)) {
> +                /* 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_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)*/
> +                    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);
> +                    }

This looks weird to me...
Shouldn't this simply be s->irq_state[irq].group = value & (1 << i) ?
(possibly behind macros allthough I must say these macros are confusing)

> +                }
> +            }
>          } 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),
>          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))


Can we make the group a bool or some integer?
An interrupt can only belong to one group at a time AFAIK, so we don't really 
need bit masking...
Or am I missing something?

Cheers,
Edgar


> +#define GIC_TEST_GROUP0(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.8.3.2
> 
>