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

[Chen-Yu Tsai <wenst@chromium.org>]

Hi,

On Thu, Aug 12, 2021 at 2:31 AM Marc Zyngier <maz@kernel.org> wrote:
>
> + Alex, who introduced this.
>
> On Wed, 11 Aug 2021 18:15:05 +0100,
> Chen-Yu Tsai <wenst@chromium.org> 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;                                             \
>
> This doesn't reflect what the pseudocode says of a read of ICC_RPR_EL1
> AFAICS. When the priority is activated, it is indeed shifted. But a
> read of RPR does appear to shift things back (and you loose the lowest
> bit in the process). Please see 'aarch64/support/ICC_RPR_EL1' in the
> architecture spec.
>
> Can you confirm that SCR_EL3.FIQ is set on your system?

I gave this a test with mainline on an ROC-RK3399-PC. I figure this is
more available and stable than the MT8195 [1].

My board is running:

- ATF v2.4-561-g465af20ce (based on git commit 8078b5c5a) with console and
  some erratas enabled, but otherwise standard rk3399 config
  "-dirty" was due to the SCR_EL3 line.

NOTICE:  BL31: v2.4(debug):v2.4-561-g465af20ce-dirty
NOTICE:  BL31: Built : 22:59:42, Aug 16 2021
INFO:    GICv3 with legacy support detected.
INFO:    ARM GICv3 driver initialized in EL3
INFO:    Maximum SPI INTID supported: 287
INFO:    plat_rockchip_pmu_init(1628): pd status 3e
INFO:    BL31: Initializing runtime services
INFO:    BL31: cortex_a53: CPU workaround for 855873 was applied
INFO:    BL31: cortex_a53: CPU workaround for 1530924 was applied
INFO:    BL31: Preparing for EL3 exit to normal world
INFO:    BL31: SCR_EL3=0x238, FIQ not set
INFO:    Entry point address = 0x200000
INFO:    SPSR = 0x3c9

- kernel next-20210813 + "NMI as IPI" series + debug printks scattered
  in the GICv3 driver

GICv3: GIC: Using split EOI/Deactivate mode
GICv3: 256 SPIs implemented
GICv3: 0 Extended SPIs implemented
GICv3: Distributor has no Range Selector support
Root IRQ handler: gic_handle_irq
GICv3: 16 PPIs implemented
GICv3: CPU0: found redistributor 0 region 0:0x00000000fef00000
GICv3: Pseudo-NMIs enabled using relaxed ICC_PMR_EL1 synchronisation
GICv3: Priority bits (gic_get_pribits()): 5
GICv3: Group0 available (gic_has_group0())
GICv3: Distributor security enabled (gic_dist_security_disabled()
GICv3: non-secure priorities used (gic_nonsecure_priorities)
GICv3: gic_irq_nmi_setup start: irq 7
GICv3: interrupt 7 priority a0
GICv3: interrupt 7 priority 20

So the RK3399 actually works correctly _without_ my fix. The NMI backtrace
triggers going through `gic_handle_nmi()`. If my fix is applied, it goes
through `gic_handle_irq()` instead.


However the MT8195 needs this to work correctly.

 === ATF ===
NOTICE:  MT8195 bl31_setup
NOTICE:  BL31: v2.5(debug):
NOTICE:  BL31: Built : Wed Jul  7 11:17:50 UTC 2021
INFO:    GICv3 without legacy support detected.
INFO:    ARM GICv3 driver initialized in EL3
INFO:    Maximum SPI INTID supported: 895
NOTICE:  MT8195 spm_boot_init
INFO:    BL31: Initializing runtime services
INFO:    BL31: cortex_a55: CPU workaround for 1530923 was applied
INFO:    SPM: enable CPC mode
INFO:    mcdi ready for mcusys-off-idle and system suspend
INFO:    BL31: Preparing for EL3 exit to normal world
INFO:    Entry point address = 0x80000000
INFO:    SPSR = 0x8

 === kernel GICv3 ===
GICv3: GIC: Using split EOI/Deactivate mode
GICv3: 864 SPIs implemented
GICv3: 0 Extended SPIs implemented
GICv3: Distributor has no Range Selector support
GICv3: 16 PPIs implemented
GICv3: CPU0: found redistributor 0 region 0:0x000000000c040000
GICv3: Pseudo-NMIs enabled using relaxed ICC_PMR_EL1 synchronisation
GICv3: Priority bits: 5
GICv3: Group0 available
GICv3: Distributor security enabled
GICv3: non-secure priorities used
GICv3: gic_irq_nmi_setup start: irq 7
GICv3: interrupt 7 priority a0
GICv3: interrupt 7 priority 20

I will dig through our ATF code base and try to see what's different.


ChenYu


[1] On a side note, the latest Chrome OS kernel 5.10 for the MT8195 gives
    a spinlock recursion during boot and hangs if pseudo-NMIs are enabled.
    I had to dig through my reflog to find the early version I developed
    on.


> Thanks,
>
>         M.
>
> > +     })
> > +
> >  /* 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;
> >       }
>
>
> --
> Without deviation from the norm, progress is not possible.