[PATCH v5 0/5] Add PSCI v1.3 SYSTEM_OFF2 support for hibernation

[PATCH v5 0/5] Add PSCI v1.3 SYSTEM_OFF2 support for hibernation

[David Woodhouse]

The PSCI v1.3 spec (https://developer.arm.com/documentation/den0022)
adds support for a SYSTEM_OFF2 function enabling a HIBERNATE_OFF state
which is analogous to ACPI S4. 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).

This updates KVM to support advertising PSCI v1.3, and unconditionally
enables the SYSTEM_OFF2 support when PSCI v1.3 is enabled.

For the guest side, add a new SYS_OFF_MODE_POWER_OFF handler with higher
priority than the EFI one, but which *only* triggers when there's a
hibernation in progress. There are other ways to do this (see the commit
message for more details) but this seemed like the simplest.

Version 2 of the patch series splits out the psci.h definitions into a
separate commit (a dependency for both the guest and KVM side), and adds
definitions for the other new functions added in v1.3. It also moves the
pKVM psci-relay support to a separate commit; although in arch/arm64/kvm
that's actually about the *guest* side of SYSTEM_OFF2 (i.e. using it
from the host kernel, relayed through nVHE).

Version 3 dropped the KVM_CAP which allowed userspace to explicitly opt
in to the new feature like with SYSTEM_SUSPEND, and makes it depend only
on PSCI v1.3 being exposed to the guest.

Version 4 is no longer RFC, as the PSCI v1.3 spec is finally published.
Minor fixes from the last round of review, and an added KVM self test.

Version 5 drops some of the changes which didn't make it to the final 
v1.3 spec, and cleans up a couple of places which still referred to it 
as 'alpha' or 'beta'. It also temporarily drops the guest-side patch to 
invoke SYSTEM_OFF2 for hibernation, pending confirmation that the final 
PSCI v1.3 spec just has a typo where it changed to saying that 0x1 
should be passed to mean HIBERNATE_OFF, even though it's advertised as 
bit 0. That can be sent under separate cover, and perhaps should have 
been anyway. The change in question doesn't matter for any of the KVM 
patches, because we just treat SYSTEM_OFF2 like the existing 
SYSTEM_RESET2, setting a flag to indicate that it was a SYSTEM_OFF2 
call, but not actually caring about the argument; that's for userspace 
to worry about.

David Woodhouse (5):
      firmware/psci: Add definitions for PSCI v1.3 specification
      KVM: arm64: Add PSCI v1.3 SYSTEM_OFF2 function for hibernation
      KVM: arm64: Add support for PSCI v1.2 and v1.3
      KVM: selftests: Add test for PSCI SYSTEM_OFF2
      KVM: arm64: nvhe: Pass through PSCI v1.3 SYSTEM_OFF2 call

 Documentation/virt/kvm/api.rst                  | 11 +++++
 arch/arm64/include/uapi/asm/kvm.h               |  6 +++
 arch/arm64/kvm/hyp/nvhe/psci-relay.c            |  2 +
 arch/arm64/kvm/hypercalls.c                     |  2 +
 arch/arm64/kvm/psci.c                           | 43 ++++++++++++++++-
 include/kvm/arm_psci.h                          |  4 +-
 include/uapi/linux/psci.h                       |  5 ++
 tools/testing/selftests/kvm/aarch64/psci_test.c | 61 +++++++++++++++++++++++++
 8 files changed, 132 insertions(+), 2 deletions(-)

[PATCH v5 1/5] firmware/psci: Add definitions for PSCI v1.3 specification

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

The v1.3 PSCI spec (https://developer.arm.com/documentation/den0022) adds
the SYSTEM_OFF2 function. Add definitions for it and its hibernation type
parameter.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 include/uapi/linux/psci.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/uapi/linux/psci.h b/include/uapi/linux/psci.h
index 42a40ad3fb62..f9a015ebe221 100644
--- a/include/uapi/linux/psci.h
+++ b/include/uapi/linux/psci.h
@@ -59,6 +59,7 @@
 #define PSCI_1_1_FN_SYSTEM_RESET2		PSCI_0_2_FN(18)
 #define PSCI_1_1_FN_MEM_PROTECT			PSCI_0_2_FN(19)
 #define PSCI_1_1_FN_MEM_PROTECT_CHECK_RANGE	PSCI_0_2_FN(20)
+#define PSCI_1_3_FN_SYSTEM_OFF2			PSCI_0_2_FN(21)
 
 #define PSCI_1_0_FN64_CPU_DEFAULT_SUSPEND	PSCI_0_2_FN64(12)
 #define PSCI_1_0_FN64_NODE_HW_STATE		PSCI_0_2_FN64(13)
@@ -68,6 +69,7 @@
 
 #define PSCI_1_1_FN64_SYSTEM_RESET2		PSCI_0_2_FN64(18)
 #define PSCI_1_1_FN64_MEM_PROTECT_CHECK_RANGE	PSCI_0_2_FN64(20)
+#define PSCI_1_3_FN64_SYSTEM_OFF2		PSCI_0_2_FN64(21)
 
 /* PSCI v0.2 power state encoding for CPU_SUSPEND function */
 #define PSCI_0_2_POWER_STATE_ID_MASK		0xffff
@@ -100,6 +102,9 @@
 #define PSCI_1_1_RESET_TYPE_SYSTEM_WARM_RESET	0
 #define PSCI_1_1_RESET_TYPE_VENDOR_START	0x80000000U
 
+/* PSCI v1.3 hibernate type for SYSTEM_OFF2 */
+#define PSCI_1_3_HIBERNATE_TYPE_OFF		0
+
 /* PSCI version decoding (independent of PSCI version) */
 #define PSCI_VERSION_MAJOR_SHIFT		16
 #define PSCI_VERSION_MINOR_MASK			\
-- 
2.44.0

[PATCH v5 2/5] KVM: arm64: Add PSCI v1.3 SYSTEM_OFF2 function for hibernation

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

The PSCI v1.3 specification 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).

