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

From: Marc Zyngier <maz@kernel.org>
To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
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: Fri, 11 Jun 2021 09:19:22 +0100	[thread overview]
Message-ID: <87eed8reyd.wl-maz@kernel.org> (raw)
In-Reply-To: <20210610162823.GA20025@lpieralisi>

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