From: Luc Michel <luc@lmichel.fr>
To: Sebastian Huber <sebastian.huber@embedded-brains.de>
Cc: qemu-arm@nongnu.org, qemu-devel@nongnu.org
Subject: Re: [PATCH] hw/intc/arm_gic: Fix set/clear pending of PPI/SPI
Date: Sun, 25 Jul 2021 10:08:17 +0200 [thread overview]
Message-ID: <20210725080817.ivlkutnow7sojoyd@sekoia-pc.home.lmichel.fr> (raw)
In-Reply-To: <20210709094948.60344-1-sebastian.huber@embedded-brains.de>
Hi Sebastian,
On 11:49 Fri 09 Jul , Sebastian Huber wrote:
> According to the GICv3 specification register GICD_ISPENDR0 is Banked for each
You're referring to GICv3 but actually modifying GICv2 model. Having a
look at GICv2 reference manual, your affirmation still hold though.
> connected PE with GICR_TYPER.Processor_Number < 8. For Qemu this is the case
> since GIC_NCPU == 8.
This is GICv3 specific. For GICv2 the architectural limit is 8 CPUs.
>
> For SPI, make the interrupt pending on all CPUs and not just the processor
> targets of the interrupt.
So you're not referring to GICD_ISPENDR0 anymore right? SPIs starts at
IRQ number 32. GICD_ISPENDR0 is for IRQs 0 to 31, which are SGIs and
PPIs (This is why this reg is banked, meaning that a CPU can only
trigger a PPI of its own). Maybe make it clear in your commit message
that you are now talking about GICD_ISPENDRn with n > 0
Moreover your statement regarding SPIs seems weird to me. Setting an
SPI pending (in GICD_ISPENDRn with n > 0) should really be like having
it being triggered from the IRQ line. It makes it pending in the
distributor. The distributor then forward it as normal. Why the
GICD_ITARGETSRn configuration should be ignored in this case? At least I
can't find any reference to such a behaviour in the reference manual.
>
> This behaviour is at least present on the i.MX7D which uses an Cortex-A7MPCore.
Which has a GICv2, not a v3 right?
>
> Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de>
> ---
> hw/intc/arm_gic.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index a994b1f024..8e377bac59 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -1294,12 +1294,14 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
>
> for (i = 0; i < 8; i++) {
> if (value & (1 << i)) {
> + int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
> +
Indeed, I think the current implementation for PPIs is wrong
(GIC_DIST_TARGET(irq + i) is probably 0 in this case, so a set pending
request for a PPI will get incorrectly ignored). So I agree with the irq <
GIC_INTERNAL case. But for SPIs my concerns still hold (see my comment
in the commit message).
> if (s->security_extn && !attrs.secure &&
> !GIC_DIST_TEST_GROUP(irq + i, 1 << cpu)) {
> continue; /* Ignore Non-secure access of Group0 IRQ */
> }
>
> - GIC_DIST_SET_PENDING(irq + i, GIC_DIST_TARGET(irq + i));
> + GIC_DIST_SET_PENDING(irq + i, cm);
> }
> }
> } else if (offset < 0x300) {
> @@ -1317,11 +1319,10 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
> continue; /* Ignore Non-secure access of Group0 IRQ */
> }
>
> - /* ??? 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_DIST_CLEAR_PENDING(irq + i, ALL_CPU_MASK);
> + int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
> +
> + GIC_DIST_CLEAR_PENDING(irq + i, cm);
I agree with this change too, but you are modifying the GICD_ICPENDRn
register behaviour without mentioning it in the commit message.
Thanks,
--
Luc
next prev parent reply other threads:[~2021-07-25 8:07 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-09 9:49 [PATCH] hw/intc/arm_gic: Fix set/clear pending of PPI/SPI Sebastian Huber
2021-07-23 14:04 ` Sebastian Huber
2021-07-23 14:12 ` Philippe Mathieu-Daudé
2021-07-25 8:08 ` Luc Michel [this message]
2021-07-26 8:04 ` Sebastian Huber
2021-07-26 13:02 ` Peter Maydell
2024-05-07 12:56 ` [PATCH 1/2] hw/intc/arm_gic: Fix set pending of PPIs Sebastian Huber
2024-05-07 12:56 ` [PATCH 2/2] hw/intc/arm_gic: Fix writes to GICD_ITARGETSRn Sebastian Huber
2024-05-07 13:00 ` [PATCH 1/2] hw/intc/arm_gic: Fix set pending of PPIs Sebastian Huber
2024-05-20 13:27 ` Peter Maydell
2024-05-07 13:00 ` [PATCH 2/2] hw/intc/arm_gic: Fix writes to GICD_ITARGETSRn Sebastian Huber
2024-05-20 13:33 ` Peter Maydell
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210725080817.ivlkutnow7sojoyd@sekoia-pc.home.lmichel.fr \
--to=luc@lmichel.fr \
--cc=qemu-arm@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=sebastian.huber@embedded-brains.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).