Although this new feature is inflicted unconditionally on unexpecting
userspace, it should be OK because it still results in the same
KVM_SYSTEM_EVENT_SHUTDOWN event, just with a new flag which hopefully
won't cause userspace to get unhappy.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 Documentation/virt/kvm/api.rst    | 11 +++++++++
 arch/arm64/include/uapi/asm/kvm.h |  6 +++++
 arch/arm64/kvm/psci.c             | 37 +++++++++++++++++++++++++++++++
 3 files changed, 54 insertions(+)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index b3be87489108..2918898b7047 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -6840,6 +6840,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.
 
@@ -6873,6 +6877,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 when PSCI v1.3
+is enabled. 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).
+
 ::
 
 		/* KVM_EXIT_IOAPIC_EOI */
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index 964df31da975..66736ff04011 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -484,6 +484,12 @@ enum {
  */
 #define KVM_SYSTEM_EVENT_RESET_FLAG_PSCI_RESET2	(1ULL << 0)
 
+/*
+ * Shutdown caused by a PSCI v1.3 SYSTEM_OFF2 call.
+ * Valid only when the system event has a type of KVM_SYSTEM_EVENT_SHUTDOWN.
+ */
+#define KVM_SYSTEM_EVENT_SHUTDOWN_FLAG_PSCI_OFF2	(1ULL << 0)
+
 /* run->fail_entry.hardware_entry_failure_reason codes. */
 #define KVM_EXIT_FAIL_ENTRY_CPU_UNSUPPORTED	(1ULL << 0)
 
diff --git a/arch/arm64/kvm/psci.c b/arch/arm64/kvm/psci.c
index 1f69b667332b..fd0f82464f7d 100644
--- a/arch/arm64/kvm/psci.c
+++ b/arch/arm64/kvm/psci.c
@@ -194,6 +194,12 @@ static void kvm_psci_system_off(struct kvm_vcpu *vcpu)
 	kvm_prepare_system_event(vcpu, KVM_SYSTEM_EVENT_SHUTDOWN, 0);
 }
 
+static void kvm_psci_system_off2(struct kvm_vcpu *vcpu)
+{
+	kvm_prepare_system_event(vcpu, KVM_SYSTEM_EVENT_SHUTDOWN,
+				 KVM_SYSTEM_EVENT_SHUTDOWN_FLAG_PSCI_OFF2);
+}
+
 static void kvm_psci_system_reset(struct kvm_vcpu *vcpu)
 {
 	kvm_prepare_system_event(vcpu, KVM_SYSTEM_EVENT_RESET, 0);
@@ -358,6 +364,11 @@ static int kvm_psci_1_x_call(struct kvm_vcpu *vcpu, u32 minor)
 			if (minor >= 1)
 				val = 0;
 			break;
+		case PSCI_1_3_FN_SYSTEM_OFF2:
+		case PSCI_1_3_FN64_SYSTEM_OFF2:
+			if (minor >= 3)
+				val = BIT(PSCI_1_3_HIBERNATE_TYPE_OFF);
+			break;
 		}
 		break;
 	case PSCI_1_0_FN_SYSTEM_SUSPEND:
@@ -392,6 +403,32 @@ static int kvm_psci_1_x_call(struct kvm_vcpu *vcpu, u32 minor)
 			break;
 		}
 		break;
+	case PSCI_1_3_FN_SYSTEM_OFF2:
+		kvm_psci_narrow_to_32bit(vcpu);
+		fallthrough;
+	case PSCI_1_3_FN64_SYSTEM_OFF2:
+		if (minor < 3)
+			break;
+
+		arg = smccc_get_arg1(vcpu);
+		if (arg != PSCI_1_3_HIBERNATE_TYPE_OFF) {
+			val = PSCI_RET_INVALID_PARAMS;
+			break;
+		}
+		kvm_psci_system_off2(vcpu);
+		/*
+		 * We shouldn't be going back to guest VCPU after
+		 * receiving SYSTEM_OFF2 request.
+		 *
+		 * If user space accidentally/deliberately resumes
+		 * guest VCPU after SYSTEM_OFF2 request then guest
+		 * VCPU should see internal failure from PSCI return
+		 * value. To achieve this, we preload r0 (or x0) with
+		 * PSCI return value INTERNAL_FAILURE.
+		 */
+		val = PSCI_RET_INTERNAL_FAILURE;
+		ret = 0;
+		break;
 	default:
 		return kvm_psci_0_2_call(vcpu);
 	}
