[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [v2] nvic: set pending status for not active interrupts
From: |
Krzeminski, Marcin (Nokia - PL/Wroclaw) |
Subject: |
Re: [Qemu-devel] [v2] nvic: set pending status for not active interrupts |
Date: |
Mon, 31 Oct 2016 09:11:42 +0000 |
Peter,
> -----Original Message-----
> From: Peter Maydell [mailto:address@hidden
> Sent: Monday, October 24, 2016 5:26 PM
> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> <address@hidden>
> Cc: QEMU Developers <address@hidden>; qemu-arm <qemu-
> address@hidden>; address@hidden
> Subject: Re: [v2] nvic: set pending status for not active interrupts
>
> On 18 October 2016 at 07:14, <address@hidden> wrote:
> > From: Marcin Krzeminski <address@hidden>
> >
> > According to ARM DUI 0552A 4.2.10. NVIC set pending status also for
> > disabled interrupts. This patch adds possibility to emulate this in
> > Qemu.
> >
> > Signed-off-by: Marcin Krzeminski <address@hidden>
>
> Thanks for rolling a v2. Unfortunately I think we don't have the logic quite
> right yet...
>
Yeap, I separated it but not update to work only with NVIC.
> > ---
> > Changes for v2:
> > - add a dedicated unction for nvic
> > - update complete_irq
> > hw/intc/arm_gic.c | 27 +++++++++++++++++++++++++--
> > 1 file changed, 25 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c index
> > b30cc91..72e4c01 100644
> > --- a/hw/intc/arm_gic.c
> > +++ b/hw/intc/arm_gic.c
> > @@ -156,6 +156,21 @@ static void gic_set_irq_11mpcore(GICState *s, int
> irq, int level,
> > }
> > }
> >
> > +static void gic_set_irq_nvic(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)
> > + || !GIC_TEST_ACTIVE(irq, cm)) {
> > + DPRINTF("Set %d pending mask %x\n", irq, target);
> > + GIC_SET_PENDING(irq, target);
> > + }
>
> Why is GIC_TEST_ENABLED() in this condition, when the idea is that we don't
> care about the enabled status for whether we can pend the interrupt?
>
> I think this should actually just always do
> GIC_SET_PENDING(irq, target)
> whenever level is high
>
> because:
> (1) pending status can be set for disabled interrupts
> (2) edge-triggered IRQs definitely always re-pend on rising edge
> (3) I think level-triggered IRQs do too (it's a bit
> less clear in the docs, but DUI0552A 4.2.9 says we pend on
> a rising edge and doesn't say that applies only to edge
> triggered IRQs)
>
> I don't have any real hardware to hand to test against, though.
>
Yes, it works exactly as you're saying (checked on HW), if level is still
high after exit interrupt handler, interrupt is re-pend.
> > + } else {
> > + GIC_CLEAR_LEVEL(irq, cm);
> > + }
> > +}
> > +
> > static void gic_set_irq_generic(GICState *s, int irq, int level,
> > int cm, int target) { @@ -201,8
> > +216,10 @@ static void gic_set_irq(void *opaque, int irq, int level)
> > return;
> > }
> >
> > - if (s->revision == REV_11MPCORE || s->revision == REV_NVIC) {
> > + if (s->revision == REV_11MPCORE) {
> > gic_set_irq_11mpcore(s, irq, level, cm, target);
> > + } else if (s->revision == REV_NVIC) {
> > + gic_set_irq_nvic(s, irq, level, cm, target);
> > } else {
> > gic_set_irq_generic(s, irq, level, cm, target);
> > }
> > @@ -568,7 +585,7 @@ void gic_complete_irq(GICState *s, int cpu, int irq,
> MemTxAttrs attrs)
> > return; /* No active IRQ. */
> > }
> >
> > - if (s->revision == REV_11MPCORE || s->revision == REV_NVIC) {
> > + if (s->revision == REV_11MPCORE) {
> > /* Mark level triggered interrupts as pending if they are still
> > raised. */
> > if (!GIC_TEST_EDGE_TRIGGER(irq) && GIC_TEST_ENABLED(irq, cm)
> > @@ -576,6 +593,12 @@ void gic_complete_irq(GICState *s, int cpu, int irq,
> MemTxAttrs attrs)
> > DPRINTF("Set %d pending mask %x\n", irq, cm);
> > GIC_SET_PENDING(irq, cm);
> > }
> > + } else if (s->revision == REV_NVIC) {
> > + if (GIC_TEST_ENABLED(irq, cm) && GIC_TEST_LEVEL(irq, cm)
> > + && (GIC_TARGET(irq) & cm) != 0) {
> > + DPRINTF("Set nvic %d pending mask %x\n", irq, cm);
> > + GIC_SET_PENDING(irq, cm);
> > + }
>
> Similarly, I think this should just be
> if (GIC_TEST_LEVEL(irq, cm) {
> GIC_SET_PENDING(irq, cm);
> }
>
> because:
> (1) for the NVIC there is only one CPU and GIC_TARGET(irq) always indicates
> that IRQs target it
> (2) we don't care whether the interrupt is enabled when deciding whether
> it should be pended
> (3) we don't care whether the interrupt is level-triggered or edge-triggered
> for deciding whether to re-pend it (see statement R_XVWM in the v8M
> ARM ARM DDI0553A.c)
>
Same as above. I'll send v3.
Thanks,
Marcin
> > }
> >
> > group = gic_has_groups(s) && GIC_TEST_GROUP(irq, cm);
> > --
> > 2.7.4
>
> thanks
> -- PMM