[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 3/8] arm_gic: Fix GIC pending behavior
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [PATCH v5 3/8] arm_gic: Fix GIC pending behavior |
Date: |
Fri, 31 Jan 2014 18:09:20 +0000 |
On 28 January 2014 20:32, Christoffer Dall <address@hidden> wrote:
> The existing implementation of the pending behavior in gic_set_irq,
> gic_acknowledge_irq, gic_complete_irq, and the distributor pending
> set/clear registers does not follow the semantics of the GICv2.0 specs,
> but may implement the 11MPCore support. Therefore, maintain the
> existing semantics for 11MPCore and v7M NVIC and change the behavior to
> be in accordance with the GICv2.0 specs for "generic implementations"
> (s->revision == 1 || s->revision == 2).
>
> Generic implementations distinguish between setting a level-triggered
> interrupt pending through writes to the GICD_ISPENDR and when hardware
> raises the interrupt line. Writing to the GICD_ICPENDR will not cause
> the interrupt to become non-pending if the line is still active, and
> conversely, if the line is deactivated but the interrupt is marked as
> pending through a write to GICD_ISPENDR, the interrupt remains pending.
> Handle this situation in the GIC_TEST_PENDING (which now becomes a
> static inline named gic_test_pending) and let the 'pending' field
> correspond only to the latched state of the D-flip flop in the GICv2.0
> specs Figure 4-10.
>
> The following changes are added:
>
> gic_test_pending:
> Make this a static inline and split out the 11MPCore from the generic
> behavior. For the generic behavior, consider interrupts pending if:
> ((s->irq_state[irq].pending & (cm) != 0) ||
> (!GIC_TEST_EDGE_TRIGGER(irq) && GIC_TEST_LEVEL(irq, cm))
>
> gic_set_irq:
> Split out the 11MPCore from the generic behavior. For the generic
> behavior, always GIC_SET_LEVEL(irq, 1) on positive level, but only
> GIC_SET_PENDING for edge-triggered interrupts and vice versa on a
> negative level.
>
> gic_complete_irq:
> Only resample the line for line-triggered interrupts on an 11MPCore.
> Generic implementations will sample the line directly in
> gic_test_pending().
>
> Signed-off-by: Christoffer Dall <address@hidden>
This looks broadly right; a couple of comments below.
> ---
> Changes [v4 -> v5]:
> - Factor out GIC_NR_SGIS and gic_dist_writeb bugfixes
> - Change meaning of pending field for level-triggered interrupts for
> GIC v1/v2, to only capture manually written state to pending
> registers. Add or-clause to gic_test_pending to sample the line
> state, as per Peter's suggestions:
> https://lists.cs.columbia.edu/pipermail/kvmarm/2014-January/008798.html
>
> Changes [v3 -> v4]:
> - Maintain 11MPCore semantics
> - Combine all pending interrupts fixing patches into this patch. See
> the detailed description above.
>
> Changes [v1 -> v2]:
> - Fix bisection issue, by not using gic_clear_pending yet.
>
> hw/intc/arm_gic.c | 74
> ++++++++++++++++++++++++++++++++++++--------------
> hw/intc/gic_internal.h | 16 ++++++++++-
> 2 files changed, 69 insertions(+), 21 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 1c4a114..5e2cf14 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -66,7 +66,7 @@ void gic_update(GICState *s)
> best_prio = 0x100;
> best_irq = 1023;
> for (irq = 0; irq < s->num_irq; irq++) {
> - if (GIC_TEST_ENABLED(irq, cm) && GIC_TEST_PENDING(irq, cm)) {
> + if (GIC_TEST_ENABLED(irq, cm) && gic_test_pending(s, irq, cm)) {
> if (GIC_GET_PRIORITY(irq, cpu) < best_prio) {
> best_prio = GIC_GET_PRIORITY(irq, cpu);
> best_irq = irq;
> @@ -89,14 +89,46 @@ void gic_set_pending_private(GICState *s, int cpu, int
> irq)
> {
> int cm = 1 << cpu;
>
> - if (GIC_TEST_PENDING(irq, cm))
> + if (gic_test_pending(s, irq, cm)) {
> return;
> + }
>
> DPRINTF("Set %d pending cpu %d\n", irq, cpu);
> GIC_SET_PENDING(irq, cm);
> gic_update(s);
> }
>
> +static void gic_set_irq_11mpcore(GICState *s, int irq, int level,
> + int cm, int target)
> +{
> + if (level) {
> + GIC_SET_LEVEL(irq, cm);
> + if (GIC_TEST_EDGE_TRIGGER(irq) || GIC_TEST_ENABLED(irq, cm)) {
> + DPRINTF("Set %d pending mask %x\n", irq, target);
> + GIC_SET_PENDING(irq, target);
> + }
> + } else {
> + GIC_CLEAR_LEVEL(irq, cm);
> + }
> +}
> +
> +static void gic_set_irq_generic(GICState *s, int irq, int level,
> + int cm, int target)
> +{
> + if (level) {
> + GIC_SET_LEVEL(irq, cm);
> + DPRINTF("Set %d pending mask %x\n", irq, target);
> + if (GIC_TEST_EDGE_TRIGGER(irq)) {
> + GIC_SET_PENDING(irq, target);
> + }
> + } else {
> + if (GIC_TEST_EDGE_TRIGGER(irq)) {
> + GIC_CLEAR_PENDING(irq, target);
> + }
This doesn't look right. A falling edge for an edge triggered
interrupt should just CLEAR_LEVEL and leave the state of the
pending latch alone.
> + GIC_CLEAR_LEVEL(irq, cm);
> + }
> +}
> +
> /* Process a change in an external IRQ input. */
> static void gic_set_irq(void *opaque, int irq, int level)
> {
> @@ -126,15 +158,12 @@ static void gic_set_irq(void *opaque, int irq, int
> level)
> return;
> }
>
> - if (level) {
> - GIC_SET_LEVEL(irq, cm);
> - if (GIC_TEST_EDGE_TRIGGER(irq) || GIC_TEST_ENABLED(irq, cm)) {
> - DPRINTF("Set %d pending mask %x\n", irq, target);
> - GIC_SET_PENDING(irq, target);
> - }
> + if (s->revision == REV_11MPCORE) {
Or NVIC.
> + gic_set_irq_11mpcore(s, irq, level, cm, target);
> } else {
> - GIC_CLEAR_LEVEL(irq, cm);
> + gic_set_irq_generic(s, irq, level, cm, target);
> }
> +
> gic_update(s);
> }
>
> @@ -160,9 +189,10 @@ uint32_t gic_acknowledge_irq(GICState *s, int cpu)
> return 1023;
> }
> s->last_active[new_irq][cpu] = s->running_irq[cpu];
> - /* Clear pending flags for both level and edge triggered interrupts.
> - Level triggered IRQs will be reasserted once they become inactive. */
> - GIC_CLEAR_PENDING(new_irq, GIC_TEST_MODEL(new_irq) ? ALL_CPU_MASK : cm);
> +
> + cm = GIC_TEST_MODEL(new_irq) ? ALL_CPU_MASK : cm;
> + GIC_CLEAR_PENDING(new_irq, cm);
> +
This assignment to cm ends up changing behaviour of following
code for 11mpcore/NVIC, I think. It looks to me like you can
just drop this hunk.
> gic_set_running_irq(s, cpu, new_irq);
> DPRINTF("ACK %d\n", new_irq);
> return new_irq;
> @@ -195,14 +225,18 @@ void gic_complete_irq(GICState *s, int cpu, int irq)
> }
> if (s->running_irq[cpu] == 1023)
> return; /* No active IRQ. */
> - /* Mark level triggered interrupts as pending if they are still
> - raised. */
> - if (!GIC_TEST_EDGE_TRIGGER(irq) && GIC_TEST_ENABLED(irq, cm)
> - && GIC_TEST_LEVEL(irq, cm) && (GIC_TARGET(irq) & cm) != 0) {
> - DPRINTF("Set %d pending mask %x\n", irq, cm);
> - GIC_SET_PENDING(irq, cm);
> - update = 1;
> +
> + if (s->revision == REV_11MPCORE) {
Or NVIC.
> + /* Mark level triggered interrupts as pending if they are still
> + raised. */
> + if (!GIC_TEST_EDGE_TRIGGER(irq) && GIC_TEST_ENABLED(irq, cm)
> + && GIC_TEST_LEVEL(irq, cm) && (GIC_TARGET(irq) & cm) != 0) {
> + DPRINTF("Set %d pending mask %x\n", irq, cm);
> + GIC_SET_PENDING(irq, cm);
> + update = 1;
> + }
> }
> +
> if (irq != s->running_irq[cpu]) {
> /* Complete an IRQ that is not currently running. */
> int tmp = s->running_irq[cpu];
> @@ -273,7 +307,7 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr
> offset)
> res = 0;
> mask = (irq < GIC_INTERNAL) ? cm : ALL_CPU_MASK;
> for (i = 0; i < 8; i++) {
> - if (GIC_TEST_PENDING(irq + i, mask)) {
> + if (gic_test_pending(s, irq + i, mask)) {
> res |= (1 << i);
> }
> }
> diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
> index 8c02d58..92a6f7a 100644
> --- a/hw/intc/gic_internal.h
> +++ b/hw/intc/gic_internal.h
> @@ -34,7 +34,6 @@
> #define GIC_TEST_ENABLED(irq, cm) ((s->irq_state[irq].enabled & (cm)) != 0)
> #define GIC_SET_PENDING(irq, cm) s->irq_state[irq].pending |= (cm)
> #define GIC_CLEAR_PENDING(irq, cm) s->irq_state[irq].pending &= ~(cm)
> -#define GIC_TEST_PENDING(irq, cm) ((s->irq_state[irq].pending & (cm)) != 0)
> #define GIC_SET_ACTIVE(irq, cm) s->irq_state[irq].active |= (cm)
> #define GIC_CLEAR_ACTIVE(irq, cm) s->irq_state[irq].active &= ~(cm)
> #define GIC_TEST_ACTIVE(irq, cm) ((s->irq_state[irq].active & (cm)) != 0)
> @@ -63,4 +62,19 @@ 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);
>
> +static inline bool gic_test_pending(GICState *s, int irq, int cm)
> +{
> + if (s->revision == REV_NVIC || s->revision == REV_11MPCORE) {
> + return s->irq_state[irq].pending & cm;
> + } else {
> + /* Edge-triggered interrupts are marked pending on a rising edge, but
> + * level-triggered interrupts are either considered pending when the
> + * level is active or if software has explicitly written to
> + * GICD_ISPENDR to set the state pending.
> + */
> + return (s->irq_state[irq].pending & cm) ||
> + (!GIC_TEST_EDGE_TRIGGER(irq) && GIC_TEST_LEVEL(irq, cm));
> + }
> +}
> +
> #endif /* !QEMU_ARM_GIC_INTERNAL_H */
thanks
-- PMM
- [Qemu-devel] [PATCH v5 0/8] Support arm-gic-kvm save/restore, Christoffer Dall, 2014/01/28
- [Qemu-devel] [PATCH v5 2/8] arm_gic: Fix GICD_ICPENDR and GICD_ISPENDR writes, Christoffer Dall, 2014/01/28
- [Qemu-devel] [PATCH v5 1/8] arm_gic: Introduce define for GIC_NR_SGIS, Christoffer Dall, 2014/01/28
- [Qemu-devel] [PATCH v5 3/8] arm_gic: Fix GIC pending behavior, Christoffer Dall, 2014/01/28
- Re: [Qemu-devel] [PATCH v5 3/8] arm_gic: Fix GIC pending behavior,
Peter Maydell <=
- [Qemu-devel] [PATCH v5 4/8] hw: arm_gic: Keep track of SGI sources, Christoffer Dall, 2014/01/28
- [Qemu-devel] [PATCH v5 5/8] arm_gic: Support setting/getting binary point reg, Christoffer Dall, 2014/01/28
- [Qemu-devel] [PATCH v5 6/8] vmstate: Add uint32 2D-array support, Christoffer Dall, 2014/01/28
- [Qemu-devel] [PATCH v5 7/8] arm_gic: Add GICC_APRn state to the GICState, Christoffer Dall, 2014/01/28
- [Qemu-devel] [PATCH v5 8/8] hw: arm_gic_kvm: Add KVM VGIC save/restore logic, Christoffer Dall, 2014/01/28
- Re: [Qemu-devel] [PATCH v5 0/8] Support arm-gic-kvm save/restore, Peter Maydell, 2014/01/29