LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Alexandru Elisei <alexandru.elisei@arm.com>
To: Chen-Yu Tsai <wenst@chromium.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Marc Zyngier <maz@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] irqchip/gic-v3: Fix priority comparison when non-secure priorities are used
Date: Fri, 20 Aug 2021 14:31:39 +0100	[thread overview]
Message-ID: <d279bf85-aeac-1745-9c65-911794343e36@arm.com> (raw)
In-Reply-To: <20210811171505.1502090-1-wenst@chromium.org>

Hello,

Pending Marc's testing (I realized I don't have any hardware to test this on at
the moment), this patch looks correct to me. One comment below.

On 8/11/21 6:15 PM, Chen-Yu Tsai wrote:
> When non-secure priorities are used, compared to the raw priority set,
> the value read back from RPR is also right-shifted by one and the
> highest bit set.
>
> Add a macro to do the modifications to the raw priority when doing the
> comparison against the RPR value. This corrects the pseudo-NMI behavior
> when non-secure priorities in the GIC are used. Tested on 5.10 with
> the "IPI as pseudo-NMI" series [1] applied on MT8195.
>
> [1] https://lore.kernel.org/linux-arm-kernel/1604317487-14543-1-git-send-email-sumit.garg@linaro.org/
>
> Fixes: 336780590990 ("irqchip/gic-v3: Support pseudo-NMIs when SCR_EL3.FIQ == 0")
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
>  drivers/irqchip/irq-gic-v3.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index e0f4debe64e1..e7a0b55413db 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -100,6 +100,15 @@ EXPORT_SYMBOL(gic_pmr_sync);
>  DEFINE_STATIC_KEY_FALSE(gic_nonsecure_priorities);
>  EXPORT_SYMBOL(gic_nonsecure_priorities);
>  
> +#define GICD_INT_RPR_PRI(priority)					\
> +	({								\
> +		u32 __priority = (priority);				\
> +		if (static_branch_unlikely(&gic_nonsecure_priorities))	\
> +			__priority = 0x80 | (__priority >> 1);		\
> +									\
> +		__priority;						\
> +	})

Would you mind adding a comment to the macro explaining why it's needed? This
behaviour is rather subtle and I'm hoping it will save someone's time at some
point in the future. I'm thinking something like this (please ignore it if you can
think of something better):

When the Non-secure world has access to group 0 interrupts (SCR_EL3.FIQ = 0),
reading the ICC_RPR_EL1 register will return the Distributor's view of the
interrupt priority. When GIC security is enabled (GICD_CTLR.DS = 0), the interrupt
priority written by software is moved to the Non-secure range by the Distributor.
If both are true (which is the situation where gic_nonsecure_priorities gets
enabled), then we need to shift down the priority programmed by software if we
want match it against the value returned from ICC_RPR_EL1.

With a comment added:

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

Thanks,

Alex

> +
>  /* ppi_nmi_refs[n] == number of cpus having ppi[n + 16] set as NMI */
>  static refcount_t *ppi_nmi_refs;
>  
> @@ -687,7 +696,7 @@ static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs
>  		return;
>  
>  	if (gic_supports_nmi() &&
> -	    unlikely(gic_read_rpr() == GICD_INT_NMI_PRI)) {
> +	    unlikely(gic_read_rpr() == GICD_INT_RPR_PRI(GICD_INT_NMI_PRI))) {
>  		gic_handle_nmi(irqnr, regs);
>  		return;
>  	}

  parent reply	other threads:[~2021-08-20 13:30 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-11 17:15 Chen-Yu Tsai
2021-08-11 18:31 ` Marc Zyngier
2021-08-12 11:51   ` Alexandru Elisei
2021-08-12 13:09     ` Marc Zyngier
2021-08-12 14:24       ` Alexandru Elisei
2021-08-20 12:58         ` Marc Zyngier
2021-08-16 15:11   ` Chen-Yu Tsai
2021-08-20 13:31 ` Alexandru Elisei [this message]
2021-08-20 13:55   ` Marc Zyngier
2021-08-20 13:34 ` [irqchip: irq/irqchip-next] " irqchip-bot for Chen-Yu Tsai
2021-08-20 14:05 ` irqchip-bot for Chen-Yu Tsai

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=d279bf85-aeac-1745-9c65-911794343e36@arm.com \
    --to=alexandru.elisei@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maz@kernel.org \
    --cc=tglx@linutronix.de \
    --cc=wenst@chromium.org \
    --subject='Re: [PATCH] irqchip/gic-v3: Fix priority comparison when non-secure priorities are used' \
    /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

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 NNTP newsgroup(s).