Messages in this thread | | | Date | Fri, 20 Aug 2021 14:55:20 +0100 | From | Marc Zyngier <> | Subject | 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 <alexandru.elisei@arm.com> 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: > > 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>
Let me fold this into the commit and push it out again.
Thanks,
M.
-- Without deviation from the norm, progress is not possible.
| |