[RFC,v3,03/10] hw: arm_gic: Keep track of SGI sources

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 <christoffer.dall@linaro.org>

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(-)

On 19 November 2013 06:18, Christoffer Dall <christoffer.dall@linaro.org> 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 <christoffer.dall@linaro.org>
>
> 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
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm


thanks
-- PMM