Received-SPF: pass (google.com: domain of linux-kernel+bounces-111786-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Fri, 22 Mar 2024 16:37:19 +0000
Message-ID: <86edc2z0hs.wl-maz@kernel.org>
From: Marc Zyngier <maz@kernel.org>
To: David Woodhouse <dwmw2@infradead.org>
Cc: linux-arm-kernel@lists.infradead.org,
	kvm@vger.kernel.org,
	Paolo Bonzini
 <pbonzini@redhat.com>,
	Jonathan Corbet <corbet@lwn.net>,
	Oliver Upton
 <oliver.upton@linux.dev>,
	James Morse <james.morse@arm.com>,
	Suzuki K
 Poulose <suzuki.poulose@arm.com>,
	Zenghui Yu <yuzenghui@huawei.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Lorenzo Pieralisi
 <lpieralisi@kernel.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Len Brown
 <len.brown@intel.com>,
	Pavel Machek <pavel@ucw.cz>,
	Mostafa Saleh
 <smostafa@google.com>,
	Jean-Philippe Brucker <jean-philippe@linaro.org>,
	linux-doc@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	kvmarm@lists.linux.dev,
	linux-pm@vger.kernel.org
Subject: Re: [RFC PATCH v3 5/5] arm64: Use SYSTEM_OFF2 PSCI call to power off for hibernate
In-Reply-To: <9efb39597fa7b36b6c4202ab73fae6610194e45e.camel@infradead.org>
References: <20240319130957.1050637-1-dwmw2@infradead.org>
	<20240319130957.1050637-6-dwmw2@infradead.org>
	<86jzluz24b.wl-maz@kernel.org>
	<9efb39597fa7b36b6c4202ab73fae6610194e45e.camel@infradead.org>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/29.1
 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO)
Precedence: bulk
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Fri, 22 Mar 2024 16:12:44 +0000,
David Woodhouse <dwmw2@infradead.org> wrote:
>=20
> On Fri, 2024-03-22 at 16:02 +0000, Marc Zyngier wrote:
> > On Tue, 19 Mar 2024 12:59:06 +0000,
> > David Woodhouse <dwmw2@infradead.org> wrote:
> >=20
> > [...]
> >=20
> > > +static void __init psci_init_system_off2(void)
> > > +{
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0int ret;
> > > +
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0ret =3D psci_features(PSCI=
_FN_NATIVE(1_3, SYSTEM_OFF2));
> > > +
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (ret !=3D PSCI_RET_NOT_=
SUPPORTED)
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0psci_system_off2_supported =3D true;
> >=20
> > It'd be worth considering the (slightly broken) case where SYSTEM_OFF2
> > is supported, but HIBERNATE_OFF is not set in the response, as the
> > spec doesn't say that this bit is mandatory (it seems legal to
> > implement SYSTEM_OFF2 without any hibernate type, making it similar to
> > SYSTEM_OFF).
>=20
> Such is not my understanding. If SYSTEM_OFF2 is supported, then
> HIBERNATE_OFF *is* mandatory.
>=20
> The next update to the spec is turning the PSCI_FEATURES response into
> a *bitmap* of the available features, and I believe it will mandate
> that bit zero is set.

The bitmap is already present in the current Alpha spec:

<quote>
5.16.2 Implementation responsibilities

[...]

Bits[31] Reserved, must be zero.

Bits[30:0] Hibernate types supported.
	- 0x0 - HIBERNATE_OFF

All other values are reserved for future use.
</quote>

and doesn't say (yet) that HIBERNATE_OFF is mandatory. Furthermore,

<quote>
5.11.2 Caller responsibilities

The calling OS uses the PSCI_FEATURES API, with the SYSTEM_OFF2
function ID, to discover whether the function is present:

- If the function is implemented, PSCI_FEATURES returns the hibernate
  types supported.

- If the function is not implemented, PSCI_FEATURES returns
  NOT_SUPPORTED.
</quote>

which doesn't say anything about which hibernate type must be
implemented. Which makes sense, as I expect it to, in the fine ARM
tradition, grow things such as "HIBERNATE_WITH_ROT13_ENCRYPTION" and
even "HIBERNATE_WITH_ERRATA_XYZ", because firmware is where people
dump their crap. And we will need some special handling for these
tasty variants.

> And if for whatever reason that SYSTEM_OFF2/HIBERNATE_OFF call
> *doesn't* work, Linux will end up doing a 'real' poweroff, first
> through EFI and then finally as a last resort with a PSCI SYSTEM_OFF.
> So it would be OK to have false positives in the detection.

I agree that nothing really breaks, but I also hold the view that
broken firmware implementations should be given the finger, specially
given that you have done this work *ahead* of the spec. I would really
like this to fail immediately on these and not even try to suspend.

With that in mind, if doesn't really matter whether HIBERNATE_OFF is
mandatory or not. We really should check for it and pretend it doesn't
exist if the correct flag isn't set.

	M.

--=20
Without deviation from the norm, progress is not possible.