2024-03-18 17:32:26

by Marc Zyngier

[permalink] [raw]
Subject: Re: [RFC PATCH v2 2/4] KVM: arm64: Add PSCI SYSTEM_OFF2 function for hibernation

On Mon, 18 Mar 2024 16:14:24 +0000,
David Woodhouse <[email protected]> wrote:
>
> From: David Woodhouse <[email protected]>
>
> The PSCI v1.3 specification (alpha) adds support for a SYSTEM_OFF2 function
> which is analogous to ACPI S4 state. This will allow hosting environments
> to determine that a guest is hibernated rather than just powered off, and
> ensure that they preserve the virtual environment appropriately to allow
> the guest to resume safely (or bump the hardware_signature in the FACS to
> trigger a clean reboot instead).
>
> The beta version will be changed to say that PSCI_FEATURES returns a bit
> mask of the supported hibernate types, which is implemented here.
>
> Signed-off-by: David Woodhouse <[email protected]>
> ---
> Documentation/virt/kvm/api.rst | 11 +++++++++
> arch/arm64/include/asm/kvm_host.h | 2 ++
> arch/arm64/include/uapi/asm/kvm.h | 6 +++++
> arch/arm64/kvm/arm.c | 5 +++++
> arch/arm64/kvm/psci.c | 37 +++++++++++++++++++++++++++++++
> include/uapi/linux/kvm.h | 1 +
> 6 files changed, 62 insertions(+)
>
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index 0b5a33ee71ee..ff061b6a2393 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -6761,6 +6761,10 @@ the first `ndata` items (possibly zero) of the data array are valid.
> the guest issued a SYSTEM_RESET2 call according to v1.1 of the PSCI
> specification.
>
> + - for arm64, data[0] is set to KVM_SYSTEM_EVENT_SHUTDOWN_FLAG_PSCI_OFF2
> + if the guest issued a SYSTEM_OFF2 call according to v1.3 of the PSCI
> + specification.
> +
> - for RISC-V, data[0] is set to the value of the second argument of the
> ``sbi_system_reset`` call.
>
> @@ -6794,6 +6798,13 @@ either:
> - Deny the guest request to suspend the VM. See ARM DEN0022D.b 5.19.2
> "Caller responsibilities" for possible return values.
>
> +Hibernation using the PSCI SYSTEM_OFF2 call is enabled with the
> +KVM_CAP_ARM_SYSTEM_OFF2 VM capability. If a guest invokes the PSCI
> +SYSTEM_OFF2 function, KVM will exit to userspace with the
> +KVM_SYSTEM_EVENT_SHUTDOWN event type and with data[0] set to
> +KVM_SYSTEM_EVENT_SHUTDOWN_FLAG_PSCI_OFF2. The only supported hibernate
> +type for the SYSTEM_OFF2 function is HIBERNATE_OFF (0x0).

Again, I really oppose this way of doing things. We already have an
infrastructure for selecting PSCI levels. You may not like it, but it
exists, and I'm not going entertain supporting yet another bike-shed
model. Adding an orthogonal cap for a feature that is specific to a
new PSCI version is just awful.

Please make PSCI 1.3 the only version of PSCI supporting suspend in a
non-optional way, and be done with it.

M.

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


2024-03-18 20:21:33

by David Woodhouse

[permalink] [raw]
Subject: Re: [RFC PATCH v2 2/4] KVM: arm64: Add PSCI SYSTEM_OFF2 function for hibernation

On Mon, 2024-03-18 at 17:29 +0000, Marc Zyngier wrote:
>
> Again, I really oppose this way of doing things. We already have an
> infrastructure for selecting PSCI levels. You may not like it, but it
> exists, and I'm not going entertain supporting yet another bike-shed
> model. Adding an orthogonal cap for a feature that is specific to a
> new PSCI version is just awful.

Huh? This isn't a "new bike-shed model". This is a straight copy of
what we *already* have for SYSTEM_RESET2.

If I were bike-shedding, I wouldn't do separate caps for them; I'd have
done it as a *bitmask* of the optional PSCI calls that should be
enabled.

The *mandatory* ones should obviously come from the PSCI version alone,
but I can't see how that makes sense for the optional ones...

> Please make PSCI 1.3 the only version of PSCI supporting suspend in a
> non-optional way, and be done with it.

SYSTEM_OFF2 is an *optional* feature in PSCI v1.3. As are
CLEAR_INV_MEMREGION and CLEAR_INV_MEMREGION_ATTRIBUTES.

Are you suggesting that enabling v1.3 should automatically enable *all*
of the optional features that were defined in that version (and
previous versions) of the spec?


Attachments:
smime.p7s (5.83 kB)