On 22 August 2014 05:29, Fabian Aggeler <[email protected]> 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 <[email protected]>
> ---
> 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
>
>
