On 19 November 2013 06:18, 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.
>
> 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@xxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
thanks
-- PMM
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