-- 
2.44.0

[PATCH v5 3/5] KVM: arm64: Add support for PSCI v1.2 and v1.3

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 arch/arm64/kvm/hypercalls.c | 2 ++
 arch/arm64/kvm/psci.c       | 6 +++++-
 include/kvm/arm_psci.h      | 4 +++-
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
index 5763d979d8ca..9c6267ca2b82 100644
--- a/arch/arm64/kvm/hypercalls.c
+++ b/arch/arm64/kvm/hypercalls.c
@@ -575,6 +575,8 @@ int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 		case KVM_ARM_PSCI_0_2:
 		case KVM_ARM_PSCI_1_0:
 		case KVM_ARM_PSCI_1_1:
+		case KVM_ARM_PSCI_1_2:
+		case KVM_ARM_PSCI_1_3:
 			if (!wants_02)
 				return -EINVAL;
 			vcpu->kvm->arch.psci_version = val;
diff --git a/arch/arm64/kvm/psci.c b/arch/arm64/kvm/psci.c
index fd0f82464f7d..5177dda5a411 100644
--- a/arch/arm64/kvm/psci.c
+++ b/arch/arm64/kvm/psci.c
@@ -328,7 +328,7 @@ static int kvm_psci_1_x_call(struct kvm_vcpu *vcpu, u32 minor)
 
 	switch(psci_fn) {
 	case PSCI_0_2_FN_PSCI_VERSION:
-		val = minor == 0 ? KVM_ARM_PSCI_1_0 : KVM_ARM_PSCI_1_1;
+		val = PSCI_VERSION(1, minor);
 		break;
 	case PSCI_1_0_FN_PSCI_FEATURES:
 		arg = smccc_get_arg1(vcpu);
@@ -486,6 +486,10 @@ int kvm_psci_call(struct kvm_vcpu *vcpu)
 	}
 
 	switch (version) {
+	case KVM_ARM_PSCI_1_3:
+		return kvm_psci_1_x_call(vcpu, 3);
+	case KVM_ARM_PSCI_1_2:
+		return kvm_psci_1_x_call(vcpu, 2);
 	case KVM_ARM_PSCI_1_1:
 		return kvm_psci_1_x_call(vcpu, 1);
 	case KVM_ARM_PSCI_1_0:
diff --git a/include/kvm/arm_psci.h b/include/kvm/arm_psci.h
index e8fb624013d1..cbaec804eb83 100644
--- a/include/kvm/arm_psci.h
+++ b/include/kvm/arm_psci.h
@@ -14,8 +14,10 @@
 #define KVM_ARM_PSCI_0_2	PSCI_VERSION(0, 2)
 #define KVM_ARM_PSCI_1_0	PSCI_VERSION(1, 0)
 #define KVM_ARM_PSCI_1_1	PSCI_VERSION(1, 1)
+#define KVM_ARM_PSCI_1_2	PSCI_VERSION(1, 2)
+#define KVM_ARM_PSCI_1_3	PSCI_VERSION(1, 3)
 
-#define KVM_ARM_PSCI_LATEST	KVM_ARM_PSCI_1_1
+#define KVM_ARM_PSCI_LATEST	KVM_ARM_PSCI_1_3
 
 static inline int kvm_psci_version(struct kvm_vcpu *vcpu)
 {
-- 
2.44.0

[PATCH v5 4/5] KVM: selftests: Add test for PSCI SYSTEM_OFF2

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 .../testing/selftests/kvm/aarch64/psci_test.c | 61 +++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/tools/testing/selftests/kvm/aarch64/psci_test.c b/tools/testing/selftests/kvm/aarch64/psci_test.c
index 61731a950def..b7e37956aecf 100644
--- a/tools/testing/selftests/kvm/aarch64/psci_test.c
+++ b/tools/testing/selftests/kvm/aarch64/psci_test.c
@@ -54,6 +54,15 @@ static uint64_t psci_system_suspend(uint64_t entry_addr, uint64_t context_id)
 	return res.a0;
 }
 
+static uint64_t psci_system_off2(uint64_t type)
+{
+	struct arm_smccc_res res;
+
+	smccc_hvc(PSCI_1_3_FN64_SYSTEM_OFF2, type, 0, 0, 0, 0, 0, 0, &res);
+
+	return res.a0;
+}
+
 static uint64_t psci_features(uint32_t func_id)
 {
 	struct arm_smccc_res res;
@@ -188,11 +197,63 @@ static void host_test_system_suspend(void)
 	kvm_vm_free(vm);
 }
 
+static void guest_test_system_off2(void)
+{
+	uint64_t ret;
+
+	/* assert that SYSTEM_OFF2 is discoverable */
+	GUEST_ASSERT(psci_features(PSCI_1_3_FN_SYSTEM_OFF2) &
+		     BIT(PSCI_1_3_HIBERNATE_TYPE_OFF));
+	GUEST_ASSERT(psci_features(PSCI_1_3_FN64_SYSTEM_OFF2) &
+		     BIT(PSCI_1_3_HIBERNATE_TYPE_OFF));
+
+	ret = psci_system_off2(PSCI_1_3_HIBERNATE_TYPE_OFF);
+	GUEST_SYNC(ret);
+}
+
+static void host_test_system_off2(void)
+{
+	struct kvm_vcpu *source, *target;
+	uint64_t psci_version = 0;
+	struct kvm_run *run;
+	struct kvm_vm *vm;
+
+	vm = setup_vm(guest_test_system_off2, &source, &target);
+	vcpu_get_reg(target, KVM_REG_ARM_PSCI_VERSION, &psci_version);
+	TEST_ASSERT(psci_version >= PSCI_VERSION(0, 2),
+		    "Unexpected PSCI version %lu.%lu",
+		    PSCI_VERSION_MAJOR(psci_version),
+		    PSCI_VERSION_MINOR(psci_version));
+
+	if (psci_version < PSCI_VERSION(1,3))
+		goto skip;
+
+	vcpu_power_off(target);
+	run = source->run;
+
+	enter_guest(source);
+
+	TEST_ASSERT_KVM_EXIT_REASON(source, KVM_EXIT_SYSTEM_EVENT);
+	TEST_ASSERT(run->system_event.type == KVM_SYSTEM_EVENT_SHUTDOWN,
+		    "Unhandled system event: %u (expected: %u)",
+		    run->system_event.type, KVM_SYSTEM_EVENT_SHUTDOWN);
+	TEST_ASSERT(run->system_event.ndata >= 1,
+		    "Unexpected amount of system event data: %u (expected, >= 1)",
+		    run->system_event.ndata);
+	TEST_ASSERT(run->system_event.data[0] & KVM_SYSTEM_EVENT_SHUTDOWN_FLAG_PSCI_OFF2,
+		    "PSCI_OFF2 flag not set. Flags %llu (expected %llu)",
+		    run->system_event.data[0], KVM_SYSTEM_EVENT_SHUTDOWN_FLAG_PSCI_OFF2);
+
+ skip:
+	kvm_vm_free(vm);
+}
+
 int main(void)
 {
 	TEST_REQUIRE(kvm_has_cap(KVM_CAP_ARM_SYSTEM_SUSPEND));
 
 	host_test_cpu_on();
 	host_test_system_suspend();
+	host_test_system_off2();
 	return 0;
 }
-- 
2.44.0

[PATCH v5 5/5] KVM: arm64: nvhe: Pass through PSCI v1.3 SYSTEM_OFF2 call

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

Pass through the SYSTEM_OFF2 function for hibernation, just like SYSTEM_OFF.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 arch/arm64/kvm/hyp/nvhe/psci-relay.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/kvm/hyp/nvhe/psci-relay.c b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
index dfe8fe0f7eaf..9c2ce1e0e99a 100644
--- a/arch/arm64/kvm/hyp/nvhe/psci-relay.c
+++ b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
@@ -265,6 +265,8 @@ static unsigned long psci_1_0_handler(u64 func_id, struct kvm_cpu_context *host_
 	case PSCI_1_0_FN_PSCI_FEATURES:
 	case PSCI_1_0_FN_SET_SUSPEND_MODE:
 	case PSCI_1_1_FN64_SYSTEM_RESET2:
+	case PSCI_1_3_FN_SYSTEM_OFF2:
+	case PSCI_1_3_FN64_SYSTEM_OFF2:
 		return psci_forward(host_ctxt);
 	case PSCI_1_0_FN64_SYSTEM_SUSPEND:
 		return psci_system_suspend(func_id, host_ctxt);
-- 
2.44.0

Re: [PATCH v5 2/5] KVM: arm64: Add PSCI v1.3 SYSTEM_OFF2 function for hibernation

[Oliver Upton]

Hi David,

On Thu, Sep 26, 2024 at 07:37:57PM +0100, David Woodhouse wrote:
> @@ -392,6 +403,32 @@ static int kvm_psci_1_x_call(struct kvm_vcpu *vcpu, u32 minor)
>  			break;
>  		}
>  		break;
> +	case PSCI_1_3_FN_SYSTEM_OFF2:
> +		kvm_psci_narrow_to_32bit(vcpu);
> +		fallthrough;
> +	case PSCI_1_3_FN64_SYSTEM_OFF2:
> +		if (minor < 3)
> +			break;
> +
> +		arg = smccc_get_arg1(vcpu);
> +		if (arg != PSCI_1_3_HIBERNATE_TYPE_OFF) {
> +			val = PSCI_RET_INVALID_PARAMS;
> +			break;
> +		}

This is missing a check that arg2 must be zero.

> +		kvm_psci_system_off2(vcpu);
> +		/*
> +		 * We shouldn't be going back to guest VCPU after
> +		 * receiving SYSTEM_OFF2 request.
> +		 *
> +		 * If user space accidentally/deliberately resumes
> +		 * guest VCPU after SYSTEM_OFF2 request then guest
> +		 * VCPU should see internal failure from PSCI return
> +		 * value. To achieve this, we preload r0 (or x0) with
> +		 * PSCI return value INTERNAL_FAILURE.
> +		 */
> +		val = PSCI_RET_INTERNAL_FAILURE;
> +		ret = 0;
> +		break;
>  	default:
>  		return kvm_psci_0_2_call(vcpu);
>  	}
> -- 
> 2.44.0
>

