Re: [Qemu-devel] [PATCH v2 08/16] hw/intc/arm_gic: Make ICCICR/GICC_CTLR banked

[Peter Maydell]

On 30 October 2014 at 22:12, Greg Bellows <address@hidden> wrote:
> From: Fabian Aggeler <address@hidden>
>
> ICCICR/GICC_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
> the CPU interfaces to the connected processors for Group0 and Group1.
>
> We also allow to set additional bits like AckCtl and FIQEn by changing
> the type from bool to uint32. Since the field does not only store the
> enable bit anymore and since we are touching the vmstate, we use the
> opportunity to rename the field to cpu_control.
>
> Signed-off-by: Fabian Aggeler <address@hidden>
>
> ---
>
> v1 -> v2
> - Rework gic_set_cpu_control() and gic_get_cpu_control() to close gap on
>   handling GICv1 wihtout security extensions.
> - Fix use of incorrect control index in update.
> ---
>  hw/intc/arm_gic.c                | 82 
> +++++++++++++++++++++++++++++++++++++---
>  hw/intc/arm_gic_common.c         |  5 ++-
>  hw/intc/arm_gic_kvm.c            |  8 ++--
>  hw/intc/armv7m_nvic.c            |  2 +-
>  hw/intc/gic_internal.h           | 14 +++++++
>  include/hw/intc/arm_gic_common.h |  2 +-
>  6 files changed, 100 insertions(+), 13 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 1db15aa..3c0414f 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -66,7 +66,7 @@ void gic_update(GICState *s)
>      for (cpu = 0; cpu < NUM_CPU(s); cpu++) {
>          cm = 1 << cpu;
>          s->current_pending[cpu] = 1023;
> -        if (!s->enabled || !s->cpu_enabled[cpu]) {
> +        if (!s->enabled || !(s->cpu_control[cpu][1] & 1)) {
>              qemu_irq_lower(s->parent_irq[cpu]);
>              return;
>          }
> @@ -240,6 +240,80 @@ void gic_set_priority(GICState *s, int cpu, int irq, 
> uint8_t val)
>      }
>  }
>
> +uint32_t gic_get_cpu_control(GICState *s, int cpu)
> +{
> +    uint32_t ret;
> +
> +    if (!s->security_extn) {
> +        if (s->revision == 1) {
> +            ret = s->cpu_control[cpu][1];
> +            ret &= 0x1;     /* Mask of reserved bits */
> +        } else {
> +            ret = s->cpu_control[cpu][0];
> +            ret &= GICC_CTLR_S_MASK;   /* Mask of reserved bits */
> +        }
> +    } else {
> +        if (ns_access()) {
> +            ret = s->cpu_control[cpu][1];
> +            ret &= GICC_CTLR_NS_MASK;   /* Mask of reserved bits */
> +            if (s->revision == 1) {
> +                ret &= 0x1; /* Mask of reserved bits */
> +            }
> +        } else {
> +            ret = s->cpu_control[cpu][0];
> +            ret &= GICC_CTLR_S_MASK;   /* Mask of reserved bits */
> +        }
> +    }
> +
> +    return ret;
> +}
> +
> +void gic_set_cpu_control(GICState *s, int cpu, uint32_t value)
> +{
> +    if (!s->security_extn) {
> +        if (s->revision == 1) {
> +            s->cpu_control[cpu][1] = value & 0x1;
> +            DPRINTF("CPU Interface %d %sabled\n", cpu,
> +                s->cpu_control[cpu][1] ? "En" : "Dis");
> +        } else {
> +            /* Write to Secure instance of the register */
> +            s->cpu_control[cpu][0] = value & GICC_CTLR_S_MASK;
> +            /* Synchronize EnableGrp1 alias of Non-secure copy */
> +            s->cpu_control[cpu][1] &= ~GICC_CTLR_NS_EN_GRP1;
> +            s->cpu_control[cpu][1] |=
> +                (value & GICC_CTLR_S_EN_GRP1) ? GICC_CTLR_NS_EN_GRP1 : 0;
> +            DPRINTF("CPU Interface %d: Group0 Interrupts %sabled, "
> +                "Group1 Interrupts %sabled\n", cpu,
> +                (s->cpu_control[cpu][0] & GICC_CTLR_S_EN_GRP0) ? "En" : 
> "Dis",
> +                (s->cpu_control[cpu][0] & GICC_CTLR_S_EN_GRP1) ? "En" : 
> "Dis");
> +        }
> +    } else {
> +        if (ns_access()) {
> +            if (s->revision == 1) {
> +                s->cpu_control[cpu][1] = value & 0x1;
> +                DPRINTF("CPU Interface %d %sabled\n", cpu,
> +                    s->cpu_control[cpu][1] ? "En" : "Dis");
> +            } else {
> +                /* Write to Non-secure instance of the register */
> +                s->cpu_control[cpu][1] = value & GICC_CTLR_NS_MASK;
> +                /* Synchronize EnableGrp1 alias of Secure copy */
> +                s->cpu_control[cpu][0] &= ~GICC_CTLR_S_EN_GRP1;
> +                s->cpu_control[cpu][0] |=
> +                    (value & GICC_CTLR_NS_EN_GRP1) ? GICC_CTLR_S_EN_GRP1 : 0;
> +            }
> +            DPRINTF("CPU Interface %d: Group1 Interrupts %sabled\n", cpu,
> +                (s->cpu_control[cpu][1] & GICC_CTLR_NS_EN_GRP1) ? "En" : 
> "Dis");
> +        } else {
> +            /* Write to Secure instance of the register */
> +            s->cpu_control[cpu][0] = value & GICC_CTLR_S_MASK;
> +            /* Synchronize EnableGrp1 alias of Non-secure copy */
> +            s->cpu_control[cpu][1] &= ~GICC_CTLR_NS_EN_GRP1;
> +            s->cpu_control[cpu][1] |=
> +                (value & GICC_CTLR_S_EN_GRP1) ? GICC_CTLR_NS_EN_GRP1 : 0;
> +        }
> +    }
> +}

