[PATCH 0/3] arm64: Fix cpuidle with pseudo-NMI enabled

[PATCH 0/3] arm64: Fix cpuidle with pseudo-NMI enabled

[Marc Zyngier]

It appears that although cpu_do_idle() is correctly dealing with the
PMR/DAIF duality, the PSCI cpu-suspend code has been left unaware of
it.

On a system that uses PSCI for idle (such as the Ampere Altra I have
access to), the kernel dies as soon as it enters idle (interrupts are
off at the GIC CPU interface level). Boo.

Instead of spreading more magic code around, I've elected to provide a
pair of helpers (arm_cpuidle_{save,restore}_context()) which do the
heavy lifting.

With that in place, I can finally boot the above system with
irqchip.gicv3_pseudo_nmi=1. I'd welcome feedback from people who may
have experienced similar issues in the past (and on different
machines).

Marc Zyngier (3):
  arm64: Add cpuidle context save/restore helpers
  arm64: Convert cpu_do_idle() to using cpuidle context helpers
  PSCI: Use cpuidle context helpers in psci_cpu_suspend_enter()

 arch/arm/include/asm/cpuidle.h   |  5 ++++
 arch/arm64/include/asm/cpuidle.h | 35 +++++++++++++++++++++++++++
 arch/arm64/kernel/process.c      | 41 +++++++-------------------------
 drivers/firmware/psci/psci.c     |  5 ++++
 4 files changed, 54 insertions(+), 32 deletions(-)

-- 
2.30.2


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

[PATCH 1/3] arm64: Add cpuidle context save/restore helpers

[Marc Zyngier]

As we need to start doing some additional work on all idle
paths, let's introduce a set of macros that will perform
the work related to the GICv3 pseudo-NMI idle entry exit.

Stubs are introduced to 32bit ARM for compatibility.
As these helpers are currently unused, the is no functional
change.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm/include/asm/cpuidle.h   |  5 +++++
 arch/arm64/include/asm/cpuidle.h | 35 ++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/arch/arm/include/asm/cpuidle.h b/arch/arm/include/asm/cpuidle.h
index 0d67ed682e07..1e0b8da12d96 100644
--- a/arch/arm/include/asm/cpuidle.h
+++ b/arch/arm/include/asm/cpuidle.h
@@ -49,4 +49,9 @@ extern int arm_cpuidle_suspend(int index);
 
 extern int arm_cpuidle_init(int cpu);
 
+struct arm_cpuidle_context { };
+
+#define arm_cpuidle_save_context(c)	(void)c
+#define arm_cpuidle_restore_context(c)	(void)c
+
 #endif
diff --git a/arch/arm64/include/asm/cpuidle.h b/arch/arm64/include/asm/cpuidle.h
index 3c5ddb429ea2..53adad0a5c7e 100644
--- a/arch/arm64/include/asm/cpuidle.h
+++ b/arch/arm64/include/asm/cpuidle.h
@@ -18,4 +18,39 @@ static inline int arm_cpuidle_suspend(int index)
 	return -EOPNOTSUPP;
 }
 #endif
+
+#ifdef CONFIG_ARM64_PSEUDO_NMI
+#include <asm/arch_gicv3.h>
+
+struct arm_cpuidle_context {
+	unsigned long pmr;
+	unsigned long daif_bits;
+};
+
+#define arm_cpuidle_save_context(__c)					\
+	do {								\
+		struct arm_cpuidle_context *c = __c;			\
+		if (system_uses_irq_prio_masking()) {			\
+			c->daif_bits = read_sysreg(daif);		\
+			write_sysreg(c->daif_bits | PSR_I_BIT | PSR_F_BIT, \
+				     daif);				\
+			c->pmr = gic_read_pmr();			\
+			gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET); \
+		}							\
+	} while (0)
+
+#define arm_cpuidle_restore_context(__c)				\
+	do {								\
+		struct arm_cpuidle_context *c = __c;			\
+		if (system_uses_irq_prio_masking()) {			\
+			gic_write_pmr(c->pmr);				\
+			write_sysreg(c->daif_bits, daif);		\
+		}							\
+	} while (0)
+#else
+struct arm_cpuidle_context { };
+
+#define arm_cpuidle_save_context(c)	(void)c
+#define arm_cpuidle_restore_context(c)	(void)c
+#endif
 #endif
-- 
2.30.2


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

Re: [PATCH 1/3] arm64: Add cpuidle context save/restore helpers

[Lorenzo Pieralisi]

On Tue, Jun 08, 2021 at 06:27:13PM +0100, Marc Zyngier wrote:
> As we need to start doing some additional work on all idle
> paths, let's introduce a set of macros that will perform
> the work related to the GICv3 pseudo-NMI idle entry exit.
> 
> Stubs are introduced to 32bit ARM for compatibility.
> As these helpers are currently unused, the is no functional

s/the/there

