On Mon, Oct 02, 2023 at 09:45:29AM -0700, Douglas Anderson wrote:
> Some mediatek devices have the property
> "mediatek,broken-save-restore-fw" in their GIC. This means that,
> although the hardware supports pseudo-NMI, the firmware has a bug
> that blocks enabling it. When we're in this state,
> system_uses_irq_prio_masking() will return true but we'll fail to
> actually enable the IRQ in the GIC.
>
> Let's make the code handle this. We'll detect that we failed to
> request an IPI as NMI and fallback to requesting it normally. Though
> we expect that either all of our requests will fail or all will
> succeed, it's just as cheap to keep a per-IPI bitmap and that keeps us
> robust.
>
> Fixes: 331a1b3a836c ("arm64: smp: Add arch support for backtrace using pseudo-NMI")
> Reported-by: Chen-Yu Tsai <[email protected]>
> Closes: https://issuetracker.google.com/issues/197061987#comment68
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
>
> arch/arm64/kernel/smp.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
I'm not too keen on falling back here when we have no idea why the request failed.
I'd prefer if we could check the `supports_pseudo_nmis` static key directly to
account for the case of broken FW, e.g. as below.
Mark.
---->8----
From 72fdec05c64a74f21871b44c7c760bbe07cac044 Mon Sep 17 00:00:00 2001
From: Mark Rutland <[email protected]>
Date: Mon, 2 Oct 2023 18:00:36 +0100
Subject: [PATCH] arm64: smp: avoid NMI IPIs with broken MediaTek FW
Some MediaTek devices have broken firmware which corrupts some GICR
registers behind the back of the OS, and pseudo-NMIs cannot be used on
these devices. For more details see commit:
44bd78dd2b8897f5 ("irqchip/gic-v3: Disable pseudo NMIs on Mediatek devices w/ firmware issues")
We did not take this problem into account in commit:
331a1b3a836c0f38 ("arm64: smp: Add arch support for backtrace using pseudo-NMI")
Since that commit arm64's SMP code will try to setup some IPIs as
pseudo-NMIs, even on systems with broken FW. The GICv3 code will
(rightly) reject attempts to request interrupts as pseudo-NMIs,
resulting in boot-time failures.
Avoid the problem by taking the broken FW into account when deciding to
request IPIs as pseudo-NMIs. The GICv3 driver maintains a static_key
named "supports_pseudo_nmis" which is false on systems with broken FW,
and we can consult this within ipi_should_be_nmi().
Fixes: 331a1b3a836c0f38 ("arm64: smp: Add arch support for backtrace using pseudo-NMI")
Reported-by: Chen-Yu Tsai <[email protected]>
Closes: https://issuetracker.google.com/issues/197061987#comment68
Signed-off-by: Mark Rutland <[email protected]>
Cc: Douglas Anderson <[email protected]>
Cc: Marc Zyngier <[email protected]>
---
arch/arm64/kernel/smp.c | 5 ++++-
drivers/irqchip/irq-gic-v3.c | 2 +-
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 814d9aa93b21b..061c69160f90f 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -964,7 +964,10 @@ static void smp_cross_call(const struct cpumask *target, unsigned int ipinr)
static bool ipi_should_be_nmi(enum ipi_msg_type ipi)
{
- if (!system_uses_irq_prio_masking())
+ DECLARE_STATIC_KEY_FALSE(supports_pseudo_nmis);
+
+ if (!system_uses_irq_prio_masking() ||
+ !static_branch_likely(&supports_pseudo_nmis))
return false;
switch (ipi) {
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 787ccc880b22d..737da1b9aabf2 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -106,7 +106,7 @@ static DEFINE_STATIC_KEY_TRUE(supports_deactivate_key);
* - Figure 4-7 Secure read of the priority field for a Non-secure Group 1
* interrupt.
*/
-static DEFINE_STATIC_KEY_FALSE(supports_pseudo_nmis);
+DEFINE_STATIC_KEY_FALSE(supports_pseudo_nmis);
DEFINE_STATIC_KEY_FALSE(gic_nonsecure_priorities);
EXPORT_SYMBOL(gic_nonsecure_priorities);
--
2.30.2
Hi,
On Mon, Oct 2, 2023 at 10:24 AM Mark Rutland <[email protected]> wrote:
>
> On Mon, Oct 02, 2023 at 09:45:29AM -0700, Douglas Anderson wrote:
> > Some mediatek devices have the property
> > "mediatek,broken-save-restore-fw" in their GIC. This means that,
> > although the hardware supports pseudo-NMI, the firmware has a bug
> > that blocks enabling it. When we're in this state,
> > system_uses_irq_prio_masking() will return true but we'll fail to
> > actually enable the IRQ in the GIC.
> >
> > Let's make the code handle this. We'll detect that we failed to
> > request an IPI as NMI and fallback to requesting it normally. Though
> > we expect that either all of our requests will fail or all will
> > succeed, it's just as cheap to keep a per-IPI bitmap and that keeps us
> > robust.
> >
> > Fixes: 331a1b3a836c ("arm64: smp: Add arch support for backtrace using pseudo-NMI")
> > Reported-by: Chen-Yu Tsai <[email protected]>
> > Closes: https://issuetracker.google.com/issues/197061987#comment68
> > Signed-off-by: Douglas Anderson <[email protected]>
> > ---
> >
> > arch/arm64/kernel/smp.c | 19 ++++++++++++-------
> > 1 file changed, 12 insertions(+), 7 deletions(-)
>
> I'm not too keen on falling back here when we have no idea why the request failed.
>
> I'd prefer if we could check the `supports_pseudo_nmis` static key directly to
> account for the case of broken FW, e.g. as below.
>
> Mark.
>
> ---->8----
> From 72fdec05c64a74f21871b44c7c760bbe07cac044 Mon Sep 17 00:00:00 2001
> From: Mark Rutland <[email protected]>
> Date: Mon, 2 Oct 2023 18:00:36 +0100
> Subject: [PATCH] arm64: smp: avoid NMI IPIs with broken MediaTek FW
>
> Some MediaTek devices have broken firmware which corrupts some GICR
> registers behind the back of the OS, and pseudo-NMIs cannot be used on
> these devices. For more details see commit:
>
> 44bd78dd2b8897f5 ("irqchip/gic-v3: Disable pseudo NMIs on Mediatek devices w/ firmware issues")
>
> We did not take this problem into account in commit:
>
> 331a1b3a836c0f38 ("arm64: smp: Add arch support for backtrace using pseudo-NMI")
>
> Since that commit arm64's SMP code will try to setup some IPIs as
> pseudo-NMIs, even on systems with broken FW. The GICv3 code will
> (rightly) reject attempts to request interrupts as pseudo-NMIs,
> resulting in boot-time failures.
>
> Avoid the problem by taking the broken FW into account when deciding to
> request IPIs as pseudo-NMIs. The GICv3 driver maintains a static_key
> named "supports_pseudo_nmis" which is false on systems with broken FW,
> and we can consult this within ipi_should_be_nmi().
>
> Fixes: 331a1b3a836c0f38 ("arm64: smp: Add arch support for backtrace using pseudo-NMI")
> Reported-by: Chen-Yu Tsai <[email protected]>
> Closes: https://issuetracker.google.com/issues/197061987#comment68
> Signed-off-by: Mark Rutland <[email protected]>
> Cc: Douglas Anderson <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> ---
> arch/arm64/kernel/smp.c | 5 ++++-
> drivers/irqchip/irq-gic-v3.c | 2 +-
> 2 files changed, 5 insertions(+), 2 deletions(-)
Sure, this is OK w/ me as long as folks don't mind accessing the
global here, it's OK w/ me:
Reviewed-by: Douglas Anderson <[email protected]>
It seems to work for me, thus:
Tested-by: Douglas Anderson <[email protected]>
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index 814d9aa93b21b..061c69160f90f 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -964,7 +964,10 @@ static void smp_cross_call(const struct cpumask *target, unsigned int ipinr)
>
> static bool ipi_should_be_nmi(enum ipi_msg_type ipi)
> {
> - if (!system_uses_irq_prio_masking())
> + DECLARE_STATIC_KEY_FALSE(supports_pseudo_nmis);
> +
> + if (!system_uses_irq_prio_masking() ||
> + !static_branch_likely(&supports_pseudo_nmis))
One thought, actually, is whether we should actually change
system_uses_irq_prio_masking() to return the correct value. What do
you think?
-Doug
On Mon, Oct 02, 2023 at 12:16:17PM -0700, Doug Anderson wrote:
> Hi,
>
> On Mon, Oct 2, 2023 at 10:24 AM Mark Rutland <[email protected]> wrote:
> >
> > On Mon, Oct 02, 2023 at 09:45:29AM -0700, Douglas Anderson wrote:
> > > Some mediatek devices have the property
> > > "mediatek,broken-save-restore-fw" in their GIC. This means that,
> > > although the hardware supports pseudo-NMI, the firmware has a bug
> > > that blocks enabling it. When we're in this state,
> > > system_uses_irq_prio_masking() will return true but we'll fail to
> > > actually enable the IRQ in the GIC.
> > >
> > > Let's make the code handle this. We'll detect that we failed to
> > > request an IPI as NMI and fallback to requesting it normally. Though
> > > we expect that either all of our requests will fail or all will
> > > succeed, it's just as cheap to keep a per-IPI bitmap and that keeps us
> > > robust.
> > >
> > > Fixes: 331a1b3a836c ("arm64: smp: Add arch support for backtrace using pseudo-NMI")
> > > Reported-by: Chen-Yu Tsai <[email protected]>
> > > Closes: https://issuetracker.google.com/issues/197061987#comment68
> > > Signed-off-by: Douglas Anderson <[email protected]>
> > > ---
> > >
> > > arch/arm64/kernel/smp.c | 19 ++++++++++++-------
> > > 1 file changed, 12 insertions(+), 7 deletions(-)
> >
> > I'm not too keen on falling back here when we have no idea why the request failed.
> >
> > I'd prefer if we could check the `supports_pseudo_nmis` static key directly to
> > account for the case of broken FW, e.g. as below.
> >
> > Mark.
> >
> > ---->8----
> > From 72fdec05c64a74f21871b44c7c760bbe07cac044 Mon Sep 17 00:00:00 2001
> > From: Mark Rutland <[email protected]>
> > Date: Mon, 2 Oct 2023 18:00:36 +0100
> > Subject: [PATCH] arm64: smp: avoid NMI IPIs with broken MediaTek FW
> >
> > Some MediaTek devices have broken firmware which corrupts some GICR
> > registers behind the back of the OS, and pseudo-NMIs cannot be used on
> > these devices. For more details see commit:
> >
> > 44bd78dd2b8897f5 ("irqchip/gic-v3: Disable pseudo NMIs on Mediatek devices w/ firmware issues")
> >
> > We did not take this problem into account in commit:
> >
> > 331a1b3a836c0f38 ("arm64: smp: Add arch support for backtrace using pseudo-NMI")
> >
> > Since that commit arm64's SMP code will try to setup some IPIs as
> > pseudo-NMIs, even on systems with broken FW. The GICv3 code will
> > (rightly) reject attempts to request interrupts as pseudo-NMIs,
> > resulting in boot-time failures.
> >
> > Avoid the problem by taking the broken FW into account when deciding to
> > request IPIs as pseudo-NMIs. The GICv3 driver maintains a static_key
> > named "supports_pseudo_nmis" which is false on systems with broken FW,
> > and we can consult this within ipi_should_be_nmi().
> >
> > Fixes: 331a1b3a836c0f38 ("arm64: smp: Add arch support for backtrace using pseudo-NMI")
> > Reported-by: Chen-Yu Tsai <[email protected]>
> > Closes: https://issuetracker.google.com/issues/197061987#comment68
> > Signed-off-by: Mark Rutland <[email protected]>
> > Cc: Douglas Anderson <[email protected]>
> > Cc: Marc Zyngier <[email protected]>
> > ---
> > arch/arm64/kernel/smp.c | 5 ++++-
> > drivers/irqchip/irq-gic-v3.c | 2 +-
> > 2 files changed, 5 insertions(+), 2 deletions(-)
>
> Sure, this is OK w/ me as long as folks don't mind accessing the
> global here, it's OK w/ me:
>
> Reviewed-by: Douglas Anderson <[email protected]>
>
> It seems to work for me, thus:
>
> Tested-by: Douglas Anderson <[email protected]>
>
>
> > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> > index 814d9aa93b21b..061c69160f90f 100644
> > --- a/arch/arm64/kernel/smp.c
> > +++ b/arch/arm64/kernel/smp.c
> > @@ -964,7 +964,10 @@ static void smp_cross_call(const struct cpumask *target, unsigned int ipinr)
> >
> > static bool ipi_should_be_nmi(enum ipi_msg_type ipi)
> > {
> > - if (!system_uses_irq_prio_masking())
> > + DECLARE_STATIC_KEY_FALSE(supports_pseudo_nmis);
> > +
> > + if (!system_uses_irq_prio_masking() ||
> > + !static_branch_likely(&supports_pseudo_nmis))
>
> One thought, actually, is whether we should actually change
> system_uses_irq_prio_masking() to return the correct value. What do
> you think?
I don't think we should add this to system_uses_irq_prio_masking(); that's used
by the low-level flags manipulation code that gets inlined all over the place,
and that code will work regarldess of whether we actually use NMI priorities.
If we want to avoid using PMR masking *at all* on these platforms, we'd need to
detect that within can_use_gic_priorities() or early_enable_pseudo_nmi().
Thanks,
Mark.
Hi,
On Tue, Oct 3, 2023 at 5:29 AM Mark Rutland <[email protected]> wrote:
>
> On Mon, Oct 02, 2023 at 12:16:17PM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Mon, Oct 2, 2023 at 10:24 AM Mark Rutland <[email protected]> wrote:
> > >
> > > On Mon, Oct 02, 2023 at 09:45:29AM -0700, Douglas Anderson wrote:
> > > > Some mediatek devices have the property
> > > > "mediatek,broken-save-restore-fw" in their GIC. This means that,
> > > > although the hardware supports pseudo-NMI, the firmware has a bug
> > > > that blocks enabling it. When we're in this state,
> > > > system_uses_irq_prio_masking() will return true but we'll fail to
> > > > actually enable the IRQ in the GIC.
> > > >
> > > > Let's make the code handle this. We'll detect that we failed to
> > > > request an IPI as NMI and fallback to requesting it normally. Though
> > > > we expect that either all of our requests will fail or all will
> > > > succeed, it's just as cheap to keep a per-IPI bitmap and that keeps us
> > > > robust.
> > > >
> > > > Fixes: 331a1b3a836c ("arm64: smp: Add arch support for backtrace using pseudo-NMI")
> > > > Reported-by: Chen-Yu Tsai <[email protected]>
> > > > Closes: https://issuetracker.google.com/issues/197061987#comment68
> > > > Signed-off-by: Douglas Anderson <[email protected]>
> > > > ---
> > > >
> > > > arch/arm64/kernel/smp.c | 19 ++++++++++++-------
> > > > 1 file changed, 12 insertions(+), 7 deletions(-)
> > >
> > > I'm not too keen on falling back here when we have no idea why the request failed.
> > >
> > > I'd prefer if we could check the `supports_pseudo_nmis` static key directly to
> > > account for the case of broken FW, e.g. as below.
> > >
> > > Mark.
> > >
> > > ---->8----
> > > From 72fdec05c64a74f21871b44c7c760bbe07cac044 Mon Sep 17 00:00:00 2001
> > > From: Mark Rutland <[email protected]>
> > > Date: Mon, 2 Oct 2023 18:00:36 +0100
> > > Subject: [PATCH] arm64: smp: avoid NMI IPIs with broken MediaTek FW
> > >
> > > Some MediaTek devices have broken firmware which corrupts some GICR
> > > registers behind the back of the OS, and pseudo-NMIs cannot be used on
> > > these devices. For more details see commit:
> > >
> > > 44bd78dd2b8897f5 ("irqchip/gic-v3: Disable pseudo NMIs on Mediatek devices w/ firmware issues")
> > >
> > > We did not take this problem into account in commit:
> > >
> > > 331a1b3a836c0f38 ("arm64: smp: Add arch support for backtrace using pseudo-NMI")
> > >
> > > Since that commit arm64's SMP code will try to setup some IPIs as
> > > pseudo-NMIs, even on systems with broken FW. The GICv3 code will
> > > (rightly) reject attempts to request interrupts as pseudo-NMIs,
> > > resulting in boot-time failures.
> > >
> > > Avoid the problem by taking the broken FW into account when deciding to
> > > request IPIs as pseudo-NMIs. The GICv3 driver maintains a static_key
> > > named "supports_pseudo_nmis" which is false on systems with broken FW,
> > > and we can consult this within ipi_should_be_nmi().
> > >
> > > Fixes: 331a1b3a836c0f38 ("arm64: smp: Add arch support for backtrace using pseudo-NMI")
> > > Reported-by: Chen-Yu Tsai <[email protected]>
> > > Closes: https://issuetracker.google.com/issues/197061987#comment68
> > > Signed-off-by: Mark Rutland <[email protected]>
> > > Cc: Douglas Anderson <[email protected]>
> > > Cc: Marc Zyngier <[email protected]>
> > > ---
> > > arch/arm64/kernel/smp.c | 5 ++++-
> > > drivers/irqchip/irq-gic-v3.c | 2 +-
> > > 2 files changed, 5 insertions(+), 2 deletions(-)
> >
> > Sure, this is OK w/ me as long as folks don't mind accessing the
> > global here, it's OK w/ me:
> >
> > Reviewed-by: Douglas Anderson <[email protected]>
> >
> > It seems to work for me, thus:
> >
> > Tested-by: Douglas Anderson <[email protected]>
> >
> >
> > > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> > > index 814d9aa93b21b..061c69160f90f 100644
> > > --- a/arch/arm64/kernel/smp.c
> > > +++ b/arch/arm64/kernel/smp.c
> > > @@ -964,7 +964,10 @@ static void smp_cross_call(const struct cpumask *target, unsigned int ipinr)
> > >
> > > static bool ipi_should_be_nmi(enum ipi_msg_type ipi)
> > > {
> > > - if (!system_uses_irq_prio_masking())
> > > + DECLARE_STATIC_KEY_FALSE(supports_pseudo_nmis);
> > > +
> > > + if (!system_uses_irq_prio_masking() ||
> > > + !static_branch_likely(&supports_pseudo_nmis))
> >
> > One thought, actually, is whether we should actually change
> > system_uses_irq_prio_masking() to return the correct value. What do
> > you think?
>
> I don't think we should add this to system_uses_irq_prio_masking(); that's used
> by the low-level flags manipulation code that gets inlined all over the place,
> and that code will work regarldess of whether we actually use NMI priorities.
>
> If we want to avoid using PMR masking *at all* on these platforms, we'd need to
> detect that within can_use_gic_priorities() or early_enable_pseudo_nmi().
I suspect that anyone trying to use PMR masking on these systems for
any purpose will be unhappy. The issue is talked about in:
https://issuetracker.google.com/281831288
...where you can see that the firmware on these systems isn't properly
saving/restoring some registers, including GICR_IPRIORITYR.
-Doug
On Tue, Oct 03, 2023 at 06:43:07AM -0700, Doug Anderson wrote:
> Hi,
>
> On Tue, Oct 3, 2023 at 5:29 AM Mark Rutland <[email protected]> wrote:
> >
> > On Mon, Oct 02, 2023 at 12:16:17PM -0700, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Mon, Oct 2, 2023 at 10:24 AM Mark Rutland <[email protected]> wrote:
> > > >
> > > > On Mon, Oct 02, 2023 at 09:45:29AM -0700, Douglas Anderson wrote:
> > > > > Some mediatek devices have the property
> > > > > "mediatek,broken-save-restore-fw" in their GIC. This means that,
> > > > > although the hardware supports pseudo-NMI, the firmware has a bug
> > > > > that blocks enabling it. When we're in this state,
> > > > > system_uses_irq_prio_masking() will return true but we'll fail to
> > > > > actually enable the IRQ in the GIC.
> > > > >
> > > > > Let's make the code handle this. We'll detect that we failed to
> > > > > request an IPI as NMI and fallback to requesting it normally. Though
> > > > > we expect that either all of our requests will fail or all will
> > > > > succeed, it's just as cheap to keep a per-IPI bitmap and that keeps us
> > > > > robust.
> > > > >
> > > > > Fixes: 331a1b3a836c ("arm64: smp: Add arch support for backtrace using pseudo-NMI")
> > > > > Reported-by: Chen-Yu Tsai <[email protected]>
> > > > > Closes: https://issuetracker.google.com/issues/197061987#comment68
> > > > > Signed-off-by: Douglas Anderson <[email protected]>
> > > > > ---
> > > > >
> > > > > arch/arm64/kernel/smp.c | 19 ++++++++++++-------
> > > > > 1 file changed, 12 insertions(+), 7 deletions(-)
> > > >
> > > > I'm not too keen on falling back here when we have no idea why the request failed.
> > > >
> > > > I'd prefer if we could check the `supports_pseudo_nmis` static key directly to
> > > > account for the case of broken FW, e.g. as below.
> > > >
> > > > Mark.
> > > >
> > > > ---->8----
> > > > From 72fdec05c64a74f21871b44c7c760bbe07cac044 Mon Sep 17 00:00:00 2001
> > > > From: Mark Rutland <[email protected]>
> > > > Date: Mon, 2 Oct 2023 18:00:36 +0100
> > > > Subject: [PATCH] arm64: smp: avoid NMI IPIs with broken MediaTek FW
> > > >
> > > > Some MediaTek devices have broken firmware which corrupts some GICR
> > > > registers behind the back of the OS, and pseudo-NMIs cannot be used on
> > > > these devices. For more details see commit:
> > > >
> > > > 44bd78dd2b8897f5 ("irqchip/gic-v3: Disable pseudo NMIs on Mediatek devices w/ firmware issues")
> > > >
> > > > We did not take this problem into account in commit:
> > > >
> > > > 331a1b3a836c0f38 ("arm64: smp: Add arch support for backtrace using pseudo-NMI")
> > > >
> > > > Since that commit arm64's SMP code will try to setup some IPIs as
> > > > pseudo-NMIs, even on systems with broken FW. The GICv3 code will
> > > > (rightly) reject attempts to request interrupts as pseudo-NMIs,
> > > > resulting in boot-time failures.
> > > >
> > > > Avoid the problem by taking the broken FW into account when deciding to
> > > > request IPIs as pseudo-NMIs. The GICv3 driver maintains a static_key
> > > > named "supports_pseudo_nmis" which is false on systems with broken FW,
> > > > and we can consult this within ipi_should_be_nmi().
> > > >
> > > > Fixes: 331a1b3a836c0f38 ("arm64: smp: Add arch support for backtrace using pseudo-NMI")
> > > > Reported-by: Chen-Yu Tsai <[email protected]>
> > > > Closes: https://issuetracker.google.com/issues/197061987#comment68
> > > > Signed-off-by: Mark Rutland <[email protected]>
> > > > Cc: Douglas Anderson <[email protected]>
> > > > Cc: Marc Zyngier <[email protected]>
> > > > ---
> > > > arch/arm64/kernel/smp.c | 5 ++++-
> > > > drivers/irqchip/irq-gic-v3.c | 2 +-
> > > > 2 files changed, 5 insertions(+), 2 deletions(-)
> > >
> > > Sure, this is OK w/ me as long as folks don't mind accessing the
> > > global here, it's OK w/ me:
> > >
> > > Reviewed-by: Douglas Anderson <[email protected]>
> > >
> > > It seems to work for me, thus:
> > >
> > > Tested-by: Douglas Anderson <[email protected]>
> > >
> > >
> > > > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> > > > index 814d9aa93b21b..061c69160f90f 100644
> > > > --- a/arch/arm64/kernel/smp.c
> > > > +++ b/arch/arm64/kernel/smp.c
> > > > @@ -964,7 +964,10 @@ static void smp_cross_call(const struct cpumask *target, unsigned int ipinr)
> > > >
> > > > static bool ipi_should_be_nmi(enum ipi_msg_type ipi)
> > > > {
> > > > - if (!system_uses_irq_prio_masking())
> > > > + DECLARE_STATIC_KEY_FALSE(supports_pseudo_nmis);
> > > > +
> > > > + if (!system_uses_irq_prio_masking() ||
> > > > + !static_branch_likely(&supports_pseudo_nmis))
> > >
> > > One thought, actually, is whether we should actually change
> > > system_uses_irq_prio_masking() to return the correct value. What do
> > > you think?
> >
> > I don't think we should add this to system_uses_irq_prio_masking(); that's used
> > by the low-level flags manipulation code that gets inlined all over the place,
> > and that code will work regarldess of whether we actually use NMI priorities.
> >
> > If we want to avoid using PMR masking *at all* on these platforms, we'd need to
> > detect that within can_use_gic_priorities() or early_enable_pseudo_nmi().
>
> I suspect that anyone trying to use PMR masking on these systems for
> any purpose will be unhappy. The issue is talked about in:
>
> https://issuetracker.google.com/281831288
>
> ...where you can see that the firmware on these systems isn't properly
> saving/restoring some registers, including GICR_IPRIORITYR.
Ok, then that's a latent bug even before the IPI changes, going back to the
original workaround in commit:
44bd78dd2b8897f5 ("irqchip/gic-v3: Disable pseudo NMIs on Mediatek devices w/ firmware issues")
For the sake of those reading the archive, can we have a better description of
what exactly happens on these boards?
IIUC on these boards the firmware fails to save+restore (some?) GICR registers
across (some?) PSCI CPU_SUSPEND idle states.
Which registers does it save+restore?
Does it reset other registers into a specific state?
Thanks,
Mark.
Hi,
On Tue, Oct 3, 2023 at 9:32 AM Mark Rutland <[email protected]> wrote:
>
> On Tue, Oct 03, 2023 at 06:43:07AM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Oct 3, 2023 at 5:29 AM Mark Rutland <[email protected]> wrote:
> > >
> > > On Mon, Oct 02, 2023 at 12:16:17PM -0700, Doug Anderson wrote:
> > > > Hi,
> > > >
> > > > On Mon, Oct 2, 2023 at 10:24 AM Mark Rutland <[email protected]> wrote:
> > > > >
> > > > > On Mon, Oct 02, 2023 at 09:45:29AM -0700, Douglas Anderson wrote:
> > > > > > Some mediatek devices have the property
> > > > > > "mediatek,broken-save-restore-fw" in their GIC. This means that,
> > > > > > although the hardware supports pseudo-NMI, the firmware has a bug
> > > > > > that blocks enabling it. When we're in this state,
> > > > > > system_uses_irq_prio_masking() will return true but we'll fail to
> > > > > > actually enable the IRQ in the GIC.
> > > > > >
> > > > > > Let's make the code handle this. We'll detect that we failed to
> > > > > > request an IPI as NMI and fallback to requesting it normally. Though
> > > > > > we expect that either all of our requests will fail or all will
> > > > > > succeed, it's just as cheap to keep a per-IPI bitmap and that keeps us
> > > > > > robust.
> > > > > >
> > > > > > Fixes: 331a1b3a836c ("arm64: smp: Add arch support for backtrace using pseudo-NMI")
> > > > > > Reported-by: Chen-Yu Tsai <[email protected]>
> > > > > > Closes: https://issuetracker.google.com/issues/197061987#comment68
> > > > > > Signed-off-by: Douglas Anderson <[email protected]>
> > > > > > ---
> > > > > >
> > > > > > arch/arm64/kernel/smp.c | 19 ++++++++++++-------
> > > > > > 1 file changed, 12 insertions(+), 7 deletions(-)
> > > > >
> > > > > I'm not too keen on falling back here when we have no idea why the request failed.
> > > > >
> > > > > I'd prefer if we could check the `supports_pseudo_nmis` static key directly to
> > > > > account for the case of broken FW, e.g. as below.
> > > > >
> > > > > Mark.
> > > > >
> > > > > ---->8----
> > > > > From 72fdec05c64a74f21871b44c7c760bbe07cac044 Mon Sep 17 00:00:00 2001
> > > > > From: Mark Rutland <[email protected]>
> > > > > Date: Mon, 2 Oct 2023 18:00:36 +0100
> > > > > Subject: [PATCH] arm64: smp: avoid NMI IPIs with broken MediaTek FW
> > > > >
> > > > > Some MediaTek devices have broken firmware which corrupts some GICR
> > > > > registers behind the back of the OS, and pseudo-NMIs cannot be used on
> > > > > these devices. For more details see commit:
> > > > >
> > > > > 44bd78dd2b8897f5 ("irqchip/gic-v3: Disable pseudo NMIs on Mediatek devices w/ firmware issues")
> > > > >
> > > > > We did not take this problem into account in commit:
> > > > >
> > > > > 331a1b3a836c0f38 ("arm64: smp: Add arch support for backtrace using pseudo-NMI")
> > > > >
> > > > > Since that commit arm64's SMP code will try to setup some IPIs as
> > > > > pseudo-NMIs, even on systems with broken FW. The GICv3 code will
> > > > > (rightly) reject attempts to request interrupts as pseudo-NMIs,
> > > > > resulting in boot-time failures.
> > > > >
> > > > > Avoid the problem by taking the broken FW into account when deciding to
> > > > > request IPIs as pseudo-NMIs. The GICv3 driver maintains a static_key
> > > > > named "supports_pseudo_nmis" which is false on systems with broken FW,
> > > > > and we can consult this within ipi_should_be_nmi().
> > > > >
> > > > > Fixes: 331a1b3a836c0f38 ("arm64: smp: Add arch support for backtrace using pseudo-NMI")
> > > > > Reported-by: Chen-Yu Tsai <[email protected]>
> > > > > Closes: https://issuetracker.google.com/issues/197061987#comment68
> > > > > Signed-off-by: Mark Rutland <[email protected]>
> > > > > Cc: Douglas Anderson <[email protected]>
> > > > > Cc: Marc Zyngier <[email protected]>
> > > > > ---
> > > > > arch/arm64/kernel/smp.c | 5 ++++-
> > > > > drivers/irqchip/irq-gic-v3.c | 2 +-
> > > > > 2 files changed, 5 insertions(+), 2 deletions(-)
> > > >
> > > > Sure, this is OK w/ me as long as folks don't mind accessing the
> > > > global here, it's OK w/ me:
> > > >
> > > > Reviewed-by: Douglas Anderson <[email protected]>
> > > >
> > > > It seems to work for me, thus:
> > > >
> > > > Tested-by: Douglas Anderson <[email protected]>
> > > >
> > > >
> > > > > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> > > > > index 814d9aa93b21b..061c69160f90f 100644
> > > > > --- a/arch/arm64/kernel/smp.c
> > > > > +++ b/arch/arm64/kernel/smp.c
> > > > > @@ -964,7 +964,10 @@ static void smp_cross_call(const struct cpumask *target, unsigned int ipinr)
> > > > >
> > > > > static bool ipi_should_be_nmi(enum ipi_msg_type ipi)
> > > > > {
> > > > > - if (!system_uses_irq_prio_masking())
> > > > > + DECLARE_STATIC_KEY_FALSE(supports_pseudo_nmis);
> > > > > +
> > > > > + if (!system_uses_irq_prio_masking() ||
> > > > > + !static_branch_likely(&supports_pseudo_nmis))
> > > >
> > > > One thought, actually, is whether we should actually change
> > > > system_uses_irq_prio_masking() to return the correct value. What do
> > > > you think?
> > >
> > > I don't think we should add this to system_uses_irq_prio_masking(); that's used
> > > by the low-level flags manipulation code that gets inlined all over the place,
> > > and that code will work regarldess of whether we actually use NMI priorities.
> > >
> > > If we want to avoid using PMR masking *at all* on these platforms, we'd need to
> > > detect that within can_use_gic_priorities() or early_enable_pseudo_nmi().
> >
> > I suspect that anyone trying to use PMR masking on these systems for
> > any purpose will be unhappy. The issue is talked about in:
> >
> > https://issuetracker.google.com/281831288
> >
> > ...where you can see that the firmware on these systems isn't properly
> > saving/restoring some registers, including GICR_IPRIORITYR.
>
> Ok, then that's a latent bug even before the IPI changes, going back to the
> original workaround in commit:
>
> 44bd78dd2b8897f5 ("irqchip/gic-v3: Disable pseudo NMIs on Mediatek devices w/ firmware issues")
>
> For the sake of those reading the archive, can we have a better description of
> what exactly happens on these boards?
>
> IIUC on these boards the firmware fails to save+restore (some?) GICR registers
> across (some?) PSCI CPU_SUSPEND idle states.
>
> Which registers does it save+restore?
>
> Does it reset other registers into a specific state?
Though I'm not an expert in this area, my understanding is that in
some of the deeper idle states then GICR registers are lost. That
matches a thread [0] I found. In early investigation I found that I
could comment out `cpu-idle-states` in the device tree to avoid the
problems [1]. I believe this is fully expected which is why firmware
is supposed to save/restore these registers whenever a low power is
entered/exited. I'd presume that any register not properly
saved/restored comes up in whatever its default state is.
As far as pseudo-NMI was concerned, all I really needed to
save/restore was "GICR_NUM_IPRIORITYR" [2], but Marc Zyngier looked at
the code and identified [3] at least these in addition:
* GICR_CTLR
* GICR_ISPENDR0
* GICR_ISACTIVER0
* GICR_NSACR
That list seems to match the Arm Trusted Firmware patch that fixed the
issue [4]. ...but it will be impossible to ever get the fix rolled out
to all devices. Even if we could get firmware spins Qualified for
every device there will still be cases where we'll boot with the old
firmware. Since we _don't_ bundle the device tree with the firmware,
we believe that the quirk mechanism that we came up with (add a quirk
in never device trees and firmware removes the quirk when we have a
fix) is at least a robust/reliable way to detect the issue.
The whole issue seems rather concerning, but (apparently) it never
caused issues in the kernel until we tried to use pseudo-NMI.
[0] https://github.com/ARM-software/tf-issues/issues/464
[1] https://issuetracker.google.com/issues/197061987#comment27
[2] https://crrev.com/c/4519877
[3] https://issuetracker.google.com/issues/281831288#comment5
[4] https://github.com/ARM-software/arm-trusted-firmware/commit/1c62cc7fbdf2ec2a7e69b3c027d507fcafdcaa12
On Tue, Oct 03, 2023 at 12:32:39PM -0700, Doug Anderson wrote:
> Hi,
>
> On Tue, Oct 3, 2023 at 9:32 AM Mark Rutland <[email protected]> wrote:
> >
> > On Tue, Oct 03, 2023 at 06:43:07AM -0700, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Tue, Oct 3, 2023 at 5:29 AM Mark Rutland <[email protected]> wrote:
> > > >
> > > > On Mon, Oct 02, 2023 at 12:16:17PM -0700, Doug Anderson wrote:
> > > > > Hi,
> > > > >
> > > > > On Mon, Oct 2, 2023 at 10:24 AM Mark Rutland <[email protected]> wrote:
> > > > > >
> > > > > > On Mon, Oct 02, 2023 at 09:45:29AM -0700, Douglas Anderson wrote:
> > > > > > > Some mediatek devices have the property
> > > > > > > "mediatek,broken-save-restore-fw" in their GIC. This means that,
> > > > > > > although the hardware supports pseudo-NMI, the firmware has a bug
> > > > > > > that blocks enabling it. When we're in this state,
> > > > > > > system_uses_irq_prio_masking() will return true but we'll fail to
> > > > > > > actually enable the IRQ in the GIC.
> > > > > > >
> > > > > > > Let's make the code handle this. We'll detect that we failed to
> > > > > > > request an IPI as NMI and fallback to requesting it normally. Though
> > > > > > > we expect that either all of our requests will fail or all will
> > > > > > > succeed, it's just as cheap to keep a per-IPI bitmap and that keeps us
> > > > > > > robust.
> > > > > > >
> > > > > > > Fixes: 331a1b3a836c ("arm64: smp: Add arch support for backtrace using pseudo-NMI")
> > > > > > > Reported-by: Chen-Yu Tsai <[email protected]>
> > > > > > > Closes: https://issuetracker.google.com/issues/197061987#comment68
> > > > > > > Signed-off-by: Douglas Anderson <[email protected]>
> > > > > > > ---
> > > > > > >
> > > > > > > arch/arm64/kernel/smp.c | 19 ++++++++++++-------
> > > > > > > 1 file changed, 12 insertions(+), 7 deletions(-)
> > > > > >
> > > > > > I'm not too keen on falling back here when we have no idea why the request failed.
> > > > > >
> > > > > > I'd prefer if we could check the `supports_pseudo_nmis` static key directly to
> > > > > > account for the case of broken FW, e.g. as below.
> > > > > >
> > > > > > Mark.
> > > > > >
> > > > > > ---->8----
> > > > > > From 72fdec05c64a74f21871b44c7c760bbe07cac044 Mon Sep 17 00:00:00 2001
> > > > > > From: Mark Rutland <[email protected]>
> > > > > > Date: Mon, 2 Oct 2023 18:00:36 +0100
> > > > > > Subject: [PATCH] arm64: smp: avoid NMI IPIs with broken MediaTek FW
> > > > > >
> > > > > > Some MediaTek devices have broken firmware which corrupts some GICR
> > > > > > registers behind the back of the OS, and pseudo-NMIs cannot be used on
> > > > > > these devices. For more details see commit:
> > > > > >
> > > > > > 44bd78dd2b8897f5 ("irqchip/gic-v3: Disable pseudo NMIs on Mediatek devices w/ firmware issues")
> > > > > >
> > > > > > We did not take this problem into account in commit:
> > > > > >
> > > > > > 331a1b3a836c0f38 ("arm64: smp: Add arch support for backtrace using pseudo-NMI")
> > > > > >
> > > > > > Since that commit arm64's SMP code will try to setup some IPIs as
> > > > > > pseudo-NMIs, even on systems with broken FW. The GICv3 code will
> > > > > > (rightly) reject attempts to request interrupts as pseudo-NMIs,
> > > > > > resulting in boot-time failures.
> > > > > >
> > > > > > Avoid the problem by taking the broken FW into account when deciding to
> > > > > > request IPIs as pseudo-NMIs. The GICv3 driver maintains a static_key
> > > > > > named "supports_pseudo_nmis" which is false on systems with broken FW,
> > > > > > and we can consult this within ipi_should_be_nmi().
> > > > > >
> > > > > > Fixes: 331a1b3a836c0f38 ("arm64: smp: Add arch support for backtrace using pseudo-NMI")
> > > > > > Reported-by: Chen-Yu Tsai <[email protected]>
> > > > > > Closes: https://issuetracker.google.com/issues/197061987#comment68
> > > > > > Signed-off-by: Mark Rutland <[email protected]>
> > > > > > Cc: Douglas Anderson <[email protected]>
> > > > > > Cc: Marc Zyngier <[email protected]>
> > > > > > ---
> > > > > > arch/arm64/kernel/smp.c | 5 ++++-
> > > > > > drivers/irqchip/irq-gic-v3.c | 2 +-
> > > > > > 2 files changed, 5 insertions(+), 2 deletions(-)
> > > > >
> > > > > Sure, this is OK w/ me as long as folks don't mind accessing the
> > > > > global here, it's OK w/ me:
> > > > >
> > > > > Reviewed-by: Douglas Anderson <[email protected]>
> > > > >
> > > > > It seems to work for me, thus:
> > > > >
> > > > > Tested-by: Douglas Anderson <[email protected]>
> > > > >
> > > > >
> > > > > > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> > > > > > index 814d9aa93b21b..061c69160f90f 100644
> > > > > > --- a/arch/arm64/kernel/smp.c
> > > > > > +++ b/arch/arm64/kernel/smp.c
> > > > > > @@ -964,7 +964,10 @@ static void smp_cross_call(const struct cpumask *target, unsigned int ipinr)
> > > > > >
> > > > > > static bool ipi_should_be_nmi(enum ipi_msg_type ipi)
> > > > > > {
> > > > > > - if (!system_uses_irq_prio_masking())
> > > > > > + DECLARE_STATIC_KEY_FALSE(supports_pseudo_nmis);
> > > > > > +
> > > > > > + if (!system_uses_irq_prio_masking() ||
> > > > > > + !static_branch_likely(&supports_pseudo_nmis))
> > > > >
> > > > > One thought, actually, is whether we should actually change
> > > > > system_uses_irq_prio_masking() to return the correct value. What do
> > > > > you think?
> > > >
> > > > I don't think we should add this to system_uses_irq_prio_masking(); that's used
> > > > by the low-level flags manipulation code that gets inlined all over the place,
> > > > and that code will work regarldess of whether we actually use NMI priorities.
> > > >
> > > > If we want to avoid using PMR masking *at all* on these platforms, we'd need to
> > > > detect that within can_use_gic_priorities() or early_enable_pseudo_nmi().
> > >
> > > I suspect that anyone trying to use PMR masking on these systems for
> > > any purpose will be unhappy. The issue is talked about in:
> > >
> > > https://issuetracker.google.com/281831288
> > >
> > > ...where you can see that the firmware on these systems isn't properly
> > > saving/restoring some registers, including GICR_IPRIORITYR.
> >
> > Ok, then that's a latent bug even before the IPI changes, going back to the
> > original workaround in commit:
> >
> > 44bd78dd2b8897f5 ("irqchip/gic-v3: Disable pseudo NMIs on Mediatek devices w/ firmware issues")
> >
> > For the sake of those reading the archive, can we have a better description of
> > what exactly happens on these boards?
> >
> > IIUC on these boards the firmware fails to save+restore (some?) GICR registers
> > across (some?) PSCI CPU_SUSPEND idle states.
> >
> > Which registers does it save+restore?
> >
> > Does it reset other registers into a specific state?
>
> Though I'm not an expert in this area, my understanding is that in
> some of the deeper idle states then GICR registers are lost. That
> matches a thread [0] I found. In early investigation I found that I
> could comment out `cpu-idle-states` in the device tree to avoid the
> problems [1]. I believe this is fully expected which is why firmware
> is supposed to save/restore these registers whenever a low power is
> entered/exited. I'd presume that any register not properly
> saved/restored comes up in whatever its default state is.
>
> As far as pseudo-NMI was concerned, all I really needed to
> save/restore was "GICR_NUM_IPRIORITYR" [2], but Marc Zyngier looked at
> the code and identified [3] at least these in addition:
> * GICR_CTLR
> * GICR_ISPENDR0
> * GICR_ISACTIVER0
> * GICR_NSACR
Looking at the GIC spec (Arm IHI 0069H), page 12-673, I see for all of the
GICR_IPRIORITYR<n>.Priority_offset_*B fields:
| The reset behavior of this field is:
| • On a GIC reset, this field resets to an architecturally UNKNOWN value.
... so at least per the architecture these could be reset to arbitrary values,
and that priority might permit SGI/PPIs to be taken IRQs are priority-masked,
or to prevent SGI/PPIs to be taken when priority-unmasked.
I also see for GICR_CTLR that EnableLPIs would be reset to 0, and IIUC that
means LPIs won't work on these parts, which seems like a problem.
> That list seems to match the Arm Trusted Firmware patch that fixed the
> issue [4]. ...but it will be impossible to ever get the fix rolled out
> to all devices. Even if we could get firmware spins Qualified for
> every device there will still be cases where we'll boot with the old
> firmware. Since we _don't_ bundle the device tree with the firmware,
> we believe that the quirk mechanism that we came up with (add a quirk
> in never device trees and firmware removes the quirk when we have a
> fix) is at least a robust/reliable way to detect the issue.
>
> The whole issue seems rather concerning, but (apparently) it never
> caused issues in the kernel until we tried to use pseudo-NMI.
Given you haven't seen any issues, I suspect those are getting reset to fixed
values that happens to work out for us, but it is a bit worrisome more
generally (e.g. the LPI case above).
Mark.
>
> [0] https://github.com/ARM-software/tf-issues/issues/464
> [1] https://issuetracker.google.com/issues/197061987#comment27
> [2] https://crrev.com/c/4519877
> [3] https://issuetracker.google.com/issues/281831288#comment5
> [4] https://github.com/ARM-software/arm-trusted-firmware/commit/1c62cc7fbdf2ec2a7e69b3c027d507fcafdcaa12
On Wed, 04 Oct 2023 10:59:50 +0100,
Mark Rutland <[email protected]> wrote:
>
> Given you haven't seen any issues, I suspect those are getting reset to fixed
> values that happens to work out for us, but it is a bit worrisome more
> generally (e.g. the LPI case above).
It is likely that these SoCs don't even have an ITS.
M.
--
Without deviation from the norm, progress is not possible.
Hi,
On Wed, Oct 4, 2023 at 3:15 AM Marc Zyngier <[email protected]> wrote:
>
> On Wed, 04 Oct 2023 10:59:50 +0100,
> Mark Rutland <[email protected]> wrote:
> >
> > Given you haven't seen any issues, I suspect those are getting reset to fixed
> > values that happens to work out for us, but it is a bit worrisome more
> > generally (e.g. the LPI case above).
>
> It is likely that these SoCs don't even have an ITS.
Right. That was what we decided [1] when Marc pointed this out earlier.
Overall: we know that this firmware behavior is not good but we're
stuck with it. :( At the very least, any new devices coming out will
have this fixed. Presumably if old devices are working OK enough today
(as long as you don't enable pseudo-NMI) then they can be made to keep
working?
So circling back: what patch should we actually land? As of right now
only pseudo-NMI is broken, but it would be good to make sure that if
the kernel later adds other features that would be broken on this
hardware that it gets handled properly...
[1] https://issuetracker.google.com/issues/281831288#comment4
On Wed, Oct 04, 2023 at 07:04:12AM -0700, Doug Anderson wrote:
> On Wed, Oct 4, 2023 at 3:15 AM Marc Zyngier <[email protected]> wrote:
> >
> > On Wed, 04 Oct 2023 10:59:50 +0100,
> > Mark Rutland <[email protected]> wrote:
> > >
> > > Given you haven't seen any issues, I suspect those are getting reset to fixed
> > > values that happens to work out for us, but it is a bit worrisome more
> > > generally (e.g. the LPI case above).
> >
> > It is likely that these SoCs don't even have an ITS.
>
> Right. That was what we decided [1] when Marc pointed this out earlier.
>
> Overall: we know that this firmware behavior is not good but we're
> stuck with it. :( At the very least, any new devices coming out will
> have this fixed. Presumably if old devices are working OK enough today
> (as long as you don't enable pseudo-NMI) then they can be made to keep
> working?
>
> So circling back: what patch should we actually land?
For now I'd prefer we took the patch I sent in:
https://lore.kernel.org/linux-arm-kernel/[email protected]/
... as that leaves us no worse than before this series, and it's pretty simple.
> As of right now only pseudo-NMI is broken, but it would be good to make sure
> that if the kernel later adds other features that would be broken on this
> hardware that it gets handled properly...
Going further than the above, I think there are three options here:
1) Complete fix: depend on a working firmware, and throw this workaround away.
IIUC from the above, that's not something you can commit to.
2) Partial fix: have the kernel save/restore everything.
IIUC this is unpalatable.
3) Partial fix: make the ARM64_HAS_GIC_PRIO_MASKING cpucap depend on the
absence of a "mediatek,broken-save-restore-fw" property in the DT. I believe
we can check that in early_enable_pseudo_nmi() or can_use_gic_priorities().
That'll avoid potential issues if/when we change the priorities used for
pNMI (which is something I've been looking at).
I'm happy with (3) if Marc is.
Mark.
Hi,
On Thu, Oct 5, 2023 at 3:27 AM Mark Rutland <[email protected]> wrote:
>
> On Wed, Oct 04, 2023 at 07:04:12AM -0700, Doug Anderson wrote:
> > On Wed, Oct 4, 2023 at 3:15 AM Marc Zyngier <[email protected]> wrote:
> > >
> > > On Wed, 04 Oct 2023 10:59:50 +0100,
> > > Mark Rutland <[email protected]> wrote:
> > > >
> > > > Given you haven't seen any issues, I suspect those are getting reset to fixed
> > > > values that happens to work out for us, but it is a bit worrisome more
> > > > generally (e.g. the LPI case above).
> > >
> > > It is likely that these SoCs don't even have an ITS.
> >
> > Right. That was what we decided [1] when Marc pointed this out earlier.
> >
> > Overall: we know that this firmware behavior is not good but we're
> > stuck with it. :( At the very least, any new devices coming out will
> > have this fixed. Presumably if old devices are working OK enough today
> > (as long as you don't enable pseudo-NMI) then they can be made to keep
> > working?
> >
> > So circling back: what patch should we actually land?
>
> For now I'd prefer we took the patch I sent in:
>
> https://lore.kernel.org/linux-arm-kernel/[email protected]/
>
> ... as that leaves us no worse than before this series, and it's pretty simple.
Sounds good to me!
Catalin / Will: Please yell if there's anything you need me to do.
Otherwise I'll assume you'll pick up Mark's patch instead of my patch
#1 and then you'll pick up my patch #2.
> > As of right now only pseudo-NMI is broken, but it would be good to make sure
> > that if the kernel later adds other features that would be broken on this
> > hardware that it gets handled properly...
>
> Going further than the above, I think there are three options here:
>
> 1) Complete fix: depend on a working firmware, and throw this workaround away.
>
> IIUC from the above, that's not something you can commit to.
Right. We've landed the fix in the firmware branch for many of the
devices that had the issue but there's a whole process (and cost
involved) in getting this actually rolled out. Each unique board needs
to kick off a FW qual. Given that nothing is actually broken today
it's hard to justify a FW qual just for this, so we're left
piggybacking on the next reason for a FW qual (if there is one).
...even if we could kick them off all instantly, though, it's always
best not to rely on a FW fix, especially if (as in this case) it's to
keep us from crashing. There'll always be some case where someone
tries to boot a new OS with an old firmware. One such case can happen
when booting recovery images on ChromeOS where the device always boots
from the Read-only (not updatable) firmware.
> 2) Partial fix: have the kernel save/restore everything.
>
> IIUC this is unpalatable.
Yeah, Marc already NAKed it.
> 3) Partial fix: make the ARM64_HAS_GIC_PRIO_MASKING cpucap depend on the
> absence of a "mediatek,broken-save-restore-fw" property in the DT. I believe
> we can check that in early_enable_pseudo_nmi() or can_use_gic_priorities().
>
> That'll avoid potential issues if/when we change the priorities used for
> pNMI (which is something I've been looking at).
>
> I'm happy with (3) if Marc is.
Yeah, that seems best to me as a long term solution, too.
-Doug
On Mon, 02 Oct 2023 18:24:11 +0100,
Mark Rutland <[email protected]> wrote:
>
> On Mon, Oct 02, 2023 at 09:45:29AM -0700, Douglas Anderson wrote:
> > Some mediatek devices have the property
> > "mediatek,broken-save-restore-fw" in their GIC. This means that,
> > although the hardware supports pseudo-NMI, the firmware has a bug
> > that blocks enabling it. When we're in this state,
> > system_uses_irq_prio_masking() will return true but we'll fail to
> > actually enable the IRQ in the GIC.
> >
> > Let's make the code handle this. We'll detect that we failed to
> > request an IPI as NMI and fallback to requesting it normally. Though
> > we expect that either all of our requests will fail or all will
> > succeed, it's just as cheap to keep a per-IPI bitmap and that keeps us
> > robust.
> >
> > Fixes: 331a1b3a836c ("arm64: smp: Add arch support for backtrace using pseudo-NMI")
> > Reported-by: Chen-Yu Tsai <[email protected]>
> > Closes: https://issuetracker.google.com/issues/197061987#comment68
> > Signed-off-by: Douglas Anderson <[email protected]>
> > ---
> >
> > arch/arm64/kernel/smp.c | 19 ++++++++++++-------
> > 1 file changed, 12 insertions(+), 7 deletions(-)
>
> I'm not too keen on falling back here when we have no idea why the request failed.
>
> I'd prefer if we could check the `supports_pseudo_nmis` static key directly to
> account for the case of broken FW, e.g. as below.
>
> Mark.
>
> ---->8----
> From 72fdec05c64a74f21871b44c7c760bbe07cac044 Mon Sep 17 00:00:00 2001
> From: Mark Rutland <[email protected]>
> Date: Mon, 2 Oct 2023 18:00:36 +0100
> Subject: [PATCH] arm64: smp: avoid NMI IPIs with broken MediaTek FW
>
> Some MediaTek devices have broken firmware which corrupts some GICR
> registers behind the back of the OS, and pseudo-NMIs cannot be used on
> these devices. For more details see commit:
>
> 44bd78dd2b8897f5 ("irqchip/gic-v3: Disable pseudo NMIs on Mediatek devices w/ firmware issues")
>
> We did not take this problem into account in commit:
>
> 331a1b3a836c0f38 ("arm64: smp: Add arch support for backtrace using pseudo-NMI")
>
> Since that commit arm64's SMP code will try to setup some IPIs as
> pseudo-NMIs, even on systems with broken FW. The GICv3 code will
> (rightly) reject attempts to request interrupts as pseudo-NMIs,
> resulting in boot-time failures.
>
> Avoid the problem by taking the broken FW into account when deciding to
> request IPIs as pseudo-NMIs. The GICv3 driver maintains a static_key
> named "supports_pseudo_nmis" which is false on systems with broken FW,
> and we can consult this within ipi_should_be_nmi().
>
> Fixes: 331a1b3a836c0f38 ("arm64: smp: Add arch support for backtrace using pseudo-NMI")
> Reported-by: Chen-Yu Tsai <[email protected]>
> Closes: https://issuetracker.google.com/issues/197061987#comment68
> Signed-off-by: Mark Rutland <[email protected]>
> Cc: Douglas Anderson <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> ---
> arch/arm64/kernel/smp.c | 5 ++++-
> drivers/irqchip/irq-gic-v3.c | 2 +-
> 2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index 814d9aa93b21b..061c69160f90f 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -964,7 +964,10 @@ static void smp_cross_call(const struct cpumask *target, unsigned int ipinr)
>
> static bool ipi_should_be_nmi(enum ipi_msg_type ipi)
> {
> - if (!system_uses_irq_prio_masking())
> + DECLARE_STATIC_KEY_FALSE(supports_pseudo_nmis);
> +
> + if (!system_uses_irq_prio_masking() ||
> + !static_branch_likely(&supports_pseudo_nmis))
> return false;
>
> switch (ipi) {
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 787ccc880b22d..737da1b9aabf2 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -106,7 +106,7 @@ static DEFINE_STATIC_KEY_TRUE(supports_deactivate_key);
> * - Figure 4-7 Secure read of the priority field for a Non-secure Group 1
> * interrupt.
> */
> -static DEFINE_STATIC_KEY_FALSE(supports_pseudo_nmis);
> +DEFINE_STATIC_KEY_FALSE(supports_pseudo_nmis);
>
> DEFINE_STATIC_KEY_FALSE(gic_nonsecure_priorities);
> EXPORT_SYMBOL(gic_nonsecure_priorities);
This last hunk is going to result in more spam from the robots about
global objects without a previous declaration. Not that I care the
least, but worth mentioning.
Other than that:
Reviewed-by: Marc Zyngier <[email protected]>
Please take it via the arm64 tree with patch #1
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
On Thu, 05 Oct 2023 11:27:26 +0100,
Mark Rutland <[email protected]> wrote:
> 3) Partial fix: make the ARM64_HAS_GIC_PRIO_MASKING cpucap depend on the
> absence of a "mediatek,broken-save-restore-fw" property in the DT. I believe
> we can check that in early_enable_pseudo_nmi() or can_use_gic_priorities().
>
> That'll avoid potential issues if/when we change the priorities used for
> pNMI (which is something I've been looking at).
>
> I'm happy with (3) if Marc is.
Definitely worth investigating if you have the bandwidth.
M.
--
Without deviation from the norm, progress is not possible.
On Tue, Oct 3, 2023 at 1:24 AM Mark Rutland <[email protected]> wrote:
>
> On Mon, Oct 02, 2023 at 09:45:29AM -0700, Douglas Anderson wrote:
> > Some mediatek devices have the property
> > "mediatek,broken-save-restore-fw" in their GIC. This means that,
> > although the hardware supports pseudo-NMI, the firmware has a bug
> > that blocks enabling it. When we're in this state,
> > system_uses_irq_prio_masking() will return true but we'll fail to
> > actually enable the IRQ in the GIC.
> >
> > Let's make the code handle this. We'll detect that we failed to
> > request an IPI as NMI and fallback to requesting it normally. Though
> > we expect that either all of our requests will fail or all will
> > succeed, it's just as cheap to keep a per-IPI bitmap and that keeps us
> > robust.
> >
> > Fixes: 331a1b3a836c ("arm64: smp: Add arch support for backtrace using pseudo-NMI")
> > Reported-by: Chen-Yu Tsai <[email protected]>
> > Closes: https://issuetracker.google.com/issues/197061987#comment68
> > Signed-off-by: Douglas Anderson <[email protected]>
> > ---
> >
> > arch/arm64/kernel/smp.c | 19 ++++++++++++-------
> > 1 file changed, 12 insertions(+), 7 deletions(-)
>
> I'm not too keen on falling back here when we have no idea why the request failed.
>
> I'd prefer if we could check the `supports_pseudo_nmis` static key directly to
> account for the case of broken FW, e.g. as below.
>
> Mark.
>
> ---->8----
> From 72fdec05c64a74f21871b44c7c760bbe07cac044 Mon Sep 17 00:00:00 2001
> From: Mark Rutland <[email protected]>
> Date: Mon, 2 Oct 2023 18:00:36 +0100
> Subject: [PATCH] arm64: smp: avoid NMI IPIs with broken MediaTek FW
>
> Some MediaTek devices have broken firmware which corrupts some GICR
> registers behind the back of the OS, and pseudo-NMIs cannot be used on
> these devices. For more details see commit:
>
> 44bd78dd2b8897f5 ("irqchip/gic-v3: Disable pseudo NMIs on Mediatek devices w/ firmware issues")
>
> We did not take this problem into account in commit:
>
> 331a1b3a836c0f38 ("arm64: smp: Add arch support for backtrace using pseudo-NMI")
>
> Since that commit arm64's SMP code will try to setup some IPIs as
> pseudo-NMIs, even on systems with broken FW. The GICv3 code will
> (rightly) reject attempts to request interrupts as pseudo-NMIs,
> resulting in boot-time failures.
>
> Avoid the problem by taking the broken FW into account when deciding to
> request IPIs as pseudo-NMIs. The GICv3 driver maintains a static_key
> named "supports_pseudo_nmis" which is false on systems with broken FW,
> and we can consult this within ipi_should_be_nmi().
>
> Fixes: 331a1b3a836c0f38 ("arm64: smp: Add arch support for backtrace using pseudo-NMI")
> Reported-by: Chen-Yu Tsai <[email protected]>
> Closes: https://issuetracker.google.com/issues/197061987#comment68
> Signed-off-by: Mark Rutland <[email protected]>
> Cc: Douglas Anderson <[email protected]>
> Cc: Marc Zyngier <[email protected]>
Tested-by: Chen-Yu Tsai <[email protected]>
> ---
> arch/arm64/kernel/smp.c | 5 ++++-
> drivers/irqchip/irq-gic-v3.c | 2 +-
> 2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index 814d9aa93b21b..061c69160f90f 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -964,7 +964,10 @@ static void smp_cross_call(const struct cpumask *target, unsigned int ipinr)
>
> static bool ipi_should_be_nmi(enum ipi_msg_type ipi)
> {
> - if (!system_uses_irq_prio_masking())
> + DECLARE_STATIC_KEY_FALSE(supports_pseudo_nmis);
> +
> + if (!system_uses_irq_prio_masking() ||
> + !static_branch_likely(&supports_pseudo_nmis))
> return false;
>
> switch (ipi) {
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 787ccc880b22d..737da1b9aabf2 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -106,7 +106,7 @@ static DEFINE_STATIC_KEY_TRUE(supports_deactivate_key);
> * - Figure 4-7 Secure read of the priority field for a Non-secure Group 1
> * interrupt.
> */
> -static DEFINE_STATIC_KEY_FALSE(supports_pseudo_nmis);
> +DEFINE_STATIC_KEY_FALSE(supports_pseudo_nmis);
>
> DEFINE_STATIC_KEY_FALSE(gic_nonsecure_priorities);
> EXPORT_SYMBOL(gic_nonsecure_priorities);
> --
> 2.30.2
>
>
On Thu, Oct 05, 2023 at 08:34:56AM -0700, Doug Anderson wrote:
> On Thu, Oct 5, 2023 at 3:27 AM Mark Rutland <[email protected]> wrote:
> > On Wed, Oct 04, 2023 at 07:04:12AM -0700, Doug Anderson wrote:
> > > On Wed, Oct 4, 2023 at 3:15 AM Marc Zyngier <[email protected]> wrote:
> > > > On Wed, 04 Oct 2023 10:59:50 +0100,
> > > > Mark Rutland <[email protected]> wrote:
> > > > > Given you haven't seen any issues, I suspect those are getting reset to fixed
> > > > > values that happens to work out for us, but it is a bit worrisome more
> > > > > generally (e.g. the LPI case above).
> > > >
> > > > It is likely that these SoCs don't even have an ITS.
> > >
> > > Right. That was what we decided [1] when Marc pointed this out earlier.
> > >
> > > Overall: we know that this firmware behavior is not good but we're
> > > stuck with it. :( At the very least, any new devices coming out will
> > > have this fixed. Presumably if old devices are working OK enough today
> > > (as long as you don't enable pseudo-NMI) then they can be made to keep
> > > working?
> > >
> > > So circling back: what patch should we actually land?
> >
> > For now I'd prefer we took the patch I sent in:
> >
> > https://lore.kernel.org/linux-arm-kernel/[email protected]/
> >
> > ... as that leaves us no worse than before this series, and it's pretty simple.
>
> Sounds good to me!
>
> Catalin / Will: Please yell if there's anything you need me to do.
> Otherwise I'll assume you'll pick up Mark's patch instead of my patch
> #1 and then you'll pick up my patch #2.
I applied both to the arm64 for-next/backtrace-ipi branch. Thanks.
--
Catalin
Hi,
On Fri, Oct 6, 2023 at 3:20 AM Marc Zyngier <[email protected]> wrote:
>
> On Thu, 05 Oct 2023 11:27:26 +0100,
> Mark Rutland <[email protected]> wrote:
>
> > 3) Partial fix: make the ARM64_HAS_GIC_PRIO_MASKING cpucap depend on the
> > absence of a "mediatek,broken-save-restore-fw" property in the DT. I believe
> > we can check that in early_enable_pseudo_nmi() or can_use_gic_priorities().
> >
> > That'll avoid potential issues if/when we change the priorities used for
> > pNMI (which is something I've been looking at).
> >
> > I'm happy with (3) if Marc is.
>
> Definitely worth investigating if you have the bandwidth.
I made an attempt at it. See:
https://lore.kernel.org/20231006151547.1.Ide945748593cffd8ff0feb9ae22b795935b944d6@changeid