From 91f4e18d9550a19ccb33fdac37ab0caf084549c0 Mon Sep 17 00:00:00 2001 From: Luc MICHEL Date: Thu, 25 Jan 2018 11:45:29 +0000 Subject: hw/intc/arm_gic: Prevent the GIC from signaling an IRQ when it's "active and pending" In the GIC, when an IRQ is acknowledged, its state goes from "pending" to: - "active" if the corresponding IRQ pin has been de-asserted - "active and pending" otherwise. The GICv2 manual states that when a IRQ becomes active (or active and pending), the GIC should either signal another (higher priority) IRQ to the CPU if there is one, or de-assert the CPU IRQ pin. The current implementation of the GIC in QEMU does not check if the IRQ is already active when looking for pending interrupts with sufficient priority in gic_update(). This can lead to signaling an interrupt that is already active. This usually happens when splitting priority drop and interrupt deactivation. On priority drop, the IRQ stays active until deactivation. If it becomes pending again, chances are that it will be incorrectly selected as best_irq in gic_update(). This commit fixes this by checking if the IRQ is not already active when looking for best_irq in gic_update(). Note that regarding the ARM11MPCore GIC version, the corresponding manual is not clear on that point, but it has has no priority drop/interrupt deactivation separation, so this case should not happen. Signed-off-by: Luc MICHEL Message-id: 20180119145756.7629-3-luc.michel@greensocs.com Reviewed-by: Peter Maydell Signed-off-by: Peter Maydell --- hw/intc/arm_gic.c | 1 + 1 file changed, 1 insertion(+) (limited to 'hw/intc') diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c index d701e49ff9..dad383ea12 100644 --- a/hw/intc/arm_gic.c +++ b/hw/intc/arm_gic.c @@ -93,6 +93,7 @@ void gic_update(GICState *s) best_irq = 1023; for (irq = 0; irq < s->num_irq; irq++) { if (GIC_TEST_ENABLED(irq, cm) && gic_test_pending(s, irq, cm) && + (!GIC_TEST_ACTIVE(irq, cm)) && (irq < GIC_INTERNAL || GIC_TARGET(irq) & cm)) { if (GIC_GET_PRIORITY(irq, cpu) < best_prio) { best_prio = GIC_GET_PRIORITY(irq, cpu); -- cgit v1.2.3 From 71aa735b0a12201c2b14a76004ee3a58ff43798c Mon Sep 17 00:00:00 2001 From: Luc MICHEL Date: Thu, 25 Jan 2018 11:45:30 +0000 Subject: hw/intc/arm_gic: Fix C_RPR value on idle priority When there is no active interrupts in the GIC, a read to the C_RPR register should return the value of the "Idle priority", which is either the maximum value an IRQ priority field can be set to, or 0xff. Since the QEMU GIC model implements all the 8 priority bits, the Idle priority is 0xff. Internally, when there is no active interrupt, the running priority value is 0x100. The gic_get_running_priority function returns an uint8_t and thus, truncate this value to 0x00 when returning it. This is wrong since a value of 0x00 correspond to the maximum possible priority. This commit fixes the returned value when the internal value is 0x100. Note that it is correct for the Non-Secure view to return 0xff even though from the NS world point of view, only 7 priority bits are implemented. The specification states that the Idle priority can be 0xff even when not all the 8 priority bits are implemented. This has been verified against a real GICv2 hardware on a Xilinx ZynqMP based board. Regarding the ARM11MPCore version of the GIC, the specification is not clear on that point, so this commit does not alter its behavior. Signed-off-by: Luc MICHEL Message-id: 20180119145756.7629-4-luc.michel@greensocs.com Reviewed-by: Peter Maydell Signed-off-by: Peter Maydell --- hw/intc/arm_gic.c | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'hw/intc') diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c index dad383ea12..713de3084f 100644 --- a/hw/intc/arm_gic.c +++ b/hw/intc/arm_gic.c @@ -504,6 +504,11 @@ static void gic_set_cpu_control(GICState *s, int cpu, uint32_t value, static uint8_t gic_get_running_priority(GICState *s, int cpu, MemTxAttrs attrs) { + if ((s->revision != REV_11MPCORE) && (s->running_priority[cpu] > 0xff)) { + /* Idle priority */ + return 0xff; + } + if (s->security_extn && !attrs.secure) { if (s->running_priority[cpu] & 0x80) { /* Running priority in upper half of range: return the Non-secure -- cgit v1.2.3 From fc05a6f22a15503b1e95be640a62e44a06c95d25 Mon Sep 17 00:00:00 2001 From: Luc MICHEL Date: Thu, 25 Jan 2018 11:45:30 +0000 Subject: hw/intc/arm_gic: Fix group priority computation for group 1 IRQs When determining the group priority of a group 1 IRQ, if C_CTRL.CBPR is 0, the non-secure BPR value is used. However, this value must be incremented by one so that it matches the secure world number of implemented priority bits (NS world has one less priority bit compared to the Secure world). Signed-off-by: Luc MICHEL Message-id: 20180119145756.7629-5-luc.michel@greensocs.com Reviewed-by: Peter Maydell [PMM: add assert, as the gicv3 code has] Signed-off-by: Peter Maydell --- hw/intc/arm_gic.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'hw/intc') diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c index 713de3084f..b7989d267f 100644 --- a/hw/intc/arm_gic.c +++ b/hw/intc/arm_gic.c @@ -256,7 +256,8 @@ static int gic_get_group_priority(GICState *s, int cpu, int irq) if (gic_has_groups(s) && !(s->cpu_ctlr[cpu] & GICC_CTLR_CBPR) && GIC_TEST_GROUP(irq, (1 << cpu))) { - bpr = s->abpr[cpu]; + bpr = s->abpr[cpu] - 1; + assert(bpr >= 0); } else { bpr = s->bpr[cpu]; } -- cgit v1.2.3 From 421a3c224e2c4c5d2dfd9309d7357f3369ed30e0 Mon Sep 17 00:00:00 2001 From: Luc MICHEL Date: Thu, 25 Jan 2018 11:45:30 +0000 Subject: hw/intc/arm_gic: Fix the NS view of C_BPR when C_CTRL.CBPR is 1 When C_CTRL.CBPR is 1, the Non-Secure view of C_BPR is altered: - A Non-Secure read of C_BPR should return the BPR value plus 1, saturated to 7, - A Non-Secure write should be ignored. Signed-off-by: Luc MICHEL Message-id: 20180119145756.7629-6-luc.michel@greensocs.com Reviewed-by: Peter Maydell [PMM: fixed comment typo] Signed-off-by: Peter Maydell --- hw/intc/arm_gic.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) (limited to 'hw/intc') diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c index b7989d267f..724bc9fa61 100644 --- a/hw/intc/arm_gic.c +++ b/hw/intc/arm_gic.c @@ -1212,8 +1212,13 @@ static MemTxResult gic_cpu_read(GICState *s, int cpu, int offset, break; case 0x08: /* Binary Point */ if (s->security_extn && !attrs.secure) { - /* BPR is banked. Non-secure copy stored in ABPR. */ - *data = s->abpr[cpu]; + if (s->cpu_ctlr[cpu] & GICC_CTLR_CBPR) { + /* NS view of BPR when CBPR is 1 */ + *data = MIN(s->bpr[cpu] + 1, 7); + } else { + /* BPR is banked. Non-secure copy stored in ABPR. */ + *data = s->abpr[cpu]; + } } else { *data = s->bpr[cpu]; } @@ -1286,7 +1291,12 @@ static MemTxResult gic_cpu_write(GICState *s, int cpu, int offset, break; case 0x08: /* Binary Point */ if (s->security_extn && !attrs.secure) { - s->abpr[cpu] = MAX(value & 0x7, GIC_MIN_ABPR); + if (s->cpu_ctlr[cpu] & GICC_CTLR_CBPR) { + /* WI when CBPR is 1 */ + return MEMTX_OK; + } else { + s->abpr[cpu] = MAX(value & 0x7, GIC_MIN_ABPR); + } } else { s->bpr[cpu] = MAX(value & 0x7, GIC_MIN_BPR); } -- cgit v1.2.3