On 22 August 2014 05:29, Fabian Aggeler wrote: > 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 > --- > hw/intc/arm_gic.c | 54 > ++++++++++++++++++++++++++++++++++++---- > 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, 72 insertions(+), 13 deletions(-) > > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c > index c78b301..7f7fac3 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][0] & 1)) { > qemu_irq_lower(s->parent_irq[cpu]); > return; > } > @@ -239,6 +239,52 @@ void gic_set_priority(GICState *s, int cpu, int irq, > uint8_t val) > } > } > > +uint32_t gic_get_cpu_control(GICState *s, int cpu) > +{ > + if ((s->revision >= 2 && !s->security_extn) > + || (s->security_extn && !ns_access())) { > + return s->cpu_control[cpu][0]; > + } else if (s->security_extn && ns_access()) { > + return s->cpu_control[cpu][1]; > + } else { > + return s->cpu_control[cpu][0]; > + } > +} > This conditional is not sufficient. In the case of GICv1 without the security extension, we fall through to the else case which returns [0] instead of a masked version of [1]. This may be better: if (!s->security_extn) { if (s->revision == 1) { res = s->cpu_control[cpu][1]; res &= 0x1; } else { res = s->cpu_control[cpu][0]; } } else { if (ns_access()) { res = s->cpu_control[cpu][1]; if (s->revision == 1) { res &= 0x1; } } else { res = s->cpu_control[cpu][0]; } } Similar may apply below. + > +void gic_set_cpu_control(GICState *s, int cpu, uint32_t value) > +{ > + /* CPU Interface Control is banked for GICv2 and GICv1 with Security > Extn */ > + if ((s->revision >= 2 && !s->security_extn) > + || (s->security_extn && !ns_access())) { > + /* 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 (s->security_extn && ns_access()) { > + /* 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 { > + s->cpu_control[cpu][0] = (value & 1); > + > + DPRINTF("CPU Interface %d %sabled\n", cpu, > + s->cpu_control[cpu][0] ? "En" : "Dis"); > + } > +} > + > void gic_complete_irq(GICState *s, int cpu, int irq) > { > int update = 0; > @@ -742,7 +788,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 */ > @@ -768,9 +814,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); > 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 7652754..c873f7b 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; > } > for (i = 0; i < 16; 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]; > kvm_gicc_access(s, 0x00, cpu, ®, 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, ®, 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, ®, false); > diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c > index 1a7af45..57486fc 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 b0430ff..e9814f4 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 */ > + > +#define GICC_CTLR_S_MASK 0x7ff > + > +#define GICC_CTLR_NS_EN_GRP1 (1U << 0) > +#define GICC_CTLR_NS_MASK (1 | 3 << 5 | 1 << 9) > + > > /* 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, int num_irq); > 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 a39b066..a912972 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 > >