Re: [Qemu-devel] [RFC PATCH v2 3/6] hw: arm_gic: Keep track of SGI sources

[Peter Maydell]

On 26 September 2013 22:03, 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.

Shouldn't the state you have in sgi_source[][] be surfaced as the
GICD_CPENDSGIR/GICD_SPENDSGIR registers [for a v2 GIC; they don't
exist in v1]? It might then be better to represent the state in
our data structures in the same layout as the registers.

>
> Signed-off-by: Christoffer Dall <address@hidden>
>
> ---
>
> Changelog [v2]:
>  - Fixed endless loop bug
>  - Bump version_id and minimum_version_id on vmstate struct
> ---
>  hw/intc/arm_gic.c        |   41 ++++++++++++++++++++++++++++++++---------
>  hw/intc/arm_gic_common.c |    5 +++--
>  hw/intc/gic_internal.h   |    3 +++
>  3 files changed, 38 insertions(+), 11 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 7eaa55f..6470d37 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -97,6 +97,20 @@ 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)
> +{

I think that in the cases where we pass in 0 for src that the
irq can't be < GIC_NR_SGIS.

> +    unsigned cpu;
> +
> +    GIC_CLEAR_PENDING(irq, cm);
> +    if (irq < GIC_NR_SGIS) {
> +        cpu = (unsigned)ffs(cm) - 1;

If you used ctz32() rather than ffs() it would save you having to
subtract one all the time. Also, those unsigned casts are pretty
ugly: better to just make 'cpu' an int...

> +        while (cpu < NCPU) {
> +            s->sgi_source[irq][cpu] &= ~(1 << src);
> +            cpu = (unsigned)ffs(cm) - 1;
> +        }

...this still seems to be an infinite loop: cm isn't modified
inside the loop so cpu will always have the same value each time.

     610:       eb fe                   jmp    610 <gic_clear_pending+0x50>

> +    }

Are you sure the logic in this function is right? (ie that we
should only clear the sgi_source[][] bit for this source, and
not completely? If nothing else, I think the interrupt should
stay pending if some other source cpu still wants it to be
pending. That is, I think we need to track the pending status
separately for each (irq,target-cpu,source-cpu) separately for
SGIs. (I'm not totally sure I have this right though, the spec
is quite confusing.)

> +}
> +

>  /* Maximum number of possible CPU interfaces, determined by GIC architecture 
> */
>  #define NCPU 8
>
> @@ -58,6 +59,7 @@
>                                      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)

WARNING: line over 80 characters
#161: FILE: hw/intc/gic_internal.h:62:
+#define GIC_SGI_SRC(irq, cpu) (((irq) < GIC_NR_SGIS) ?
ffs(s->sgi_source[irq][cpu]) - 1 : 0)

-- PMM