| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAGXv+5GjVZ582u29_bbSKoLjb00vtvPo_ACn4WKDEfFiPULxPQ@mail.gmail.com>
Date: Mon, 16 Aug 2021 23:11:50 +0800
From: Chen-Yu Tsai <wenst@...omium.org>
To: Marc Zyngier <maz@...nel.org>
Cc: Thomas Gleixner <tglx@...utronix.de>,
"moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE"
<linux-arm-kernel@...ts.infradead.org>,
LKML <linux-kernel@...r.kernel.org>,
Alexandru Elisei <Alexandru.Elisei@....com>
Subject: Re: [PATCH] irqchip/gic-v3: Fix priority comparison when non-secure
priorities are used
Hi,
On Thu, Aug 12, 2021 at 2:31 AM Marc Zyngier <maz@...nel.org> wrote:
>
> + Alex, who introduced this.
>
> On Wed, 11 Aug 2021 18:15:05 +0100,
> Chen-Yu Tsai <wenst@...omium.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@...omium.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.
Powered by blists - more mailing lists