Re: [Qemu-devel] [RFC PATCH v3 03/10] hw: arm_gic: Keep track of SGI sources

[Peter Maydell]

On 19 November 2013 06:18, Christoffer Dall <address@hidden> wrote:
> Right now the arm gic emulation doesn't keep track of the source of an
> SGI (which apparently Linux guests don't use, or they're fine with
> assuming CPU 0 always).
>
> Add the necessary matrix on the GICState structure and maintain the data
> when setting and clearing the pending state of an IRQ.
>
> Note that we always choose to present the source as the lowest-numbered
> CPU in case multiple cores have signalled the same SGI number to a core
> on the system.
>
> Also fixes a subtle ancient bug in handling of writes to the GICD_SPENDr
> where the irq variable was set to 0 if the IRQ was an SGI (irq < 16),
> but the intention was to set the written value, the "value" variable, to
> 0.  Make the check explicit instead and ignore any such writes.
>
> Signed-off-by: Christoffer Dall <address@hidden>
>
> Changelog [v3]:
>  - Changed ffs(x) - 1 to ctz32
>  - Changed cpu type to int in gic_clear_pending to avoid cast
>  - Really try to fix the endless loop bug
>  - Change gic_clear_pending to only clear the pending bit of SGIs if all
>    CPUs do not have that IRQ pending from any CPUs.
>  - Wrap long line in gic_internal.h
>  - Fix bug allowing setting SGIs through the GICD_SPENDR
>
> Changelog [v2]:
>  - Fixed endless loop bug
>  - Bump version_id and minimum_version_id on vmstate struct
> ---
>  hw/intc/arm_gic.c                | 62 
> ++++++++++++++++++++++++++++++----------
>  hw/intc/arm_gic_common.c         |  5 ++--
>  hw/intc/gic_internal.h           |  3 ++
>  include/hw/intc/arm_gic_common.h |  2 ++
>  4 files changed, 55 insertions(+), 17 deletions(-)

I realised that the way we store the state in the GICState struct
isn't actually very far from the way the hardware
does (ie like the banked GICD_SPENDSGIR*), but I had
to go look at the translate functions in the last patch
in this series to figure this out -- the GICD_SPENDSGIR*
are effectively the concatenation of the CPU's
sgi_source[irq][cpu] bits. I think we could make this a lot
more obvious by choice of struct member name and with a
comment in the GICState struct:

    /* For each SGI on the target CPU, we store 8 bits
     * indicating which source CPUs have made this SGI
     * pending on the target CPU. These correspond to
     * the bytes in the GIC_SPENDSGIR* registers as
     * read by the target CPU.
     */
    uint8_t sgi_pending[GIC_NR_SGIS][GIC_NCPU];

