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

From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
To: Marc Zyngier <maz@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org,
	Will Deacon <will@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Mark Rutland <mark.rutland@arm.com>,
	Valentin Schneider <valentin.schneider@arm.com>,
	Alexandru Elisei <alexandru.elisei@arm.com>,
	Russell King <linux@armlinux.org.uk>,
	kernel-team@android.com
Subject: Re: [PATCH 0/3] arm64: Fix cpuidle with pseudo-NMI enabled
Date: Thu, 10 Jun 2021 17:28:23 +0100	[thread overview]
Message-ID: <20210610162823.GA20025@lpieralisi> (raw)
In-Reply-To: <20210608172715.2396787-1-maz@kernel.org>

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