These feel a bit verbose -- I shall look to see if there's any
simplification possible here.

> +
>  void gic_complete_irq(GICState *s, int cpu, int irq)
>  {
>      int update = 0;
> @@ -762,7 +836,7 @@ static uint32_t gic_cpu_read(GICState *s, int cpu, int 
> offset)
>  {
>      switch (offset) {
>      case 0x00: /* Control */
> -        return s->cpu_enabled[cpu];
> +        return gic_get_cpu_control(s, cpu);
>      case 0x04: /* Priority mask */
>          return s->priority_mask[cpu];
>      case 0x08: /* Binary Point */
> @@ -788,9 +862,7 @@ static void gic_cpu_write(GICState *s, int cpu, int 
> offset, uint32_t value)
>  {
>      switch (offset) {
>      case 0x00: /* Control */
> -        s->cpu_enabled[cpu] = (value & 1);
> -        DPRINTF("CPU %d %sabled\n", cpu, s->cpu_enabled[cpu] ? "En" : "Dis");
> -        break;
> +        return gic_set_cpu_control(s, cpu, value);

gic_set_cpu_control() has a 'void' return type, so you
can't use it like this.

>      case 0x04: /* Priority mask */
>          s->priority_mask[cpu] = (value & 0xff);
>          break;
> diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
> index c44050d..e225f2b 100644
> --- a/hw/intc/arm_gic_common.c
> +++ b/hw/intc/arm_gic_common.c
> @@ -65,7 +65,7 @@ static const VMStateDescription vmstate_gic = {
>      .post_load = gic_post_load,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT8_ARRAY(enabled_grp, GICState, GIC_NR_GROUP),
> -        VMSTATE_BOOL_ARRAY(cpu_enabled, GICState, GIC_NCPU),
> +        VMSTATE_UINT32_2DARRAY(cpu_control, GICState, GIC_NCPU, 
> GIC_NR_GROUP),
>          VMSTATE_STRUCT_ARRAY(irq_state, GICState, GIC_MAXIRQ, 1,
>                               vmstate_gic_irq_state, gic_irq_state),
>          VMSTATE_UINT8_ARRAY(irq_target, GICState, GIC_MAXIRQ),
> @@ -127,7 +127,8 @@ static void arm_gic_common_reset(DeviceState *dev)
>          s->current_pending[i] = 1023;
>          s->running_irq[i] = 1023;
>          s->running_priority[i] = 0x100;
> -        s->cpu_enabled[i] = false;
> +        s->cpu_control[i][0] = false;
> +        s->cpu_control[i][1] = false;

The type has changed from bool to uint32_t, so we should
be initializing these to 0, not false.

>      }
>      for (i = 0; i < GIC_NR_SGIS; i++) {
>          GIC_SET_ENABLED(i, ALL_CPU_MASK);
> diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
> index 5038885..9a6a2bb 100644
> --- a/hw/intc/arm_gic_kvm.c
> +++ b/hw/intc/arm_gic_kvm.c
> @@ -379,8 +379,8 @@ static void kvm_arm_gic_put(GICState *s)
>       */
>
>      for (cpu = 0; cpu < s->num_cpu; cpu++) {
> -        /* s->cpu_enabled[cpu] -> GICC_CTLR */
> -        reg = s->cpu_enabled[cpu];
> +        /* s->cpu_enabled[cpu][0] -> GICC_CTLR */
> +        reg = s->cpu_control[cpu];

This is missing the second array index.  (Comment should have
the same var name as the code, too.)

>          kvm_gicc_access(s, 0x00, cpu, &reg, true);
>
>          /* s->priority_mask[cpu] -> GICC_PMR */
> @@ -478,9 +478,9 @@ static void kvm_arm_gic_get(GICState *s)
>       */
>
>      for (cpu = 0; cpu < s->num_cpu; cpu++) {
> -        /* GICC_CTLR -> s->cpu_enabled[cpu] */
> +        /* GICC_CTLR -> s->cpu_control[cpu][0] */
>          kvm_gicc_access(s, 0x00, cpu, &reg, false);
> -        s->cpu_enabled[cpu] = (reg & 1);
> +        s->cpu_control[cpu][0] = reg;
>
>          /* GICC_PMR -> s->priority_mask[cpu] */
>          kvm_gicc_access(s, 0x04, cpu, &reg, false);
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index d0543d4..7f4a55c 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -465,7 +465,7 @@ static void armv7m_nvic_reset(DeviceState *dev)
>       * as enabled by default, and with a priority mask which allows
>       * all interrupts through.
>       */
> -    s->gic.cpu_enabled[0] = true;
> +    s->gic.cpu_control[0][0] = true;
>      s->gic.priority_mask[0] = 0x100;
>      /* The NVIC as a whole is always enabled. */
>      s->gic.enabled = true;
> diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
> index f01955a..e360de6 100644
> --- a/hw/intc/gic_internal.h
> +++ b/hw/intc/gic_internal.h
> @@ -54,6 +54,17 @@
>  #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)
>
> +#define GICC_CTLR_S_EN_GRP0   (1U << 0)
> +#define GICC_CTLR_S_EN_GRP1   (1U << 1)
> +#define GICC_CTLR_S_ACK_CTL       (1U << 2)
> +#define GICC_CTLR_S_FIQ_EN        (1U << 3)
> +#define GICC_CTLR_S_CBPR          (1U << 4) /* GICv1: SBPR */

Odd indent here.

> +
> +#define GICC_CTLR_S_MASK          0x7ff
> +
> +#define GICC_CTLR_NS_EN_GRP1  (1U << 0)
> +#define GICC_CTLR_NS_MASK         (1 | 3 << 5 | 1 << 9)

More brackets in this expression would help clarity.

> +
>
>  /* The special cases for the revision property: */
>  #define REV_11MPCORE 0
> @@ -65,6 +76,9 @@ void gic_complete_irq(GICState *s, int cpu, int irq);
>  void gic_update(GICState *s);
>  void gic_init_irqs_and_distributor(GICState *s);
>  void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val);
> +uint32_t gic_get_cpu_control(GICState *s, int cpu);
> +void gic_set_cpu_control(GICState *s, int cpu, uint32_t value);
> +
>
>  static inline bool gic_test_pending(GICState *s, int irq, int cm)
>  {
> diff --git a/include/hw/intc/arm_gic_common.h 
> b/include/hw/intc/arm_gic_common.h
> index 16e193d..1daa672 100644
> --- a/include/hw/intc/arm_gic_common.h
> +++ b/include/hw/intc/arm_gic_common.h
> @@ -58,7 +58,7 @@ typedef struct GICState {
>          uint8_t enabled;
>          uint8_t enabled_grp[GIC_NR_GROUP]; /* EnableGrp0 and EnableGrp1 */
>      };
> -    bool cpu_enabled[GIC_NCPU];
> +    uint32_t cpu_control[GIC_NCPU][GIC_NR_GROUP];
>
>      gic_irq_state irq_state[GIC_MAXIRQ];
>      uint8_t irq_target[GIC_MAXIRQ];
> --
> 1.8.3.2

-- PMM