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

[Edgar E. Iglesias]

On Fri, May 01, 2015 at 06:50:34PM +0100, Peter Maydell 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_ctlr.
> 
> Signed-off-by: Fabian Aggeler <address@hidden>
> Signed-off-by: Greg Bellows <address@hidden>
> Message-id: address@hidden
> [PMM: rewrote to store state in a single uint32_t rather than
>  keeping the NS and S banked variants separate; this considerably
>  simplifies the get/set functions]
> Signed-off-by: Peter Maydell <address@hidden>

Reviewed-by: Edgar E. Iglesias <address@hidden>


> ---
>  hw/intc/arm_gic.c                | 51 
> ++++++++++++++++++++++++++++++++++++----
>  hw/intc/arm_gic_common.c         |  8 +++----
>  hw/intc/arm_gic_kvm.c            |  8 +++----
>  hw/intc/armv7m_nvic.c            |  2 +-
>  hw/intc/gic_internal.h           | 16 +++++++++++++
>  include/hw/intc/arm_gic_common.h |  5 +++-
>  6 files changed, 76 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index e6ad8de..4aaaac2 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -68,7 +68,7 @@ void gic_update(GICState *s)
>          cm = 1 << cpu;
>          s->current_pending[cpu] = 1023;
>          if (!(s->ctlr & (GICD_CTLR_EN_GRP0 | GICD_CTLR_EN_GRP1))
> -            || !s->cpu_enabled[cpu]) {
> +            || !(s->cpu_ctlr[cpu] & (GICC_CTLR_EN_GRP0 | 
> GICC_CTLR_EN_GRP1))) {
>              qemu_irq_lower(s->parent_irq[cpu]);
>              return;
>          }
> @@ -242,6 +242,50 @@ void gic_set_priority(GICState *s, int cpu, int irq, 
> uint8_t val)
>      }
>  }
>  
> +static uint32_t gic_get_cpu_control(GICState *s, int cpu, MemTxAttrs attrs)
> +{
> +    uint32_t ret = s->cpu_ctlr[cpu];
> +
> +    if (s->security_extn && !attrs.secure) {
> +        /* Construct the NS banked view of GICC_CTLR from the correct
> +         * bits of the S banked view. We don't need to move the bypass
> +         * control bits because we don't implement that (IMPDEF) part
> +         * of the GIC architecture.
> +         */
> +        ret = (ret & (GICC_CTLR_EN_GRP1 | GICC_CTLR_EOIMODE_NS)) >> 1;
> +    }
> +    return ret;
> +}
> +
> +static void gic_set_cpu_control(GICState *s, int cpu, uint32_t value,
> +                                MemTxAttrs attrs)
> +{
> +    uint32_t mask;
> +
> +    if (s->security_extn && !attrs.secure) {
> +        /* The NS view can only write certain bits in the register;
> +         * the rest are unchanged
> +         */
> +        mask = GICC_CTLR_EN_GRP1;
> +        if (s->revision == 2) {
> +            mask |= GICC_CTLR_EOIMODE_NS;
> +        }
> +        s->cpu_ctlr[cpu] &= ~mask;
> +        s->cpu_ctlr[cpu] |= (value << 1) & mask;
> +    } else {
> +        if (s->revision == 2) {
> +            mask = s->security_extn ? GICC_CTLR_V2_S_MASK : 
> GICC_CTLR_V2_MASK;
> +        } else {
> +            mask = s->security_extn ? GICC_CTLR_V1_S_MASK : 
> GICC_CTLR_V1_MASK;
> +        }
> +        s->cpu_ctlr[cpu] = value & mask;
> +    }
> +    DPRINTF("CPU Interface %d: Group0 Interrupts %sabled, "
> +            "Group1 Interrupts %sabled\n", cpu,
> +            (s->cpu_ctlr[cpu] & GICC_CTLR_EN_GRP0) ? "En" : "Dis",
> +            (s->cpu_ctlr[cpu] & GICC_CTLR_EN_GRP1) ? "En" : "Dis");
> +}
> +
>  void gic_complete_irq(GICState *s, int cpu, int irq)
>  {
>      int update = 0;
> @@ -756,7 +800,7 @@ static MemTxResult gic_cpu_read(GICState *s, int cpu, int 
> offset,
>  {
>      switch (offset) {
>      case 0x00: /* Control */
> -        *data = s->cpu_enabled[cpu];
> +        *data = gic_get_cpu_control(s, cpu, attrs);
>          break;
>      case 0x04: /* Priority mask */
>          *data = s->priority_mask[cpu];
> @@ -806,8 +850,7 @@ static MemTxResult gic_cpu_write(GICState *s, int cpu, 
> int offset,
>  {
>      switch (offset) {
>      case 0x00: /* Control */
> -        s->cpu_enabled[cpu] = (value & 1);
> -        DPRINTF("CPU %d %sabled\n", cpu, s->cpu_enabled[cpu] ? "En" : "Dis");
> +        gic_set_cpu_control(s, cpu, value, attrs);
>          break;
>      case 0x04: /* Priority mask */
>          s->priority_mask[cpu] = (value & 0xff);
> diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
> index bef76fc..044ad66 100644
> --- a/hw/intc/arm_gic_common.c
> +++ b/hw/intc/arm_gic_common.c
> @@ -59,13 +59,13 @@ static const VMStateDescription vmstate_gic_irq_state = {
>  
>  static const VMStateDescription vmstate_gic = {
>      .name = "arm_gic",
> -    .version_id = 9,
> -    .minimum_version_id = 9,
> +    .version_id = 10,
> +    .minimum_version_id = 10,
>      .pre_save = gic_pre_save,
>      .post_load = gic_post_load,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT32(ctlr, GICState),
> -        VMSTATE_BOOL_ARRAY(cpu_enabled, GICState, GIC_NCPU),
> +        VMSTATE_UINT32_ARRAY(cpu_ctlr, GICState, GIC_NCPU),
>          VMSTATE_STRUCT_ARRAY(irq_state, GICState, GIC_MAXIRQ, 1,
>                               vmstate_gic_irq_state, gic_irq_state),
>          VMSTATE_UINT8_ARRAY(irq_target, GICState, GIC_MAXIRQ),
> @@ -134,7 +134,7 @@ 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_ctlr[i] = 0;
>      }
>      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 3591ca7..c5a2f81 100644
> --- a/hw/intc/arm_gic_kvm.c
> +++ b/hw/intc/arm_gic_kvm.c
> @@ -414,8 +414,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_ctlr[cpu] -> GICC_CTLR */
> +        reg = s->cpu_ctlr[cpu];
>          kvm_gicc_access(s, 0x00, cpu, &reg, true);
>  
>          /* s->priority_mask[cpu] -> GICC_PMR */
> @@ -506,9 +506,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_ctlr[cpu] */
>          kvm_gicc_access(s, 0x00, cpu, &reg, false);
> -        s->cpu_enabled[cpu] = (reg & 1);
> +        s->cpu_ctlr[cpu] = 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 4e6456e..c226daf 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_ctlr[0] = GICC_CTLR_EN_GRP0;
>      s->gic.priority_mask[0] = 0x100;
>      /* The NVIC as a whole is always enabled. */
>      s->gic.ctlr = 1;
> diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
> index 3b4b3fb..81c764c 100644
> --- a/hw/intc/gic_internal.h
> +++ b/hw/intc/gic_internal.h
> @@ -57,6 +57,22 @@
>  #define GICD_CTLR_EN_GRP0 (1U << 0)
>  #define GICD_CTLR_EN_GRP1 (1U << 1)
>  
> +#define GICC_CTLR_EN_GRP0    (1U << 0)
> +#define GICC_CTLR_EN_GRP1    (1U << 1)
> +#define GICC_CTLR_ACK_CTL    (1U << 2)
> +#define GICC_CTLR_FIQ_EN     (1U << 3)
> +#define GICC_CTLR_CBPR       (1U << 4) /* GICv1: SBPR */
> +#define GICC_CTLR_EOIMODE    (1U << 9)
> +#define GICC_CTLR_EOIMODE_NS (1U << 10)
> +
> +/* Valid bits for GICC_CTLR for GICv1, v1 with security extensions,
> + * GICv2 and GICv2 with security extensions:
> + */
> +#define GICC_CTLR_V1_MASK    0x1
> +#define GICC_CTLR_V1_S_MASK  0x1f
> +#define GICC_CTLR_V2_MASK    0x21f
> +#define GICC_CTLR_V2_S_MASK  0x61f
> +
>  /* The special cases for the revision property: */
>  #define REV_11MPCORE 0
>  #define REV_NVIC 0xffffffff
> diff --git a/include/hw/intc/arm_gic_common.h 
> b/include/hw/intc/arm_gic_common.h
> index 261402f..899db3d 100644
> --- a/include/hw/intc/arm_gic_common.h
> +++ b/include/hw/intc/arm_gic_common.h
> @@ -59,7 +59,10 @@ typedef struct GICState {
>       * of this register is just an alias of bit 1 of the S banked version.
>       */
>      uint32_t ctlr;
> -    bool cpu_enabled[GIC_NCPU];
> +    /* GICC_CTLR; again, the NS banked version is just aliases of bits of
> +     * the S banked register, so our state only needs to store the S version.
> +     */
> +    uint32_t cpu_ctlr[GIC_NCPU];
>  
>      gic_irq_state irq_state[GIC_MAXIRQ];
>      uint8_t irq_target[GIC_MAXIRQ];
> -- 
> 1.9.1
>