> change.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm/include/asm/cpuidle.h   |  5 +++++
>  arch/arm64/include/asm/cpuidle.h | 35 ++++++++++++++++++++++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/arch/arm/include/asm/cpuidle.h b/arch/arm/include/asm/cpuidle.h
> index 0d67ed682e07..1e0b8da12d96 100644
> --- a/arch/arm/include/asm/cpuidle.h
> +++ b/arch/arm/include/asm/cpuidle.h
> @@ -49,4 +49,9 @@ extern int arm_cpuidle_suspend(int index);
>  
>  extern int arm_cpuidle_init(int cpu);
>  
> +struct arm_cpuidle_context { };
> +
> +#define arm_cpuidle_save_context(c)	(void)c
> +#define arm_cpuidle_restore_context(c)	(void)c
> +
>  #endif
> diff --git a/arch/arm64/include/asm/cpuidle.h b/arch/arm64/include/asm/cpuidle.h
> index 3c5ddb429ea2..53adad0a5c7e 100644
> --- a/arch/arm64/include/asm/cpuidle.h
> +++ b/arch/arm64/include/asm/cpuidle.h
> @@ -18,4 +18,39 @@ static inline int arm_cpuidle_suspend(int index)
>  	return -EOPNOTSUPP;
>  }
>  #endif
> +
> +#ifdef CONFIG_ARM64_PSEUDO_NMI
> +#include <asm/arch_gicv3.h>
> +
> +struct arm_cpuidle_context {
> +	unsigned long pmr;
> +	unsigned long daif_bits;
> +};
> +
> +#define arm_cpuidle_save_context(__c)					\
> +	do {								\
> +		struct arm_cpuidle_context *c = __c;			\
> +		if (system_uses_irq_prio_masking()) {			\
> +			c->daif_bits = read_sysreg(daif);		\
> +			write_sysreg(c->daif_bits | PSR_I_BIT | PSR_F_BIT, \
> +				     daif);				\
> +			c->pmr = gic_read_pmr();			\
> +			gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET); \
> +		}							\
> +	} while (0)
> +
> +#define arm_cpuidle_restore_context(__c)				\
> +	do {								\
> +		struct arm_cpuidle_context *c = __c;			\
> +		if (system_uses_irq_prio_masking()) {			\
> +			gic_write_pmr(c->pmr);				\
> +			write_sysreg(c->daif_bits, daif);		\
> +		}							\
> +	} while (0)
> +#else
> +struct arm_cpuidle_context { };
> +
> +#define arm_cpuidle_save_context(c)	(void)c
> +#define arm_cpuidle_restore_context(c)	(void)c
> +#endif
>  #endif

It looks good to me - maybe I would define it irq_context for clarity
but that's just a naming convention.

Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

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

Re: [PATCH 1/3] arm64: Add cpuidle context save/restore helpers

[Marc Zyngier]

On Fri, 11 Jun 2021 17:46:57 +0100,
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> 
> On Tue, Jun 08, 2021 at 06:27:13PM +0100, Marc Zyngier wrote:
> > As we need to start doing some additional work on all idle
> > paths, let's introduce a set of macros that will perform
> > the work related to the GICv3 pseudo-NMI idle entry exit.
> > 
> > Stubs are introduced to 32bit ARM for compatibility.
> > As these helpers are currently unused, the is no functional
> 
> s/the/there
> 
> > change.
> > 
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm/include/asm/cpuidle.h   |  5 +++++
> >  arch/arm64/include/asm/cpuidle.h | 35 ++++++++++++++++++++++++++++++++
> >  2 files changed, 40 insertions(+)
> > 
> > diff --git a/arch/arm/include/asm/cpuidle.h b/arch/arm/include/asm/cpuidle.h
> > index 0d67ed682e07..1e0b8da12d96 100644
> > --- a/arch/arm/include/asm/cpuidle.h
> > +++ b/arch/arm/include/asm/cpuidle.h
> > @@ -49,4 +49,9 @@ extern int arm_cpuidle_suspend(int index);
> >  
> >  extern int arm_cpuidle_init(int cpu);
> >  
> > +struct arm_cpuidle_context { };
> > +
> > +#define arm_cpuidle_save_context(c)	(void)c
> > +#define arm_cpuidle_restore_context(c)	(void)c
> > +
> >  #endif
> > diff --git a/arch/arm64/include/asm/cpuidle.h b/arch/arm64/include/asm/cpuidle.h
> > index 3c5ddb429ea2..53adad0a5c7e 100644
> > --- a/arch/arm64/include/asm/cpuidle.h
> > +++ b/arch/arm64/include/asm/cpuidle.h
> > @@ -18,4 +18,39 @@ static inline int arm_cpuidle_suspend(int index)
> >  	return -EOPNOTSUPP;
> >  }
> >  #endif
> > +
> > +#ifdef CONFIG_ARM64_PSEUDO_NMI
> > +#include <asm/arch_gicv3.h>
> > +
> > +struct arm_cpuidle_context {
> > +	unsigned long pmr;
> > +	unsigned long daif_bits;
> > +};
> > +
> > +#define arm_cpuidle_save_context(__c)					\
> > +	do {								\
> > +		struct arm_cpuidle_context *c = __c;			\
> > +		if (system_uses_irq_prio_masking()) {			\
> > +			c->daif_bits = read_sysreg(daif);		\
> > +			write_sysreg(c->daif_bits | PSR_I_BIT | PSR_F_BIT, \
> > +				     daif);				\
> > +			c->pmr = gic_read_pmr();			\
> > +			gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET); \
> > +		}							\
> > +	} while (0)
> > +
> > +#define arm_cpuidle_restore_context(__c)				\
> > +	do {								\
> > +		struct arm_cpuidle_context *c = __c;			\
> > +		if (system_uses_irq_prio_masking()) {			\
> > +			gic_write_pmr(c->pmr);				\
> > +			write_sysreg(c->daif_bits, daif);		\
> > +		}							\
> > +	} while (0)
> > +#else
> > +struct arm_cpuidle_context { };
> > +
> > +#define arm_cpuidle_save_context(c)	(void)c
> > +#define arm_cpuidle_restore_context(c)	(void)c
> > +#endif
> >  #endif
> 
> It looks good to me - maybe I would define it irq_context for clarity
> but that's just a naming convention.

would:

struct arm_cpuidle_irq_context { ...  };
#define arm_cpuidle_save_irq_context(c)		...
#define arm_cpuidle_restore_irq_context(c)	...

be OK for you?

> Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

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

Re: [PATCH 1/3] arm64: Add cpuidle context save/restore helpers

[Lorenzo Pieralisi]

