On 26 September 2013 22:03, Christoffer Dall
<christoffer.dall@xxxxxxxxxx> 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 <christoffer.dall@xxxxxxxxxx>
>
> ---
>
> 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
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
