From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:46304)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <peter.maydell@linaro.org>) id 1W9IuW-0000LN-Vf
	for qemu-devel@nongnu.org; Fri, 31 Jan 2014 13:33:57 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <peter.maydell@linaro.org>) id 1W9IuQ-0004u0-8k
	for qemu-devel@nongnu.org; Fri, 31 Jan 2014 13:33:52 -0500
Received: from mail-la0-f42.google.com ([209.85.215.42]:38365)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <peter.maydell@linaro.org>) id 1W9IuQ-0004to-1u
	for qemu-devel@nongnu.org; Fri, 31 Jan 2014 13:33:46 -0500
Received: by mail-la0-f42.google.com with SMTP id hr13so3768325lab.15
	for <qemu-devel@nongnu.org>; Fri, 31 Jan 2014 10:33:45 -0800 (PST)
MIME-Version: 1.0
In-Reply-To: <1390941165-2079-5-git-send-email-christoffer.dall@linaro.org>
References: <1390941165-2079-1-git-send-email-christoffer.dall@linaro.org>
	<1390941165-2079-5-git-send-email-christoffer.dall@linaro.org>
From: Peter Maydell <peter.maydell@linaro.org>
Date: Fri, 31 Jan 2014 18:33:25 +0000
Message-ID: <CAFEAcA9D9j73zoL7WZReto6cU3Xxrc=kOE5WUOV0C0GaYQw4mA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Subject: Re: [Qemu-devel] [PATCH v5 4/8] hw: arm_gic: Keep track of SGI
	sources
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Christoffer Dall <christoffer.dall@linaro.org>
Cc: Patch Tracking <patches@linaro.org>, QEMU Developers <qemu-devel@nongnu.org>, "kvmarm@lists.cs.columbia.edu" <kvmarm@lists.cs.columbia.edu>

On 28 January 2014 20:32, 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 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 <peter.maydell@linaro.org>

thanks
-- PMM