On Sat, Jun 12, 2021 at 01:04:16PM +0100, Marc Zyngier wrote:
> On Fri, 11 Jun 2021 17:46:57 +0100,
> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> > 
> > On Tue, Jun 08, 2021 at 06:27:13PM +0100, Marc Zyngier wrote:
> > > As we need to start doing some additional work on all idle
> > > paths, let's introduce a set of macros that will perform
> > > the work related to the GICv3 pseudo-NMI idle entry exit.
> > > 
> > > Stubs are introduced to 32bit ARM for compatibility.
> > > As these helpers are currently unused, the is no functional
> > 
> > s/the/there
> > 
> > > change.
> > > 
> > > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > > ---
> > >  arch/arm/include/asm/cpuidle.h   |  5 +++++
> > >  arch/arm64/include/asm/cpuidle.h | 35 ++++++++++++++++++++++++++++++++
> > >  2 files changed, 40 insertions(+)
> > > 
> > > diff --git a/arch/arm/include/asm/cpuidle.h b/arch/arm/include/asm/cpuidle.h
> > > index 0d67ed682e07..1e0b8da12d96 100644
> > > --- a/arch/arm/include/asm/cpuidle.h
> > > +++ b/arch/arm/include/asm/cpuidle.h
> > > @@ -49,4 +49,9 @@ extern int arm_cpuidle_suspend(int index);
> > >  
> > >  extern int arm_cpuidle_init(int cpu);
> > >  
> > > +struct arm_cpuidle_context { };
> > > +
> > > +#define arm_cpuidle_save_context(c)	(void)c
> > > +#define arm_cpuidle_restore_context(c)	(void)c
> > > +
> > >  #endif
> > > diff --git a/arch/arm64/include/asm/cpuidle.h b/arch/arm64/include/asm/cpuidle.h
> > > index 3c5ddb429ea2..53adad0a5c7e 100644
> > > --- a/arch/arm64/include/asm/cpuidle.h
> > > +++ b/arch/arm64/include/asm/cpuidle.h
> > > @@ -18,4 +18,39 @@ static inline int arm_cpuidle_suspend(int index)
> > >  	return -EOPNOTSUPP;
> > >  }
> > >  #endif
> > > +
> > > +#ifdef CONFIG_ARM64_PSEUDO_NMI
> > > +#include <asm/arch_gicv3.h>
> > > +
> > > +struct arm_cpuidle_context {
> > > +	unsigned long pmr;
> > > +	unsigned long daif_bits;
> > > +};
> > > +
> > > +#define arm_cpuidle_save_context(__c)					\
> > > +	do {								\
> > > +		struct arm_cpuidle_context *c = __c;			\
> > > +		if (system_uses_irq_prio_masking()) {			\
> > > +			c->daif_bits = read_sysreg(daif);		\
> > > +			write_sysreg(c->daif_bits | PSR_I_BIT | PSR_F_BIT, \
> > > +				     daif);				\
> > > +			c->pmr = gic_read_pmr();			\
> > > +			gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET); \
> > > +		}							\
> > > +	} while (0)
> > > +
> > > +#define arm_cpuidle_restore_context(__c)				\
> > > +	do {								\
> > > +		struct arm_cpuidle_context *c = __c;			\
> > > +		if (system_uses_irq_prio_masking()) {			\
> > > +			gic_write_pmr(c->pmr);				\
> > > +			write_sysreg(c->daif_bits, daif);		\
> > > +		}							\
> > > +	} while (0)
> > > +#else
> > > +struct arm_cpuidle_context { };
> > > +
> > > +#define arm_cpuidle_save_context(c)	(void)c
> > > +#define arm_cpuidle_restore_context(c)	(void)c
> > > +#endif
> > >  #endif
> > 
> > It looks good to me - maybe I would define it irq_context for clarity
> > but that's just a naming convention.
> 
> would:
> 
> struct arm_cpuidle_irq_context { ...  };
> #define arm_cpuidle_save_irq_context(c)		...
> #define arm_cpuidle_restore_irq_context(c)	...
> 
> be OK for you?

Yes absolutely, thanks a lot.

Lorenzo

> > Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> 
> 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

[PATCH 2/3] arm64: Convert cpu_do_idle() to using cpuidle context helpers

[Marc Zyngier]

Now that we have helpers that are aware of the pseudo-NMI
feature, introduce them to cpu_do_idle(). This allows for
some nice cleanup.

No functional change intended.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kernel/process.c | 41 ++++++++-----------------------------
 1 file changed, 9 insertions(+), 32 deletions(-)

diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index b4bb67f17a2c..f99144eda8dc 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -46,9 +46,9 @@
 #include <linux/prctl.h>
 
 #include <asm/alternative.h>
-#include <asm/arch_gicv3.h>
 #include <asm/compat.h>
 #include <asm/cpufeature.h>
+#include <asm/cpuidle.h>
 #include <asm/cacheflush.h>
 #include <asm/exec.h>
 #include <asm/fpsimd.h>
@@ -74,33 +74,6 @@ EXPORT_SYMBOL_GPL(pm_power_off);
 
 void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
 
-static void noinstr __cpu_do_idle(void)
-{
-	dsb(sy);
-	wfi();
-}
-
-static void noinstr __cpu_do_idle_irqprio(void)
-{
-	unsigned long pmr;
-	unsigned long daif_bits;
-
-	daif_bits = read_sysreg(daif);
-	write_sysreg(daif_bits | PSR_I_BIT | PSR_F_BIT, daif);
-
-	/*
-	 * Unmask PMR before going idle to make sure interrupts can
-	 * be raised.
-	 */
-	pmr = gic_read_pmr();
-	gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET);
-
-	__cpu_do_idle();
-
-	gic_write_pmr(pmr);
-	write_sysreg(daif_bits, daif);
-}
-
 /*
  *	cpu_do_idle()
  *
@@ -112,10 +85,14 @@ static void noinstr __cpu_do_idle_irqprio(void)
  */
 void noinstr cpu_do_idle(void)
 {
-	if (system_uses_irq_prio_masking())
-		__cpu_do_idle_irqprio();
-	else
-		__cpu_do_idle();
+	struct arm_cpuidle_context context;
+
+	arm_cpuidle_save_context(&context);
+
+	dsb(sy);
+	wfi();
+
+	arm_cpuidle_restore_context(&context);
 }
 
 /*
-- 
2.30.2


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

Re: [PATCH 2/3] arm64: Convert cpu_do_idle() to using cpuidle context helpers

[Lorenzo Pieralisi]

On Tue, Jun 08, 2021 at 06:27:14PM +0100, Marc Zyngier wrote:
> Now that we have helpers that are aware of the pseudo-NMI
> feature, introduce them to cpu_do_idle(). This allows for
> some nice cleanup.
> 
> No functional change intended.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kernel/process.c | 41 ++++++++-----------------------------
>  1 file changed, 9 insertions(+), 32 deletions(-)

Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index b4bb67f17a2c..f99144eda8dc 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -46,9 +46,9 @@
>  #include <linux/prctl.h>
>  
>  #include <asm/alternative.h>
> -#include <asm/arch_gicv3.h>
>  #include <asm/compat.h>
>  #include <asm/cpufeature.h>
> +#include <asm/cpuidle.h>
>  #include <asm/cacheflush.h>
>  #include <asm/exec.h>
>  #include <asm/fpsimd.h>
> @@ -74,33 +74,6 @@ EXPORT_SYMBOL_GPL(pm_power_off);
>  
>  void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
>  
> -static void noinstr __cpu_do_idle(void)
> -{
> -	dsb(sy);
> -	wfi();
> -}
> -
> -static void noinstr __cpu_do_idle_irqprio(void)
> -{
> -	unsigned long pmr;
> -	unsigned long daif_bits;
> -
> -	daif_bits = read_sysreg(daif);
> -	write_sysreg(daif_bits | PSR_I_BIT | PSR_F_BIT, daif);
> -
> -	/*
> -	 * Unmask PMR before going idle to make sure interrupts can
> -	 * be raised.
> -	 */
> -	pmr = gic_read_pmr();
> -	gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET);
> -
> -	__cpu_do_idle();
> -
> -	gic_write_pmr(pmr);
> -	write_sysreg(daif_bits, daif);
> -}
> -
>  /*
>   *	cpu_do_idle()
>   *
> @@ -112,10 +85,14 @@ static void noinstr __cpu_do_idle_irqprio(void)
>   */
>  void noinstr cpu_do_idle(void)
>  {
> -	if (system_uses_irq_prio_masking())
> -		__cpu_do_idle_irqprio();
> -	else
> -		__cpu_do_idle();
> +	struct arm_cpuidle_context context;
> +
> +	arm_cpuidle_save_context(&context);
> +
> +	dsb(sy);
> +	wfi();
> +
> +	arm_cpuidle_restore_context(&context);
>  }
>  
>  /*
> -- 
> 2.30.2
> 

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

[PATCH 3/3] PSCI: Use cpuidle context helpers in psci_cpu_suspend_enter()

[Marc Zyngier]

The PSCI CPU suspend code isn't aware of the PMR vs DAIF game,
resulting in a system that locks up if entering CPU suspend
with GICv3 pNMI enabled.

To save the day, teach the suspend code about our new cpuidle
context helpers, which will do everything that's required just
like the usual WFI cpuidle code.

This fixes my Altra system, which would otherwise lock-up at
boot time when booted with irqchip.gicv3_pseudo_nmi=1.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/firmware/psci/psci.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
index 3c1c5daf6df2..d10675bdd9d0 100644
--- a/drivers/firmware/psci/psci.c
+++ b/drivers/firmware/psci/psci.c
@@ -333,13 +333,18 @@ static int psci_suspend_finisher(unsigned long state)
 
 int psci_cpu_suspend_enter(u32 state)
 {
+	struct arm_cpuidle_context context;
 	int ret;
 
+	arm_cpuidle_save_context(&context);
+
 	if (!psci_power_state_loses_context(state))
 		ret = psci_ops.cpu_suspend(state, 0);
 	else
 		ret = cpu_suspend(state, psci_suspend_finisher);
 
+	arm_cpuidle_restore_context(&context);
+
 	return ret;
 }
 #endif
-- 
2.30.2


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

Re: [PATCH 3/3] PSCI: Use cpuidle context helpers in psci_cpu_suspend_enter()

[Sudeep Holla]

(I see Lorenzo has replied on the other thread including me where the issue
was reported asking more details)

On Tue, Jun 08, 2021 at 06:27:15PM +0100, Marc Zyngier wrote:
> The PSCI CPU suspend code isn't aware of the PMR vs DAIF game,
> resulting in a system that locks up if entering CPU suspend
> with GICv3 pNMI enabled.
>
> To save the day, teach the suspend code about our new cpuidle
> context helpers, which will do everything that's required just
> like the usual WFI cpuidle code.
>
> This fixes my Altra system, which would otherwise lock-up at
> boot time when booted with irqchip.gicv3_pseudo_nmi=1.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  drivers/firmware/psci/psci.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> index 3c1c5daf6df2..d10675bdd9d0 100644
> --- a/drivers/firmware/psci/psci.c
> +++ b/drivers/firmware/psci/psci.c
> @@ -333,13 +333,18 @@ static int psci_suspend_finisher(unsigned long state)
>
>  int psci_cpu_suspend_enter(u32 state)
>  {
> +	struct arm_cpuidle_context context;
>  	int ret;
>
> +	arm_cpuidle_save_context(&context);
> +
>  	if (!psci_power_state_loses_context(state))
>  		ret = psci_ops.cpu_suspend(state, 0);
>  	else
>  		ret = cpu_suspend(state, psci_suspend_finisher);
>
> +	arm_cpuidle_restore_context(&context);
> +

We need similar save/restore for system suspend as well I believe
(psci_system_suspend_enter)

--
Regards,
Sudeep

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

Re: [PATCH 3/3] PSCI: Use cpuidle context helpers in psci_cpu_suspend_enter()

[Lorenzo Pieralisi]

On Tue, Jun 08, 2021 at 06:27:15PM +0100, Marc Zyngier wrote:
> The PSCI CPU suspend code isn't aware of the PMR vs DAIF game,
> resulting in a system that locks up if entering CPU suspend
> with GICv3 pNMI enabled.
> 
> To save the day, teach the suspend code about our new cpuidle
> context helpers, which will do everything that's required just
> like the usual WFI cpuidle code.
> 
> This fixes my Altra system, which would otherwise lock-up at
> boot time when booted with irqchip.gicv3_pseudo_nmi=1.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  drivers/firmware/psci/psci.c | 5 +++++
>  1 file changed, 5 insertions(+)

We need an additional patch for PSCI suspend, regardless:

Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> index 3c1c5daf6df2..d10675bdd9d0 100644
> --- a/drivers/firmware/psci/psci.c
> +++ b/drivers/firmware/psci/psci.c
> @@ -333,13 +333,18 @@ static int psci_suspend_finisher(unsigned long state)
>  
>  int psci_cpu_suspend_enter(u32 state)
>  {
> +	struct arm_cpuidle_context context;
>  	int ret;
>  
> +	arm_cpuidle_save_context(&context);
> +
>  	if (!psci_power_state_loses_context(state))
>  		ret = psci_ops.cpu_suspend(state, 0);
>  	else
>  		ret = cpu_suspend(state, psci_suspend_finisher);
>  
> +	arm_cpuidle_restore_context(&context);
> +
>  	return ret;
>  }
>  #endif
> -- 
> 2.30.2
> 

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

Re: [PATCH 0/3] arm64: Fix cpuidle with pseudo-NMI enabled

[Valentin Schneider]

Hi Marc,

On 08/06/21 18:27, Marc Zyngier wrote:
> It appears that although cpu_do_idle() is correctly dealing with the
> PMR/DAIF duality, the PSCI cpu-suspend code has been left unaware of
> it.
>
> On a system that uses PSCI for idle (such as the Ampere Altra I have
> access to), the kernel dies as soon as it enters idle (interrupts are
> off at the GIC CPU interface level). Boo.
>
> Instead of spreading more magic code around, I've elected to provide a
> pair of helpers (arm_cpuidle_{save,restore}_context()) which do the
> heavy lifting.
>
> With that in place, I can finally boot the above system with
> irqchip.gicv3_pseudo_nmi=1. I'd welcome feedback from people who may
> have experienced similar issues in the past (and on different
> machines).
>

With pNMIs my Ampere eMAG would always hang at boot, and your patches make
it actually survive. I briefly kicked perf to get some PMU IRQs and that
also seems to be going just fine. Thanks for digging into this mess!

Tested-by: Valentin Schneider <valentin.schneider@arm.com>

> Marc Zyngier (3):
>   arm64: Add cpuidle context save/restore helpers
>   arm64: Convert cpu_do_idle() to using cpuidle context helpers
>   PSCI: Use cpuidle context helpers in psci_cpu_suspend_enter()
>
>  arch/arm/include/asm/cpuidle.h   |  5 ++++
>  arch/arm64/include/asm/cpuidle.h | 35 +++++++++++++++++++++++++++
>  arch/arm64/kernel/process.c      | 41 +++++++-------------------------
>  drivers/firmware/psci/psci.c     |  5 ++++
>  4 files changed, 54 insertions(+), 32 deletions(-)
>
> --
> 2.30.2

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

Re: [PATCH 0/3] arm64: Fix cpuidle with pseudo-NMI enabled

[Valentin Schneider]

Hi Marc,

On 08/06/21 18:27, Marc Zyngier wrote:
> It appears that although cpu_do_idle() is correctly dealing with the
> PMR/DAIF duality, the PSCI cpu-suspend code has been left unaware of
> it.
>
> On a system that uses PSCI for idle (such as the Ampere Altra I have
> access to), the kernel dies as soon as it enters idle (interrupts are
> off at the GIC CPU interface level). Boo.
>
> Instead of spreading more magic code around, I've elected to provide a
> pair of helpers (arm_cpuidle_{save,restore}_context()) which do the
> heavy lifting.
>
> With that in place, I can finally boot the above system with
> irqchip.gicv3_pseudo_nmi=1. I'd welcome feedback from people who may
> have experienced similar issues in the past (and on different
> machines).
>

With pNMIs my Ampere eMAG would always hang at boot, and your patches make
it actually survive. I briefly kicked perf to get some PMU IRQs and that
also seems to be going just fine. Thanks for digging into this mess!

Tested-by: Valentin Schneider <valentin.schneider@arm.com>

> Marc Zyngier (3):
>   arm64: Add cpuidle context save/restore helpers
>   arm64: Convert cpu_do_idle() to using cpuidle context helpers
>   PSCI: Use cpuidle context helpers in psci_cpu_suspend_enter()
>
>  arch/arm/include/asm/cpuidle.h   |  5 ++++
>  arch/arm64/include/asm/cpuidle.h | 35 +++++++++++++++++++++++++++
>  arch/arm64/kernel/process.c      | 41 +++++++-------------------------
>  drivers/firmware/psci/psci.c     |  5 ++++
>  4 files changed, 54 insertions(+), 32 deletions(-)
>
> --
> 2.30.2

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

Re: [PATCH 0/3] arm64: Fix cpuidle with pseudo-NMI enabled

[Lorenzo Pieralisi]

On Tue, Jun 08, 2021 at 06:27:12PM +0100, Marc Zyngier wrote:
> It appears that although cpu_do_idle() is correctly dealing with the
> PMR/DAIF duality, the PSCI cpu-suspend code has been left unaware of
> it.
> 
> On a system that uses PSCI for idle (such as the Ampere Altra I have
> access to), the kernel dies as soon as it enters idle (interrupts are
> off at the GIC CPU interface level). Boo.

After investigating a bit I realised that this should depend on
ICC_CTLR_EL3.PMHE - if that's clear the PMR should not affect the
GICR->CPU IRQ forwarding (or WakeRequest signal generation when the
GICR_WAKER.ProcessorSleep==1).

IIUC if PMHE == 0, the PMR plays no role in wfi completion (and
WakeSignal generation for a CPU/GICR in quiescent state).

I assume on Ampere Altra PMHE == 1.

This changes almost nothing to the need for this patchset but
at least we clarify this behaviour.

Also, we should not be writing ICC_PMR_EL1 when
GICR_WAKER.ProcessorSleep == 1 (which may be set in
gic_cpu_pm_notifier()), this can hang the system.

I wonder whether this arm_cpuidle_{save,restore}_context() should
be moved into the gic_cpu_pm_notifier() itself - which would
solve also the PSCI suspend issue Sudeep raised - it would be
a bit ugly though (CPU PM notifiers are run in S2R and CPUidle
automatically and this would work for any S2R/CPUidle backend
other than PSCI even though that does not/will never exist on
arm64 ;-))

https://lore.kernel.org/linux-arm-kernel/20210608182044.ayqa6fbab4jyz7kp@bogus

I still believe this series is right - just raised these points
for discussion.

Thanks,
Lorenzo

> Instead of spreading more magic code around, I've elected to provide a
> pair of helpers (arm_cpuidle_{save,restore}_context()) which do the
> heavy lifting.
> 
> With that in place, I can finally boot the above system with
> irqchip.gicv3_pseudo_nmi=1. I'd welcome feedback from people who may
> have experienced similar issues in the past (and on different
> machines).
> 
> Marc Zyngier (3):
>   arm64: Add cpuidle context save/restore helpers
>   arm64: Convert cpu_do_idle() to using cpuidle context helpers
>   PSCI: Use cpuidle context helpers in psci_cpu_suspend_enter()
> 
>  arch/arm/include/asm/cpuidle.h   |  5 ++++
>  arch/arm64/include/asm/cpuidle.h | 35 +++++++++++++++++++++++++++
>  arch/arm64/kernel/process.c      | 41 +++++++-------------------------
>  drivers/firmware/psci/psci.c     |  5 ++++
>  4 files changed, 54 insertions(+), 32 deletions(-)
> 
> -- 
> 2.30.2
> 

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

Re: [PATCH 0/3] arm64: Fix cpuidle with pseudo-NMI enabled

[Sudeep Holla]

On Thu, Jun 10, 2021 at 05:28:23PM +0100, Lorenzo Pieralisi wrote:
> On Tue, Jun 08, 2021 at 06:27:12PM +0100, Marc Zyngier wrote:
> > It appears that although cpu_do_idle() is correctly dealing with the
> > PMR/DAIF duality, the PSCI cpu-suspend code has been left unaware of
> > it.
> > 
> > On a system that uses PSCI for idle (such as the Ampere Altra I have
> > access to), the kernel dies as soon as it enters idle (interrupts are
> > off at the GIC CPU interface level). Boo.
> 

[...]

> I wonder whether this arm_cpuidle_{save,restore}_context() should
> be moved into the gic_cpu_pm_notifier() itself - which would
> solve also the PSCI suspend issue Sudeep raised - it would be
> a bit ugly though (CPU PM notifiers are run in S2R and CPUidle

+1 if possible, I hadn't fully understood the issue to make this
suggestion. But yes if possible, we must to honour the abstraction even
though PSCI is the only user 😄.

> automatically and this would work for any S2R/CPUidle backend
> other than PSCI even though that does not/will never exist on
> arm64 ;-))
>

I almost wrote the same thing in my earlier email and deleted 😉 before
sending.

--
Regards,
Sudeep

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

Re: [PATCH 0/3] arm64: Fix cpuidle with pseudo-NMI enabled

[Marc Zyngier]

Hi Sudeep,

On Thu, 10 Jun 2021 18:43:52 +0100,
Sudeep Holla <sudeep.holla@arm.com> wrote:
> 
> On Thu, Jun 10, 2021 at 05:28:23PM +0100, Lorenzo Pieralisi wrote:
> > On Tue, Jun 08, 2021 at 06:27:12PM +0100, Marc Zyngier wrote:
> > > It appears that although cpu_do_idle() is correctly dealing with the
> > > PMR/DAIF duality, the PSCI cpu-suspend code has been left unaware of
> > > it.
> > > 
> > > On a system that uses PSCI for idle (such as the Ampere Altra I have
> > > access to), the kernel dies as soon as it enters idle (interrupts are
> > > off at the GIC CPU interface level). Boo.
> > 
> 
> [...]
> 
> > I wonder whether this arm_cpuidle_{save,restore}_context() should
> > be moved into the gic_cpu_pm_notifier() itself - which would
> > solve also the PSCI suspend issue Sudeep raised - it would be
> > a bit ugly though (CPU PM notifiers are run in S2R and CPUidle
> 
> +1 if possible, I hadn't fully understood the issue to make this
> suggestion. But yes if possible, we must to honour the abstraction even
> though PSCI is the only user 😄.

See my reply to Lorenzo. Once we switch from PMR to DAIF masking, none
of the local interrupt control helpers work anymore. If we start
leaking this change of behaviour at a higher level in the stack, I
have no idea what happens anymore.

Which is why we perform such switching in very localised cases such as
exception, guest entry, and (oh surprise! ;-) cpu_do_idle().

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

Re: [PATCH 0/3] arm64: Fix cpuidle with pseudo-NMI enabled

[Marc Zyngier]

Hi Lorenzo,

On Thu, 10 Jun 2021 17:28:23 +0100,
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> 
> On Tue, Jun 08, 2021 at 06:27:12PM +0100, Marc Zyngier wrote:
> > It appears that although cpu_do_idle() is correctly dealing with the
> > PMR/DAIF duality, the PSCI cpu-suspend code has been left unaware of
> > it.
> > 
> > On a system that uses PSCI for idle (such as the Ampere Altra I have
> > access to), the kernel dies as soon as it enters idle (interrupts are
> > off at the GIC CPU interface level). Boo.
> 
> After investigating a bit I realised that this should depend on
> ICC_CTLR_EL3.PMHE - if that's clear the PMR should not affect the
> GICR->CPU IRQ forwarding (or WakeRequest signal generation when the
> GICR_WAKER.ProcessorSleep==1).

You lost me here. I don't see what PMHE has to do here. It is solely
used for 1:N distribution, and is the only way PMR does affect the
propagation of interrupts to the CPU interface. Fortunately, nobody
uses 1:N.

> IIUC if PMHE == 0, the PMR plays no role in wfi completion (and
> WakeSignal generation for a CPU/GICR in quiescent state).

Of course it does. PMR gates interrupts *before* they are signalled to
the CPU, meaning that if you keep interrupt masked at the PMR level,
you will never wake up from WFI. Or am I missing your point entirely?

> 
> I assume on Ampere Altra PMHE == 1.

No, it is 0, as indicated by:

<quote>
[    0.000000] GICv3: Pseudo-NMIs enabled using relaxed ICC_PMR_EL1 synchronisation
</quote>

> This changes almost nothing to the need for this patchset but
> at least we clarify this behaviour.
> 
> Also, we should not be writing ICC_PMR_EL1 when
> GICR_WAKER.ProcessorSleep == 1 (which may be set in
> gic_cpu_pm_notifier()), this can hang the system.

Why? PMR defines what interrupts will be presented to the CPU
interface and trigger an exception. It doesn't affect putting the CPU
to sleep nor the wake-up.

> I wonder whether this arm_cpuidle_{save,restore}_context() should
> be moved into the gic_cpu_pm_notifier() itself - which would
> solve also the PSCI suspend issue Sudeep raised - it would be

Moving from PMR to DAIF masking is something we only do on particular
spots (exception entry/exit, guest entry/exit) as it affects the
behaviour of simple things such as local_irq_*(). Moving it to a
higher level feels super dangerous.

> a bit ugly though (CPU PM notifiers are run in S2R and CPUidle
> automatically and this would work for any S2R/CPUidle backend
> other than PSCI even though that does not/will never exist on
> arm64 ;-))
>
> https://lore.kernel.org/linux-arm-kernel/20210608182044.ayqa6fbab4jyz7kp@bogus
> 
> I still believe this series is right - just raised these points
> for discussion.

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

Re: [PATCH 0/3] arm64: Fix cpuidle with pseudo-NMI enabled

[Lorenzo Pieralisi]

On Fri, Jun 11, 2021 at 09:19:22AM +0100, Marc Zyngier wrote:
> Hi Lorenzo,
> 
> On Thu, 10 Jun 2021 17:28:23 +0100,
> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> > 
> > On Tue, Jun 08, 2021 at 06:27:12PM +0100, Marc Zyngier wrote:
> > > It appears that although cpu_do_idle() is correctly dealing with the
> > > PMR/DAIF duality, the PSCI cpu-suspend code has been left unaware of
> > > it.
> > > 
> > > On a system that uses PSCI for idle (such as the Ampere Altra I have
> > > access to), the kernel dies as soon as it enters idle (interrupts are
> > > off at the GIC CPU interface level). Boo.
> > 
> > After investigating a bit I realised that this should depend on
> > ICC_CTLR_EL3.PMHE - if that's clear the PMR should not affect the
> > GICR->CPU IRQ forwarding (or WakeRequest signal generation when the
> > GICR_WAKER.ProcessorSleep==1).
> 
> You lost me here. I don't see what PMHE has to do here. It is solely
> used for 1:N distribution, and is the only way PMR does affect the
> propagation of interrupts to the CPU interface. Fortunately, nobody
> uses 1:N.
> 
> > IIUC if PMHE == 0, the PMR plays no role in wfi completion (and
> > WakeSignal generation for a CPU/GICR in quiescent state).
> 
> Of course it does. PMR gates interrupts *before* they are signalled to
> the CPU, meaning that if you keep interrupt masked at the PMR level,
> you will never wake up from WFI. Or am I missing your point entirely?

For "simple" wfi (as in executing the wfi instruction) yes. The
IRQs are forwarded to the CPU interface that filters the IRQs based
on priorities and signal the I/F "pin" so that the core wakes up
and wfi completes - forgive me my misunderstanding.

For deep sleep states where GICR_WAKER.ProcessorSleep == 1, the
WakeSignal (ie CPU reset) is generated independently of the PMR
value AFAIK. This means that even *if* an IRQ is supposed to be
masked by the PMR it would wake up a sleeping core _anyway_.

This behaviour is different from shallow C-states (and simple wfi).

That's why I asked what path is causing trouble in
psci_cpu_suspend_enter().

> > I assume on Ampere Altra PMHE == 1.
> 
> No, it is 0, as indicated by:
> 
> <quote>
> [    0.000000] GICv3: Pseudo-NMIs enabled using relaxed ICC_PMR_EL1 synchronisation
> </quote>
> 
> > This changes almost nothing to the need for this patchset but
> > at least we clarify this behaviour.
> > 
> > Also, we should not be writing ICC_PMR_EL1 when
> > GICR_WAKER.ProcessorSleep == 1 (which may be set in
> > gic_cpu_pm_notifier()), this can hang the system.
> 
> Why? PMR defines what interrupts will be presented to the CPU
> interface and trigger an exception. It doesn't affect putting the CPU
> to sleep nor the wake-up.

I don't think we are allowed to have traffic between the CPU IF and
the GICR when ProcessorSleep == 1. So, again IIUC, we can't write
the PMR (if PMHE == 1) after putting the GICR in ProcessorSleep ==1 
because this would sync with the GICR.

> > I wonder whether this arm_cpuidle_{save,restore}_context() should
> > be moved into the gic_cpu_pm_notifier() itself - which would
> > solve also the PSCI suspend issue Sudeep raised - it would be
> 
> Moving from PMR to DAIF masking is something we only do on particular
> spots (exception entry/exit, guest entry/exit) as it affects the
> behaviour of simple things such as local_irq_*(). Moving it to a
> higher level feels super dangerous.

Yes it is dangerous and ugly. It is also true that what needs to
be saved/restore on idle entry is becoming extremely complicated
and hard to track across kernel subsystems and that's unfortunate
too, that's what is causing these bugs.

Thanks,
Lorenzo

> > a bit ugly though (CPU PM notifiers are run in S2R and CPUidle
> > automatically and this would work for any S2R/CPUidle backend
> > other than PSCI even though that does not/will never exist on
> > arm64 ;-))
> >
> > https://lore.kernel.org/linux-arm-kernel/20210608182044.ayqa6fbab4jyz7kp@bogus
> > 
> > I still believe this series is right - just raised these points
> > for discussion.
> 
> 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

Re: [PATCH 0/3] arm64: Fix cpuidle with pseudo-NMI enabled

[Marc Zyngier]

On Fri, 11 Jun 2021 10:41:34 +0100,
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> 
> On Fri, Jun 11, 2021 at 09:19:22AM +0100, Marc Zyngier wrote:
> > Hi Lorenzo,
> > 
> > On Thu, 10 Jun 2021 17:28:23 +0100,
> > Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> > > 
> > > On Tue, Jun 08, 2021 at 06:27:12PM +0100, Marc Zyngier wrote:
> > > > It appears that although cpu_do_idle() is correctly dealing with the
> > > > PMR/DAIF duality, the PSCI cpu-suspend code has been left unaware of
> > > > it.
> > > > 
> > > > On a system that uses PSCI for idle (such as the Ampere Altra I have
> > > > access to), the kernel dies as soon as it enters idle (interrupts are
> > > > off at the GIC CPU interface level). Boo.
> > > 
> > > After investigating a bit I realised that this should depend on
> > > ICC_CTLR_EL3.PMHE - if that's clear the PMR should not affect the
> > > GICR->CPU IRQ forwarding (or WakeRequest signal generation when the
> > > GICR_WAKER.ProcessorSleep==1).
> > 
> > You lost me here. I don't see what PMHE has to do here. It is solely
> > used for 1:N distribution, and is the only way PMR does affect the
> > propagation of interrupts to the CPU interface. Fortunately, nobody
> > uses 1:N.
> > 
> > > IIUC if PMHE == 0, the PMR plays no role in wfi completion (and
> > > WakeSignal generation for a CPU/GICR in quiescent state).
> > 
> > Of course it does. PMR gates interrupts *before* they are signalled to
> > the CPU, meaning that if you keep interrupt masked at the PMR level,
> > you will never wake up from WFI. Or am I missing your point entirely?
> 
> For "simple" wfi (as in executing the wfi instruction) yes. The
> IRQs are forwarded to the CPU interface that filters the IRQs based
> on priorities and signal the I/F "pin" so that the core wakes up
> and wfi completes - forgive me my misunderstanding.

No worries. We're talking about the GIC architecture here, which has
the potential to confuse anyone! :D

> For deep sleep states where GICR_WAKER.ProcessorSleep == 1, the
> WakeSignal (ie CPU reset) is generated independently of the PMR
> value AFAIK. This means that even *if* an IRQ is supposed to be
> masked by the PMR it would wake up a sleeping core _anyway_.

I'm not sure we can draw this conclusion. It certainly isn't the
behaviour I'm observing. Otherwise, my system would be able to wake-up
without any additional hacks. It may wake-up the CPU interface, but
not the whole core. It is also completely possible that firmware will
use the PMR value as a hint to decide which interrupts can wake up the
CPU if it is so inclined.

> This behaviour is different from shallow C-states (and simple wfi).
> 
> That's why I asked what path is causing trouble in
> psci_cpu_suspend_enter().

It is the one where we don't loose context. Which makes sense, as it
would otherwise behave like a full reset, and we'd end-up with some
sane values.

> > > I assume on Ampere Altra PMHE == 1.
> > 
> > No, it is 0, as indicated by:
> > 
> > <quote>
> > [    0.000000] GICv3: Pseudo-NMIs enabled using relaxed ICC_PMR_EL1 synchronisation
> > </quote>
> > 
> > > This changes almost nothing to the need for this patchset but
> > > at least we clarify this behaviour.
> > > 
> > > Also, we should not be writing ICC_PMR_EL1 when
> > > GICR_WAKER.ProcessorSleep == 1 (which may be set in
> > > gic_cpu_pm_notifier()), this can hang the system.
> > 
> > Why? PMR defines what interrupts will be presented to the CPU
> > interface and trigger an exception. It doesn't affect putting the CPU
> > to sleep nor the wake-up.
> 
> I don't think we are allowed to have traffic between the CPU IF and
> the GICR when ProcessorSleep == 1. So, again IIUC, we can't write
> the PMR (if PMHE == 1) after putting the GICR in ProcessorSleep ==1 
> because this would sync with the GICR.

Hmmm. This restriction isn't obvious to me. There is some vague
threats in 10.1, but nothing concrete. A.4.11 is more explicit, but
doesn't really define what 'traffic' is. It is also tied to the stream
protocol, which isn't actually mandated.

But if it exists, PHME doesn't have any influence on PMR being sent
back to the RD. That can happen at any time (it is just that we
enforce it with a DSB in the case where PHME is set). Note that
do_cpu_idle() already does this, so we'd already be on thin ice. It is
also unclear whether 'traffic' occurs when the group enables are set
to 0, which is the case when we set ProcessorSleep=1.

And to be honest, setting ProcessorSleep=1 in the kernel (which
implies DS=1) has always been pretty odd. I don't know of any HW that
requires it. I'd be tempted to get rid of it for good.

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