Re: [Qemu-devel] [PATCH v5 4/8] hw: arm_gic: Keep track of SGI sources

[Peter Maydell]

On 28 January 2014 20:32, 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 and make the state
> visible to the guest.
>
> 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.

> @@ -531,9 +576,29 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
>                  GIC_CLEAR_EDGE_TRIGGER(irq + i);
>              }
>          }
> -    } else {
> +    } else if (offset < 0xf10) {
>          /* 0xf00 is only handled for 32-bit writes.  */
>          goto bad_reg;
> +    } else if (offset < 0xf20) {
> +        /* GICD_CPENDSGIRn */
> +        if (s->revision == REV_11MPCORE || s->revision == REV_NVIC) {
> +            goto bad_reg;
> +        }
> +        irq = (offset - 0xf10);
> +
> +        GIC_CLEAR_PENDING(irq, 1 << cpu);
> +        s->sgi_pending[irq][cpu] &= ~value;

This doesn't look quite right. If the SGI is pending
from multiple source CPUs and we use CPENDSGIRn to
clear the bits corresponding to only some of those
source CPUs, then the interrupt as a whole should stay
pending on this (target) CPU. I think this is:

    s->sgi_pending[irq][cpu] &= ~value;
    if (s->sgi_pending[irq][cpu] == 0) {
        GIC_CLEAR_PENDING(irq, 1 << cpu);
    }

(compare the code in gic_acknowledge_irq())

If you fix that, then
Reviewed-by: Peter Maydell <address@hidden>

thanks
-- PMM