>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 7eaa55f..2ed9a1a 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -97,6 +97,32 @@ void gic_set_pending_private(GICState *s, int cpu, int irq)
>      gic_update(s);
>  }
>
> +static void gic_clear_pending(GICState *s, int irq, int cm, uint8_t src)
> +{
> +    int cpu;
> +
> +    DPRINTF("%s: irq %d, cm 0x%x, src %d\n", __func__, irq, cm, src);
> +    if (irq < GIC_NR_SGIS) {
> +        bool pend = false;
> +
> +        for (cpu = 0; cpu < s->num_cpu; cpu++) {
> +            if (cm & (1 << cpu)) {
> +                s->sgi_source[irq][cpu] &= ~(1 << src);
> +            } else {
> +                if (s->sgi_source[irq][cpu]) {
> +                    pend = true;
> +                }
> +            }
> +        }

This loop looks very odd. In general we should only be
clearing sgi_source[][] bits in two cases:
 (1) CPU X has just read its GICC_IAR for an interrupt ID,
     which happens to be an SGI with source CPU Y
 (2) CPU X wrote to its GICD_CPENDSGIR register

In the current code there is also a code path where we
might think we want to clear a pending SGI which
comes from gic_set_irq(). However this should I think
never actually happen, because it would represent
hardware setting a software-only interrupt. We should
probably assert in gic_set_irq() that the irq number
doesn't represent an SGI [or change everything using
GIC interrupt numbers to a different convention which
doesn't have irq line numbers for the SGIs, but urgh.].

So in either case we should know exactly both the
source CPU and the destination CPU number, so don't
need to loop around doing "for each cpu maybe
clear a bit" at all.

The destination CPU's overall pending status for
the SGI, which is the other thing we need to know,
is just the logical OR of the per-source pending
bits, so conveniently is just
  (s->sgi_source[irq][destcpu] != 0)

> +
> +        if (!pend) {
> +            GIC_CLEAR_PENDING(irq, cm);
> +        }
> +    } else {
> +        GIC_CLEAR_PENDING(irq, cm);
> +    }



> +}
> +
>  /* Process a change in an external IRQ input.  */
>  static void gic_set_irq(void *opaque, int irq, int level)
>  {
> @@ -132,7 +158,7 @@ static void gic_set_irq(void *opaque, int irq, int level)
>          GIC_SET_PENDING(irq, target);
>      } else {
>          if (!GIC_TEST_TRIGGER(irq)) {
> -            GIC_CLEAR_PENDING(irq, target);
> +            gic_clear_pending(s, irq, target, 0);
>          }
>          GIC_CLEAR_LEVEL(irq, cm);
>      }
> @@ -163,7 +189,8 @@ 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(new_irq, GIC_TEST_MODEL(new_irq) ? ALL_CPU_MASK : cm);
> +    gic_clear_pending(s, new_irq, GIC_TEST_MODEL(new_irq) ? ALL_CPU_MASK : 
> cm,
> +                                  GIC_SGI_SRC(new_irq, cpu));
>      gic_set_running_irq(s, cpu, new_irq);
>      DPRINTF("ACK %d\n", new_irq);
>      return new_irq;
> @@ -424,12 +451,9 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
>          irq = (offset - 0x200) * 8 + GIC_BASE_IRQ;
>          if (irq >= s->num_irq)
>              goto bad_reg;
> -        if (irq < 16)
> -          irq = 0;
> -
> -        for (i = 0; i < 8; i++) {
> -            if (value & (1 << i)) {
> -                GIC_SET_PENDING(irq + i, GIC_TARGET(irq + i));
> +        for (i = 0; i < 8; i++, irq++) {
> +            if (irq >= GIC_NR_SGIS && value & (1 << i)) {

You might as well haul this test out of the loop, we know we
won't cross the SGI-to-not-SGI boundary in the middle of a byte.
Ditto below.

> +                GIC_SET_PENDING(irq, GIC_TARGET(irq));
>              }
>          }
>      } else if (offset < 0x300) {
> @@ -437,12 +461,9 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
>          irq = (offset - 0x280) * 8 + GIC_BASE_IRQ;
>          if (irq >= s->num_irq)
>              goto bad_reg;
> -        for (i = 0; i < 8; i++) {
> -            /* ??? This currently clears the pending bit for all CPUs, even
> -               for per-CPU interrupts.  It's unclear whether this is the
> -               corect behavior.  */
> -            if (value & (1 << i)) {
> -                GIC_CLEAR_PENDING(irq + i, ALL_CPU_MASK);
> +        for (i = 0; i < 8; i++, irq++) {
> +            if (irq >= GIC_NR_SGIS && value & (1 << i)) {
> +                gic_clear_pending(s, irq, 1 << cpu, 0);
>              }
>          }
>      } else if (offset < 0x400) {
> @@ -515,6 +536,7 @@ static void gic_dist_writel(void *opaque, hwaddr offset,
>          int cpu;
>          int irq;
>          int mask;
> +        unsigned target_cpu;
>
>          cpu = gic_get_current_cpu(s);
>          irq = value & 0x3ff;
> @@ -534,6 +556,12 @@ static void gic_dist_writel(void *opaque, hwaddr offset,
>              break;
>          }
>          GIC_SET_PENDING(irq, mask);
> +        target_cpu = (unsigned)ffs(mask) - 1;
> +        while (target_cpu < GIC_NCPU) {
> +            s->sgi_source[irq][target_cpu] |= (1 << cpu);
> +            mask &= ~(1 << target_cpu);
> +            target_cpu = (unsigned)ffs(mask) - 1;
> +        }
>          gic_update(s);
>          return;
>      }
> @@ -551,6 +579,8 @@ static const MemoryRegionOps gic_dist_ops = {
>
>  static uint32_t gic_cpu_read(GICState *s, int cpu, int offset)
>  {
> +    int value;
> +
>      switch (offset) {
>      case 0x00: /* Control */
>          return s->cpu_enabled[cpu];
> @@ -560,7 +590,9 @@ static uint32_t gic_cpu_read(GICState *s, int cpu, int 
> offset)
>          /* ??? Not implemented.  */
>          return 0;
>      case 0x0c: /* Acknowledge */
> -        return gic_acknowledge_irq(s, cpu);
> +        value = gic_acknowledge_irq(s, cpu);
> +        value |= (GIC_SGI_SRC(value, cpu) & 0x7) << 10;

We should just have gic_acknowledge_irq() return the
value with the source CPU bits set correctly -- after
all that function already needs to know which source
CPU it's selected in order to clear the right SGI
pending bit.

> +        return value;
>      case 0x14: /* Running Priority */
>          return s->running_priority[cpu];
>      case 0x18: /* Highest Pending Interrupt */
> diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
> index c765850..d1a1b0f 100644
> --- a/hw/intc/arm_gic_common.c
> +++ b/hw/intc/arm_gic_common.c
> @@ -58,8 +58,8 @@ static const VMStateDescription vmstate_gic_irq_state = {
>
>  static const VMStateDescription vmstate_gic = {
>      .name = "arm_gic",
> -    .version_id = 4,
> -    .minimum_version_id = 4,
> +    .version_id = 5,
> +    .minimum_version_id = 5,
>      .pre_save = gic_pre_save,
>      .post_load = gic_post_load,
>      .fields = (VMStateField[]) {
> @@ -71,6 +71,7 @@ static const VMStateDescription vmstate_gic = {
>          VMSTATE_UINT8_2DARRAY(priority1, GICState, GIC_INTERNAL, GIC_NCPU),
>          VMSTATE_UINT8_ARRAY(priority2, GICState, GIC_MAXIRQ - GIC_INTERNAL),
>          VMSTATE_UINT16_2DARRAY(last_active, GICState, GIC_MAXIRQ, GIC_NCPU),
> +        VMSTATE_UINT8_2DARRAY(sgi_source, GICState, GIC_NR_SGIS, GIC_NCPU),
>          VMSTATE_UINT16_ARRAY(priority_mask, GICState, GIC_NCPU),
>          VMSTATE_UINT16_ARRAY(running_irq, GICState, GIC_NCPU),
>          VMSTATE_UINT16_ARRAY(running_priority, GICState, GIC_NCPU),
> diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
> index f916725..3d36653 100644
> --- a/hw/intc/gic_internal.h
> +++ b/hw/intc/gic_internal.h
> @@ -51,6 +51,9 @@
>                                      s->priority1[irq][cpu] :            \
>                                      s->priority2[(irq) - GIC_INTERNAL])
>  #define GIC_TARGET(irq) s->irq_target[irq]
> +#define GIC_SGI_SRC(irq, cpu) (((irq) < GIC_NR_SGIS) ? \
> +                               ffs(s->sgi_source[irq][cpu]) - 1 : \
> +                               0)

You could add a comment that it is IMPDEF which CPU we pick
when multiple source CPUs have asserted an SGI simultaneously.

Also using ctz32() would avoid that "- 1".

I assume we never call this macro unless there's really
guaranteed to be a pending bit in the sgi_source[][]
entry, but it's not totally obvious this is true :-)

>  /* The special cases for the revision property: */
>  #define REV_11MPCORE 0
> diff --git a/include/hw/intc/arm_gic_common.h 
> b/include/hw/intc/arm_gic_common.h
> index ed18bb8..e19481b 100644
> --- a/include/hw/intc/arm_gic_common.h
> +++ b/include/hw/intc/arm_gic_common.h
> @@ -27,6 +27,7 @@
>  #define GIC_MAXIRQ 1020
>  /* First 32 are private to each CPU (SGIs and PPIs). */
>  #define GIC_INTERNAL 32
> +#define GIC_NR_SGIS  16
>  /* Maximum number of possible CPU interfaces, determined by GIC architecture 
> */
>  #define GIC_NCPU 8
>
> @@ -54,6 +55,7 @@ typedef struct GICState {
>      uint8_t priority1[GIC_INTERNAL][GIC_NCPU];
>      uint8_t priority2[GIC_MAXIRQ - GIC_INTERNAL];
>      uint16_t last_active[GIC_MAXIRQ][GIC_NCPU];
> +    uint8_t sgi_source[GIC_NR_SGIS][GIC_NCPU];
>
>      uint16_t priority_mask[GIC_NCPU];
>      uint16_t running_irq[GIC_NCPU];
> --
> 1.8.4.3
>
> _______________________________________________
> kvmarm mailing list
> address@hidden
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm


thanks
-- PMM