-- 
Thanks,
Oliver

Re: [PATCH v5 4/5] KVM: selftests: Add test for PSCI SYSTEM_OFF2

[Oliver Upton]

On Thu, Sep 26, 2024 at 07:37:59PM +0100, David Woodhouse wrote:
> +static void guest_test_system_off2(void)
> +{
> +	uint64_t ret;
> +
> +	/* assert that SYSTEM_OFF2 is discoverable */
> +	GUEST_ASSERT(psci_features(PSCI_1_3_FN_SYSTEM_OFF2) &
> +		     BIT(PSCI_1_3_HIBERNATE_TYPE_OFF));
> +	GUEST_ASSERT(psci_features(PSCI_1_3_FN64_SYSTEM_OFF2) &
> +		     BIT(PSCI_1_3_HIBERNATE_TYPE_OFF));
> +

Can you also assert that the guest gets INVALID_PARAMETERS if it sets
arg1 or arg2 to a reserved value?

> +	ret = psci_system_off2(PSCI_1_3_HIBERNATE_TYPE_OFF);
> +	GUEST_SYNC(ret);
> +}
> +
> +static void host_test_system_off2(void)
> +{
> +	struct kvm_vcpu *source, *target;
> +	uint64_t psci_version = 0;
> +	struct kvm_run *run;
> +	struct kvm_vm *vm;
> +
> +	vm = setup_vm(guest_test_system_off2, &source, &target);
> +	vcpu_get_reg(target, KVM_REG_ARM_PSCI_VERSION, &psci_version);
> +	TEST_ASSERT(psci_version >= PSCI_VERSION(0, 2),
> +		    "Unexpected PSCI version %lu.%lu",
> +		    PSCI_VERSION_MAJOR(psci_version),
> +		    PSCI_VERSION_MINOR(psci_version));
> +
> +	if (psci_version < PSCI_VERSION(1,3))
> +		goto skip;

I'm not following this. Is there a particular reason why we'd want to
skip for v1.2 and fail the test for anything less than that?

Just do TEST_REQUIRE(psci_version >= PSCI_VERSION(1, 3)), it makes the
requirements obvious in the case someone runs new selftests on an old
kernel.

-- 
Thanks,
Oliver

Re: [PATCH v5 3/5] KVM: arm64: Add support for PSCI v1.2 and v1.3

[Oliver Upton]

On Thu, Sep 26, 2024 at 07:37:58PM +0100, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>

Please, add changelogs to your patches.

What we really need here is the detail on *why* we can just bump the
PSCI version like this, i.e. no new required ABI. On top of that, you
could mention that KVM has made the implementation choice to provide
SYSTEM_OFF2 unconditionally in its PSCIv1.3 implementation.

> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>  arch/arm64/kvm/hypercalls.c | 2 ++
>  arch/arm64/kvm/psci.c       | 6 +++++-
>  include/kvm/arm_psci.h      | 4 +++-
>  3 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
> index 5763d979d8ca..9c6267ca2b82 100644
> --- a/arch/arm64/kvm/hypercalls.c
> +++ b/arch/arm64/kvm/hypercalls.c
> @@ -575,6 +575,8 @@ int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  		case KVM_ARM_PSCI_0_2:
>  		case KVM_ARM_PSCI_1_0:
>  		case KVM_ARM_PSCI_1_1:
> +		case KVM_ARM_PSCI_1_2:
> +		case KVM_ARM_PSCI_1_3:
>  			if (!wants_02)
>  				return -EINVAL;
>  			vcpu->kvm->arch.psci_version = val;
> diff --git a/arch/arm64/kvm/psci.c b/arch/arm64/kvm/psci.c
> index fd0f82464f7d..5177dda5a411 100644
> --- a/arch/arm64/kvm/psci.c
> +++ b/arch/arm64/kvm/psci.c
> @@ -328,7 +328,7 @@ static int kvm_psci_1_x_call(struct kvm_vcpu *vcpu, u32 minor)
>  
>  	switch(psci_fn) {
>  	case PSCI_0_2_FN_PSCI_VERSION:
> -		val = minor == 0 ? KVM_ARM_PSCI_1_0 : KVM_ARM_PSCI_1_1;
> +		val = PSCI_VERSION(1, minor);
>  		break;
>  	case PSCI_1_0_FN_PSCI_FEATURES:
>  		arg = smccc_get_arg1(vcpu);
> @@ -486,6 +486,10 @@ int kvm_psci_call(struct kvm_vcpu *vcpu)
>  	}
>  
>  	switch (version) {
> +	case KVM_ARM_PSCI_1_3:
> +		return kvm_psci_1_x_call(vcpu, 3);
> +	case KVM_ARM_PSCI_1_2:
> +		return kvm_psci_1_x_call(vcpu, 2);
>  	case KVM_ARM_PSCI_1_1:
>  		return kvm_psci_1_x_call(vcpu, 1);
>  	case KVM_ARM_PSCI_1_0:
> diff --git a/include/kvm/arm_psci.h b/include/kvm/arm_psci.h
> index e8fb624013d1..cbaec804eb83 100644
> --- a/include/kvm/arm_psci.h
> +++ b/include/kvm/arm_psci.h
> @@ -14,8 +14,10 @@
>  #define KVM_ARM_PSCI_0_2	PSCI_VERSION(0, 2)
>  #define KVM_ARM_PSCI_1_0	PSCI_VERSION(1, 0)
>  #define KVM_ARM_PSCI_1_1	PSCI_VERSION(1, 1)
> +#define KVM_ARM_PSCI_1_2	PSCI_VERSION(1, 2)
> +#define KVM_ARM_PSCI_1_3	PSCI_VERSION(1, 3)
>  
> -#define KVM_ARM_PSCI_LATEST	KVM_ARM_PSCI_1_1
> +#define KVM_ARM_PSCI_LATEST	KVM_ARM_PSCI_1_3
>  
>  static inline int kvm_psci_version(struct kvm_vcpu *vcpu)
>  {
> -- 
> 2.44.0
> 

-- 
Thanks,
Oliver

Re: [PATCH v5 4/5] KVM: selftests: Add test for PSCI SYSTEM_OFF2

[David Woodhouse]

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

On Tue, 2024-10-01 at 08:33 -0700, Oliver Upton wrote:
> On Thu, Sep 26, 2024 at 07:37:59PM +0100, David Woodhouse wrote:
> > +static void guest_test_system_off2(void)
> > +{
> > +       uint64_t ret;
> > +
> > +       /* assert that SYSTEM_OFF2 is discoverable */
> > +       GUEST_ASSERT(psci_features(PSCI_1_3_FN_SYSTEM_OFF2) &
> > +                    BIT(PSCI_1_3_HIBERNATE_TYPE_OFF));
> > +       GUEST_ASSERT(psci_features(PSCI_1_3_FN64_SYSTEM_OFF2) &
> > +                    BIT(PSCI_1_3_HIBERNATE_TYPE_OFF));
> > +
> 
> Can you also assert that the guest gets INVALID_PARAMETERS if it sets
> arg1 or arg2 to a reserved value?

Ack (having actually made KVM do so, as you noted on a previous patch).

> > +       ret = psci_system_off2(PSCI_1_3_HIBERNATE_TYPE_OFF);
> > +       GUEST_SYNC(ret);
> > +}
> > +
> > +static void host_test_system_off2(void)
> > +{
> > +       struct kvm_vcpu *source, *target;
> > +       uint64_t psci_version = 0;
> > +       struct kvm_run *run;
> > +       struct kvm_vm *vm;
> > +
> > +       vm = setup_vm(guest_test_system_off2, &source, &target);
> > +       vcpu_get_reg(target, KVM_REG_ARM_PSCI_VERSION, &psci_version);
> > +       TEST_ASSERT(psci_version >= PSCI_VERSION(0, 2),
> > +                   "Unexpected PSCI version %lu.%lu",
> > +                   PSCI_VERSION_MAJOR(psci_version),
> > +                   PSCI_VERSION_MINOR(psci_version));
> > +
> > +       if (psci_version < PSCI_VERSION(1,3))
> > +               goto skip;
> 
> I'm not following this. Is there a particular reason why we'd want to
> skip for v1.2 and fail the test for anything less than that?

These tests unconditionally set KVM_ARM_VCPU_PSCI_0_2 in setup_vm().
Which is probably OK assuming support for that that predates
KVM_CAP_ARM_SYSTEM_SUSPEND (which is already a TEST_REQUIRE() right at
the start).

So the world is very broken if KVM actually starts a VM but the version
isn't at least 0.2, and it seemed like it warranted an actual failure.

> Just do TEST_REQUIRE(psci_version >= PSCI_VERSION(1, 3)), it makes the
> requirements obvious in the case someone runs new selftests on an old
> kernel.

I don't think we want to put that in main() and skip the other checks
that would run on earlier kernels. (Even if we had easy access to
psci_version without actually running a test and starting a VM).

I could put it into host_test_system_off2() which runs last (and
comment the invocations in main() to say that they're in increasing
order of PSCI version) to accommodate such). But then it seems that I'd
be the target of this comment in ksft_exit_skip()...

        /*
         * FIXME: several tests misuse ksft_exit_skip so produce
         * something sensible if some tests have already been run
         * or a plan has been printed.  Those tests should use
         * ksft_test_result_skip or ksft_exit_fail_msg instead.
         */

I suspect the real answer here is that the individual tests here be
calling ksft_test_result_pass(), and the system_off2 one should call
ksft_test_result_skip() if it skips?

That's probably material for a completely separate patch series, but it
seems like we're better off leaving host_test_system_off2() as I have
it here, so that it's just a case of adding that call before the 'goto
skip'.

I'll add an explicit comment about the 0.2 check though, saying that it
should never happen so we might as well have the ASSERT for it.


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

Re: [PATCH v5 4/5] KVM: selftests: Add test for PSCI SYSTEM_OFF2

[Oliver Upton]

On Sat, Oct 12, 2024 at 10:28:10AM +0100, David Woodhouse wrote:
> On Tue, 2024-10-01 at 08:33 -0700, Oliver Upton wrote:
> > On Thu, Sep 26, 2024 at 07:37:59PM +0100, David Woodhouse wrote:
> > > +       vm = setup_vm(guest_test_system_off2, &source, &target);
> > > +       vcpu_get_reg(target, KVM_REG_ARM_PSCI_VERSION, &psci_version);
> > > +       TEST_ASSERT(psci_version >= PSCI_VERSION(0, 2),
> > > +                   "Unexpected PSCI version %lu.%lu",
> > > +                   PSCI_VERSION_MAJOR(psci_version),
> > > +                   PSCI_VERSION_MINOR(psci_version));
> > > +
> > > +       if (psci_version < PSCI_VERSION(1,3))
> > > +               goto skip;
> > 
> > I'm not following this. Is there a particular reason why we'd want to
> > skip for v1.2 and fail the test for anything less than that?
> 
> These tests unconditionally set KVM_ARM_VCPU_PSCI_0_2 in setup_vm().
> Which is probably OK assuming support for that that predates
> KVM_CAP_ARM_SYSTEM_SUSPEND (which is already a TEST_REQUIRE() right at
> the start).
> 
> So the world is very broken if KVM actually starts a VM but the version
> isn't at least 0.2, and it seemed like it warranted an actual failure.

If we're looking at this from a testing lens then KVM coming up with any
PSCI version other than KVM_ARM_PSCI_LATEST (i.e. v1.3) is a bug. So
maybe we can tighten that assertion because...

> > Just do TEST_REQUIRE(psci_version >= PSCI_VERSION(1, 3)), it makes the
> > requirements obvious in the case someone runs new selftests on an old
> > kernel.
> 
> I don't think we want to put that in main() and skip the other checks
> that would run on earlier kernels.

Running KVM selftests on older kernels in a meaningful way isn't
something we support. At all. An example of this is commit
8a53e1302133 ("KVM: selftests: Require KVM_CAP_USER_MEMORY2 for
tests that create memslots"), which skips ~everything for kernels older
than 6.8.

> (Even if we had easy access to
> psci_version without actually running a test and starting a VM).
> 
> I could put it into host_test_system_off2() which runs last (and
> comment the invocations in main() to say that they're in increasing
> order of PSCI version) to accommodate such). But then it seems that I'd
> be the target of this comment in ksft_exit_skip()...
> 
>         /*
>          * FIXME: several tests misuse ksft_exit_skip so produce
>          * something sensible if some tests have already been run
>          * or a plan has been printed.  Those tests should use
>          * ksft_test_result_skip or ksft_exit_fail_msg instead.
>          */
> 
> I suspect the real answer here is that the individual tests here be
> calling ksft_test_result_pass(), and the system_off2 one should call
> ksft_test_result_skip() if it skips?

modulo a few one-offs, KVM selftests doesn't use the kselftest harness
so it isn't subject to this comment. Since there's no test plan, we can
skip at any time.

> I'll add an explicit comment about the 0.2 check though, saying that it
> should never happen so we might as well have the ASSERT for it.

After looking at this again, I think we should do one of the following:

 - TEST_REQUIRE() that the PSCI version is at least v1.3, making the
   dependency clear on older kernels.

 - TEST_REQUIRE() for v1.3, which would provide better test coverage on
   upstream.

-- 
Thanks,
Oliver

Re: [PATCH v5 4/5] KVM: selftests: Add test for PSCI SYSTEM_OFF2

[Oliver Upton]

On Tue, Oct 15, 2024 at 09:05:18AM -0700, Oliver Upton wrote:
> On Sat, Oct 12, 2024 at 10:28:10AM +0100, David Woodhouse wrote:
> > On Tue, 2024-10-01 at 08:33 -0700, Oliver Upton wrote:
> > > On Thu, Sep 26, 2024 at 07:37:59PM +0100, David Woodhouse wrote:
> > > > +       vm = setup_vm(guest_test_system_off2, &source, &target);
> > > > +       vcpu_get_reg(target, KVM_REG_ARM_PSCI_VERSION, &psci_version);
> > > > +       TEST_ASSERT(psci_version >= PSCI_VERSION(0, 2),
> > > > +                   "Unexpected PSCI version %lu.%lu",
> > > > +                   PSCI_VERSION_MAJOR(psci_version),
> > > > +                   PSCI_VERSION_MINOR(psci_version));
> > > > +
> > > > +       if (psci_version < PSCI_VERSION(1,3))
> > > > +               goto skip;
> > > 
> > > I'm not following this. Is there a particular reason why we'd want to
> > > skip for v1.2 and fail the test for anything less than that?
> > 
> > These tests unconditionally set KVM_ARM_VCPU_PSCI_0_2 in setup_vm().
> > Which is probably OK assuming support for that that predates
> > KVM_CAP_ARM_SYSTEM_SUSPEND (which is already a TEST_REQUIRE() right at
> > the start).
> > 
> > So the world is very broken if KVM actually starts a VM but the version
> > isn't at least 0.2, and it seemed like it warranted an actual failure.
> 
> If we're looking at this from a testing lens then KVM coming up with any
> PSCI version other than KVM_ARM_PSCI_LATEST (i.e. v1.3) is a bug. So
> maybe we can tighten that assertion because...
> 
> > > Just do TEST_REQUIRE(psci_version >= PSCI_VERSION(1, 3)), it makes the
> > > requirements obvious in the case someone runs new selftests on an old
> > > kernel.
> > 
> > I don't think we want to put that in main() and skip the other checks
> > that would run on earlier kernels.
> 
> Running KVM selftests on older kernels in a meaningful way isn't
> something we support. At all. An example of this is commit
> 8a53e1302133 ("KVM: selftests: Require KVM_CAP_USER_MEMORY2 for
> tests that create memslots"), which skips ~everything for kernels older
> than 6.8.
> 
> > (Even if we had easy access to
> > psci_version without actually running a test and starting a VM).
> > 
> > I could put it into host_test_system_off2() which runs last (and
> > comment the invocations in main() to say that they're in increasing
> > order of PSCI version) to accommodate such). But then it seems that I'd
> > be the target of this comment in ksft_exit_skip()...
> > 
> >         /*
> >          * FIXME: several tests misuse ksft_exit_skip so produce
> >          * something sensible if some tests have already been run
> >          * or a plan has been printed.  Those tests should use
> >          * ksft_test_result_skip or ksft_exit_fail_msg instead.
> >          */
> > 
> > I suspect the real answer here is that the individual tests here be
> > calling ksft_test_result_pass(), and the system_off2 one should call
> > ksft_test_result_skip() if it skips?
> 
> modulo a few one-offs, KVM selftests doesn't use the kselftest harness
> so it isn't subject to this comment. Since there's no test plan, we can
> skip at any time.
> 
> > I'll add an explicit comment about the 0.2 check though, saying that it
> > should never happen so we might as well have the ASSERT for it.
> 
> After looking at this again, I think we should do one of the following:
> 
>  - TEST_REQUIRE() that the PSCI version is at least v1.3, making the
>    dependency clear on older kernels.
> 
>  - TEST_REQUIRE() for v1.3, which would provide better test coverage on
>    upstream.

Sorry, I meant TEST_ASSERT() here.

> -- 
> Thanks,
> Oliver
> 

Re: [PATCH v5 4/5] KVM: selftests: Add test for PSCI SYSTEM_OFF2

[Sean Christopherson]

On Tue, Oct 15, 2024, Oliver Upton wrote:
> On Sat, Oct 12, 2024 at 10:28:10AM +0100, David Woodhouse wrote:
> > I suspect the real answer here is that the individual tests here be
> > calling ksft_test_result_pass(), and the system_off2 one should call
> > ksft_test_result_skip() if it skips?
> 
> modulo a few one-offs, KVM selftests doesn't use the kselftest harness
> so it isn't subject to this comment. Since there's no test plan, we can
> skip at any time.

FWIW, I have some ideas on how to use the nicer pieces of kselftest harness, if
anyone is looking for a project.  :-)

https://lore.kernel.org/all/ZjUwqEXPA5QVItyX@google.com

Re: [PATCH v5 4/5] KVM: selftests: Add test for PSCI SYSTEM_OFF2

[David Woodhouse]

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

On Tue, 2024-10-15 at 10:16 -0700, Oliver Upton wrote:
> 
> > After looking at this again, I think we should do one of the following:
> > 
> >   - TEST_REQUIRE() that the PSCI version is at least v1.3, making the
> >     dependency clear on older kernels.
> > 
> >   - TEST_REQUIRE() for v1.3, which would provide better test coverage on
> >     upstream.
> 
> Sorry, I meant TEST_ASSERT() here.

Ack. I'll do the latter.

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