Re: [PATCH] irqchip/gic-v3: Fix priority comparison when non-secure priorities are used

On Fri, 20 Aug 2021 14:31:39 +0100,
Alexandru Elisei [off-list ref] wrote:

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.

Seems good so far. I tested it both in a VM, on a FIQ==1 host, and on
D05, which runs with FIQ==0. Can't be more broken than it was... ;-)

On 8/11/21 6:15 PM, Chen-Yu Tsai wrote:

quoted

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/ (local)

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 <redacted>

Let me fold this into the commit and push it out again.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel