Re: [RFC PATCH 1/2] KVM: arm64: Add PSCI SYSTEM_OFF2 function for hibernation

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 2491 bytes --]

On Tue, 2024-03-12 at 15:36 +0000, Marc Zyngier wrote:
> On Tue, 12 Mar 2024 13:51:28 +0000,
> David Woodhouse <dwmw2@infradead.org> wrote:
> > 
> > +Hibernation using the PSCI SYSTEM_OFF2 call is enabled with the
> > +KVM_CAP_ARM_SYSTEM_OFF2 VM capability. If a guest invokes the PSCI
> 
> Checking that PSCI 1.3 is enabled for the guest should be enough, no?
> I don't think providing yet another level of optionally brings us
> much, other than complexity.

This is just following what we already do for SYSTEM_RESET2. Regardless
of the PSCI version, these calls are *optional*. Shouldn't exposing
them to the guest be a deliberate choice on the part of the userspace
VMM?

I was originally thinking of a KVM_CAP with a bitmask of the optional
features to be enabled (and which would return the bitmask of supported
features). But that isn't how it was already being done, so I just
followed the existing precedent.

> > --- a/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> > @@ -264,6 +264,8 @@ static unsigned long psci_1_0_handler(u64 func_id, struct kvm_cpu_context *host_
> >         switch (func_id) {
> >         case PSCI_1_0_FN_PSCI_FEATURES:
> >         case PSCI_1_0_FN_SET_SUSPEND_MODE:
> > +       case PSCI_1_3_FN_SYSTEM_OFF2:
> > +       case PSCI_1_3_FN64_SYSTEM_OFF2:
> 
> nit: order by version number.

Ack.

> > @@ -353,6 +359,11 @@ static int kvm_psci_1_x_call(struct kvm_vcpu *vcpu, u32 minor)
> >                         if (test_bit(KVM_ARCH_FLAG_SYSTEM_SUSPEND_ENABLED, &kvm->arch.flags))
> >                                 val = 0;
> >                         break;
> > +               case PSCI_1_3_FN_SYSTEM_OFF2:
> > +               case PSCI_1_3_FN64_SYSTEM_OFF2:
> > +                       if (test_bit(KVM_ARCH_FLAG_SYSTEM_OFF2_ENABLED, &kvm->arch.flags))
> > +                               val = 1UL << PSCI_1_3_HIBERNATE_TYPE_OFF;
> > +                       break;
> 
> Testing the PSCI version should be enough (minor >= 3). Same thing
> goes the the capability: checking that the host supports 1.3 should be
> enough.

Wouldn't that mean we should implement *all* the new functions which
are optional in v1.3? I really think the opt-in should be per feature,
for the optional ones.



[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]