Re: [Qemu-devel] [PATCH] arm_gic: Keep track of GICD_CPENDR and GICD_SPENDR

[Christoffer Dall]

On Tue, Oct 29, 2013 at 04:10:59PM +0000, Bhushan Bharat-R65777 wrote:
> Hi Christoffer,
> 
> Not related to the patch, for edge type of interrupt, will setting bit in 
> ICD_SPENDR generate interrupt?
> 
Hi Bharat,

That depends, if the interrupt was not previously pending, setting the
bit for the corresponding interrupt will cause it to be pending, if the
interrupt is also enabled and has high enough priority, then the
interrupt gets forwarded to the CPU interface and if the corresponding
CPU is not masking interrupts, it will cause an interrupt on the CPU.

The same is actually valid for level-triggered interrupts too, however,
the difference is that if the interrupt was already pending, it has no
effect whatsoever on edge-triggered interrupts, but level-triggered
interrupts will stay pending even if the external line is deasserted.

-Christoffer

> 
> > -----Original Message-----
> > From: Christoffer Dall [mailto:address@hidden
> > Sent: Wednesday, October 23, 2013 8:57 PM
> > To: address@hidden
> > Cc: address@hidden; address@hidden; address@hidden
> > Subject: [PATCH] arm_gic: Keep track of GICD_CPENDR and GICD_SPENDR
> > 
> > If software writes to the ISPENDR and sets the pending state of a level-
> > triggered interrupt, the falling edge of the hardware input must not clear 
> > the
> > pending state.  Conversely, if software writes to the ICPENDR, the pending 
> > state
> > of a level-triggered interrupt should only be cleared if the hardware input 
> > is
> > not asserted.
> > 
> > This requires an extra state variable to keep track of software writes.
> > 
> > Signed-off-by: Christoffer Dall <address@hidden>
> > ---
> >  hw/intc/arm_gic.c        | 20 +++++++++++++++++---
> >  hw/intc/arm_gic_common.c |  5 +++--
> >  hw/intc/gic_internal.h   |  4 ++++
> >  3 files changed, 24 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c index d1ddac1..db54061 
> > 100644
> > --- a/hw/intc/arm_gic.c
> > +++ b/hw/intc/arm_gic.c
> > @@ -101,6 +101,12 @@ static void gic_clear_pending(GICState *s, int irq, 
> > int cm,
> > uint8_t src)  {
> >      unsigned cpu;
> > 
> > +    /* If a level-triggered interrupt has been set to pending through the
> > +     * GICD_SPENDR, then a falling edge does not clear the pending state.
> > +     */
> > +    if (GIC_TEST_SW_PENDING(irq, cm))
> > +        return;
> > +
> >      GIC_CLEAR_PENDING(irq, cm);
> >      if (irq < GIC_NR_SGIS) {
> >          cpu = (unsigned)ffs(cm) - 1;
> > @@ -177,8 +183,9 @@ uint32_t gic_acknowledge_irq(GICState *s, int cpu)
> >      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(s, new_irq, GIC_TEST_MODEL(new_irq) ? ALL_CPU_MASK : 
> > cm,
> > -                                  GIC_SGI_SRC(new_irq, cpu));
> > +    cm = GIC_TEST_MODEL(new_irq) ? ALL_CPU_MASK : cm;
> > +    GIC_CLEAR_SW_PENDING(new_irq, cm);
> > +    gic_clear_pending(s, new_irq, cm, GIC_SGI_SRC(new_irq, cpu));
> >      gic_set_running_irq(s, cpu, new_irq);
> >      DPRINTF("ACK %d\n", new_irq);
> >      return new_irq;
> > @@ -445,16 +452,23 @@ static void gic_dist_writeb(void *opaque, hwaddr 
> > offset,
> >          for (i = 0; i < 8; i++) {
> >              if (value & (1 << i)) {
> >                  GIC_SET_PENDING(irq + i, GIC_TARGET(irq + i));
> > +                if (!GIC_TEST_TRIGGER(irq + i)) {
> > +                    GIC_SET_SW_PENDING(irq + i, GIC_TARGET(irq + i));
> > +                }
> >              }
> >          }
> >      } else if (offset < 0x300) {
> > +        int cm = (1 << cpu);
> >          /* Interrupt Clear Pending.  */
> >          irq = (offset - 0x280) * 8 + GIC_BASE_IRQ;
> >          if (irq >= s->num_irq)
> >              goto bad_reg;
> >          for (i = 0; i < 8; i++, irq++) {
> >              if (irq > GIC_NR_SGIS && value & (1 << i)) {
> > -                gic_clear_pending(s, irq, 1 << cpu, 0);
> > +                GIC_CLEAR_SW_PENDING(irq, cm);
> > +                if (GIC_TEST_TRIGGER(irq + i) || !GIC_TEST_LEVEL(irq, cm)) 
> > {
> > +                    GIC_CLEAR_PENDING(irq, cm);
> > +                }
> >              }
> >          }
> >      } else if (offset < 0x400) {
> > diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c index
> > 1d3b738..7f0615f 100644
> > --- a/hw/intc/arm_gic_common.c
> > +++ b/hw/intc/arm_gic_common.c
> > @@ -43,11 +43,12 @@ static int gic_post_load(void *opaque, int version_id)
> > 
> >  static const VMStateDescription vmstate_gic_irq_state = {
> >      .name = "arm_gic_irq_state",
> > -    .version_id = 1,
> > -    .minimum_version_id = 1,
> > +    .version_id = 2,
> > +    .minimum_version_id = 2,
> >      .fields = (VMStateField[]) {
> >          VMSTATE_UINT8(enabled, gic_irq_state),
> >          VMSTATE_UINT8(pending, gic_irq_state),
> > +        VMSTATE_UINT8(sw_pending, gic_irq_state),
> >          VMSTATE_UINT8(active, gic_irq_state),
> >          VMSTATE_UINT8(level, gic_irq_state),
> >          VMSTATE_BOOL(model, gic_irq_state), diff --git 
> > a/hw/intc/gic_internal.h
> > b/hw/intc/gic_internal.h index f9133b9..173c607 100644
> > --- a/hw/intc/gic_internal.h
> > +++ b/hw/intc/gic_internal.h
> > @@ -43,6 +43,9 @@
> >  #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_SW_PENDING(irq, cm) s->irq_state[irq].sw_pending |=
> > +(cm) #define GIC_CLEAR_SW_PENDING(irq, cm) s->irq_state[irq].sw_pending
> > +&= ~(cm) #define GIC_TEST_SW_PENDING(irq, cm)
> > +((s->irq_state[irq].sw_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) @@ -65,6 
> > +68,7
> > @@ typedef struct gic_irq_state {
> >      /* The enable bits are only banked for per-cpu interrupts.  */
> >      uint8_t enabled;
> >      uint8_t pending;
> > +    uint8_t sw_pending; /* keep track of GICD_ISPENDR and GICD_ICPENDR
> > + writes */
> >      uint8_t active;
> >      uint8_t level;
> >      bool model; /* 0 = N:N, 1 = 1:N */
> > --
> > 1.8.1.2
> > 
> > _______________________________________________
> > kvmarm mailing list
> > address@hidden
> > https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
> 
> 

-- 
Christoffer