[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH v3 01/10] hw: arm_gic: Fix gic_set_irq handl
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [RFC PATCH v3 01/10] hw: arm_gic: Fix gic_set_irq handling |
Date: |
Thu, 28 Nov 2013 16:17:43 +0000 |
On 19 November 2013 06:18, Christoffer Dall <address@hidden> wrote:
> For some reason only edge-triggered or enabled level-triggered
> interrupts would set the pending state of a raised IRQ. This is not in
> compliance with the specs, which indicate that the pending state is
> separate from the enabled state, which only controls if a pending
> interrupt is actually forwarded to the CPU interface.
>
> Therefore, simply always set the pending state on a rising edge, but
> only clear the pending state of falling edge if the interrupt is level
> triggered.
>
> Changelog [v2]:
> - Fix bisection issue, by not using gic_clear_pending yet.
>
> Signed-off-by: Christoffer Dall <address@hidden>
> ---
> hw/intc/arm_gic.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index d431b7a..c7a24d5 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -128,11 +128,12 @@ static void gic_set_irq(void *opaque, int irq, int
> level)
>
> if (level) {
> GIC_SET_LEVEL(irq, cm);
> - if (GIC_TEST_TRIGGER(irq) || GIC_TEST_ENABLED(irq, cm)) {
> - DPRINTF("Set %d pending mask %x\n", irq, target);
> - GIC_SET_PENDING(irq, target);
> - }
> + DPRINTF("Set %d pending mask %x\n", irq, target);
> + GIC_SET_PENDING(irq, target);
> } else {
> + if (!GIC_TEST_TRIGGER(irq)) {
> + GIC_CLEAR_PENDING(irq, target);
> + }
> GIC_CLEAR_LEVEL(irq, cm);
> }
> gic_update(s);
So I think this is a correct change in the sense that
it's fixing the behaviour of this function. However
we seem to get our pending behaviour for level triggered
interrupts wrong in several places:
* here
* gic_acknowledge_irq (which you fix in a later patch)
* gic_complete_irq, which currently says "enabled
level triggered still-raised interrupts should be
remarked as pending"
This feels to me like a cluster of errors which have come
from somebody's misreading of the spec and which probably
combine to produce a coherent not-too-far-from-correct
result, and which we should therefore fix all at once rather
than only partially.
thanks
-- PMM
- [Qemu-devel] [RFC PATCH v3 00/10] Support arm-gic-kvm save/restore, Christoffer Dall, 2013/11/19
- [Qemu-devel] [RFC PATCH v3 01/10] hw: arm_gic: Fix gic_set_irq handling, Christoffer Dall, 2013/11/19
- Re: [Qemu-devel] [RFC PATCH v3 01/10] hw: arm_gic: Fix gic_set_irq handling,
Peter Maydell <=
- [Qemu-devel] [RFC PATCH v3 02/10] hw: arm_gic: Introduce gic_set_priority function, Christoffer Dall, 2013/11/19
- [Qemu-devel] [RFC PATCH v3 03/10] hw: arm_gic: Keep track of SGI sources, Christoffer Dall, 2013/11/19
- [Qemu-devel] [RFC PATCH v3 04/10] arm_gic: Support setting/getting binary point reg, Christoffer Dall, 2013/11/19
- [Qemu-devel] [RFC PATCH v3 05/10] arm_gic: Rename GIC_X_TRIGGER to GIC_X_EDGE_TRIGGER, Christoffer Dall, 2013/11/19
- [Qemu-devel] [RFC PATCH v3 06/10] arm_gic: Keep track of GICD_CPENDR and GICD_SPENDR, Christoffer Dall, 2013/11/19
- [Qemu-devel] [RFC PATCH v3 07/10] arm_gic: Fix gic_acknowledge_irq pending bit clear, Christoffer Dall, 2013/11/19