[PATCH v2 00/11] KVM: arm64: Implement PSCI SYSTEM_SUSPEND support

[PATCH v2 00/11] KVM: arm64: Implement PSCI SYSTEM_SUSPEND support

[Oliver Upton]

Certain VMMs/operators may wish to give their guests the ability to
initiate a system suspend that could result in the VM being saved to
persistent storage to be resumed at a later time. The PSCI v1.0
specification describes an SMC, SYSTEM_SUSPEND, that allows a kernel to
request a system suspend. This call is optional for v1.0, and KVM
elected to not support the call in its v1.0 implementation.

This series adds support for the SYSTEM_SUSPEND PSCI call to KVM/arm64.
Since this is a system-scoped event, KVM cannot quiesce the VM on its
own. We add a new system exit type in this series to clue in userspace
that a suspend was requested. Per the KVM_EXIT_SYSTEM_EVENT ABI, a VMM
that doesn't care about this event can simply resume the guest without
issue (we set up the calling vCPU to come out of reset correctly on next
KVM_RUN). If a VMM would like to have KVM emulate the suspend, it can do
so by setting the vCPU's MP state to KVM_MP_STATE_HALTED. Support for
this state has been added in this series.

Patch 1 is an unrelated cleanup, dropping an unused parameter

Patch 2 simplifies how KVM filters SMC64 functions for AArch32 guests.

Patch 3 wraps up the vCPU reset logic used by the PSCI CPU_ON
implementation in KVM for subsequent use, as we must queue up a reset
for the vCPU that requested a system suspend.

Patch 4 is another unrelated cleanup, fixing the naming for the
KVM_REQ_SLEEP handler to avoid confusion and remain consistent with the
handler introduced in this series.

Patch 5 changes how WFI-like events are handled in KVM (WFI instruction,
PSCI CPU_SUSPEND). Instead of directly blocking the vCPU in the
respective handlers, set a request bit and block before resuming the
guest. WFI and PSCI CPU_SUSPEND do not require deferral of
kvm_vcpu_block(), but SYSTEM_SUSPEND does. Rather than adding a deferral
mechanism just for SYSTEM_SUSPEND, it is a bit cleaner to have all
blocking events just request the event.

Patch 6 actually adds PSCI SYSTEM_SUSPEND support to KVM, and adds the
necessary UAPI to pair with the call.

Patch 7 renames the PSCI selftest to something more generic, as we will
test more than just CPU_ON.

Patch 8 creates a common helper for making SMC64 calls in KVM selftests,
rather than having tests open-code their own approach.

Patch 9 makes the PSCI test use KVM_SET_MP_STATE for powering off a vCPU
rather than the vCPU init flag. This change is necessary to separate
generic VM setup from the setup for a particular PSCI test.

Patch 10 reworks psci_test into a bunch of helpers, making it easier to
build additional test cases with the common parts.

Finally, patch 11 adds 2 test cases for the SYSTEM_SUSPEND PSCI call.
Verify that the call succeeds if all other vCPUs have been powered off
and that it fails if more than the calling vCPU is powered on.

This series applies cleanly to v5.15-rc2. Testing was performed on an
Ampere Mt. Jade system.

Oliver Upton (11):
  KVM: arm64: Drop unused vcpu param to kvm_psci_valid_affinity()
  KVM: arm64: Clean up SMC64 PSCI filtering for AArch32 guests
  KVM: arm64: Encapsulate reset request logic in a helper function
  KVM: arm64: Rename the KVM_REQ_SLEEP handler
  KVM: arm64: Defer WFI emulation as a requested event
  KVM: arm64: Add support for SYSTEM_SUSPEND PSCI call
  selftests: KVM: Rename psci_cpu_on_test to psci_test
  selftests: KVM: Create helper for making SMCCC calls
  selftests: KVM: Use KVM_SET_MP_STATE to power off vCPU in psci_test
  selftests: KVM: Refactor psci_test to make it amenable to new tests
  selftests: KVM: Test SYSTEM_SUSPEND PSCI call

 Documentation/virt/kvm/api.rst                |   6 +
 arch/arm64/include/asm/kvm_host.h             |   4 +
 arch/arm64/kvm/arm.c                          |  21 +-
 arch/arm64/kvm/handle_exit.c                  |   3 +-
 arch/arm64/kvm/psci.c                         | 138 ++++++++---
 include/uapi/linux/kvm.h                      |   2 +
 tools/testing/selftests/kvm/.gitignore        |   2 +-
 tools/testing/selftests/kvm/Makefile          |   2 +-
 .../selftests/kvm/aarch64/psci_cpu_on_test.c  | 121 ----------
 .../testing/selftests/kvm/aarch64/psci_test.c | 218 ++++++++++++++++++
 .../selftests/kvm/include/aarch64/processor.h |  22 ++
 .../selftests/kvm/lib/aarch64/processor.c     |  25 ++
 tools/testing/selftests/kvm/steal_time.c      |  13 +-
 13 files changed, 403 insertions(+), 174 deletions(-)
 delete mode 100644 tools/testing/selftests/kvm/aarch64/psci_cpu_on_test.c
 create mode 100644 tools/testing/selftests/kvm/aarch64/psci_test.c

-- 
2.33.0.685.g46640cef36-goog

[PATCH v2 01/11] KVM: arm64: Drop unused vcpu param to kvm_psci_valid_affinity()

[Oliver Upton]

The helper function does not need a pointer to the vCPU, as it only
consults a constant mask; drop the unused vcpu parameter.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/kvm/psci.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/psci.c b/arch/arm64/kvm/psci.c
index 74c47d420253..d46842f45b0a 100644
--- a/arch/arm64/kvm/psci.c
+++ b/arch/arm64/kvm/psci.c
@@ -59,8 +59,7 @@ static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu)
 	kvm_vcpu_kick(vcpu);
 }
 
-static inline bool kvm_psci_valid_affinity(struct kvm_vcpu *vcpu,
-					   unsigned long affinity)
+static inline bool kvm_psci_valid_affinity(unsigned long affinity)
 {
 	return !(affinity & ~MPIDR_HWID_BITMASK);
 }
@@ -73,7 +72,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
 	unsigned long cpu_id;
 
 	cpu_id = smccc_get_arg1(source_vcpu);
-	if (!kvm_psci_valid_affinity(source_vcpu, cpu_id))
+	if (!kvm_psci_valid_affinity(cpu_id))
 		return PSCI_RET_INVALID_PARAMS;
 
 	vcpu = kvm_mpidr_to_vcpu(kvm, cpu_id);
@@ -132,7 +131,7 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu)
 	target_affinity = smccc_get_arg1(vcpu);
 	lowest_affinity_level = smccc_get_arg2(vcpu);
 
-	if (!kvm_psci_valid_affinity(vcpu, target_affinity))
+	if (!kvm_psci_valid_affinity(target_affinity))
 		return PSCI_RET_INVALID_PARAMS;
 
 	/* Determine target affinity mask */
-- 
2.33.0.685.g46640cef36-goog

[PATCH v2 02/11] KVM: arm64: Clean up SMC64 PSCI filtering for AArch32 guests

[Oliver Upton]

The only valid calling SMC calling convention from an AArch32 state is
SMC32. Disallow any PSCI function that sets the SMC64 function ID bit
when called from AArch32 rather than comparing against known SMC64 PSCI
functions.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/kvm/psci.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/kvm/psci.c b/arch/arm64/kvm/psci.c
index d46842f45b0a..310b9cb2b32b 100644
--- a/arch/arm64/kvm/psci.c
+++ b/arch/arm64/kvm/psci.c
@@ -208,15 +208,11 @@ static void kvm_psci_narrow_to_32bit(struct kvm_vcpu *vcpu)
 
 static unsigned long kvm_psci_check_allowed_function(struct kvm_vcpu *vcpu, u32 fn)
 {
-	switch(fn) {
-	case PSCI_0_2_FN64_CPU_SUSPEND:
-	case PSCI_0_2_FN64_CPU_ON:
-	case PSCI_0_2_FN64_AFFINITY_INFO:
-		/* Disallow these functions for 32bit guests */
-		if (vcpu_mode_is_32bit(vcpu))
-			return PSCI_RET_NOT_SUPPORTED;
-		break;
-	}
+	/*
+	 * Prevent 32 bit guests from calling 64 bit PSCI functions.
+	 */
+	if ((fn & PSCI_0_2_64BIT) && vcpu_mode_is_32bit(vcpu))
+		return PSCI_RET_NOT_SUPPORTED;
 
 	return 0;
 }
-- 
2.33.0.685.g46640cef36-goog

[PATCH v2 03/11] KVM: arm64: Encapsulate reset request logic in a helper function

[Oliver Upton]

In its implementation of the PSCI function, KVM needs to request that a
target vCPU resets before its next entry into the guest. Wrap the logic
for requesting a reset in a function for later use by other implemented
PSCI calls.

No functional change intended.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/kvm/psci.c | 59 +++++++++++++++++++++++++------------------
 1 file changed, 35 insertions(+), 24 deletions(-)

diff --git a/arch/arm64/kvm/psci.c b/arch/arm64/kvm/psci.c
index 310b9cb2b32b..bb59b692998b 100644
--- a/arch/arm64/kvm/psci.c
+++ b/arch/arm64/kvm/psci.c
@@ -64,9 +64,40 @@ static inline bool kvm_psci_valid_affinity(unsigned long affinity)
 	return !(affinity & ~MPIDR_HWID_BITMASK);
 }
 
-static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
+static void kvm_psci_vcpu_request_reset(struct kvm_vcpu *vcpu,
+					unsigned long entry_addr,
+					unsigned long context_id,
+					bool big_endian)
 {
 	struct vcpu_reset_state *reset_state;
+
+	lockdep_assert_held(&vcpu->kvm->lock);
+
+	reset_state = &vcpu->arch.reset_state;
+	reset_state->pc = entry_addr;
+
+	/* Propagate caller endianness */
+	reset_state->be = big_endian;
+
+	/*
+	 * NOTE: We always update r0 (or x0) because for PSCI v0.1
+	 * the general purpose registers are undefined upon CPU_ON.
+	 */
+	reset_state->r0 = context_id;
+
+	WRITE_ONCE(reset_state->reset, true);
+	kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
+
+	/*
+	 * Make sure the reset request is observed if the change to
+	 * power_state is observed.
+	 */
+	smp_wmb();
+	vcpu->arch.power_off = false;
+}
+
+static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
+{
 	struct kvm *kvm = source_vcpu->kvm;
 	struct kvm_vcpu *vcpu = NULL;
 	unsigned long cpu_id;
@@ -90,29 +121,9 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
 			return PSCI_RET_INVALID_PARAMS;
 	}
 
-	reset_state = &vcpu->arch.reset_state;
-
-	reset_state->pc = smccc_get_arg2(source_vcpu);
-
-	/* Propagate caller endianness */
-	reset_state->be = kvm_vcpu_is_be(source_vcpu);
-
-	/*
-	 * NOTE: We always update r0 (or x0) because for PSCI v0.1
-	 * the general purpose registers are undefined upon CPU_ON.
-	 */
-	reset_state->r0 = smccc_get_arg3(source_vcpu);
-
-	WRITE_ONCE(reset_state->reset, true);
-	kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
-
-	/*
-	 * Make sure the reset request is observed if the change to
-	 * power_state is observed.
-	 */
-	smp_wmb();
-
-	vcpu->arch.power_off = false;
+	kvm_psci_vcpu_request_reset(vcpu, smccc_get_arg2(source_vcpu),
+				    smccc_get_arg3(source_vcpu),
+				    kvm_vcpu_is_be(source_vcpu));
 	kvm_vcpu_wake_up(vcpu);
 
 	return PSCI_RET_SUCCESS;
-- 
2.33.0.685.g46640cef36-goog

[PATCH v2 04/11] KVM: arm64: Rename the KVM_REQ_SLEEP handler

[Oliver Upton]

The naming of the kvm_req_sleep function is confusing: the function
itself sleeps the vCPU, it does not request such an event. Rename the
function to make its purpose more clear.

No functional change intended.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/kvm/arm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index fe102cd2e518..3d4acd354f94 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -649,7 +649,7 @@ void kvm_arm_resume_guest(struct kvm *kvm)
 	}
 }
 
-static void vcpu_req_sleep(struct kvm_vcpu *vcpu)
+static void kvm_vcpu_sleep(struct kvm_vcpu *vcpu)
 {
 	struct rcuwait *wait = kvm_arch_vcpu_get_wait(vcpu);
 
@@ -679,7 +679,7 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
 {
 	if (kvm_request_pending(vcpu)) {
 		if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
-			vcpu_req_sleep(vcpu);
+			kvm_vcpu_sleep(vcpu);
 
 		if (kvm_check_request(KVM_REQ_VCPU_RESET, vcpu))
 			kvm_reset_vcpu(vcpu);
-- 
2.33.0.685.g46640cef36-goog

[PATCH v2 05/11] KVM: arm64: Defer WFI emulation as a requested event

[Oliver Upton]

The emulation of WFI-like instructions (WFI, PSCI CPU_SUSPEND) is done
by calling kvm_vcpu_block() directly from the respective exit handlers.
A subsequent change to KVM will allow userspace to request a vCPU be
suspended on the next KVM_RUN, necessitating a deferral mechanism for
WFI emulation.

Refactor such that there is a single WFI implementation which may be
requested with KVM_REQ_SUSPEND. Request WFI emulation from the
aforementioned handlers by making this request.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/include/asm/kvm_host.h | 1 +
 arch/arm64/kvm/arm.c              | 9 +++++++++
 arch/arm64/kvm/handle_exit.c      | 3 +--
 arch/arm64/kvm/psci.c             | 4 +---
 4 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index f8be56d5342b..1beda1189a15 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -47,6 +47,7 @@
 #define KVM_REQ_RECORD_STEAL	KVM_ARCH_REQ(3)
 #define KVM_REQ_RELOAD_GICv4	KVM_ARCH_REQ(4)
 #define KVM_REQ_RELOAD_PMU	KVM_ARCH_REQ(5)
+#define KVM_REQ_SUSPEND		KVM_ARCH_REQ(6)
 
 #define KVM_DIRTY_LOG_MANUAL_CAPS   (KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE | \
 				     KVM_DIRTY_LOG_INITIALLY_SET)
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 3d4acd354f94..f1a375648e25 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -670,6 +670,12 @@ static void kvm_vcpu_sleep(struct kvm_vcpu *vcpu)
 	smp_rmb();
 }
 
+static void kvm_vcpu_suspend(struct kvm_vcpu *vcpu)
+{
+	kvm_vcpu_block(vcpu);
+	kvm_clear_request(KVM_REQ_UNHALT, vcpu);
+}
+
 static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
 {
 	return vcpu->arch.target >= 0;
@@ -681,6 +687,9 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
 		if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
 			kvm_vcpu_sleep(vcpu);
 
+		if (kvm_check_request(KVM_REQ_SUSPEND, vcpu))
+			kvm_vcpu_suspend(vcpu);
+
 		if (kvm_check_request(KVM_REQ_VCPU_RESET, vcpu))
 			kvm_reset_vcpu(vcpu);
 
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index 275a27368a04..5e5ef9ff4fba 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -95,8 +95,7 @@ static int kvm_handle_wfx(struct kvm_vcpu *vcpu)
 	} else {
 		trace_kvm_wfx_arm64(*vcpu_pc(vcpu), false);
 		vcpu->stat.wfi_exit_stat++;
-		kvm_vcpu_block(vcpu);
-		kvm_clear_request(KVM_REQ_UNHALT, vcpu);
+		kvm_make_request(KVM_REQ_SUSPEND, vcpu);
 	}
 
 	kvm_incr_pc(vcpu);
diff --git a/arch/arm64/kvm/psci.c b/arch/arm64/kvm/psci.c
index bb59b692998b..d453666ddb83 100644
--- a/arch/arm64/kvm/psci.c
+++ b/arch/arm64/kvm/psci.c
@@ -46,9 +46,7 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)
 	 * specification (ARM DEN 0022A). This means all suspend states
 	 * for KVM will preserve the register state.
 	 */
-	kvm_vcpu_block(vcpu);
-	kvm_clear_request(KVM_REQ_UNHALT, vcpu);
-
+	kvm_make_request(KVM_REQ_SUSPEND, vcpu);
 	return PSCI_RET_SUCCESS;
 }
 
-- 
2.33.0.685.g46640cef36-goog

[PATCH v2 06/11] KVM: arm64: Add support for SYSTEM_SUSPEND PSCI call

[Oliver Upton]

ARM DEN0022D 5.19 "SYSTEM_SUSPEND" describes a PSCI call that may be
used to request a system be suspended. This is optional for PSCI v1.0
and to date KVM has elected to not implement the call. However, a
VMM/operator may wish to provide their guests with the ability to
suspend/resume, necessitating this PSCI call.

Implement support for SYSTEM_SUSPEND according to the prescribed
behavior in the specification. Add a new system event exit type,
KVM_SYSTEM_EVENT_SUSPEND, to notify userspace when a VM has requested a
system suspend. Make KVM_MP_STATE_HALTED a valid state on arm64.
Userspace can set this to request an in-kernel emulation of the suspend.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 Documentation/virt/kvm/api.rst    |  6 ++++
 arch/arm64/include/asm/kvm_host.h |  3 ++
 arch/arm64/kvm/arm.c              |  8 +++++
 arch/arm64/kvm/psci.c             | 60 +++++++++++++++++++++++++++++++
 include/uapi/linux/kvm.h          |  2 ++
 5 files changed, 79 insertions(+)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index a6729c8cf063..361a57061b8f 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -5656,6 +5656,7 @@ should put the acknowledged interrupt vector into the 'epr' field.
   #define KVM_SYSTEM_EVENT_SHUTDOWN       1
   #define KVM_SYSTEM_EVENT_RESET          2
   #define KVM_SYSTEM_EVENT_CRASH          3
+  #define KVM_SYSTEM_EVENT_SUSPEND        4
 			__u32 type;
 			__u64 flags;
 		} system_event;
@@ -5680,6 +5681,11 @@ Valid values for 'type' are:
    has requested a crash condition maintenance. Userspace can choose
    to ignore the request, or to gather VM memory core dump and/or
    reset/shutdown of the VM.
+ - KVM_SYSTEM_EVENT_SUSPEND -- the guest has requested that the VM
+   suspends. Userspace is not obliged to honor this, and may call KVM_RUN
+   again. Doing so will cause the guest to resume at its requested entry
+   point. For ARM64, userspace can request in-kernel suspend emulation
+   by setting the vCPU's MP state to KVM_MP_STATE_HALTED.
 
 ::
 
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 1beda1189a15..441eb6fa7adc 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -137,6 +137,9 @@ struct kvm_arch {
 
 	/* Memory Tagging Extension enabled for the guest */
 	bool mte_enabled;
+
+	/* PSCI SYSTEM_SUSPEND call enabled for the guest */
+	bool suspend_enabled;
 };
 
 struct kvm_vcpu_fault_info {
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index f1a375648e25..d875d3bcf3c5 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -101,6 +101,10 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
 		}
 		mutex_unlock(&kvm->lock);
 		break;
+	case KVM_CAP_ARM_SYSTEM_SUSPEND:
+		r = 0;
+		kvm->arch.suspend_enabled = true;
+		break;
 	default:
 		r = -EINVAL;
 		break;
@@ -215,6 +219,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_SET_GUEST_DEBUG:
 	case KVM_CAP_VCPU_ATTRIBUTES:
 	case KVM_CAP_PTP_KVM:
+	case KVM_CAP_ARM_SYSTEM_SUSPEND:
 		r = 1;
 		break;
 	case KVM_CAP_SET_GUEST_DEBUG2:
@@ -470,6 +475,9 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
 	int ret = 0;
 
 	switch (mp_state->mp_state) {
+	case KVM_MP_STATE_HALTED:
+		kvm_make_request(KVM_REQ_SUSPEND, vcpu);
+		fallthrough;
 	case KVM_MP_STATE_RUNNABLE:
 		vcpu->arch.power_off = false;
 		break;
diff --git a/arch/arm64/kvm/psci.c b/arch/arm64/kvm/psci.c
index d453666ddb83..cf869f1f8615 100644
--- a/arch/arm64/kvm/psci.c
+++ b/arch/arm64/kvm/psci.c
@@ -203,6 +203,46 @@ static void kvm_psci_system_reset(struct kvm_vcpu *vcpu)
 	kvm_prepare_system_event(vcpu, KVM_SYSTEM_EVENT_RESET);
 }
 
+static int kvm_psci_system_suspend(struct kvm_vcpu *vcpu)
+{
+	unsigned long entry_addr, context_id;
+	struct kvm *kvm = vcpu->kvm;
+	unsigned long psci_ret = 0;
+	struct kvm_vcpu *tmp;
+	int ret = 0;
+	int i;
+
+	/*
+	 * The SYSTEM_SUSPEND PSCI call requires that all vCPUs (except the
+	 * calling vCPU) be in an OFF state, as determined by the
+	 * implementation.
+	 *
+	 * See ARM DEN0022D, 5.19 "SYSTEM_SUSPEND" for more details.
+	 */
+	mutex_lock(&kvm->lock);
+	kvm_for_each_vcpu(i, tmp, kvm) {
+		if (tmp != vcpu && !tmp->arch.power_off) {
+			psci_ret = PSCI_RET_DENIED;
+			ret = 1;
+			goto out;
+		}
+	}
+
+	entry_addr = smccc_get_arg1(vcpu);
+	context_id = smccc_get_arg2(vcpu);
+
+	kvm_psci_vcpu_request_reset(vcpu, entry_addr, context_id,
+				    kvm_vcpu_is_be(vcpu));
+
+	memset(&vcpu->run->system_event, 0, sizeof(vcpu->run->system_event));
+	vcpu->run->system_event.type = KVM_SYSTEM_EVENT_SUSPEND;
+	vcpu->run->exit_reason = KVM_EXIT_SYSTEM_EVENT;
+out:
+	mutex_unlock(&kvm->lock);
+	smccc_set_retval(vcpu, psci_ret, 0, 0, 0);
+	return ret;
+}
+
 static void kvm_psci_narrow_to_32bit(struct kvm_vcpu *vcpu)
 {
 	int i;
@@ -223,6 +263,14 @@ static unsigned long kvm_psci_check_allowed_function(struct kvm_vcpu *vcpu, u32
 	if ((fn & PSCI_0_2_64BIT) && vcpu_mode_is_32bit(vcpu))
 		return PSCI_RET_NOT_SUPPORTED;
 
+	switch (fn) {
+	case PSCI_1_0_FN_SYSTEM_SUSPEND:
+	case PSCI_1_0_FN64_SYSTEM_SUSPEND:
+		if (!vcpu->kvm->arch.suspend_enabled)
+			return PSCI_RET_NOT_SUPPORTED;
+		break;
+	}
+
 	return 0;
 }
 
@@ -316,6 +364,10 @@ static int kvm_psci_1_0_call(struct kvm_vcpu *vcpu)
 	unsigned long val;
 	int ret = 1;
 
+	val = kvm_psci_check_allowed_function(vcpu, psci_fn);
+	if (val)
+		goto out;
+
 	switch(psci_fn) {
 	case PSCI_0_2_FN_PSCI_VERSION:
 		val = KVM_ARM_PSCI_1_0;
@@ -339,6 +391,8 @@ static int kvm_psci_1_0_call(struct kvm_vcpu *vcpu)
 		case PSCI_0_2_FN_SYSTEM_OFF:
 		case PSCI_0_2_FN_SYSTEM_RESET:
 		case PSCI_1_0_FN_PSCI_FEATURES:
+		case PSCI_1_0_FN_SYSTEM_SUSPEND:
+		case PSCI_1_0_FN64_SYSTEM_SUSPEND:
 		case ARM_SMCCC_VERSION_FUNC_ID:
 			val = 0;
 			break;
@@ -347,10 +401,16 @@ static int kvm_psci_1_0_call(struct kvm_vcpu *vcpu)
 			break;
 		}
 		break;
+	case PSCI_1_0_FN_SYSTEM_SUSPEND:
+		kvm_psci_narrow_to_32bit(vcpu);
+		fallthrough;
+	case PSCI_1_0_FN64_SYSTEM_SUSPEND:
+		return kvm_psci_system_suspend(vcpu);
 	default:
 		return kvm_psci_0_2_call(vcpu);
 	}
 
+out:
 	smccc_set_retval(vcpu, val, 0, 0, 0);
 	return ret;
 }
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index a067410ebea5..052b0e717b08 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -433,6 +433,7 @@ struct kvm_run {
 #define KVM_SYSTEM_EVENT_SHUTDOWN       1
 #define KVM_SYSTEM_EVENT_RESET          2
 #define KVM_SYSTEM_EVENT_CRASH          3
+#define KVM_SYSTEM_EVENT_SUSPEND        4
 			__u32 type;
 			__u64 flags;
 		} system_event;
@@ -1112,6 +1113,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_BINARY_STATS_FD 203
 #define KVM_CAP_EXIT_ON_EMULATION_FAILURE 204
 #define KVM_CAP_ARM_MTE 205
+#define KVM_CAP_ARM_SYSTEM_SUSPEND 206
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
-- 
2.33.0.685.g46640cef36-goog

[PATCH v2 07/11] selftests: KVM: Rename psci_cpu_on_test to psci_test

[Oliver Upton]

There are other interactions with PSCI worth testing; rename the PSCI
test to make it more generic.

No functional change intended.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 tools/testing/selftests/kvm/.gitignore                          | 2 +-
 tools/testing/selftests/kvm/Makefile                            | 2 +-
 .../selftests/kvm/aarch64/{psci_cpu_on_test.c => psci_test.c}   | 0
 3 files changed, 2 insertions(+), 2 deletions(-)
 rename tools/testing/selftests/kvm/aarch64/{psci_cpu_on_test.c => psci_test.c} (100%)

diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
index 98053d3afbda..a11b89be744b 100644
--- a/tools/testing/selftests/kvm/.gitignore
+++ b/tools/testing/selftests/kvm/.gitignore
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
 /aarch64/debug-exceptions
 /aarch64/get-reg-list
-/aarch64/psci_cpu_on_test
+/aarch64/psci_test
 /aarch64/vgic_init
 /s390x/memop
 /s390x/resets
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 5d05801ab816..6907ee8f3239 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -86,7 +86,7 @@ TEST_GEN_PROGS_x86_64 += kvm_binary_stats_test
 
 TEST_GEN_PROGS_aarch64 += aarch64/debug-exceptions
 TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list
-TEST_GEN_PROGS_aarch64 += aarch64/psci_cpu_on_test
+TEST_GEN_PROGS_aarch64 += aarch64/psci_test
 TEST_GEN_PROGS_aarch64 += aarch64/vgic_init
 TEST_GEN_PROGS_aarch64 += demand_paging_test
 TEST_GEN_PROGS_aarch64 += dirty_log_test
diff --git a/tools/testing/selftests/kvm/aarch64/psci_cpu_on_test.c b/tools/testing/selftests/kvm/aarch64/psci_test.c
similarity index 100%
rename from tools/testing/selftests/kvm/aarch64/psci_cpu_on_test.c
rename to tools/testing/selftests/kvm/aarch64/psci_test.c
-- 
2.33.0.685.g46640cef36-goog

[PATCH v2 08/11] selftests: KVM: Create helper for making SMCCC calls

[Oliver Upton]

The PSCI and PV stolen time tests both need to make SMCCC calls within
the guest. Create a helper for making SMCCC calls and rework the
existing tests to use the library function.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 .../testing/selftests/kvm/aarch64/psci_test.c | 25 ++++++-------------
 .../selftests/kvm/include/aarch64/processor.h | 22 ++++++++++++++++
 .../selftests/kvm/lib/aarch64/processor.c     | 25 +++++++++++++++++++
 tools/testing/selftests/kvm/steal_time.c      | 13 +++-------
 4 files changed, 58 insertions(+), 27 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/psci_test.c b/tools/testing/selftests/kvm/aarch64/psci_test.c
index 018c269990e1..cebea7356e5a 100644
--- a/tools/testing/selftests/kvm/aarch64/psci_test.c
+++ b/tools/testing/selftests/kvm/aarch64/psci_test.c
@@ -26,32 +26,23 @@
 static uint64_t psci_cpu_on(uint64_t target_cpu, uint64_t entry_addr,
 			    uint64_t context_id)
 {
-	register uint64_t x0 asm("x0") = PSCI_0_2_FN64_CPU_ON;
-	register uint64_t x1 asm("x1") = target_cpu;
-	register uint64_t x2 asm("x2") = entry_addr;
-	register uint64_t x3 asm("x3") = context_id;
+	struct arm_smccc_res res;
 
-	asm("hvc #0"
-	    : "=r"(x0)
-	    : "r"(x0), "r"(x1), "r"(x2), "r"(x3)
-	    : "memory");
+	smccc_hvc(PSCI_0_2_FN64_CPU_ON, target_cpu, entry_addr, context_id,
+		  0, 0, 0, 0, &res);
 
-	return x0;
+	return res.a0;
 }
 
 static uint64_t psci_affinity_info(uint64_t target_affinity,
 				   uint64_t lowest_affinity_level)
 {
-	register uint64_t x0 asm("x0") = PSCI_0_2_FN64_AFFINITY_INFO;
-	register uint64_t x1 asm("x1") = target_affinity;
-	register uint64_t x2 asm("x2") = lowest_affinity_level;
+	struct arm_smccc_res res;
 
-	asm("hvc #0"
-	    : "=r"(x0)
-	    : "r"(x0), "r"(x1), "r"(x2)
-	    : "memory");
+	smccc_hvc(PSCI_0_2_FN64_AFFINITY_INFO, target_affinity, lowest_affinity_level,
+		  0, 0, 0, 0, 0, &res);
 
-	return x0;
+	return res.a0;
 }
 
 static void guest_main(uint64_t target_cpu)
diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
index c0273aefa63d..e6b7cb65d158 100644
--- a/tools/testing/selftests/kvm/include/aarch64/processor.h
+++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
@@ -132,4 +132,26 @@ void vm_install_sync_handler(struct kvm_vm *vm,
 
 #define isb()	asm volatile("isb" : : : "memory")
 
+/**
+ * struct arm_smccc_res - Result from SMC/HVC call
+ * @a0-a3 result values from registers 0 to 3
+ */
+struct arm_smccc_res {
+	unsigned long a0;
+	unsigned long a1;
+	unsigned long a2;
+	unsigned long a3;
+};
+
+/**
+ * smccc_hvc - Invoke a SMCCC function using the hvc conduit
+ * @function_id: the SMCCC function to be called
+ * @arg0-arg6: SMCCC function arguments, corresponding to registers x1-x7
+ * @res: pointer to write the return values from registers x0-x3
+ *
+ */
+void smccc_hvc(uint32_t function_id, uint64_t arg0, uint64_t arg1,
+	       uint64_t arg2, uint64_t arg3, uint64_t arg4, uint64_t arg5,
+	       uint64_t arg6, struct arm_smccc_res *res);
+
 #endif /* SELFTEST_KVM_PROCESSOR_H */
diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
index 632b74d6b3ca..f77430e2d688 100644
--- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
+++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
@@ -426,3 +426,28 @@ void vm_install_exception_handler(struct kvm_vm *vm, int vector,
 	assert(vector < VECTOR_NUM);
 	handlers->exception_handlers[vector][0] = handler;
 }
+
+void smccc_hvc(uint32_t function_id, uint64_t arg0, uint64_t arg1,
+	       uint64_t arg2, uint64_t arg3, uint64_t arg4, uint64_t arg5,
+	       uint64_t arg6, struct arm_smccc_res *res)
+{
+	asm volatile("mov   w0, %w[function_id]\n"
+		     "mov   x1, %[arg0]\n"
+		     "mov   x2, %[arg1]\n"
+		     "mov   x3, %[arg2]\n"
+		     "mov   x4, %[arg3]\n"
+		     "mov   x5, %[arg4]\n"
+		     "mov   x6, %[arg5]\n"
+		     "mov   x7, %[arg6]\n"
+		     "hvc   #0\n"
+		     "mov   %[res0], x0\n"
+		     "mov   %[res1], x1\n"
+		     "mov   %[res2], x2\n"
+		     "mov   %[res3], x3\n"
+		     : [res0] "=r"(res->a0), [res1] "=r"(res->a1),
+		       [res2] "=r"(res->a2), [res3] "=r"(res->a3)
+		     : [function_id] "r"(function_id), [arg0] "r"(arg0),
+		       [arg1] "r"(arg1), [arg2] "r"(arg2), [arg3] "r"(arg3),
+		       [arg4] "r"(arg4), [arg5] "r"(arg5), [arg6] "r"(arg6)
+		     : "x0", "x1", "x2", "x3", "x4", "x5", "x6", "x7");
+}
diff --git a/tools/testing/selftests/kvm/steal_time.c b/tools/testing/selftests/kvm/steal_time.c
index ecec30865a74..5d52b82226c5 100644
--- a/tools/testing/selftests/kvm/steal_time.c
+++ b/tools/testing/selftests/kvm/steal_time.c
@@ -120,17 +120,10 @@ struct st_time {
 
 static int64_t smccc(uint32_t func, uint32_t arg)
 {
-	unsigned long ret;
+	struct arm_smccc_res res;
 
-	asm volatile(
-		"mov	x0, %1\n"
-		"mov	x1, %2\n"
-		"hvc	#0\n"
-		"mov	%0, x0\n"
-	: "=r" (ret) : "r" (func), "r" (arg) :
-	  "x0", "x1", "x2", "x3");
-
-	return ret;
+	smccc_hvc(func, arg, 0, 0, 0, 0, 0, 0, &res);
+	return res.a0;
 }
 
 static void check_status(struct st_time *st)
-- 
2.33.0.685.g46640cef36-goog

[PATCH v2 09/11] selftests: KVM: Use KVM_SET_MP_STATE to power off vCPU in psci_test

[Oliver Upton]

Setting a vCPU's MP state to KVM_MP_STATE_STOPPED has the effect of
powering off the vCPU. Rather than using the vCPU init feature flag, use
the KVM_SET_MP_STATE ioctl to power off the target vCPU.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 tools/testing/selftests/kvm/aarch64/psci_test.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/psci_test.c b/tools/testing/selftests/kvm/aarch64/psci_test.c
index cebea7356e5a..8d043e12b137 100644
--- a/tools/testing/selftests/kvm/aarch64/psci_test.c
+++ b/tools/testing/selftests/kvm/aarch64/psci_test.c
@@ -60,6 +60,15 @@ static void guest_main(uint64_t target_cpu)
 	GUEST_DONE();
 }
 
+static void vcpu_power_off(struct kvm_vm *vm, uint32_t vcpuid)
+{
+	struct kvm_mp_state mp_state = {
+		.mp_state = KVM_MP_STATE_STOPPED,
+	};
+
+	vcpu_set_mp_state(vm, vcpuid, &mp_state);
+}
+
 int main(void)
 {
 	uint64_t target_mpidr, obs_pc, obs_x0;
@@ -75,12 +84,12 @@ int main(void)
 	init.features[0] |= (1 << KVM_ARM_VCPU_PSCI_0_2);
 
 	aarch64_vcpu_add_default(vm, VCPU_ID_SOURCE, &init, guest_main);
+	aarch64_vcpu_add_default(vm, VCPU_ID_TARGET, &init, guest_main);
 
 	/*
 	 * make sure the target is already off when executing the test.
 	 */
-	init.features[0] |= (1 << KVM_ARM_VCPU_POWER_OFF);
-	aarch64_vcpu_add_default(vm, VCPU_ID_TARGET, &init, guest_main);
+	vcpu_power_off(vm, VCPU_ID_TARGET);
 
 	get_reg(vm, VCPU_ID_TARGET, ARM64_SYS_REG(MPIDR_EL1), &target_mpidr);
 	vcpu_args_set(vm, VCPU_ID_SOURCE, 1, target_mpidr & MPIDR_HWID_BITMASK);
-- 
2.33.0.685.g46640cef36-goog

[PATCH v2 10/11] selftests: KVM: Refactor psci_test to make it amenable to new tests

[Oliver Upton]

Split up the current test into several helpers that will be useful to
subsequent test cases added to the PSCI test suite.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 .../testing/selftests/kvm/aarch64/psci_test.c | 68 ++++++++++++-------
 1 file changed, 45 insertions(+), 23 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/psci_test.c b/tools/testing/selftests/kvm/aarch64/psci_test.c
index 8d043e12b137..90312be335da 100644
--- a/tools/testing/selftests/kvm/aarch64/psci_test.c
+++ b/tools/testing/selftests/kvm/aarch64/psci_test.c
@@ -45,7 +45,7 @@ static uint64_t psci_affinity_info(uint64_t target_affinity,
 	return res.a0;
 }
 
-static void guest_main(uint64_t target_cpu)
+static void guest_test_cpu_on(uint64_t target_cpu)
 {
 	GUEST_ASSERT(!psci_cpu_on(target_cpu, CPU_ON_ENTRY_ADDR, CPU_ON_CONTEXT_ID));
 	uint64_t target_state;
@@ -69,12 +69,10 @@ static void vcpu_power_off(struct kvm_vm *vm, uint32_t vcpuid)
 	vcpu_set_mp_state(vm, vcpuid, &mp_state);
 }
 
-int main(void)
+static struct kvm_vm *setup_vm(void *guest_code)
 {
-	uint64_t target_mpidr, obs_pc, obs_x0;
 	struct kvm_vcpu_init init;
 	struct kvm_vm *vm;
-	struct ucall uc;
 
 	vm = vm_create(VM_MODE_DEFAULT, DEFAULT_GUEST_PHY_PAGES, O_RDWR);
 	kvm_vm_elf_load(vm, program_invocation_name);
@@ -83,31 +81,28 @@ int main(void)
 	vm_ioctl(vm, KVM_ARM_PREFERRED_TARGET, &init);
 	init.features[0] |= (1 << KVM_ARM_VCPU_PSCI_0_2);
 
-	aarch64_vcpu_add_default(vm, VCPU_ID_SOURCE, &init, guest_main);
-	aarch64_vcpu_add_default(vm, VCPU_ID_TARGET, &init, guest_main);
+	aarch64_vcpu_add_default(vm, VCPU_ID_SOURCE, &init, guest_code);
+	aarch64_vcpu_add_default(vm, VCPU_ID_TARGET, &init, guest_code);
 
-	/*
-	 * make sure the target is already off when executing the test.
-	 */
-	vcpu_power_off(vm, VCPU_ID_TARGET);
+	return vm;
+}
 
-	get_reg(vm, VCPU_ID_TARGET, ARM64_SYS_REG(MPIDR_EL1), &target_mpidr);
-	vcpu_args_set(vm, VCPU_ID_SOURCE, 1, target_mpidr & MPIDR_HWID_BITMASK);
-	vcpu_run(vm, VCPU_ID_SOURCE);
+static void enter_guest(struct kvm_vm *vm, uint32_t vcpuid)
+{
+	struct ucall uc;
 
-	switch (get_ucall(vm, VCPU_ID_SOURCE, &uc)) {
-	case UCALL_DONE:
-		break;
-	case UCALL_ABORT:
+	vcpu_run(vm, vcpuid);
+	if (get_ucall(vm, vcpuid, &uc) == UCALL_ABORT)
 		TEST_FAIL("%s at %s:%ld", (const char *)uc.args[0], __FILE__,
 			  uc.args[1]);
-		break;
-	default:
-		TEST_FAIL("Unhandled ucall: %lu", uc.cmd);
-	}
+}
 
-	get_reg(vm, VCPU_ID_TARGET, ARM64_CORE_REG(regs.pc), &obs_pc);
-	get_reg(vm, VCPU_ID_TARGET, ARM64_CORE_REG(regs.regs[0]), &obs_x0);
+static void assert_vcpu_reset(struct kvm_vm *vm, uint32_t vcpuid)
+{
+	uint64_t obs_pc, obs_x0;
+
+	get_reg(vm, vcpuid, ARM64_CORE_REG(regs.pc), &obs_pc);
+	get_reg(vm, vcpuid, ARM64_CORE_REG(regs.regs[0]), &obs_x0);
 
 	TEST_ASSERT(obs_pc == CPU_ON_ENTRY_ADDR,
 		    "unexpected target cpu pc: %lx (expected: %lx)",
@@ -115,7 +110,34 @@ int main(void)
 	TEST_ASSERT(obs_x0 == CPU_ON_CONTEXT_ID,
 		    "unexpected target context id: %lx (expected: %lx)",
 		    obs_x0, CPU_ON_CONTEXT_ID);
+}
 
+static void host_test_cpu_on(void)
+{
+	uint64_t target_mpidr;
+	struct kvm_vm *vm;
+	struct ucall uc;
+
+	vm = setup_vm(guest_test_cpu_on);
+
+	/*
+	 * make sure the target is already off when executing the test.
+	 */
+	vcpu_power_off(vm, VCPU_ID_TARGET);
+
+	get_reg(vm, VCPU_ID_TARGET, ARM64_SYS_REG(MPIDR_EL1), &target_mpidr);
+	vcpu_args_set(vm, VCPU_ID_SOURCE, 1, target_mpidr & MPIDR_HWID_BITMASK);
+	enter_guest(vm, VCPU_ID_SOURCE);
+
+	if (get_ucall(vm, VCPU_ID_SOURCE, &uc) != UCALL_DONE)
+		TEST_FAIL("Unhandled ucall: %lu", uc.cmd);
+
+	assert_vcpu_reset(vm, VCPU_ID_TARGET);
 	kvm_vm_free(vm);
+}
+
+int main(void)
+{
+	host_test_cpu_on();
 	return 0;
 }
-- 
2.33.0.685.g46640cef36-goog

[PATCH v2 11/11] selftests: KVM: Test SYSTEM_SUSPEND PSCI call

[Oliver Upton]

Assert that the vCPU exits to userspace with KVM_SYSTEM_EVENT_SUSPEND if
it correctly executes the SYSTEM_SUSPEND PSCI call. Additionally, assert
that the guest PSCI call fails if preconditions are not met (more than 1
running vCPU).

Signed-off-by: Oliver Upton <oupton@google.com>
---
 .../testing/selftests/kvm/aarch64/psci_test.c | 75 +++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/tools/testing/selftests/kvm/aarch64/psci_test.c b/tools/testing/selftests/kvm/aarch64/psci_test.c
index 90312be335da..5b881ca4d102 100644
--- a/tools/testing/selftests/kvm/aarch64/psci_test.c
+++ b/tools/testing/selftests/kvm/aarch64/psci_test.c
@@ -45,6 +45,16 @@ static uint64_t psci_affinity_info(uint64_t target_affinity,
 	return res.a0;
 }
 
+static uint64_t psci_system_suspend(uint64_t entry_addr, uint64_t context_id)
+{
+	struct arm_smccc_res res;
+
+	smccc_hvc(PSCI_1_0_FN64_SYSTEM_SUSPEND, entry_addr, context_id,
+		  0, 0, 0, 0, 0, &res);
+
+	return res.a0;
+}
+
 static void guest_test_cpu_on(uint64_t target_cpu)
 {
 	GUEST_ASSERT(!psci_cpu_on(target_cpu, CPU_ON_ENTRY_ADDR, CPU_ON_CONTEXT_ID));
@@ -69,6 +79,13 @@ static void vcpu_power_off(struct kvm_vm *vm, uint32_t vcpuid)
 	vcpu_set_mp_state(vm, vcpuid, &mp_state);
 }
 
+static void guest_test_system_suspend(void)
+{
+	uint64_t r = psci_system_suspend(CPU_ON_ENTRY_ADDR, CPU_ON_CONTEXT_ID);
+
+	GUEST_SYNC(r);
+}
+
 static struct kvm_vm *setup_vm(void *guest_code)
 {
 	struct kvm_vcpu_init init;
@@ -136,8 +153,66 @@ static void host_test_cpu_on(void)
 	kvm_vm_free(vm);
 }
 
+static void enable_system_suspend(struct kvm_vm *vm)
+{
+	struct kvm_enable_cap cap = {
+		.cap = KVM_CAP_ARM_SYSTEM_SUSPEND,
+	};
+
+	vm_enable_cap(vm, &cap);
+}
+
+static void host_test_system_suspend(void)
+{
+	struct kvm_run *run;
+	struct kvm_vm *vm;
+
+	vm = setup_vm(guest_test_system_suspend);
+	enable_system_suspend(vm);
+
+	vcpu_power_off(vm, VCPU_ID_TARGET);
+	run = vcpu_state(vm, VCPU_ID_SOURCE);
+
+	enter_guest(vm, VCPU_ID_SOURCE);
+
+	TEST_ASSERT(run->exit_reason == KVM_EXIT_SYSTEM_EVENT,
+		    "Unhandled exit reason: %u (%s)",
+		    run->exit_reason, exit_reason_str(run->exit_reason));
+	TEST_ASSERT(run->system_event.type == KVM_SYSTEM_EVENT_SUSPEND,
+		    "Unhandled system event: %u (expected: %u)",
+		    run->system_event.type, KVM_SYSTEM_EVENT_SUSPEND);
+
+	assert_vcpu_reset(vm, VCPU_ID_SOURCE);
+	kvm_vm_free(vm);
+}
+
+static void host_test_system_suspend_fails(void)
+{
+	struct kvm_vm *vm;
+	struct ucall uc;
+
+	vm = setup_vm(guest_test_system_suspend);
+	enable_system_suspend(vm);
+
+	enter_guest(vm, VCPU_ID_SOURCE);
+	TEST_ASSERT(get_ucall(vm, VCPU_ID_SOURCE, &uc) == UCALL_SYNC,
+		    "Unhandled ucall: %lu", uc.cmd);
+	TEST_ASSERT(uc.args[1] == PSCI_RET_DENIED,
+		    "Unrecognized PSCI return code: %lu (expected: %u)",
+		    uc.args[1], PSCI_RET_DENIED);
+
+	kvm_vm_free(vm);
+}
+
 int main(void)
 {
+	if (!kvm_check_cap(KVM_CAP_ARM_SYSTEM_SUSPEND)) {
+		print_skip("KVM_CAP_ARM_SYSTEM_SUSPEND not supported");
+		exit(KSFT_SKIP);
+	}
+
 	host_test_cpu_on();
+	host_test_system_suspend();
+	host_test_system_suspend_fails();
 	return 0;
 }
-- 
2.33.0.685.g46640cef36-goog

Re: [PATCH v2 00/11] KVM: arm64: Implement PSCI SYSTEM_SUSPEND support

[Oliver Upton]

On Thu, Sep 23, 2021 at 07:15:59PM +0000, Oliver Upton wrote:
> Certain VMMs/operators may wish to give their guests the ability to
> initiate a system suspend that could result in the VM being saved to
> persistent storage to be resumed at a later time. The PSCI v1.0
> specification describes an SMC, SYSTEM_SUSPEND, that allows a kernel to
> request a system suspend. This call is optional for v1.0, and KVM
> elected to not support the call in its v1.0 implementation.
> 
> This series adds support for the SYSTEM_SUSPEND PSCI call to KVM/arm64.
> Since this is a system-scoped event, KVM cannot quiesce the VM on its
> own. We add a new system exit type in this series to clue in userspace
> that a suspend was requested. Per the KVM_EXIT_SYSTEM_EVENT ABI, a VMM
> that doesn't care about this event can simply resume the guest without
> issue (we set up the calling vCPU to come out of reset correctly on next
> KVM_RUN). If a VMM would like to have KVM emulate the suspend, it can do
> so by setting the vCPU's MP state to KVM_MP_STATE_HALTED. Support for
> this state has been added in this series.
> 
> Patch 1 is an unrelated cleanup, dropping an unused parameter
> 
> Patch 2 simplifies how KVM filters SMC64 functions for AArch32 guests.
> 
> Patch 3 wraps up the vCPU reset logic used by the PSCI CPU_ON
> implementation in KVM for subsequent use, as we must queue up a reset
> for the vCPU that requested a system suspend.
> 
> Patch 4 is another unrelated cleanup, fixing the naming for the
> KVM_REQ_SLEEP handler to avoid confusion and remain consistent with the
> handler introduced in this series.
> 
> Patch 5 changes how WFI-like events are handled in KVM (WFI instruction,
> PSCI CPU_SUSPEND). Instead of directly blocking the vCPU in the
> respective handlers, set a request bit and block before resuming the
> guest. WFI and PSCI CPU_SUSPEND do not require deferral of
> kvm_vcpu_block(), but SYSTEM_SUSPEND does. Rather than adding a deferral
> mechanism just for SYSTEM_SUSPEND, it is a bit cleaner to have all
> blocking events just request the event.
> 
> Patch 6 actually adds PSCI SYSTEM_SUSPEND support to KVM, and adds the
> necessary UAPI to pair with the call.
> 
> Patch 7 renames the PSCI selftest to something more generic, as we will
> test more than just CPU_ON.
> 
> Patch 8 creates a common helper for making SMC64 calls in KVM selftests,
> rather than having tests open-code their own approach.
> 
> Patch 9 makes the PSCI test use KVM_SET_MP_STATE for powering off a vCPU
> rather than the vCPU init flag. This change is necessary to separate
> generic VM setup from the setup for a particular PSCI test.
> 
> Patch 10 reworks psci_test into a bunch of helpers, making it easier to
> build additional test cases with the common parts.
> 
> Finally, patch 11 adds 2 test cases for the SYSTEM_SUSPEND PSCI call.
> Verify that the call succeeds if all other vCPUs have been powered off
> and that it fails if more than the calling vCPU is powered on.
> 
> This series applies cleanly to v5.15-rc2. Testing was performed on an
> Ampere Mt. Jade system.

Gah, forgot to summarize updates:

v1 -> v2:
 - Rebase to 5.15-rc2
 - Allow userspace to request in-kernel suspend emulation (Marc)
 - Add another test case for SYSTEM_SUSPEND, cleaning up the PSCI
   selftest
 - Create a common SMCCC function for KVM selftests

v1: http://lore.kernel.org/r/20210819223640.3564975-1-oupton@google.com

> Oliver Upton (11):
>   KVM: arm64: Drop unused vcpu param to kvm_psci_valid_affinity()
>   KVM: arm64: Clean up SMC64 PSCI filtering for AArch32 guests
>   KVM: arm64: Encapsulate reset request logic in a helper function
>   KVM: arm64: Rename the KVM_REQ_SLEEP handler
>   KVM: arm64: Defer WFI emulation as a requested event
>   KVM: arm64: Add support for SYSTEM_SUSPEND PSCI call
>   selftests: KVM: Rename psci_cpu_on_test to psci_test
>   selftests: KVM: Create helper for making SMCCC calls
>   selftests: KVM: Use KVM_SET_MP_STATE to power off vCPU in psci_test
>   selftests: KVM: Refactor psci_test to make it amenable to new tests
>   selftests: KVM: Test SYSTEM_SUSPEND PSCI call
> 
>  Documentation/virt/kvm/api.rst                |   6 +
>  arch/arm64/include/asm/kvm_host.h             |   4 +
>  arch/arm64/kvm/arm.c                          |  21 +-
>  arch/arm64/kvm/handle_exit.c                  |   3 +-
>  arch/arm64/kvm/psci.c                         | 138 ++++++++---
>  include/uapi/linux/kvm.h                      |   2 +
>  tools/testing/selftests/kvm/.gitignore        |   2 +-
>  tools/testing/selftests/kvm/Makefile          |   2 +-
>  .../selftests/kvm/aarch64/psci_cpu_on_test.c  | 121 ----------
>  .../testing/selftests/kvm/aarch64/psci_test.c | 218 ++++++++++++++++++
>  .../selftests/kvm/include/aarch64/processor.h |  22 ++
>  .../selftests/kvm/lib/aarch64/processor.c     |  25 ++
>  tools/testing/selftests/kvm/steal_time.c      |  13 +-
>  13 files changed, 403 insertions(+), 174 deletions(-)
>  delete mode 100644 tools/testing/selftests/kvm/aarch64/psci_cpu_on_test.c
>  create mode 100644 tools/testing/selftests/kvm/aarch64/psci_test.c
> 
> -- 
> 2.33.0.685.g46640cef36-goog
> 

Re: [PATCH v2 05/11] KVM: arm64: Defer WFI emulation as a requested event

[Marc Zyngier]

On Thu, 23 Sep 2021 20:16:04 +0100,
Oliver Upton <oupton@google.com> wrote:
> 
> The emulation of WFI-like instructions (WFI, PSCI CPU_SUSPEND) is done
> by calling kvm_vcpu_block() directly from the respective exit handlers.
> A subsequent change to KVM will allow userspace to request a vCPU be
> suspended on the next KVM_RUN, necessitating a deferral mechanism for
> WFI emulation.
> 
> Refactor such that there is a single WFI implementation which may be
> requested with KVM_REQ_SUSPEND. Request WFI emulation from the
> aforementioned handlers by making this request.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/include/asm/kvm_host.h | 1 +
>  arch/arm64/kvm/arm.c              | 9 +++++++++
>  arch/arm64/kvm/handle_exit.c      | 3 +--
>  arch/arm64/kvm/psci.c             | 4 +---
>  4 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index f8be56d5342b..1beda1189a15 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -47,6 +47,7 @@
>  #define KVM_REQ_RECORD_STEAL	KVM_ARCH_REQ(3)
>  #define KVM_REQ_RELOAD_GICv4	KVM_ARCH_REQ(4)
>  #define KVM_REQ_RELOAD_PMU	KVM_ARCH_REQ(5)
> +#define KVM_REQ_SUSPEND		KVM_ARCH_REQ(6)
>  
>  #define KVM_DIRTY_LOG_MANUAL_CAPS   (KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE | \
>  				     KVM_DIRTY_LOG_INITIALLY_SET)
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 3d4acd354f94..f1a375648e25 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -670,6 +670,12 @@ static void kvm_vcpu_sleep(struct kvm_vcpu *vcpu)
>  	smp_rmb();
>  }
>  
> +static void kvm_vcpu_suspend(struct kvm_vcpu *vcpu)

Bike-shed alert: I would really prefer this to have 'wfi' in the
function name, because we implement suspend in terms of WFI rather
than the other way around.

> +{
> +	kvm_vcpu_block(vcpu);
> +	kvm_clear_request(KVM_REQ_UNHALT, vcpu);
> +}
> +
>  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
>  {
>  	return vcpu->arch.target >= 0;
> @@ -681,6 +687,9 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
>  		if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
>  			kvm_vcpu_sleep(vcpu);
>  
> +		if (kvm_check_request(KVM_REQ_SUSPEND, vcpu))
> +			kvm_vcpu_suspend(vcpu);
> +
>  		if (kvm_check_request(KVM_REQ_VCPU_RESET, vcpu))
>  			kvm_reset_vcpu(vcpu);
>  
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 275a27368a04..5e5ef9ff4fba 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -95,8 +95,7 @@ static int kvm_handle_wfx(struct kvm_vcpu *vcpu)
>  	} else {
>  		trace_kvm_wfx_arm64(*vcpu_pc(vcpu), false);
>  		vcpu->stat.wfi_exit_stat++;
> -		kvm_vcpu_block(vcpu);
> -		kvm_clear_request(KVM_REQ_UNHALT, vcpu);
> +		kvm_make_request(KVM_REQ_SUSPEND, vcpu);
>  	}
>  
>  	kvm_incr_pc(vcpu);

This is a change in behaviour. At the point where the blocking
happens, PC will have already been incremented. I'd rather you don't
do that. Instead, make the helper available and call into it directly,
preserving the current semantics.

It is also likely to clash with Sean's kvm_vcpu_block() rework, but we
can work around that.

> diff --git a/arch/arm64/kvm/psci.c b/arch/arm64/kvm/psci.c
> index bb59b692998b..d453666ddb83 100644
> --- a/arch/arm64/kvm/psci.c
> +++ b/arch/arm64/kvm/psci.c
> @@ -46,9 +46,7 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)
>  	 * specification (ARM DEN 0022A). This means all suspend states
>  	 * for KVM will preserve the register state.
>  	 */
> -	kvm_vcpu_block(vcpu);
> -	kvm_clear_request(KVM_REQ_UNHALT, vcpu);
> -
> +	kvm_make_request(KVM_REQ_SUSPEND, vcpu);
>  	return PSCI_RET_SUCCESS;
>  }

I have similar feelings about this one, although things only breaks
once you enable NV (SMC is trapped and subject to PC adjustments).

Thanks,

	M.

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

Re: [PATCH v2 06/11] KVM: arm64: Add support for SYSTEM_SUSPEND PSCI call

[Marc Zyngier]

Hi Oliver,

On Thu, 23 Sep 2021 20:16:05 +0100,
Oliver Upton <oupton@google.com> wrote:
> 
> ARM DEN0022D 5.19 "SYSTEM_SUSPEND" describes a PSCI call that may be
> used to request a system be suspended. This is optional for PSCI v1.0
> and to date KVM has elected to not implement the call. However, a
> VMM/operator may wish to provide their guests with the ability to
> suspend/resume, necessitating this PSCI call.
> 
> Implement support for SYSTEM_SUSPEND according to the prescribed
> behavior in the specification. Add a new system event exit type,
> KVM_SYSTEM_EVENT_SUSPEND, to notify userspace when a VM has requested a
> system suspend. Make KVM_MP_STATE_HALTED a valid state on arm64.

KVM_MP_STATE_HALTED is a per-CPU state on x86 (it denotes HLT). Does
it make really sense to hijack this for something that is more of a
VM-wide state? I can see that it is tempting to do so as we're using
the WFI semantics (which are close to HLT's, in a twisted kind of
way), but I'm also painfully aware that gluing x86 expectations on
arm64 rarely leads to a palatable result.

> Userspace can set this to request an in-kernel emulation of the suspend.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  Documentation/virt/kvm/api.rst    |  6 ++++
>  arch/arm64/include/asm/kvm_host.h |  3 ++
>  arch/arm64/kvm/arm.c              |  8 +++++
>  arch/arm64/kvm/psci.c             | 60 +++++++++++++++++++++++++++++++
>  include/uapi/linux/kvm.h          |  2 ++
>  5 files changed, 79 insertions(+)

This patch needs splitting. PSCI interface in one, mpstate stuff in
another, userspace management last.

> 
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index a6729c8cf063..361a57061b8f 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -5656,6 +5656,7 @@ should put the acknowledged interrupt vector into the 'epr' field.
>    #define KVM_SYSTEM_EVENT_SHUTDOWN       1
>    #define KVM_SYSTEM_EVENT_RESET          2
>    #define KVM_SYSTEM_EVENT_CRASH          3
> +  #define KVM_SYSTEM_EVENT_SUSPEND        4
>  			__u32 type;
>  			__u64 flags;
>  		} system_event;
> @@ -5680,6 +5681,11 @@ Valid values for 'type' are:
>     has requested a crash condition maintenance. Userspace can choose
>     to ignore the request, or to gather VM memory core dump and/or
>     reset/shutdown of the VM.
> + - KVM_SYSTEM_EVENT_SUSPEND -- the guest has requested that the VM
> +   suspends. Userspace is not obliged to honor this, and may call KVM_RUN
> +   again. Doing so will cause the guest to resume at its requested entry
> +   point. For ARM64, userspace can request in-kernel suspend emulation
> +   by setting the vCPU's MP state to KVM_MP_STATE_HALTED.
>  
>  ::
>  
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 1beda1189a15..441eb6fa7adc 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -137,6 +137,9 @@ struct kvm_arch {
>  
>  	/* Memory Tagging Extension enabled for the guest */
>  	bool mte_enabled;
> +
> +	/* PSCI SYSTEM_SUSPEND call enabled for the guest */
> +	bool suspend_enabled;
>  };
>  
>  struct kvm_vcpu_fault_info {
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index f1a375648e25..d875d3bcf3c5 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -101,6 +101,10 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
>  		}
>  		mutex_unlock(&kvm->lock);
>  		break;
> +	case KVM_CAP_ARM_SYSTEM_SUSPEND:
> +		r = 0;
> +		kvm->arch.suspend_enabled = true;

I don't really fancy adding another control here. Given that we now
have the PSCI version being controlled by a firmware pseudo-register,
I'd rather have that there.

And if we do that, I wonder whether there would be any benefit in
bumping the PSCI version to 1.1.

> +		break;
>  	default:
>  		r = -EINVAL;
>  		break;
> @@ -215,6 +219,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  	case KVM_CAP_SET_GUEST_DEBUG:
>  	case KVM_CAP_VCPU_ATTRIBUTES:
>  	case KVM_CAP_PTP_KVM:
> +	case KVM_CAP_ARM_SYSTEM_SUSPEND:
>  		r = 1;
>  		break;
>  	case KVM_CAP_SET_GUEST_DEBUG2:
> @@ -470,6 +475,9 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
>  	int ret = 0;
>  
>  	switch (mp_state->mp_state) {
> +	case KVM_MP_STATE_HALTED:
> +		kvm_make_request(KVM_REQ_SUSPEND, vcpu);
> +		fallthrough;
>  	case KVM_MP_STATE_RUNNABLE:
>  		vcpu->arch.power_off = false;
>  		break;

> diff --git a/arch/arm64/kvm/psci.c b/arch/arm64/kvm/psci.c
> index d453666ddb83..cf869f1f8615 100644
> --- a/arch/arm64/kvm/psci.c
> +++ b/arch/arm64/kvm/psci.c
> @@ -203,6 +203,46 @@ static void kvm_psci_system_reset(struct kvm_vcpu *vcpu)
>  	kvm_prepare_system_event(vcpu, KVM_SYSTEM_EVENT_RESET);
>  }
>  
> +static int kvm_psci_system_suspend(struct kvm_vcpu *vcpu)
> +{
> +	unsigned long entry_addr, context_id;
> +	struct kvm *kvm = vcpu->kvm;
> +	unsigned long psci_ret = 0;
> +	struct kvm_vcpu *tmp;
> +	int ret = 0;
> +	int i;
> +
> +	/*
> +	 * The SYSTEM_SUSPEND PSCI call requires that all vCPUs (except the
> +	 * calling vCPU) be in an OFF state, as determined by the
> +	 * implementation.
> +	 *
> +	 * See ARM DEN0022D, 5.19 "SYSTEM_SUSPEND" for more details.
> +	 */
> +	mutex_lock(&kvm->lock);
> +	kvm_for_each_vcpu(i, tmp, kvm) {
> +		if (tmp != vcpu && !tmp->arch.power_off) {
> +			psci_ret = PSCI_RET_DENIED;
> +			ret = 1;
> +			goto out;
> +		}
> +	}
> +
> +	entry_addr = smccc_get_arg1(vcpu);
> +	context_id = smccc_get_arg2(vcpu);
> +
> +	kvm_psci_vcpu_request_reset(vcpu, entry_addr, context_id,
> +				    kvm_vcpu_is_be(vcpu));

So even if userspace doesn't want to honor the suspend request, the
CPU ends up resetting? That's not wrong, but maybe a bit surprising.

> +
> +	memset(&vcpu->run->system_event, 0, sizeof(vcpu->run->system_event));
> +	vcpu->run->system_event.type = KVM_SYSTEM_EVENT_SUSPEND;
> +	vcpu->run->exit_reason = KVM_EXIT_SYSTEM_EVENT;

Consider spinning out a helper common to this and kvm_prepare_system_event().

> +out:
> +	mutex_unlock(&kvm->lock);
> +	smccc_set_retval(vcpu, psci_ret, 0, 0, 0);
> +	return ret;
> +}
> +
>  static void kvm_psci_narrow_to_32bit(struct kvm_vcpu *vcpu)
>  {
>  	int i;
> @@ -223,6 +263,14 @@ static unsigned long kvm_psci_check_allowed_function(struct kvm_vcpu *vcpu, u32
>  	if ((fn & PSCI_0_2_64BIT) && vcpu_mode_is_32bit(vcpu))
>  		return PSCI_RET_NOT_SUPPORTED;
>  
> +	switch (fn) {
> +	case PSCI_1_0_FN_SYSTEM_SUSPEND:
> +	case PSCI_1_0_FN64_SYSTEM_SUSPEND:
> +		if (!vcpu->kvm->arch.suspend_enabled)
> +			return PSCI_RET_NOT_SUPPORTED;
> +		break;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -316,6 +364,10 @@ static int kvm_psci_1_0_call(struct kvm_vcpu *vcpu)
>  	unsigned long val;
>  	int ret = 1;
>  
> +	val = kvm_psci_check_allowed_function(vcpu, psci_fn);
> +	if (val)
> +		goto out;
> +
>  	switch(psci_fn) {
>  	case PSCI_0_2_FN_PSCI_VERSION:
>  		val = KVM_ARM_PSCI_1_0;
> @@ -339,6 +391,8 @@ static int kvm_psci_1_0_call(struct kvm_vcpu *vcpu)
>  		case PSCI_0_2_FN_SYSTEM_OFF:
>  		case PSCI_0_2_FN_SYSTEM_RESET:
>  		case PSCI_1_0_FN_PSCI_FEATURES:
> +		case PSCI_1_0_FN_SYSTEM_SUSPEND:
> +		case PSCI_1_0_FN64_SYSTEM_SUSPEND:
>  		case ARM_SMCCC_VERSION_FUNC_ID:
>  			val = 0;
>  			break;
> @@ -347,10 +401,16 @@ static int kvm_psci_1_0_call(struct kvm_vcpu *vcpu)
>  			break;
>  		}
>  		break;
> +	case PSCI_1_0_FN_SYSTEM_SUSPEND:
> +		kvm_psci_narrow_to_32bit(vcpu);
> +		fallthrough;
> +	case PSCI_1_0_FN64_SYSTEM_SUSPEND:
> +		return kvm_psci_system_suspend(vcpu);
>  	default:
>  		return kvm_psci_0_2_call(vcpu);
>  	}
>  
> +out:
>  	smccc_set_retval(vcpu, val, 0, 0, 0);
>  	return ret;
>  }
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index a067410ebea5..052b0e717b08 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -433,6 +433,7 @@ struct kvm_run {
>  #define KVM_SYSTEM_EVENT_SHUTDOWN       1
>  #define KVM_SYSTEM_EVENT_RESET          2
>  #define KVM_SYSTEM_EVENT_CRASH          3
> +#define KVM_SYSTEM_EVENT_SUSPEND        4
>  			__u32 type;
>  			__u64 flags;
>  		} system_event;
> @@ -1112,6 +1113,7 @@ struct kvm_ppc_resize_hpt {
>  #define KVM_CAP_BINARY_STATS_FD 203
>  #define KVM_CAP_EXIT_ON_EMULATION_FAILURE 204
>  #define KVM_CAP_ARM_MTE 205
> +#define KVM_CAP_ARM_SYSTEM_SUSPEND 206
>  
>  #ifdef KVM_CAP_IRQ_ROUTING
>  

I think there is an additional feature we need, which is to give
control back to userspace every time a wake-up condition occurs (I did
touch on that in [1]). This would give the opportunity to the VMM to
do whatever it needs to perform.

A typical use case would be to implement wake-up from certain
interrupts only (mask non-wake-up IRQs on suspend request, return to
the guest for WFI, wake-up returns to the VMM to restore the previous
interrupt configuration, resume).

Userspace would be free to clear the suspend state and resume the
guest, or to reenter WFI.

Thanks,

	M.

[1] https://lkml.kernel.org/kvm/87k0jauurx.wl-maz@kernel.org/

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

Re: [PATCH v2 05/11] KVM: arm64: Defer WFI emulation as a requested event

[Sean Christopherson]

On Thu, Sep 30, 2021, Marc Zyngier wrote:
> On Thu, 23 Sep 2021 20:16:04 +0100, Oliver Upton <oupton@google.com> wrote:
> > @@ -681,6 +687,9 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
> >  		if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
> >  			kvm_vcpu_sleep(vcpu);
> >  
> > +		if (kvm_check_request(KVM_REQ_SUSPEND, vcpu))
> > +			kvm_vcpu_suspend(vcpu);
> > +

...

> > diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> > index 275a27368a04..5e5ef9ff4fba 100644
> > --- a/arch/arm64/kvm/handle_exit.c
> > +++ b/arch/arm64/kvm/handle_exit.c
> > @@ -95,8 +95,7 @@ static int kvm_handle_wfx(struct kvm_vcpu *vcpu)
> >  	} else {
> >  		trace_kvm_wfx_arm64(*vcpu_pc(vcpu), false);
> >  		vcpu->stat.wfi_exit_stat++;
> > -		kvm_vcpu_block(vcpu);
> > -		kvm_clear_request(KVM_REQ_UNHALT, vcpu);
> > +		kvm_make_request(KVM_REQ_SUSPEND, vcpu);
> >  	}
> >  
> >  	kvm_incr_pc(vcpu);
> 
> This is a change in behaviour. At the point where the blocking
> happens, PC will have already been incremented. I'd rather you don't
> do that. Instead, make the helper available and call into it directly,
> preserving the current semantics.

Is there architectural behavior that KVM can emulate?  E.g. if you were to probe a
physical CPU while it's waiting, would you observe the pre-WFI PC, or the post-WFI
PC?  Following arch behavior would be ideal because it eliminates subjectivity.
Regardless of the architectural behavior, changing KVM's behavior should be
done explicitly in a separate patch.

Irrespective of PC behavior, I would caution against using a request for handling
WFI.  Deferring the WFI opens up the possibility for all sorts of ordering
oddities, e.g. if KVM exits to userspace between here and check_vcpu_requests(),
then KVM can end up with a "spurious" pending KVM_REQ_SUSPEND if maniupaltes vCPU
state.  I highly doubt that userspace VMMs would actually do that, as it would
basically require a signal from userspace, but it's not impossible, and at the
very least the pending request is yet another thing to worry about in the future.

Unlike PSCI power-off, WFI isn't cross-vCPU, thus there's no hard requirement
for using a request.  And KVM_REQ_SLEEP also has an additional guard in that it
doesn't enter rcuwait if power_off (or pause) was cleared after the request was
made, e.g. if userspace stuffed vCPU state and set the vCPU RUNNABLE.

> It is also likely to clash with Sean's kvm_vcpu_block() rework, but we
> can work around that.

Ya.  Oliver, can you Cc me on future patches?  I'll try to keep my eyeballs on this
series.

Re: [PATCH v2 06/11] KVM: arm64: Add support for SYSTEM_SUSPEND PSCI call

[Sean Christopherson]

On Thu, Sep 30, 2021, Marc Zyngier wrote:
> Hi Oliver,
> 
> On Thu, 23 Sep 2021 20:16:05 +0100,
> Oliver Upton <oupton@google.com> wrote:
> > 
> > ARM DEN0022D 5.19 "SYSTEM_SUSPEND" describes a PSCI call that may be
> > used to request a system be suspended. This is optional for PSCI v1.0
> > and to date KVM has elected to not implement the call. However, a
> > VMM/operator may wish to provide their guests with the ability to
> > suspend/resume, necessitating this PSCI call.
> > 
> > Implement support for SYSTEM_SUSPEND according to the prescribed
> > behavior in the specification. Add a new system event exit type,
> > KVM_SYSTEM_EVENT_SUSPEND, to notify userspace when a VM has requested a
> > system suspend. Make KVM_MP_STATE_HALTED a valid state on arm64.
> 
> KVM_MP_STATE_HALTED is a per-CPU state on x86 (it denotes HLT). Does
> it make really sense to hijack this for something that is more of a
> VM-wide state? I can see that it is tempting to do so as we're using
> the WFI semantics (which are close to HLT's, in a twisted kind of
> way), but I'm also painfully aware that gluing x86 expectations on
> arm64 rarely leads to a palatable result.

Agreed, we literally have billions of possible KVM_MP_STATE_* values, and I'm pretty
sure all of the existing states are arch-specific.  Some are common to multiple
architectures, but I don't think _any_ are common to all architectures.

Re: [PATCH v2 05/11] KVM: arm64: Defer WFI emulation as a requested event

[Oliver Upton]

On Thu, Sep 30, 2021 at 10:09 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Thu, Sep 30, 2021, Marc Zyngier wrote:
> > On Thu, 23 Sep 2021 20:16:04 +0100, Oliver Upton <oupton@google.com> wrote:
> > > @@ -681,6 +687,9 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
> > >             if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
> > >                     kvm_vcpu_sleep(vcpu);
> > >
> > > +           if (kvm_check_request(KVM_REQ_SUSPEND, vcpu))
> > > +                   kvm_vcpu_suspend(vcpu);
> > > +
>
> ...
>
> > > diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> > > index 275a27368a04..5e5ef9ff4fba 100644
> > > --- a/arch/arm64/kvm/handle_exit.c
> > > +++ b/arch/arm64/kvm/handle_exit.c
> > > @@ -95,8 +95,7 @@ static int kvm_handle_wfx(struct kvm_vcpu *vcpu)
> > >     } else {
> > >             trace_kvm_wfx_arm64(*vcpu_pc(vcpu), false);
> > >             vcpu->stat.wfi_exit_stat++;
> > > -           kvm_vcpu_block(vcpu);
> > > -           kvm_clear_request(KVM_REQ_UNHALT, vcpu);
> > > +           kvm_make_request(KVM_REQ_SUSPEND, vcpu);
> > >     }
> > >
> > >     kvm_incr_pc(vcpu);
> >
> > This is a change in behaviour. At the point where the blocking
> > happens, PC will have already been incremented. I'd rather you don't
> > do that. Instead, make the helper available and call into it directly,
> > preserving the current semantics.
>
> Is there architectural behavior that KVM can emulate?  E.g. if you were to probe a
> physical CPU while it's waiting, would you observe the pre-WFI PC, or the post-WFI
> PC?  Following arch behavior would be ideal because it eliminates subjectivity.
> Regardless of the architectural behavior, changing KVM's behavior should be
> done explicitly in a separate patch.
>
> Irrespective of PC behavior, I would caution against using a request for handling
> WFI.  Deferring the WFI opens up the possibility for all sorts of ordering
> oddities, e.g. if KVM exits to userspace between here and check_vcpu_requests(),
> then KVM can end up with a "spurious" pending KVM_REQ_SUSPEND if maniupaltes vCPU
> state.  I highly doubt that userspace VMMs would actually do that, as it would
> basically require a signal from userspace, but it's not impossible, and at the
> very least the pending request is yet another thing to worry about in the future.
>
> Unlike PSCI power-off, WFI isn't cross-vCPU, thus there's no hard requirement
> for using a request.  And KVM_REQ_SLEEP also has an additional guard in that it
> doesn't enter rcuwait if power_off (or pause) was cleared after the request was
> made, e.g. if userspace stuffed vCPU state and set the vCPU RUNNABLE.

Yeah, I don't think the punt is necessary for anything but the case
where userspace sets the MP state to request WFI behavior. A helper
method amongst all WFI cases is sufficient, and using the deferral for
everything is a change in behavior.

> > It is also likely to clash with Sean's kvm_vcpu_block() rework, but we
> > can work around that.
>
> Ya.  Oliver, can you Cc me on future patches?  I'll try to keep my eyeballs on this
> series.

Sure thing :)

Re: [PATCH v2 06/11] KVM: arm64: Add support for SYSTEM_SUSPEND PSCI call

[Oliver Upton]

On Thu, Sep 30, 2021 at 10:19 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Thu, Sep 30, 2021, Marc Zyngier wrote:
> > Hi Oliver,
> >
> > On Thu, 23 Sep 2021 20:16:05 +0100,
> > Oliver Upton <oupton@google.com> wrote:
> > >
> > > ARM DEN0022D 5.19 "SYSTEM_SUSPEND" describes a PSCI call that may be
> > > used to request a system be suspended. This is optional for PSCI v1.0
> > > and to date KVM has elected to not implement the call. However, a
> > > VMM/operator may wish to provide their guests with the ability to
> > > suspend/resume, necessitating this PSCI call.
> > >
> > > Implement support for SYSTEM_SUSPEND according to the prescribed
> > > behavior in the specification. Add a new system event exit type,
> > > KVM_SYSTEM_EVENT_SUSPEND, to notify userspace when a VM has requested a
> > > system suspend. Make KVM_MP_STATE_HALTED a valid state on arm64.
> >
> > KVM_MP_STATE_HALTED is a per-CPU state on x86 (it denotes HLT). Does
> > it make really sense to hijack this for something that is more of a
> > VM-wide state? I can see that it is tempting to do so as we're using
> > the WFI semantics (which are close to HLT's, in a twisted kind of
> > way), but I'm also painfully aware that gluing x86 expectations on
> > arm64 rarely leads to a palatable result.
>
> Agreed, we literally have billions of possible KVM_MP_STATE_* values, and I'm pretty
> sure all of the existing states are arch-specific.  Some are common to multiple
> architectures, but I don't think _any_ are common to all architectures.

Yeah, I was debating this as well when cooking up the series. No need
to overload the x86-ism when we can have a precisely named state for
ARM.

Re: [PATCH v2 06/11] KVM: arm64: Add support for SYSTEM_SUSPEND PSCI call

[Oliver Upton]

On Thu, Sep 30, 2021 at 5:29 AM Marc Zyngier <maz@kernel.org> wrote:
>
> Hi Oliver,
>
> On Thu, 23 Sep 2021 20:16:05 +0100,
> Oliver Upton <oupton@google.com> wrote:
> >
> > ARM DEN0022D 5.19 "SYSTEM_SUSPEND" describes a PSCI call that may be
> > used to request a system be suspended. This is optional for PSCI v1.0
> > and to date KVM has elected to not implement the call. However, a
> > VMM/operator may wish to provide their guests with the ability to
> > suspend/resume, necessitating this PSCI call.
> >
> > Implement support for SYSTEM_SUSPEND according to the prescribed
> > behavior in the specification. Add a new system event exit type,
> > KVM_SYSTEM_EVENT_SUSPEND, to notify userspace when a VM has requested a
> > system suspend. Make KVM_MP_STATE_HALTED a valid state on arm64.
>
> KVM_MP_STATE_HALTED is a per-CPU state on x86 (it denotes HLT). Does
> it make really sense to hijack this for something that is more of a
> VM-wide state? I can see that it is tempting to do so as we're using
> the WFI semantics (which are close to HLT's, in a twisted kind of
> way), but I'm also painfully aware that gluing x86 expectations on
> arm64 rarely leads to a palatable result.
>
> > Userspace can set this to request an in-kernel emulation of the suspend.
> >
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >  Documentation/virt/kvm/api.rst    |  6 ++++
> >  arch/arm64/include/asm/kvm_host.h |  3 ++
> >  arch/arm64/kvm/arm.c              |  8 +++++
> >  arch/arm64/kvm/psci.c             | 60 +++++++++++++++++++++++++++++++
> >  include/uapi/linux/kvm.h          |  2 ++
> >  5 files changed, 79 insertions(+)
>
> This patch needs splitting. PSCI interface in one, mpstate stuff in
> another, userspace management last.
>
> >
> > diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> > index a6729c8cf063..361a57061b8f 100644
> > --- a/Documentation/virt/kvm/api.rst
> > +++ b/Documentation/virt/kvm/api.rst
> > @@ -5656,6 +5656,7 @@ should put the acknowledged interrupt vector into the 'epr' field.
> >    #define KVM_SYSTEM_EVENT_SHUTDOWN       1
> >    #define KVM_SYSTEM_EVENT_RESET          2
> >    #define KVM_SYSTEM_EVENT_CRASH          3
> > +  #define KVM_SYSTEM_EVENT_SUSPEND        4
> >                       __u32 type;
> >                       __u64 flags;
> >               } system_event;
> > @@ -5680,6 +5681,11 @@ Valid values for 'type' are:
> >     has requested a crash condition maintenance. Userspace can choose
> >     to ignore the request, or to gather VM memory core dump and/or
> >     reset/shutdown of the VM.
> > + - KVM_SYSTEM_EVENT_SUSPEND -- the guest has requested that the VM
> > +   suspends. Userspace is not obliged to honor this, and may call KVM_RUN
> > +   again. Doing so will cause the guest to resume at its requested entry
> > +   point. For ARM64, userspace can request in-kernel suspend emulation
> > +   by setting the vCPU's MP state to KVM_MP_STATE_HALTED.
> >
> >  ::
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index 1beda1189a15..441eb6fa7adc 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -137,6 +137,9 @@ struct kvm_arch {
> >
> >       /* Memory Tagging Extension enabled for the guest */
> >       bool mte_enabled;
> > +
> > +     /* PSCI SYSTEM_SUSPEND call enabled for the guest */
> > +     bool suspend_enabled;
> >  };
> >
> >  struct kvm_vcpu_fault_info {
> > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > index f1a375648e25..d875d3bcf3c5 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -101,6 +101,10 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
> >               }
> >               mutex_unlock(&kvm->lock);
> >               break;
> > +     case KVM_CAP_ARM_SYSTEM_SUSPEND:
> > +             r = 0;
> > +             kvm->arch.suspend_enabled = true;
>
> I don't really fancy adding another control here. Given that we now
> have the PSCI version being controlled by a firmware pseudo-register,
> I'd rather have that there.
>
> And if we do that, I wonder whether there would be any benefit in
> bumping the PSCI version to 1.1.
>
> > +             break;
> >       default:
> >               r = -EINVAL;
> >               break;
> > @@ -215,6 +219,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> >       case KVM_CAP_SET_GUEST_DEBUG:
> >       case KVM_CAP_VCPU_ATTRIBUTES:
> >       case KVM_CAP_PTP_KVM:
> > +     case KVM_CAP_ARM_SYSTEM_SUSPEND:
> >               r = 1;
> >               break;
> >       case KVM_CAP_SET_GUEST_DEBUG2:
> > @@ -470,6 +475,9 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
> >       int ret = 0;
> >
> >       switch (mp_state->mp_state) {
> > +     case KVM_MP_STATE_HALTED:
> > +             kvm_make_request(KVM_REQ_SUSPEND, vcpu);
> > +             fallthrough;
> >       case KVM_MP_STATE_RUNNABLE:
> >               vcpu->arch.power_off = false;
> >               break;
>
> > diff --git a/arch/arm64/kvm/psci.c b/arch/arm64/kvm/psci.c
> > index d453666ddb83..cf869f1f8615 100644
> > --- a/arch/arm64/kvm/psci.c
> > +++ b/arch/arm64/kvm/psci.c
> > @@ -203,6 +203,46 @@ static void kvm_psci_system_reset(struct kvm_vcpu *vcpu)
> >       kvm_prepare_system_event(vcpu, KVM_SYSTEM_EVENT_RESET);
> >  }
> >
> > +static int kvm_psci_system_suspend(struct kvm_vcpu *vcpu)
> > +{
> > +     unsigned long entry_addr, context_id;
> > +     struct kvm *kvm = vcpu->kvm;
> > +     unsigned long psci_ret = 0;
> > +     struct kvm_vcpu *tmp;
> > +     int ret = 0;
> > +     int i;
> > +
> > +     /*
> > +      * The SYSTEM_SUSPEND PSCI call requires that all vCPUs (except the
> > +      * calling vCPU) be in an OFF state, as determined by the
> > +      * implementation.
> > +      *
> > +      * See ARM DEN0022D, 5.19 "SYSTEM_SUSPEND" for more details.
> > +      */
> > +     mutex_lock(&kvm->lock);
> > +     kvm_for_each_vcpu(i, tmp, kvm) {
> > +             if (tmp != vcpu && !tmp->arch.power_off) {
> > +                     psci_ret = PSCI_RET_DENIED;
> > +                     ret = 1;
> > +                     goto out;
> > +             }
> > +     }
> > +
> > +     entry_addr = smccc_get_arg1(vcpu);
> > +     context_id = smccc_get_arg2(vcpu);
> > +
> > +     kvm_psci_vcpu_request_reset(vcpu, entry_addr, context_id,
> > +                                 kvm_vcpu_is_be(vcpu));
>
> So even if userspace doesn't want to honor the suspend request, the
> CPU ends up resetting? That's not wrong, but maybe a bit surprising.
>
> > +
> > +     memset(&vcpu->run->system_event, 0, sizeof(vcpu->run->system_event));
> > +     vcpu->run->system_event.type = KVM_SYSTEM_EVENT_SUSPEND;
> > +     vcpu->run->exit_reason = KVM_EXIT_SYSTEM_EVENT;
>
> Consider spinning out a helper common to this and kvm_prepare_system_event().
>
> > +out:
> > +     mutex_unlock(&kvm->lock);
> > +     smccc_set_retval(vcpu, psci_ret, 0, 0, 0);
> > +     return ret;
> > +}
> > +
> >  static void kvm_psci_narrow_to_32bit(struct kvm_vcpu *vcpu)
> >  {
> >       int i;
> > @@ -223,6 +263,14 @@ static unsigned long kvm_psci_check_allowed_function(struct kvm_vcpu *vcpu, u32
> >       if ((fn & PSCI_0_2_64BIT) && vcpu_mode_is_32bit(vcpu))
> >               return PSCI_RET_NOT_SUPPORTED;
> >
> > +     switch (fn) {
> > +     case PSCI_1_0_FN_SYSTEM_SUSPEND:
> > +     case PSCI_1_0_FN64_SYSTEM_SUSPEND:
> > +             if (!vcpu->kvm->arch.suspend_enabled)
> > +                     return PSCI_RET_NOT_SUPPORTED;
> > +             break;
> > +     }
> > +
> >       return 0;
> >  }
> >
> > @@ -316,6 +364,10 @@ static int kvm_psci_1_0_call(struct kvm_vcpu *vcpu)
> >       unsigned long val;
> >       int ret = 1;
> >
> > +     val = kvm_psci_check_allowed_function(vcpu, psci_fn);
> > +     if (val)
> > +             goto out;
> > +
> >       switch(psci_fn) {
> >       case PSCI_0_2_FN_PSCI_VERSION:
> >               val = KVM_ARM_PSCI_1_0;
> > @@ -339,6 +391,8 @@ static int kvm_psci_1_0_call(struct kvm_vcpu *vcpu)
> >               case PSCI_0_2_FN_SYSTEM_OFF:
> >               case PSCI_0_2_FN_SYSTEM_RESET:
> >               case PSCI_1_0_FN_PSCI_FEATURES:
> > +             case PSCI_1_0_FN_SYSTEM_SUSPEND:
> > +             case PSCI_1_0_FN64_SYSTEM_SUSPEND:
> >               case ARM_SMCCC_VERSION_FUNC_ID:
> >                       val = 0;
> >                       break;
> > @@ -347,10 +401,16 @@ static int kvm_psci_1_0_call(struct kvm_vcpu *vcpu)
> >                       break;
> >               }
> >               break;
> > +     case PSCI_1_0_FN_SYSTEM_SUSPEND:
> > +             kvm_psci_narrow_to_32bit(vcpu);
> > +             fallthrough;
> > +     case PSCI_1_0_FN64_SYSTEM_SUSPEND:
> > +             return kvm_psci_system_suspend(vcpu);
> >       default:
> >               return kvm_psci_0_2_call(vcpu);
> >       }
> >
> > +out:
> >       smccc_set_retval(vcpu, val, 0, 0, 0);
> >       return ret;
> >  }
> > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> > index a067410ebea5..052b0e717b08 100644
> > --- a/include/uapi/linux/kvm.h
> > +++ b/include/uapi/linux/kvm.h
> > @@ -433,6 +433,7 @@ struct kvm_run {
> >  #define KVM_SYSTEM_EVENT_SHUTDOWN       1
> >  #define KVM_SYSTEM_EVENT_RESET          2
> >  #define KVM_SYSTEM_EVENT_CRASH          3
> > +#define KVM_SYSTEM_EVENT_SUSPEND        4
> >                       __u32 type;
> >                       __u64 flags;
> >               } system_event;
> > @@ -1112,6 +1113,7 @@ struct kvm_ppc_resize_hpt {
> >  #define KVM_CAP_BINARY_STATS_FD 203
> >  #define KVM_CAP_EXIT_ON_EMULATION_FAILURE 204
> >  #define KVM_CAP_ARM_MTE 205
> > +#define KVM_CAP_ARM_SYSTEM_SUSPEND 206
> >
> >  #ifdef KVM_CAP_IRQ_ROUTING
> >
>
> I think there is an additional feature we need, which is to give
> control back to userspace every time a wake-up condition occurs (I did
> touch on that in [1]). This would give the opportunity to the VMM to
> do whatever it needs to perform.
>
> A typical use case would be to implement wake-up from certain
> interrupts only (mask non-wake-up IRQs on suspend request, return to
> the guest for WFI, wake-up returns to the VMM to restore the previous
> interrupt configuration, resume).
>
> Userspace would be free to clear the suspend state and resume the
> guest, or to reenter WFI.

Yeah, this is definitely needed for the series.

Just to make sure it's clear, what should the default behavior be if
userspace doesn't touch state at all and it calls KVM_RUN again? It
would seem to me that we should resume the guest by default, much like
we would do for the SUSPEND event exit.

--
Thanks,
Oliver

Re: [PATCH v2 05/11] KVM: arm64: Defer WFI emulation as a requested event

[Sean Christopherson]

On Thu, Sep 30, 2021, Oliver Upton wrote:
> On Thu, Sep 30, 2021 at 10:09 AM Sean Christopherson <seanjc@google.com> wrote:
> > Unlike PSCI power-off, WFI isn't cross-vCPU, thus there's no hard requirement
> > for using a request.  And KVM_REQ_SLEEP also has an additional guard in that it
> > doesn't enter rcuwait if power_off (or pause) was cleared after the request was
> > made, e.g. if userspace stuffed vCPU state and set the vCPU RUNNABLE.
> 
> Yeah, I don't think the punt is necessary for anything but the case
> where userspace sets the MP state to request WFI behavior. A helper
> method amongst all WFI cases is sufficient, and using the deferral for
> everything is a change in behavior.

Is there an actual use case for letting userspace request WFI behavior?

Re: [PATCH v2 05/11] KVM: arm64: Defer WFI emulation as a requested event

[Oliver Upton]

On Thu, Sep 30, 2021 at 11:08 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Thu, Sep 30, 2021, Oliver Upton wrote:
> > On Thu, Sep 30, 2021 at 10:09 AM Sean Christopherson <seanjc@google.com> wrote:
> > > Unlike PSCI power-off, WFI isn't cross-vCPU, thus there's no hard requirement
> > > for using a request.  And KVM_REQ_SLEEP also has an additional guard in that it
> > > doesn't enter rcuwait if power_off (or pause) was cleared after the request was
> > > made, e.g. if userspace stuffed vCPU state and set the vCPU RUNNABLE.
> >
> > Yeah, I don't think the punt is necessary for anything but the case
> > where userspace sets the MP state to request WFI behavior. A helper
> > method amongst all WFI cases is sufficient, and using the deferral for
> > everything is a change in behavior.
>
> Is there an actual use case for letting userspace request WFI behavior?

So for the system event exits we say that userspace can ignore them
and call KVM_RUN again. Running the guest after the suspend exit
should appear to the guest as though it had resumed. Userspace could
do something fancy to handle the suspend (save the VM, wait for an
implementation defined event) or ask the kernel to do it. To that end,
the MP state is needed to tell KVM to WFI instead of starting the
guest immediately.

Re: [PATCH v2 01/11] KVM: arm64: Drop unused vcpu param to kvm_psci_valid_affinity()

[Reiji Watanabe]

On Thu, Sep 23, 2021 at 12:16 PM Oliver Upton <oupton@google.com> wrote:
>
> The helper function does not need a pointer to the vCPU, as it only
> consults a constant mask; drop the unused vcpu parameter.
>
> Signed-off-by: Oliver Upton <oupton@google.com>

Reviewed-by: Reiji Watanabe <reijiw@google.com>

Re: [PATCH v2 02/11] KVM: arm64: Clean up SMC64 PSCI filtering for AArch32 guests

[Reiji Watanabe]

On Thu, Sep 23, 2021 at 12:16 PM Oliver Upton <oupton@google.com> wrote:
>
> The only valid calling SMC calling convention from an AArch32 state is
> SMC32. Disallow any PSCI function that sets the SMC64 function ID bit
> when called from AArch32 rather than comparing against known SMC64 PSCI
> functions.
>
> Signed-off-by: Oliver Upton <oupton@google.com>

Looks nice.
Reviewed-by: Reiji Watanabe <reijiw@google.com>

Re: [PATCH v2 03/11] KVM: arm64: Encapsulate reset request logic in a helper function

[Reiji Watanabe]

On Thu, Sep 23, 2021 at 12:16 PM Oliver Upton <oupton@google.com> wrote:
>
> In its implementation of the PSCI function, KVM needs to request that a
> target vCPU resets before its next entry into the guest. Wrap the logic
> for requesting a reset in a function for later use by other implemented
> PSCI calls.
>
> No functional change intended.
>
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/kvm/psci.c | 59 +++++++++++++++++++++++++------------------
>  1 file changed, 35 insertions(+), 24 deletions(-)
>
> diff --git a/arch/arm64/kvm/psci.c b/arch/arm64/kvm/psci.c
> index 310b9cb2b32b..bb59b692998b 100644
> --- a/arch/arm64/kvm/psci.c
> +++ b/arch/arm64/kvm/psci.c
> @@ -64,9 +64,40 @@ static inline bool kvm_psci_valid_affinity(unsigned long affinity)
>         return !(affinity & ~MPIDR_HWID_BITMASK);
>  }
>
> -static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> +static void kvm_psci_vcpu_request_reset(struct kvm_vcpu *vcpu,
> +                                       unsigned long entry_addr,
> +                                       unsigned long context_id,
> +                                       bool big_endian)
>  {
>         struct vcpu_reset_state *reset_state;
> +
> +       lockdep_assert_held(&vcpu->kvm->lock);
> +
> +       reset_state = &vcpu->arch.reset_state;
> +       reset_state->pc = entry_addr;
> +
> +       /* Propagate caller endianness */
> +       reset_state->be = big_endian;
> +
> +       /*
> +        * NOTE: We always update r0 (or x0) because for PSCI v0.1
> +        * the general purpose registers are undefined upon CPU_ON.
> +        */
> +       reset_state->r0 = context_id;
> +
> +       WRITE_ONCE(reset_state->reset, true);
> +       kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
> +
> +       /*
> +        * Make sure the reset request is observed if the change to
> +        * power_state is observed.
> +        */
> +       smp_wmb();
> +       vcpu->arch.power_off = false;
> +}
> +
> +static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> +{
>         struct kvm *kvm = source_vcpu->kvm;
>         struct kvm_vcpu *vcpu = NULL;
>         unsigned long cpu_id;
> @@ -90,29 +121,9 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>                         return PSCI_RET_INVALID_PARAMS;
>         }
>
> -       reset_state = &vcpu->arch.reset_state;
> -
> -       reset_state->pc = smccc_get_arg2(source_vcpu);
> -
> -       /* Propagate caller endianness */
> -       reset_state->be = kvm_vcpu_is_be(source_vcpu);
> -
> -       /*
> -        * NOTE: We always update r0 (or x0) because for PSCI v0.1
> -        * the general purpose registers are undefined upon CPU_ON.
> -        */
> -       reset_state->r0 = smccc_get_arg3(source_vcpu);
> -
> -       WRITE_ONCE(reset_state->reset, true);
> -       kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
> -
> -       /*
> -        * Make sure the reset request is observed if the change to
> -        * power_state is observed.
> -        */
> -       smp_wmb();
> -
> -       vcpu->arch.power_off = false;
> +       kvm_psci_vcpu_request_reset(vcpu, smccc_get_arg2(source_vcpu),
> +                                   smccc_get_arg3(source_vcpu),
> +                                   kvm_vcpu_is_be(source_vcpu));
>         kvm_vcpu_wake_up(vcpu);
>
>         return PSCI_RET_SUCCESS;
> --
> 2.33.0.685.g46640cef36-goog

Reviewed-by: Reiji Watanabe <reijiw@google.com>

Not directly related to the patch, but the (original) code doesn't
do any sanity checking for the entry address although the PSCI spec says:

"INVALID_ADDRESS is returned when the entry point address is known
by the implementation to be invalid, because it is in a range that
is known not to be available to the caller."


Thanks,
Reiji

Re: [PATCH v2 05/11] KVM: arm64: Defer WFI emulation as a requested event

[Marc Zyngier]

On Thu, 30 Sep 2021 18:09:07 +0100,
Sean Christopherson <seanjc@google.com> wrote:
> 
> On Thu, Sep 30, 2021, Marc Zyngier wrote:
> > On Thu, 23 Sep 2021 20:16:04 +0100, Oliver Upton <oupton@google.com> wrote:
> > > @@ -681,6 +687,9 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
> > >  		if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
> > >  			kvm_vcpu_sleep(vcpu);
> > >  
> > > +		if (kvm_check_request(KVM_REQ_SUSPEND, vcpu))
> > > +			kvm_vcpu_suspend(vcpu);
> > > +
> 
> ...
> 
> > > diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> > > index 275a27368a04..5e5ef9ff4fba 100644
> > > --- a/arch/arm64/kvm/handle_exit.c
> > > +++ b/arch/arm64/kvm/handle_exit.c
> > > @@ -95,8 +95,7 @@ static int kvm_handle_wfx(struct kvm_vcpu *vcpu)
> > >  	} else {
> > >  		trace_kvm_wfx_arm64(*vcpu_pc(vcpu), false);
> > >  		vcpu->stat.wfi_exit_stat++;
> > > -		kvm_vcpu_block(vcpu);
> > > -		kvm_clear_request(KVM_REQ_UNHALT, vcpu);
> > > +		kvm_make_request(KVM_REQ_SUSPEND, vcpu);
> > >  	}
> > >  
> > >  	kvm_incr_pc(vcpu);
> > 
> > This is a change in behaviour. At the point where the blocking
> > happens, PC will have already been incremented. I'd rather you don't
> > do that. Instead, make the helper available and call into it directly,
> > preserving the current semantics.
> 
> Is there architectural behavior that KVM can emulate?  E.g. if you
> were to probe a physical CPU while it's waiting, would you observe
> the pre-WFI PC, or the post-WFI PC?  Following arch behavior would
> be ideal because it eliminates subjectivity.

The architecture doesn't really say (that's one of the many IMPDEF
behaviours). However, there are at least some extensions (such as
statistical profiling) that do require the PC to be accurately
recorded together with the effects of the instructions at that PC.

If an implementation was to pre-increment the PC, the corresponding
trace would be inaccurate.

> Regardless of the architectural behavior, changing KVM's behavior
> should be done explicitly in a separate patch.
>
> Irrespective of PC behavior, I would caution against using a request
> for handling WFI.  Deferring the WFI opens up the possibility for
> all sorts of ordering oddities, e.g. if KVM exits to userspace
> between here and check_vcpu_requests(), then KVM can end up with a
> "spurious" pending KVM_REQ_SUSPEND if maniupaltes vCPU state.  I
> highly doubt that userspace VMMs would actually do that, as it would
> basically require a signal from userspace, but it's not impossible,
> and at the very least the pending request is yet another thing to
> worry about in the future.

+1. It really isn't worth the complexity.

Thanks,

	M.

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

Re: [PATCH v2 06/11] KVM: arm64: Add support for SYSTEM_SUSPEND PSCI call

[Marc Zyngier]

On Thu, 30 Sep 2021 18:40:43 +0100,
Oliver Upton <oupton@google.com> wrote:
> 
> On Thu, Sep 30, 2021 at 5:29 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > I think there is an additional feature we need, which is to give
> > control back to userspace every time a wake-up condition occurs (I did
> > touch on that in [1]). This would give the opportunity to the VMM to
> > do whatever it needs to perform.
> >
> > A typical use case would be to implement wake-up from certain
> > interrupts only (mask non-wake-up IRQs on suspend request, return to
> > the guest for WFI, wake-up returns to the VMM to restore the previous
> > interrupt configuration, resume).
> >
> > Userspace would be free to clear the suspend state and resume the
> > guest, or to reenter WFI.
> 
> Yeah, this is definitely needed for the series.
> 
> Just to make sure it's clear, what should the default behavior be if
> userspace doesn't touch state at all and it calls KVM_RUN again? It
> would seem to me that we should resume the guest by default, much like
> we would do for the SUSPEND event exit.

My mental model is that the suspend state is "sticky". Once set, it
stays and the guest doesn't execute any instruction until cleared by
the VMM.

It has the advantage that the VMM always knows the state the vcpu is
in, because that's what it asked for, and the vcpu can't change state
on its own.

It means that the VMM would have to set the vcpu state to 'RUNNABLE'
once it is satisfied that it can be run.

Thanks,

	M.

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

Re: [PATCH v2 03/11] KVM: arm64: Encapsulate reset request logic in a helper function

[Oliver Upton]

On Thu, Sep 30, 2021 at 11:05 PM Reiji Watanabe <reijiw@google.com> wrote:
>
> On Thu, Sep 23, 2021 at 12:16 PM Oliver Upton <oupton@google.com> wrote:
> >
> > In its implementation of the PSCI function, KVM needs to request that a
> > target vCPU resets before its next entry into the guest. Wrap the logic
> > for requesting a reset in a function for later use by other implemented
> > PSCI calls.
> >
> > No functional change intended.
> >
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >  arch/arm64/kvm/psci.c | 59 +++++++++++++++++++++++++------------------
> >  1 file changed, 35 insertions(+), 24 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/psci.c b/arch/arm64/kvm/psci.c
> > index 310b9cb2b32b..bb59b692998b 100644
> > --- a/arch/arm64/kvm/psci.c
> > +++ b/arch/arm64/kvm/psci.c
> > @@ -64,9 +64,40 @@ static inline bool kvm_psci_valid_affinity(unsigned long affinity)
> >         return !(affinity & ~MPIDR_HWID_BITMASK);
> >  }
> >
> > -static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> > +static void kvm_psci_vcpu_request_reset(struct kvm_vcpu *vcpu,
> > +                                       unsigned long entry_addr,
> > +                                       unsigned long context_id,
> > +                                       bool big_endian)
> >  {
> >         struct vcpu_reset_state *reset_state;
> > +
> > +       lockdep_assert_held(&vcpu->kvm->lock);
> > +
> > +       reset_state = &vcpu->arch.reset_state;
> > +       reset_state->pc = entry_addr;
> > +
> > +       /* Propagate caller endianness */
> > +       reset_state->be = big_endian;
> > +
> > +       /*
> > +        * NOTE: We always update r0 (or x0) because for PSCI v0.1
> > +        * the general purpose registers are undefined upon CPU_ON.
> > +        */
> > +       reset_state->r0 = context_id;
> > +
> > +       WRITE_ONCE(reset_state->reset, true);
> > +       kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
> > +
> > +       /*
> > +        * Make sure the reset request is observed if the change to
> > +        * power_state is observed.
> > +        */
> > +       smp_wmb();
> > +       vcpu->arch.power_off = false;
> > +}
> > +
> > +static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> > +{
> >         struct kvm *kvm = source_vcpu->kvm;
> >         struct kvm_vcpu *vcpu = NULL;
> >         unsigned long cpu_id;
> > @@ -90,29 +121,9 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> >                         return PSCI_RET_INVALID_PARAMS;
> >         }
> >
> > -       reset_state = &vcpu->arch.reset_state;
> > -
> > -       reset_state->pc = smccc_get_arg2(source_vcpu);
> > -
> > -       /* Propagate caller endianness */
> > -       reset_state->be = kvm_vcpu_is_be(source_vcpu);
> > -
> > -       /*
> > -        * NOTE: We always update r0 (or x0) because for PSCI v0.1
> > -        * the general purpose registers are undefined upon CPU_ON.
> > -        */
> > -       reset_state->r0 = smccc_get_arg3(source_vcpu);
> > -
> > -       WRITE_ONCE(reset_state->reset, true);
> > -       kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
> > -
> > -       /*
> > -        * Make sure the reset request is observed if the change to
> > -        * power_state is observed.
> > -        */
> > -       smp_wmb();
> > -
> > -       vcpu->arch.power_off = false;
> > +       kvm_psci_vcpu_request_reset(vcpu, smccc_get_arg2(source_vcpu),
> > +                                   smccc_get_arg3(source_vcpu),
> > +                                   kvm_vcpu_is_be(source_vcpu));
> >         kvm_vcpu_wake_up(vcpu);
> >
> >         return PSCI_RET_SUCCESS;
> > --
> > 2.33.0.685.g46640cef36-goog
>
> Reviewed-by: Reiji Watanabe <reijiw@google.com>
>
> Not directly related to the patch, but the (original) code doesn't
> do any sanity checking for the entry address although the PSCI spec says:
>
> "INVALID_ADDRESS is returned when the entry point address is known
> by the implementation to be invalid, because it is in a range that
> is known not to be available to the caller."

Right, I had noticed the same but was a tad too lazy to address in
this series :) Thanks for the review, Reji!

--
Best,
Oliver

Re: [PATCH v2 01/11] KVM: arm64: Drop unused vcpu param to kvm_psci_valid_affinity()

[Andrew Jones]

On Thu, Sep 23, 2021 at 07:16:00PM +0000, Oliver Upton wrote:
> The helper function does not need a pointer to the vCPU, as it only
> consults a constant mask; drop the unused vcpu parameter.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/kvm/psci.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kvm/psci.c b/arch/arm64/kvm/psci.c
> index 74c47d420253..d46842f45b0a 100644
> --- a/arch/arm64/kvm/psci.c
> +++ b/arch/arm64/kvm/psci.c
> @@ -59,8 +59,7 @@ static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu)
>  	kvm_vcpu_kick(vcpu);
>  }
>  
> -static inline bool kvm_psci_valid_affinity(struct kvm_vcpu *vcpu,
> -					   unsigned long affinity)
> +static inline bool kvm_psci_valid_affinity(unsigned long affinity)
>  {
>  	return !(affinity & ~MPIDR_HWID_BITMASK);
>  }
> @@ -73,7 +72,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>  	unsigned long cpu_id;
>  
>  	cpu_id = smccc_get_arg1(source_vcpu);
> -	if (!kvm_psci_valid_affinity(source_vcpu, cpu_id))
> +	if (!kvm_psci_valid_affinity(cpu_id))
>  		return PSCI_RET_INVALID_PARAMS;
>  
>  	vcpu = kvm_mpidr_to_vcpu(kvm, cpu_id);
> @@ -132,7 +131,7 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu)
>  	target_affinity = smccc_get_arg1(vcpu);
>  	lowest_affinity_level = smccc_get_arg2(vcpu);
>  
> -	if (!kvm_psci_valid_affinity(vcpu, target_affinity))
> +	if (!kvm_psci_valid_affinity(target_affinity))
>  		return PSCI_RET_INVALID_PARAMS;
>  
>  	/* Determine target affinity mask */
> -- 
> 2.33.0.685.g46640cef36-goog
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [PATCH v2 02/11] KVM: arm64: Clean up SMC64 PSCI filtering for AArch32 guests

[Andrew Jones]

On Thu, Sep 23, 2021 at 07:16:01PM +0000, Oliver Upton wrote:
> The only valid calling SMC calling convention from an AArch32 state is
> SMC32. Disallow any PSCI function that sets the SMC64 function ID bit
> when called from AArch32 rather than comparing against known SMC64 PSCI
> functions.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/kvm/psci.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/kvm/psci.c b/arch/arm64/kvm/psci.c
> index d46842f45b0a..310b9cb2b32b 100644
> --- a/arch/arm64/kvm/psci.c
> +++ b/arch/arm64/kvm/psci.c
> @@ -208,15 +208,11 @@ static void kvm_psci_narrow_to_32bit(struct kvm_vcpu *vcpu)
>  
>  static unsigned long kvm_psci_check_allowed_function(struct kvm_vcpu *vcpu, u32 fn)
>  {
> -	switch(fn) {
> -	case PSCI_0_2_FN64_CPU_SUSPEND:
> -	case PSCI_0_2_FN64_CPU_ON:
> -	case PSCI_0_2_FN64_AFFINITY_INFO:
> -		/* Disallow these functions for 32bit guests */
> -		if (vcpu_mode_is_32bit(vcpu))
> -			return PSCI_RET_NOT_SUPPORTED;
> -		break;
> -	}
> +	/*
> +	 * Prevent 32 bit guests from calling 64 bit PSCI functions.
> +	 */
> +	if ((fn & PSCI_0_2_64BIT) && vcpu_mode_is_32bit(vcpu))
> +		return PSCI_RET_NOT_SUPPORTED;
>  
>  	return 0;
>  }
> -- 
> 2.33.0.685.g46640cef36-goog
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [PATCH v2 03/11] KVM: arm64: Encapsulate reset request logic in a helper function

[Andrew Jones]

On Fri, Oct 01, 2021 at 09:10:14AM -0700, Oliver Upton wrote:
> On Thu, Sep 30, 2021 at 11:05 PM Reiji Watanabe <reijiw@google.com> wrote:
> >
> > On Thu, Sep 23, 2021 at 12:16 PM Oliver Upton <oupton@google.com> wrote:
> > >
> > > In its implementation of the PSCI function, KVM needs to request that a
> > > target vCPU resets before its next entry into the guest. Wrap the logic
> > > for requesting a reset in a function for later use by other implemented
> > > PSCI calls.
> > >
> > > No functional change intended.
> > >
> > > Signed-off-by: Oliver Upton <oupton@google.com>
> > > ---
> > >  arch/arm64/kvm/psci.c | 59 +++++++++++++++++++++++++------------------
> > >  1 file changed, 35 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/arch/arm64/kvm/psci.c b/arch/arm64/kvm/psci.c
> > > index 310b9cb2b32b..bb59b692998b 100644
> > > --- a/arch/arm64/kvm/psci.c
> > > +++ b/arch/arm64/kvm/psci.c
> > > @@ -64,9 +64,40 @@ static inline bool kvm_psci_valid_affinity(unsigned long affinity)
> > >         return !(affinity & ~MPIDR_HWID_BITMASK);
> > >  }
> > >
> > > -static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> > > +static void kvm_psci_vcpu_request_reset(struct kvm_vcpu *vcpu,
> > > +                                       unsigned long entry_addr,
> > > +                                       unsigned long context_id,
> > > +                                       bool big_endian)
> > >  {
> > >         struct vcpu_reset_state *reset_state;
> > > +
> > > +       lockdep_assert_held(&vcpu->kvm->lock);
> > > +
> > > +       reset_state = &vcpu->arch.reset_state;
> > > +       reset_state->pc = entry_addr;
> > > +
> > > +       /* Propagate caller endianness */
> > > +       reset_state->be = big_endian;
> > > +
> > > +       /*
> > > +        * NOTE: We always update r0 (or x0) because for PSCI v0.1
> > > +        * the general purpose registers are undefined upon CPU_ON.
> > > +        */
> > > +       reset_state->r0 = context_id;
> > > +
> > > +       WRITE_ONCE(reset_state->reset, true);
> > > +       kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
> > > +
> > > +       /*
> > > +        * Make sure the reset request is observed if the change to
> > > +        * power_state is observed.
> > > +        */
> > > +       smp_wmb();
> > > +       vcpu->arch.power_off = false;
> > > +}
> > > +
> > > +static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> > > +{
> > >         struct kvm *kvm = source_vcpu->kvm;
> > >         struct kvm_vcpu *vcpu = NULL;
> > >         unsigned long cpu_id;
> > > @@ -90,29 +121,9 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> > >                         return PSCI_RET_INVALID_PARAMS;
> > >         }
> > >
> > > -       reset_state = &vcpu->arch.reset_state;
> > > -
> > > -       reset_state->pc = smccc_get_arg2(source_vcpu);
> > > -
> > > -       /* Propagate caller endianness */
> > > -       reset_state->be = kvm_vcpu_is_be(source_vcpu);
> > > -
> > > -       /*
> > > -        * NOTE: We always update r0 (or x0) because for PSCI v0.1
> > > -        * the general purpose registers are undefined upon CPU_ON.
> > > -        */
> > > -       reset_state->r0 = smccc_get_arg3(source_vcpu);
> > > -
> > > -       WRITE_ONCE(reset_state->reset, true);
> > > -       kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
> > > -
> > > -       /*
> > > -        * Make sure the reset request is observed if the change to
> > > -        * power_state is observed.
> > > -        */
> > > -       smp_wmb();
> > > -
> > > -       vcpu->arch.power_off = false;
> > > +       kvm_psci_vcpu_request_reset(vcpu, smccc_get_arg2(source_vcpu),
> > > +                                   smccc_get_arg3(source_vcpu),
> > > +                                   kvm_vcpu_is_be(source_vcpu));
> > >         kvm_vcpu_wake_up(vcpu);
> > >
> > >         return PSCI_RET_SUCCESS;
> > > --
> > > 2.33.0.685.g46640cef36-goog
> >
> > Reviewed-by: Reiji Watanabe <reijiw@google.com>
> >
> > Not directly related to the patch, but the (original) code doesn't
> > do any sanity checking for the entry address although the PSCI spec says:
> >
> > "INVALID_ADDRESS is returned when the entry point address is known
> > by the implementation to be invalid, because it is in a range that
> > is known not to be available to the caller."
> 
> Right, I had noticed the same but was a tad too lazy to address in
> this series :) Thanks for the review, Reji!
>

KVM doesn't reserve any subrange within [0 - max_ipa), afaik. So all
we need to do is check 'entry_addr < max_ipa', right?

Thanks,
drew

Re: [PATCH v2 04/11] KVM: arm64: Rename the KVM_REQ_SLEEP handler

[Andrew Jones]

On Thu, Sep 23, 2021 at 07:16:03PM +0000, Oliver Upton wrote:
> The naming of the kvm_req_sleep function is confusing: the function
> itself sleeps the vCPU, it does not request such an event. Rename the
> function to make its purpose more clear.
> 
> No functional change intended.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/kvm/arm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index fe102cd2e518..3d4acd354f94 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -649,7 +649,7 @@ void kvm_arm_resume_guest(struct kvm *kvm)
>  	}
>  }
>  
> -static void vcpu_req_sleep(struct kvm_vcpu *vcpu)
> +static void kvm_vcpu_sleep(struct kvm_vcpu *vcpu)
>  {
>  	struct rcuwait *wait = kvm_arch_vcpu_get_wait(vcpu);
>  
> @@ -679,7 +679,7 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
>  {
>  	if (kvm_request_pending(vcpu)) {
>  		if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
> -			vcpu_req_sleep(vcpu);
> +			kvm_vcpu_sleep(vcpu);
>  
>  		if (kvm_check_request(KVM_REQ_VCPU_RESET, vcpu))
>  			kvm_reset_vcpu(vcpu);
> -- 
> 2.33.0.685.g46640cef36-goog
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [PATCH v2 03/11] KVM: arm64: Encapsulate reset request logic in a helper function

[Andrew Jones]

On Thu, Sep 23, 2021 at 07:16:02PM +0000, Oliver Upton wrote:
> In its implementation of the PSCI function, KVM needs to request that a
> target vCPU resets before its next entry into the guest. Wrap the logic
> for requesting a reset in a function for later use by other implemented
> PSCI calls.
> 
> No functional change intended.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/kvm/psci.c | 59 +++++++++++++++++++++++++------------------
>  1 file changed, 35 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/arm64/kvm/psci.c b/arch/arm64/kvm/psci.c
> index 310b9cb2b32b..bb59b692998b 100644
> --- a/arch/arm64/kvm/psci.c
> +++ b/arch/arm64/kvm/psci.c
> @@ -64,9 +64,40 @@ static inline bool kvm_psci_valid_affinity(unsigned long affinity)
>  	return !(affinity & ~MPIDR_HWID_BITMASK);
>  }
>  
> -static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> +static void kvm_psci_vcpu_request_reset(struct kvm_vcpu *vcpu,
> +					unsigned long entry_addr,
> +					unsigned long context_id,
> +					bool big_endian)
>  {
>  	struct vcpu_reset_state *reset_state;
> +
> +	lockdep_assert_held(&vcpu->kvm->lock);
> +
> +	reset_state = &vcpu->arch.reset_state;
> +	reset_state->pc = entry_addr;
> +
> +	/* Propagate caller endianness */
> +	reset_state->be = big_endian;
> +
> +	/*
> +	 * NOTE: We always update r0 (or x0) because for PSCI v0.1
> +	 * the general purpose registers are undefined upon CPU_ON.
> +	 */
> +	reset_state->r0 = context_id;
> +
> +	WRITE_ONCE(reset_state->reset, true);
> +	kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
> +
> +	/*
> +	 * Make sure the reset request is observed if the change to
> +	 * power_state is observed.
> +	 */
> +	smp_wmb();
> +	vcpu->arch.power_off = false;
> +}
> +
> +static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> +{
>  	struct kvm *kvm = source_vcpu->kvm;
>  	struct kvm_vcpu *vcpu = NULL;
>  	unsigned long cpu_id;
> @@ -90,29 +121,9 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>  			return PSCI_RET_INVALID_PARAMS;
>  	}
>  
> -	reset_state = &vcpu->arch.reset_state;
> -
> -	reset_state->pc = smccc_get_arg2(source_vcpu);
> -
> -	/* Propagate caller endianness */
> -	reset_state->be = kvm_vcpu_is_be(source_vcpu);
> -
> -	/*
> -	 * NOTE: We always update r0 (or x0) because for PSCI v0.1
> -	 * the general purpose registers are undefined upon CPU_ON.
> -	 */
> -	reset_state->r0 = smccc_get_arg3(source_vcpu);
> -
> -	WRITE_ONCE(reset_state->reset, true);
> -	kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
> -
> -	/*
> -	 * Make sure the reset request is observed if the change to
> -	 * power_state is observed.
> -	 */
> -	smp_wmb();
> -
> -	vcpu->arch.power_off = false;
> +	kvm_psci_vcpu_request_reset(vcpu, smccc_get_arg2(source_vcpu),
> +				    smccc_get_arg3(source_vcpu),
> +				    kvm_vcpu_is_be(source_vcpu));
>  	kvm_vcpu_wake_up(vcpu);
>  
>  	return PSCI_RET_SUCCESS;
> -- 
> 2.33.0.685.g46640cef36-goog
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [PATCH v2 07/11] selftests: KVM: Rename psci_cpu_on_test to psci_test

[Andrew Jones]

On Thu, Sep 23, 2021 at 07:16:06PM +0000, Oliver Upton wrote:
> There are other interactions with PSCI worth testing; rename the PSCI
> test to make it more generic.
> 
> No functional change intended.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  tools/testing/selftests/kvm/.gitignore                          | 2 +-
>  tools/testing/selftests/kvm/Makefile                            | 2 +-
>  .../selftests/kvm/aarch64/{psci_cpu_on_test.c => psci_test.c}   | 0
>  3 files changed, 2 insertions(+), 2 deletions(-)
>  rename tools/testing/selftests/kvm/aarch64/{psci_cpu_on_test.c => psci_test.c} (100%)
> 
> diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
> index 98053d3afbda..a11b89be744b 100644
> --- a/tools/testing/selftests/kvm/.gitignore
> +++ b/tools/testing/selftests/kvm/.gitignore
> @@ -1,7 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  /aarch64/debug-exceptions
>  /aarch64/get-reg-list
> -/aarch64/psci_cpu_on_test
> +/aarch64/psci_test
>  /aarch64/vgic_init
>  /s390x/memop
>  /s390x/resets
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index 5d05801ab816..6907ee8f3239 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -86,7 +86,7 @@ TEST_GEN_PROGS_x86_64 += kvm_binary_stats_test
>  
>  TEST_GEN_PROGS_aarch64 += aarch64/debug-exceptions
>  TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list
> -TEST_GEN_PROGS_aarch64 += aarch64/psci_cpu_on_test
> +TEST_GEN_PROGS_aarch64 += aarch64/psci_test
>  TEST_GEN_PROGS_aarch64 += aarch64/vgic_init
>  TEST_GEN_PROGS_aarch64 += demand_paging_test
>  TEST_GEN_PROGS_aarch64 += dirty_log_test
> diff --git a/tools/testing/selftests/kvm/aarch64/psci_cpu_on_test.c b/tools/testing/selftests/kvm/aarch64/psci_test.c
> similarity index 100%
> rename from tools/testing/selftests/kvm/aarch64/psci_cpu_on_test.c
> rename to tools/testing/selftests/kvm/aarch64/psci_test.c
> -- 
> 2.33.0.685.g46640cef36-goog
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [PATCH v2 08/11] selftests: KVM: Create helper for making SMCCC calls

[Andrew Jones]

On Thu, Sep 23, 2021 at 07:16:07PM +0000, Oliver Upton wrote:
> The PSCI and PV stolen time tests both need to make SMCCC calls within
> the guest. Create a helper for making SMCCC calls and rework the
> existing tests to use the library function.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  .../testing/selftests/kvm/aarch64/psci_test.c | 25 ++++++-------------
>  .../selftests/kvm/include/aarch64/processor.h | 22 ++++++++++++++++
>  .../selftests/kvm/lib/aarch64/processor.c     | 25 +++++++++++++++++++
>  tools/testing/selftests/kvm/steal_time.c      | 13 +++-------
>  4 files changed, 58 insertions(+), 27 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/aarch64/psci_test.c b/tools/testing/selftests/kvm/aarch64/psci_test.c
> index 018c269990e1..cebea7356e5a 100644
> --- a/tools/testing/selftests/kvm/aarch64/psci_test.c
> +++ b/tools/testing/selftests/kvm/aarch64/psci_test.c
> @@ -26,32 +26,23 @@
>  static uint64_t psci_cpu_on(uint64_t target_cpu, uint64_t entry_addr,
>  			    uint64_t context_id)
>  {
> -	register uint64_t x0 asm("x0") = PSCI_0_2_FN64_CPU_ON;
> -	register uint64_t x1 asm("x1") = target_cpu;
> -	register uint64_t x2 asm("x2") = entry_addr;
> -	register uint64_t x3 asm("x3") = context_id;
> +	struct arm_smccc_res res;
>  
> -	asm("hvc #0"
> -	    : "=r"(x0)
> -	    : "r"(x0), "r"(x1), "r"(x2), "r"(x3)
> -	    : "memory");
> +	smccc_hvc(PSCI_0_2_FN64_CPU_ON, target_cpu, entry_addr, context_id,
> +		  0, 0, 0, 0, &res);
>  
> -	return x0;
> +	return res.a0;
>  }
>  
>  static uint64_t psci_affinity_info(uint64_t target_affinity,
>  				   uint64_t lowest_affinity_level)
>  {
> -	register uint64_t x0 asm("x0") = PSCI_0_2_FN64_AFFINITY_INFO;
> -	register uint64_t x1 asm("x1") = target_affinity;
> -	register uint64_t x2 asm("x2") = lowest_affinity_level;
> +	struct arm_smccc_res res;
>  
> -	asm("hvc #0"
> -	    : "=r"(x0)
> -	    : "r"(x0), "r"(x1), "r"(x2)
> -	    : "memory");
> +	smccc_hvc(PSCI_0_2_FN64_AFFINITY_INFO, target_affinity, lowest_affinity_level,
> +		  0, 0, 0, 0, 0, &res);
>  
> -	return x0;
> +	return res.a0;
>  }
>  
>  static void guest_main(uint64_t target_cpu)
> diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
> index c0273aefa63d..e6b7cb65d158 100644
> --- a/tools/testing/selftests/kvm/include/aarch64/processor.h
> +++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
> @@ -132,4 +132,26 @@ void vm_install_sync_handler(struct kvm_vm *vm,
>  
>  #define isb()	asm volatile("isb" : : : "memory")
>  
> +/**
> + * struct arm_smccc_res - Result from SMC/HVC call
> + * @a0-a3 result values from registers 0 to 3
> + */
> +struct arm_smccc_res {
> +	unsigned long a0;
> +	unsigned long a1;
> +	unsigned long a2;
> +	unsigned long a3;
> +};
> +
> +/**
> + * smccc_hvc - Invoke a SMCCC function using the hvc conduit
> + * @function_id: the SMCCC function to be called
> + * @arg0-arg6: SMCCC function arguments, corresponding to registers x1-x7
> + * @res: pointer to write the return values from registers x0-x3
> + *
> + */
> +void smccc_hvc(uint32_t function_id, uint64_t arg0, uint64_t arg1,
> +	       uint64_t arg2, uint64_t arg3, uint64_t arg4, uint64_t arg5,
> +	       uint64_t arg6, struct arm_smccc_res *res);
> +
>  #endif /* SELFTEST_KVM_PROCESSOR_H */
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> index 632b74d6b3ca..f77430e2d688 100644
> --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> @@ -426,3 +426,28 @@ void vm_install_exception_handler(struct kvm_vm *vm, int vector,
>  	assert(vector < VECTOR_NUM);
>  	handlers->exception_handlers[vector][0] = handler;
>  }
> +
> +void smccc_hvc(uint32_t function_id, uint64_t arg0, uint64_t arg1,
> +	       uint64_t arg2, uint64_t arg3, uint64_t arg4, uint64_t arg5,
> +	       uint64_t arg6, struct arm_smccc_res *res)
> +{
> +	asm volatile("mov   w0, %w[function_id]\n"
> +		     "mov   x1, %[arg0]\n"
> +		     "mov   x2, %[arg1]\n"
> +		     "mov   x3, %[arg2]\n"
> +		     "mov   x4, %[arg3]\n"
> +		     "mov   x5, %[arg4]\n"
> +		     "mov   x6, %[arg5]\n"
> +		     "mov   x7, %[arg6]\n"
> +		     "hvc   #0\n"
> +		     "mov   %[res0], x0\n"
> +		     "mov   %[res1], x1\n"
> +		     "mov   %[res2], x2\n"
> +		     "mov   %[res3], x3\n"
> +		     : [res0] "=r"(res->a0), [res1] "=r"(res->a1),
> +		       [res2] "=r"(res->a2), [res3] "=r"(res->a3)
> +		     : [function_id] "r"(function_id), [arg0] "r"(arg0),
> +		       [arg1] "r"(arg1), [arg2] "r"(arg2), [arg3] "r"(arg3),
> +		       [arg4] "r"(arg4), [arg5] "r"(arg5), [arg6] "r"(arg6)
> +		     : "x0", "x1", "x2", "x3", "x4", "x5", "x6", "x7");
> +}
> diff --git a/tools/testing/selftests/kvm/steal_time.c b/tools/testing/selftests/kvm/steal_time.c
> index ecec30865a74..5d52b82226c5 100644
> --- a/tools/testing/selftests/kvm/steal_time.c
> +++ b/tools/testing/selftests/kvm/steal_time.c
> @@ -120,17 +120,10 @@ struct st_time {
>  
>  static int64_t smccc(uint32_t func, uint32_t arg)
>  {
> -	unsigned long ret;
> +	struct arm_smccc_res res;
>  
> -	asm volatile(
> -		"mov	x0, %1\n"
> -		"mov	x1, %2\n"
> -		"hvc	#0\n"
> -		"mov	%0, x0\n"
> -	: "=r" (ret) : "r" (func), "r" (arg) :
> -	  "x0", "x1", "x2", "x3");
> -
> -	return ret;
> +	smccc_hvc(func, arg, 0, 0, 0, 0, 0, 0, &res);
> +	return res.a0;
>  }
>  
>  static void check_status(struct st_time *st)
> -- 
> 2.33.0.685.g46640cef36-goog
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [PATCH v2 10/11] selftests: KVM: Refactor psci_test to make it amenable to new tests

[Andrew Jones]

On Thu, Sep 23, 2021 at 07:16:09PM +0000, Oliver Upton wrote:
> Split up the current test into several helpers that will be useful to
> subsequent test cases added to the PSCI test suite.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  .../testing/selftests/kvm/aarch64/psci_test.c | 68 ++++++++++++-------
>  1 file changed, 45 insertions(+), 23 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/aarch64/psci_test.c b/tools/testing/selftests/kvm/aarch64/psci_test.c
> index 8d043e12b137..90312be335da 100644
> --- a/tools/testing/selftests/kvm/aarch64/psci_test.c
> +++ b/tools/testing/selftests/kvm/aarch64/psci_test.c
> @@ -45,7 +45,7 @@ static uint64_t psci_affinity_info(uint64_t target_affinity,
>  	return res.a0;
>  }
>  
> -static void guest_main(uint64_t target_cpu)
> +static void guest_test_cpu_on(uint64_t target_cpu)
>  {
>  	GUEST_ASSERT(!psci_cpu_on(target_cpu, CPU_ON_ENTRY_ADDR, CPU_ON_CONTEXT_ID));
>  	uint64_t target_state;
> @@ -69,12 +69,10 @@ static void vcpu_power_off(struct kvm_vm *vm, uint32_t vcpuid)
>  	vcpu_set_mp_state(vm, vcpuid, &mp_state);
>  }
>  
> -int main(void)
> +static struct kvm_vm *setup_vm(void *guest_code)
>  {
> -	uint64_t target_mpidr, obs_pc, obs_x0;
>  	struct kvm_vcpu_init init;
>  	struct kvm_vm *vm;
> -	struct ucall uc;
>  
>  	vm = vm_create(VM_MODE_DEFAULT, DEFAULT_GUEST_PHY_PAGES, O_RDWR);
>  	kvm_vm_elf_load(vm, program_invocation_name);
> @@ -83,31 +81,28 @@ int main(void)
>  	vm_ioctl(vm, KVM_ARM_PREFERRED_TARGET, &init);
>  	init.features[0] |= (1 << KVM_ARM_VCPU_PSCI_0_2);
>  
> -	aarch64_vcpu_add_default(vm, VCPU_ID_SOURCE, &init, guest_main);
> -	aarch64_vcpu_add_default(vm, VCPU_ID_TARGET, &init, guest_main);
> +	aarch64_vcpu_add_default(vm, VCPU_ID_SOURCE, &init, guest_code);
> +	aarch64_vcpu_add_default(vm, VCPU_ID_TARGET, &init, guest_code);
>  
> -	/*
> -	 * make sure the target is already off when executing the test.
> -	 */
> -	vcpu_power_off(vm, VCPU_ID_TARGET);
> +	return vm;
> +}
>  
> -	get_reg(vm, VCPU_ID_TARGET, ARM64_SYS_REG(MPIDR_EL1), &target_mpidr);
> -	vcpu_args_set(vm, VCPU_ID_SOURCE, 1, target_mpidr & MPIDR_HWID_BITMASK);
> -	vcpu_run(vm, VCPU_ID_SOURCE);
> +static void enter_guest(struct kvm_vm *vm, uint32_t vcpuid)
> +{
> +	struct ucall uc;
>  
> -	switch (get_ucall(vm, VCPU_ID_SOURCE, &uc)) {
> -	case UCALL_DONE:
> -		break;
> -	case UCALL_ABORT:
> +	vcpu_run(vm, vcpuid);
> +	if (get_ucall(vm, vcpuid, &uc) == UCALL_ABORT)
>  		TEST_FAIL("%s at %s:%ld", (const char *)uc.args[0], __FILE__,
>  			  uc.args[1]);
> -		break;
> -	default:
> -		TEST_FAIL("Unhandled ucall: %lu", uc.cmd);
> -	}
> +}
>  
> -	get_reg(vm, VCPU_ID_TARGET, ARM64_CORE_REG(regs.pc), &obs_pc);
> -	get_reg(vm, VCPU_ID_TARGET, ARM64_CORE_REG(regs.regs[0]), &obs_x0);
> +static void assert_vcpu_reset(struct kvm_vm *vm, uint32_t vcpuid)
> +{
> +	uint64_t obs_pc, obs_x0;
> +
> +	get_reg(vm, vcpuid, ARM64_CORE_REG(regs.pc), &obs_pc);
> +	get_reg(vm, vcpuid, ARM64_CORE_REG(regs.regs[0]), &obs_x0);
>  
>  	TEST_ASSERT(obs_pc == CPU_ON_ENTRY_ADDR,
>  		    "unexpected target cpu pc: %lx (expected: %lx)",
> @@ -115,7 +110,34 @@ int main(void)
>  	TEST_ASSERT(obs_x0 == CPU_ON_CONTEXT_ID,
>  		    "unexpected target context id: %lx (expected: %lx)",
>  		    obs_x0, CPU_ON_CONTEXT_ID);
> +}
>  
> +static void host_test_cpu_on(void)
> +{
> +	uint64_t target_mpidr;
> +	struct kvm_vm *vm;
> +	struct ucall uc;
> +
> +	vm = setup_vm(guest_test_cpu_on);
> +
> +	/*
> +	 * make sure the target is already off when executing the test.
> +	 */
> +	vcpu_power_off(vm, VCPU_ID_TARGET);
> +
> +	get_reg(vm, VCPU_ID_TARGET, ARM64_SYS_REG(MPIDR_EL1), &target_mpidr);
> +	vcpu_args_set(vm, VCPU_ID_SOURCE, 1, target_mpidr & MPIDR_HWID_BITMASK);
> +	enter_guest(vm, VCPU_ID_SOURCE);
> +
> +	if (get_ucall(vm, VCPU_ID_SOURCE, &uc) != UCALL_DONE)
> +		TEST_FAIL("Unhandled ucall: %lu", uc.cmd);
> +
> +	assert_vcpu_reset(vm, VCPU_ID_TARGET);
>  	kvm_vm_free(vm);
> +}
> +
> +int main(void)
> +{
> +	host_test_cpu_on();
>  	return 0;
>  }
> -- 
> 2.33.0.685.g46640cef36-goog
>

Hard to read diff, but I think the refactoring comes out right. Please do
this refactoring before adding the new test in the next revision, though.

Anyway, ignoring the new test context, which I think is changing with the
next revision

Reviewed-by: Andrew Jones <drjones@redhat.com>

Thanks,
drew

Re: [PATCH v2 11/11] selftests: KVM: Test SYSTEM_SUSPEND PSCI call

[Andrew Jones]

On Thu, Sep 23, 2021 at 07:16:10PM +0000, Oliver Upton wrote:
> Assert that the vCPU exits to userspace with KVM_SYSTEM_EVENT_SUSPEND if
> it correctly executes the SYSTEM_SUSPEND PSCI call. Additionally, assert
> that the guest PSCI call fails if preconditions are not met (more than 1
> running vCPU).
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  .../testing/selftests/kvm/aarch64/psci_test.c | 75 +++++++++++++++++++
>  1 file changed, 75 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/aarch64/psci_test.c b/tools/testing/selftests/kvm/aarch64/psci_test.c
> index 90312be335da..5b881ca4d102 100644
> --- a/tools/testing/selftests/kvm/aarch64/psci_test.c
> +++ b/tools/testing/selftests/kvm/aarch64/psci_test.c
> @@ -45,6 +45,16 @@ static uint64_t psci_affinity_info(uint64_t target_affinity,
>  	return res.a0;
>  }
>  
> +static uint64_t psci_system_suspend(uint64_t entry_addr, uint64_t context_id)
> +{
> +	struct arm_smccc_res res;
> +
> +	smccc_hvc(PSCI_1_0_FN64_SYSTEM_SUSPEND, entry_addr, context_id,
> +		  0, 0, 0, 0, 0, &res);
> +
> +	return res.a0;
> +}
> +
>  static void guest_test_cpu_on(uint64_t target_cpu)
>  {
>  	GUEST_ASSERT(!psci_cpu_on(target_cpu, CPU_ON_ENTRY_ADDR, CPU_ON_CONTEXT_ID));
> @@ -69,6 +79,13 @@ static void vcpu_power_off(struct kvm_vm *vm, uint32_t vcpuid)
>  	vcpu_set_mp_state(vm, vcpuid, &mp_state);
>  }
>  
> +static void guest_test_system_suspend(void)
> +{
> +	uint64_t r = psci_system_suspend(CPU_ON_ENTRY_ADDR, CPU_ON_CONTEXT_ID);
> +
> +	GUEST_SYNC(r);
> +}
> +
>  static struct kvm_vm *setup_vm(void *guest_code)
>  {
>  	struct kvm_vcpu_init init;
> @@ -136,8 +153,66 @@ static void host_test_cpu_on(void)
>  	kvm_vm_free(vm);
>  }
>  
> +static void enable_system_suspend(struct kvm_vm *vm)
> +{
> +	struct kvm_enable_cap cap = {
> +		.cap = KVM_CAP_ARM_SYSTEM_SUSPEND,
> +	};
> +
> +	vm_enable_cap(vm, &cap);
> +}
> +
> +static void host_test_system_suspend(void)
> +{
> +	struct kvm_run *run;
> +	struct kvm_vm *vm;
> +
> +	vm = setup_vm(guest_test_system_suspend);
> +	enable_system_suspend(vm);
> +
> +	vcpu_power_off(vm, VCPU_ID_TARGET);
> +	run = vcpu_state(vm, VCPU_ID_SOURCE);
> +
> +	enter_guest(vm, VCPU_ID_SOURCE);
> +
> +	TEST_ASSERT(run->exit_reason == KVM_EXIT_SYSTEM_EVENT,
> +		    "Unhandled exit reason: %u (%s)",
> +		    run->exit_reason, exit_reason_str(run->exit_reason));
> +	TEST_ASSERT(run->system_event.type == KVM_SYSTEM_EVENT_SUSPEND,
> +		    "Unhandled system event: %u (expected: %u)",
> +		    run->system_event.type, KVM_SYSTEM_EVENT_SUSPEND);
> +
> +	assert_vcpu_reset(vm, VCPU_ID_SOURCE);
> +	kvm_vm_free(vm);
> +}
> +
> +static void host_test_system_suspend_fails(void)
> +{
> +	struct kvm_vm *vm;
> +	struct ucall uc;
> +
> +	vm = setup_vm(guest_test_system_suspend);
> +	enable_system_suspend(vm);
> +
> +	enter_guest(vm, VCPU_ID_SOURCE);
> +	TEST_ASSERT(get_ucall(vm, VCPU_ID_SOURCE, &uc) == UCALL_SYNC,
> +		    "Unhandled ucall: %lu", uc.cmd);
> +	TEST_ASSERT(uc.args[1] == PSCI_RET_DENIED,
> +		    "Unrecognized PSCI return code: %lu (expected: %u)",
> +		    uc.args[1], PSCI_RET_DENIED);
> +
> +	kvm_vm_free(vm);
> +}
> +
>  int main(void)
>  {
> +	if (!kvm_check_cap(KVM_CAP_ARM_SYSTEM_SUSPEND)) {
> +		print_skip("KVM_CAP_ARM_SYSTEM_SUSPEND not supported");
> +		exit(KSFT_SKIP);
> +	}

How about only guarding the new tests with this, so we can still do the
cpu_on test when this feature isn't present?

> +
>  	host_test_cpu_on();
> +	host_test_system_suspend();
> +	host_test_system_suspend_fails();
>  	return 0;
>  }
> -- 
> 2.33.0.685.g46640cef36-goog
> 

Thanks,
drew

Re: [PATCH v2 10/11] selftests: KVM: Refactor psci_test to make it amenable to new tests

[Oliver Upton]

Hi Drew,

On Tue, Oct 5, 2021 at 6:45 AM Andrew Jones <drjones@redhat.com> wrote:
>
> On Thu, Sep 23, 2021 at 07:16:09PM +0000, Oliver Upton wrote:
> > Split up the current test into several helpers that will be useful to
> > subsequent test cases added to the PSCI test suite.
> >
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >  .../testing/selftests/kvm/aarch64/psci_test.c | 68 ++++++++++++-------
> >  1 file changed, 45 insertions(+), 23 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/aarch64/psci_test.c b/tools/testing/selftests/kvm/aarch64/psci_test.c
> > index 8d043e12b137..90312be335da 100644
> > --- a/tools/testing/selftests/kvm/aarch64/psci_test.c
> > +++ b/tools/testing/selftests/kvm/aarch64/psci_test.c
> > @@ -45,7 +45,7 @@ static uint64_t psci_affinity_info(uint64_t target_affinity,
> >       return res.a0;
> >  }
> >
> > -static void guest_main(uint64_t target_cpu)
> > +static void guest_test_cpu_on(uint64_t target_cpu)
> >  {
> >       GUEST_ASSERT(!psci_cpu_on(target_cpu, CPU_ON_ENTRY_ADDR, CPU_ON_CONTEXT_ID));
> >       uint64_t target_state;
> > @@ -69,12 +69,10 @@ static void vcpu_power_off(struct kvm_vm *vm, uint32_t vcpuid)
> >       vcpu_set_mp_state(vm, vcpuid, &mp_state);
> >  }
> >
> > -int main(void)
> > +static struct kvm_vm *setup_vm(void *guest_code)
> >  {
> > -     uint64_t target_mpidr, obs_pc, obs_x0;
> >       struct kvm_vcpu_init init;
> >       struct kvm_vm *vm;
> > -     struct ucall uc;
> >
> >       vm = vm_create(VM_MODE_DEFAULT, DEFAULT_GUEST_PHY_PAGES, O_RDWR);
> >       kvm_vm_elf_load(vm, program_invocation_name);
> > @@ -83,31 +81,28 @@ int main(void)
> >       vm_ioctl(vm, KVM_ARM_PREFERRED_TARGET, &init);
> >       init.features[0] |= (1 << KVM_ARM_VCPU_PSCI_0_2);
> >
> > -     aarch64_vcpu_add_default(vm, VCPU_ID_SOURCE, &init, guest_main);
> > -     aarch64_vcpu_add_default(vm, VCPU_ID_TARGET, &init, guest_main);
> > +     aarch64_vcpu_add_default(vm, VCPU_ID_SOURCE, &init, guest_code);
> > +     aarch64_vcpu_add_default(vm, VCPU_ID_TARGET, &init, guest_code);
> >
> > -     /*
> > -      * make sure the target is already off when executing the test.
> > -      */
> > -     vcpu_power_off(vm, VCPU_ID_TARGET);
> > +     return vm;
> > +}
> >
> > -     get_reg(vm, VCPU_ID_TARGET, ARM64_SYS_REG(MPIDR_EL1), &target_mpidr);
> > -     vcpu_args_set(vm, VCPU_ID_SOURCE, 1, target_mpidr & MPIDR_HWID_BITMASK);
> > -     vcpu_run(vm, VCPU_ID_SOURCE);
> > +static void enter_guest(struct kvm_vm *vm, uint32_t vcpuid)
> > +{
> > +     struct ucall uc;
> >
> > -     switch (get_ucall(vm, VCPU_ID_SOURCE, &uc)) {
> > -     case UCALL_DONE:
> > -             break;
> > -     case UCALL_ABORT:
> > +     vcpu_run(vm, vcpuid);
> > +     if (get_ucall(vm, vcpuid, &uc) == UCALL_ABORT)
> >               TEST_FAIL("%s at %s:%ld", (const char *)uc.args[0], __FILE__,
> >                         uc.args[1]);
> > -             break;
> > -     default:
> > -             TEST_FAIL("Unhandled ucall: %lu", uc.cmd);
> > -     }
> > +}
> >
> > -     get_reg(vm, VCPU_ID_TARGET, ARM64_CORE_REG(regs.pc), &obs_pc);
> > -     get_reg(vm, VCPU_ID_TARGET, ARM64_CORE_REG(regs.regs[0]), &obs_x0);
> > +static void assert_vcpu_reset(struct kvm_vm *vm, uint32_t vcpuid)
> > +{
> > +     uint64_t obs_pc, obs_x0;
> > +
> > +     get_reg(vm, vcpuid, ARM64_CORE_REG(regs.pc), &obs_pc);
> > +     get_reg(vm, vcpuid, ARM64_CORE_REG(regs.regs[0]), &obs_x0);
> >
> >       TEST_ASSERT(obs_pc == CPU_ON_ENTRY_ADDR,
> >                   "unexpected target cpu pc: %lx (expected: %lx)",
> > @@ -115,7 +110,34 @@ int main(void)
> >       TEST_ASSERT(obs_x0 == CPU_ON_CONTEXT_ID,
> >                   "unexpected target context id: %lx (expected: %lx)",
> >                   obs_x0, CPU_ON_CONTEXT_ID);
> > +}
> >
> > +static void host_test_cpu_on(void)
> > +{
> > +     uint64_t target_mpidr;
> > +     struct kvm_vm *vm;
> > +     struct ucall uc;
> > +
> > +     vm = setup_vm(guest_test_cpu_on);
> > +
> > +     /*
> > +      * make sure the target is already off when executing the test.
> > +      */
> > +     vcpu_power_off(vm, VCPU_ID_TARGET);
> > +
> > +     get_reg(vm, VCPU_ID_TARGET, ARM64_SYS_REG(MPIDR_EL1), &target_mpidr);
> > +     vcpu_args_set(vm, VCPU_ID_SOURCE, 1, target_mpidr & MPIDR_HWID_BITMASK);
> > +     enter_guest(vm, VCPU_ID_SOURCE);
> > +
> > +     if (get_ucall(vm, VCPU_ID_SOURCE, &uc) != UCALL_DONE)
> > +             TEST_FAIL("Unhandled ucall: %lu", uc.cmd);
> > +
> > +     assert_vcpu_reset(vm, VCPU_ID_TARGET);
> >       kvm_vm_free(vm);
> > +}
> > +
> > +int main(void)
> > +{
> > +     host_test_cpu_on();
> >       return 0;
> >  }
> > --
> > 2.33.0.685.g46640cef36-goog
> >
>
> Hard to read diff, but I think the refactoring comes out right.

Yeah, this one's nasty, sorry about that. Thanks for parsing it out, heh.

> Please do this refactoring before adding the new test in the next revision, though.
>

This is 10/11 in the series, and the test is 11/11. I'm not seeing any
context belonging to the last patch, but perhaps I'm missing something
obvious.

> Reviewed-by: Andrew Jones <drjones@redhat.com>

Thanks!

--
Best,
Oliver

Re: [PATCH v2 03/11] KVM: arm64: Encapsulate reset request logic in a helper function

[Oliver Upton]

Hi folks,

On Tue, Oct 5, 2021 at 6:33 AM Andrew Jones <drjones@redhat.com> wrote:
>
> On Fri, Oct 01, 2021 at 09:10:14AM -0700, Oliver Upton wrote:
> > On Thu, Sep 30, 2021 at 11:05 PM Reiji Watanabe <reijiw@google.com> wrote:
> > >
> > > On Thu, Sep 23, 2021 at 12:16 PM Oliver Upton <oupton@google.com> wrote:
> > > >
> > > > In its implementation of the PSCI function, KVM needs to request that a
> > > > target vCPU resets before its next entry into the guest. Wrap the logic
> > > > for requesting a reset in a function for later use by other implemented
> > > > PSCI calls.
> > > >
> > > > No functional change intended.
> > > >
> > > > Signed-off-by: Oliver Upton <oupton@google.com>
> > > > ---
> > > >  arch/arm64/kvm/psci.c | 59 +++++++++++++++++++++++++------------------
> > > >  1 file changed, 35 insertions(+), 24 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/kvm/psci.c b/arch/arm64/kvm/psci.c
> > > > index 310b9cb2b32b..bb59b692998b 100644
> > > > --- a/arch/arm64/kvm/psci.c
> > > > +++ b/arch/arm64/kvm/psci.c
> > > > @@ -64,9 +64,40 @@ static inline bool kvm_psci_valid_affinity(unsigned long affinity)
> > > >         return !(affinity & ~MPIDR_HWID_BITMASK);
> > > >  }
> > > >
> > > > -static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> > > > +static void kvm_psci_vcpu_request_reset(struct kvm_vcpu *vcpu,
> > > > +                                       unsigned long entry_addr,
> > > > +                                       unsigned long context_id,
> > > > +                                       bool big_endian)
> > > >  {
> > > >         struct vcpu_reset_state *reset_state;
> > > > +
> > > > +       lockdep_assert_held(&vcpu->kvm->lock);
> > > > +
> > > > +       reset_state = &vcpu->arch.reset_state;
> > > > +       reset_state->pc = entry_addr;
> > > > +
> > > > +       /* Propagate caller endianness */
> > > > +       reset_state->be = big_endian;
> > > > +
> > > > +       /*
> > > > +        * NOTE: We always update r0 (or x0) because for PSCI v0.1
> > > > +        * the general purpose registers are undefined upon CPU_ON.
> > > > +        */
> > > > +       reset_state->r0 = context_id;
> > > > +
> > > > +       WRITE_ONCE(reset_state->reset, true);
> > > > +       kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
> > > > +
> > > > +       /*
> > > > +        * Make sure the reset request is observed if the change to
> > > > +        * power_state is observed.
> > > > +        */
> > > > +       smp_wmb();
> > > > +       vcpu->arch.power_off = false;
> > > > +}
> > > > +
> > > > +static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> > > > +{
> > > >         struct kvm *kvm = source_vcpu->kvm;
> > > >         struct kvm_vcpu *vcpu = NULL;
> > > >         unsigned long cpu_id;
> > > > @@ -90,29 +121,9 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> > > >                         return PSCI_RET_INVALID_PARAMS;
> > > >         }
> > > >
> > > > -       reset_state = &vcpu->arch.reset_state;
> > > > -
> > > > -       reset_state->pc = smccc_get_arg2(source_vcpu);
> > > > -
> > > > -       /* Propagate caller endianness */
> > > > -       reset_state->be = kvm_vcpu_is_be(source_vcpu);
> > > > -
> > > > -       /*
> > > > -        * NOTE: We always update r0 (or x0) because for PSCI v0.1
> > > > -        * the general purpose registers are undefined upon CPU_ON.
> > > > -        */
> > > > -       reset_state->r0 = smccc_get_arg3(source_vcpu);
> > > > -
> > > > -       WRITE_ONCE(reset_state->reset, true);
> > > > -       kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
> > > > -
> > > > -       /*
> > > > -        * Make sure the reset request is observed if the change to
> > > > -        * power_state is observed.
> > > > -        */
> > > > -       smp_wmb();
> > > > -
> > > > -       vcpu->arch.power_off = false;
> > > > +       kvm_psci_vcpu_request_reset(vcpu, smccc_get_arg2(source_vcpu),
> > > > +                                   smccc_get_arg3(source_vcpu),
> > > > +                                   kvm_vcpu_is_be(source_vcpu));
> > > >         kvm_vcpu_wake_up(vcpu);
> > > >
> > > >         return PSCI_RET_SUCCESS;
> > > > --
> > > > 2.33.0.685.g46640cef36-goog
> > >
> > > Reviewed-by: Reiji Watanabe <reijiw@google.com>
> > >
> > > Not directly related to the patch, but the (original) code doesn't
> > > do any sanity checking for the entry address although the PSCI spec says:
> > >
> > > "INVALID_ADDRESS is returned when the entry point address is known
> > > by the implementation to be invalid, because it is in a range that
> > > is known not to be available to the caller."
> >
> > Right, I had noticed the same but was a tad too lazy to address in
> > this series :) Thanks for the review, Reji!
> >
>
> KVM doesn't reserve any subrange within [0 - max_ipa), afaik. So all
> we need to do is check 'entry_addr < max_ipa', right?
>

We could be a bit more pedantic and check if the IPA exists in a
memory slot, seems like kvm_vcpu_is_visible_gfn() should do the trick.

Thoughts?

--
Thanks,
Oliver

Re: [PATCH v2 11/11] selftests: KVM: Test SYSTEM_SUSPEND PSCI call

[Oliver Upton]

Hi Drew,

On Tue, Oct 5, 2021 at 6:49 AM Andrew Jones <drjones@redhat.com> wrote:
>
> On Thu, Sep 23, 2021 at 07:16:10PM +0000, Oliver Upton wrote:
> > Assert that the vCPU exits to userspace with KVM_SYSTEM_EVENT_SUSPEND if
> > it correctly executes the SYSTEM_SUSPEND PSCI call. Additionally, assert
> > that the guest PSCI call fails if preconditions are not met (more than 1
> > running vCPU).
> >
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >  .../testing/selftests/kvm/aarch64/psci_test.c | 75 +++++++++++++++++++
> >  1 file changed, 75 insertions(+)
> >
> > diff --git a/tools/testing/selftests/kvm/aarch64/psci_test.c b/tools/testing/selftests/kvm/aarch64/psci_test.c
> > index 90312be335da..5b881ca4d102 100644
> > --- a/tools/testing/selftests/kvm/aarch64/psci_test.c
> > +++ b/tools/testing/selftests/kvm/aarch64/psci_test.c
> > @@ -45,6 +45,16 @@ static uint64_t psci_affinity_info(uint64_t target_affinity,
> >       return res.a0;
> >  }
> >
> > +static uint64_t psci_system_suspend(uint64_t entry_addr, uint64_t context_id)
> > +{
> > +     struct arm_smccc_res res;
> > +
> > +     smccc_hvc(PSCI_1_0_FN64_SYSTEM_SUSPEND, entry_addr, context_id,
> > +               0, 0, 0, 0, 0, &res);
> > +
> > +     return res.a0;
> > +}
> > +
> >  static void guest_test_cpu_on(uint64_t target_cpu)
> >  {
> >       GUEST_ASSERT(!psci_cpu_on(target_cpu, CPU_ON_ENTRY_ADDR, CPU_ON_CONTEXT_ID));
> > @@ -69,6 +79,13 @@ static void vcpu_power_off(struct kvm_vm *vm, uint32_t vcpuid)
> >       vcpu_set_mp_state(vm, vcpuid, &mp_state);
> >  }
> >
> > +static void guest_test_system_suspend(void)
> > +{
> > +     uint64_t r = psci_system_suspend(CPU_ON_ENTRY_ADDR, CPU_ON_CONTEXT_ID);
> > +
> > +     GUEST_SYNC(r);
> > +}
> > +
> >  static struct kvm_vm *setup_vm(void *guest_code)
> >  {
> >       struct kvm_vcpu_init init;
> > @@ -136,8 +153,66 @@ static void host_test_cpu_on(void)
> >       kvm_vm_free(vm);
> >  }
> >
> > +static void enable_system_suspend(struct kvm_vm *vm)
> > +{
> > +     struct kvm_enable_cap cap = {
> > +             .cap = KVM_CAP_ARM_SYSTEM_SUSPEND,
> > +     };
> > +
> > +     vm_enable_cap(vm, &cap);
> > +}
> > +
> > +static void host_test_system_suspend(void)
> > +{
> > +     struct kvm_run *run;
> > +     struct kvm_vm *vm;
> > +
> > +     vm = setup_vm(guest_test_system_suspend);
> > +     enable_system_suspend(vm);
> > +
> > +     vcpu_power_off(vm, VCPU_ID_TARGET);
> > +     run = vcpu_state(vm, VCPU_ID_SOURCE);
> > +
> > +     enter_guest(vm, VCPU_ID_SOURCE);
> > +
> > +     TEST_ASSERT(run->exit_reason == KVM_EXIT_SYSTEM_EVENT,
> > +                 "Unhandled exit reason: %u (%s)",
> > +                 run->exit_reason, exit_reason_str(run->exit_reason));
> > +     TEST_ASSERT(run->system_event.type == KVM_SYSTEM_EVENT_SUSPEND,
> > +                 "Unhandled system event: %u (expected: %u)",
> > +                 run->system_event.type, KVM_SYSTEM_EVENT_SUSPEND);
> > +
> > +     assert_vcpu_reset(vm, VCPU_ID_SOURCE);
> > +     kvm_vm_free(vm);
> > +}
> > +
> > +static void host_test_system_suspend_fails(void)
> > +{
> > +     struct kvm_vm *vm;
> > +     struct ucall uc;
> > +
> > +     vm = setup_vm(guest_test_system_suspend);
> > +     enable_system_suspend(vm);
> > +
> > +     enter_guest(vm, VCPU_ID_SOURCE);
> > +     TEST_ASSERT(get_ucall(vm, VCPU_ID_SOURCE, &uc) == UCALL_SYNC,
> > +                 "Unhandled ucall: %lu", uc.cmd);
> > +     TEST_ASSERT(uc.args[1] == PSCI_RET_DENIED,
> > +                 "Unrecognized PSCI return code: %lu (expected: %u)",
> > +                 uc.args[1], PSCI_RET_DENIED);
> > +
> > +     kvm_vm_free(vm);
> > +}
> > +
> >  int main(void)
> >  {
> > +     if (!kvm_check_cap(KVM_CAP_ARM_SYSTEM_SUSPEND)) {
> > +             print_skip("KVM_CAP_ARM_SYSTEM_SUSPEND not supported");
> > +             exit(KSFT_SKIP);
> > +     }
>
> How about only guarding the new tests with this, so we can still do the
> cpu_on test when this feature isn't present?
>

Great suggestion, thanks!

--
Best,
Oliver

Re: [PATCH v2 06/11] KVM: arm64: Add support for SYSTEM_SUSPEND PSCI call

[Oliver Upton]

Hey Marc,

On Thu, Sep 30, 2021 at 5:29 AM Marc Zyngier <maz@kernel.org> wrote:
>
> Hi Oliver,
>
> On Thu, 23 Sep 2021 20:16:05 +0100,
> Oliver Upton <oupton@google.com> wrote:
> >
> > ARM DEN0022D 5.19 "SYSTEM_SUSPEND" describes a PSCI call that may be
> > used to request a system be suspended. This is optional for PSCI v1.0
> > and to date KVM has elected to not implement the call. However, a
> > VMM/operator may wish to provide their guests with the ability to
> > suspend/resume, necessitating this PSCI call.
> >
> > Implement support for SYSTEM_SUSPEND according to the prescribed
> > behavior in the specification. Add a new system event exit type,
> > KVM_SYSTEM_EVENT_SUSPEND, to notify userspace when a VM has requested a
> > system suspend. Make KVM_MP_STATE_HALTED a valid state on arm64.
>
> KVM_MP_STATE_HALTED is a per-CPU state on x86 (it denotes HLT). Does
> it make really sense to hijack this for something that is more of a
> VM-wide state? I can see that it is tempting to do so as we're using
> the WFI semantics (which are close to HLT's, in a twisted kind of
> way), but I'm also painfully aware that gluing x86 expectations on
> arm64 rarely leads to a palatable result.
>
> > Userspace can set this to request an in-kernel emulation of the suspend.
> >
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >  Documentation/virt/kvm/api.rst    |  6 ++++
> >  arch/arm64/include/asm/kvm_host.h |  3 ++
> >  arch/arm64/kvm/arm.c              |  8 +++++
> >  arch/arm64/kvm/psci.c             | 60 +++++++++++++++++++++++++++++++
> >  include/uapi/linux/kvm.h          |  2 ++
> >  5 files changed, 79 insertions(+)
>
> This patch needs splitting. PSCI interface in one, mpstate stuff in
> another, userspace management last.
>

I agree that the MP state could be spun off into a new patch, but I
think the PSCI interface and UAPI are tightly bound. Explanation
below.

<snip>

> > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > index f1a375648e25..d875d3bcf3c5 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -101,6 +101,10 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
> >               }
> >               mutex_unlock(&kvm->lock);
> >               break;
> > +     case KVM_CAP_ARM_SYSTEM_SUSPEND:
> > +             r = 0;
> > +             kvm->arch.suspend_enabled = true;
>
> I don't really fancy adding another control here. Given that we now
> have the PSCI version being controlled by a firmware pseudo-register,
> I'd rather have that there.
>

I would generally agree, but I believe we need a default-off switch
for this new UAPI. Otherwise, I can see this change blowing up old
VMMs that are clueless about KVM_EXIT_SYSTEM_SUSPEND.

Parallel to this, it is my understanding that firmware
pseudo-registers should default to the maximum value, such that
userspace can discover what features are available on KVM and old VMMs
can provide new KVM features to its guests.

> And if we do that, I wonder whether there would be any benefit in
> bumping the PSCI version to 1.1.
>

I mean, we already do implement PSCI v1.1, we just don't pick up any
of the optional functions added to the spec. IMO, the reported PSCI
version should have some relation to the spirit of a particular
revision (such as the optional functions).

To that end, since the SYSTEM_SUSPEND PSCI call is 1.0, I didn't think
bumping the FW register was the right call.

> > +             break;
> >       default:
> >               r = -EINVAL;
> >               break;
> > @@ -215,6 +219,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> >       case KVM_CAP_SET_GUEST_DEBUG:
> >       case KVM_CAP_VCPU_ATTRIBUTES:
> >       case KVM_CAP_PTP_KVM:
> > +     case KVM_CAP_ARM_SYSTEM_SUSPEND:
> >               r = 1;
> >               break;
> >       case KVM_CAP_SET_GUEST_DEBUG2:
> > @@ -470,6 +475,9 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
> >       int ret = 0;
> >
> >       switch (mp_state->mp_state) {
> > +     case KVM_MP_STATE_HALTED:
> > +             kvm_make_request(KVM_REQ_SUSPEND, vcpu);
> > +             fallthrough;
> >       case KVM_MP_STATE_RUNNABLE:
> >               vcpu->arch.power_off = false;
> >               break;
>
> > diff --git a/arch/arm64/kvm/psci.c b/arch/arm64/kvm/psci.c
> > index d453666ddb83..cf869f1f8615 100644
> > --- a/arch/arm64/kvm/psci.c
> > +++ b/arch/arm64/kvm/psci.c
> > @@ -203,6 +203,46 @@ static void kvm_psci_system_reset(struct kvm_vcpu *vcpu)
> >       kvm_prepare_system_event(vcpu, KVM_SYSTEM_EVENT_RESET);
> >  }
> >
> > +static int kvm_psci_system_suspend(struct kvm_vcpu *vcpu)
> > +{
> > +     unsigned long entry_addr, context_id;
> > +     struct kvm *kvm = vcpu->kvm;
> > +     unsigned long psci_ret = 0;
> > +     struct kvm_vcpu *tmp;
> > +     int ret = 0;
> > +     int i;
> > +
> > +     /*
> > +      * The SYSTEM_SUSPEND PSCI call requires that all vCPUs (except the
> > +      * calling vCPU) be in an OFF state, as determined by the
> > +      * implementation.
> > +      *
> > +      * See ARM DEN0022D, 5.19 "SYSTEM_SUSPEND" for more details.
> > +      */
> > +     mutex_lock(&kvm->lock);
> > +     kvm_for_each_vcpu(i, tmp, kvm) {
> > +             if (tmp != vcpu && !tmp->arch.power_off) {
> > +                     psci_ret = PSCI_RET_DENIED;
> > +                     ret = 1;
> > +                     goto out;
> > +             }
> > +     }
> > +
> > +     entry_addr = smccc_get_arg1(vcpu);
> > +     context_id = smccc_get_arg2(vcpu);
> > +
> > +     kvm_psci_vcpu_request_reset(vcpu, entry_addr, context_id,
> > +                                 kvm_vcpu_is_be(vcpu));
>
> So even if userspace doesn't want to honor the suspend request, the
> CPU ends up resetting? That's not wrong, but maybe a bit surprising.
>

I think it's the only consistent state that we can really put the vCPU
in. If, by default, we refuse the guest's request (returning
INTERNAL_ERROR), then we must also stash the reset context for later
use on the next KVM_RUN if userspace honors the guest. Then we are in
the weird state where vcpu->arch.reset_state is valid, but
KVM_REQ_RESET is not set. In this case, if userspace were to save the
vCPU, the reset context is lost, missing the check from commit
6826c6849b46 ("KVM: arm64: Handle PSCI resets before userspace touches
vCPU state").

Sorry if this is a bit too drawn out, but wanted to share the thought
process behind it in case you had any other ideas.

--
Thanks,
Oliver

Re: [PATCH v2 03/11] KVM: arm64: Encapsulate reset request logic in a helper function

[Andrew Jones]

On Tue, Oct 05, 2021 at 08:05:02AM -0700, Oliver Upton wrote:
> Hi folks,
> 
> On Tue, Oct 5, 2021 at 6:33 AM Andrew Jones <drjones@redhat.com> wrote:
> >
> > On Fri, Oct 01, 2021 at 09:10:14AM -0700, Oliver Upton wrote:
> > > On Thu, Sep 30, 2021 at 11:05 PM Reiji Watanabe <reijiw@google.com> wrote:
> > > >
> > > > On Thu, Sep 23, 2021 at 12:16 PM Oliver Upton <oupton@google.com> wrote:
> > > > >
> > > > > In its implementation of the PSCI function, KVM needs to request that a
> > > > > target vCPU resets before its next entry into the guest. Wrap the logic
> > > > > for requesting a reset in a function for later use by other implemented
> > > > > PSCI calls.
> > > > >
> > > > > No functional change intended.
> > > > >
> > > > > Signed-off-by: Oliver Upton <oupton@google.com>
> > > > > ---
> > > > >  arch/arm64/kvm/psci.c | 59 +++++++++++++++++++++++++------------------
> > > > >  1 file changed, 35 insertions(+), 24 deletions(-)
> > > > >
> > > > > diff --git a/arch/arm64/kvm/psci.c b/arch/arm64/kvm/psci.c
> > > > > index 310b9cb2b32b..bb59b692998b 100644
> > > > > --- a/arch/arm64/kvm/psci.c
> > > > > +++ b/arch/arm64/kvm/psci.c
> > > > > @@ -64,9 +64,40 @@ static inline bool kvm_psci_valid_affinity(unsigned long affinity)
> > > > >         return !(affinity & ~MPIDR_HWID_BITMASK);
> > > > >  }
> > > > >
> > > > > -static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> > > > > +static void kvm_psci_vcpu_request_reset(struct kvm_vcpu *vcpu,
> > > > > +                                       unsigned long entry_addr,
> > > > > +                                       unsigned long context_id,
> > > > > +                                       bool big_endian)
> > > > >  {
> > > > >         struct vcpu_reset_state *reset_state;
> > > > > +
> > > > > +       lockdep_assert_held(&vcpu->kvm->lock);
> > > > > +
> > > > > +       reset_state = &vcpu->arch.reset_state;
> > > > > +       reset_state->pc = entry_addr;
> > > > > +
> > > > > +       /* Propagate caller endianness */
> > > > > +       reset_state->be = big_endian;
> > > > > +
> > > > > +       /*
> > > > > +        * NOTE: We always update r0 (or x0) because for PSCI v0.1
> > > > > +        * the general purpose registers are undefined upon CPU_ON.
> > > > > +        */
> > > > > +       reset_state->r0 = context_id;
> > > > > +
> > > > > +       WRITE_ONCE(reset_state->reset, true);
> > > > > +       kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
> > > > > +
> > > > > +       /*
> > > > > +        * Make sure the reset request is observed if the change to
> > > > > +        * power_state is observed.
> > > > > +        */
> > > > > +       smp_wmb();
> > > > > +       vcpu->arch.power_off = false;
> > > > > +}
> > > > > +
> > > > > +static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> > > > > +{
> > > > >         struct kvm *kvm = source_vcpu->kvm;
> > > > >         struct kvm_vcpu *vcpu = NULL;
> > > > >         unsigned long cpu_id;
> > > > > @@ -90,29 +121,9 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> > > > >                         return PSCI_RET_INVALID_PARAMS;
> > > > >         }
> > > > >
> > > > > -       reset_state = &vcpu->arch.reset_state;
> > > > > -
> > > > > -       reset_state->pc = smccc_get_arg2(source_vcpu);
> > > > > -
> > > > > -       /* Propagate caller endianness */
> > > > > -       reset_state->be = kvm_vcpu_is_be(source_vcpu);
> > > > > -
> > > > > -       /*
> > > > > -        * NOTE: We always update r0 (or x0) because for PSCI v0.1
> > > > > -        * the general purpose registers are undefined upon CPU_ON.
> > > > > -        */
> > > > > -       reset_state->r0 = smccc_get_arg3(source_vcpu);
> > > > > -
> > > > > -       WRITE_ONCE(reset_state->reset, true);
> > > > > -       kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
> > > > > -
> > > > > -       /*
> > > > > -        * Make sure the reset request is observed if the change to
> > > > > -        * power_state is observed.
> > > > > -        */
> > > > > -       smp_wmb();
> > > > > -
> > > > > -       vcpu->arch.power_off = false;
> > > > > +       kvm_psci_vcpu_request_reset(vcpu, smccc_get_arg2(source_vcpu),
> > > > > +                                   smccc_get_arg3(source_vcpu),
> > > > > +                                   kvm_vcpu_is_be(source_vcpu));
> > > > >         kvm_vcpu_wake_up(vcpu);
> > > > >
> > > > >         return PSCI_RET_SUCCESS;
> > > > > --
> > > > > 2.33.0.685.g46640cef36-goog
> > > >
> > > > Reviewed-by: Reiji Watanabe <reijiw@google.com>
> > > >
> > > > Not directly related to the patch, but the (original) code doesn't
> > > > do any sanity checking for the entry address although the PSCI spec says:
> > > >
> > > > "INVALID_ADDRESS is returned when the entry point address is known
> > > > by the implementation to be invalid, because it is in a range that
> > > > is known not to be available to the caller."
> > >
> > > Right, I had noticed the same but was a tad too lazy to address in
> > > this series :) Thanks for the review, Reji!
> > >
> >
> > KVM doesn't reserve any subrange within [0 - max_ipa), afaik. So all
> > we need to do is check 'entry_addr < max_ipa', right?
> >
> 
> We could be a bit more pedantic and check if the IPA exists in a
> memory slot, seems like kvm_vcpu_is_visible_gfn() should do the trick.
> 
> Thoughts?

Are we sure that all emulated devices, nvram, etc. will always use a
memslot for regions that contain executable code? If there's any doubt,
then we can't be sure about non-memslot regions within the max_ipa range.
That'd be up to userspace.

Thanks,
drew

Re: [PATCH v2 10/11] selftests: KVM: Refactor psci_test to make it amenable to new tests

[Andrew Jones]

On Tue, Oct 05, 2021 at 07:54:01AM -0700, Oliver Upton wrote:
> Hi Drew,
> 
> On Tue, Oct 5, 2021 at 6:45 AM Andrew Jones <drjones@redhat.com> wrote:
> >
> > On Thu, Sep 23, 2021 at 07:16:09PM +0000, Oliver Upton wrote:
> > > Split up the current test into several helpers that will be useful to
> > > subsequent test cases added to the PSCI test suite.
> > >
> > > Signed-off-by: Oliver Upton <oupton@google.com>
> > > ---
> > >  .../testing/selftests/kvm/aarch64/psci_test.c | 68 ++++++++++++-------
> > >  1 file changed, 45 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/kvm/aarch64/psci_test.c b/tools/testing/selftests/kvm/aarch64/psci_test.c
> > > index 8d043e12b137..90312be335da 100644
> > > --- a/tools/testing/selftests/kvm/aarch64/psci_test.c
> > > +++ b/tools/testing/selftests/kvm/aarch64/psci_test.c
> > > @@ -45,7 +45,7 @@ static uint64_t psci_affinity_info(uint64_t target_affinity,
> > >       return res.a0;
> > >  }
> > >
> > > -static void guest_main(uint64_t target_cpu)
> > > +static void guest_test_cpu_on(uint64_t target_cpu)
> > >  {
> > >       GUEST_ASSERT(!psci_cpu_on(target_cpu, CPU_ON_ENTRY_ADDR, CPU_ON_CONTEXT_ID));
> > >       uint64_t target_state;
> > > @@ -69,12 +69,10 @@ static void vcpu_power_off(struct kvm_vm *vm, uint32_t vcpuid)
> > >       vcpu_set_mp_state(vm, vcpuid, &mp_state);
> > >  }

Context from last patch.

> > >
> > > -int main(void)
> > > +static struct kvm_vm *setup_vm(void *guest_code)
> > >  {
> > > -     uint64_t target_mpidr, obs_pc, obs_x0;
> > >       struct kvm_vcpu_init init;
> > >       struct kvm_vm *vm;
> > > -     struct ucall uc;
> > >
> > >       vm = vm_create(VM_MODE_DEFAULT, DEFAULT_GUEST_PHY_PAGES, O_RDWR);
> > >       kvm_vm_elf_load(vm, program_invocation_name);
> > > @@ -83,31 +81,28 @@ int main(void)
> > >       vm_ioctl(vm, KVM_ARM_PREFERRED_TARGET, &init);
> > >       init.features[0] |= (1 << KVM_ARM_VCPU_PSCI_0_2);
> > >
> > > -     aarch64_vcpu_add_default(vm, VCPU_ID_SOURCE, &init, guest_main);
> > > -     aarch64_vcpu_add_default(vm, VCPU_ID_TARGET, &init, guest_main);
> > > +     aarch64_vcpu_add_default(vm, VCPU_ID_SOURCE, &init, guest_code);
> > > +     aarch64_vcpu_add_default(vm, VCPU_ID_TARGET, &init, guest_code);

Context from last patch.

> > >
> > > -     /*
> > > -      * make sure the target is already off when executing the test.
> > > -      */
> > > -     vcpu_power_off(vm, VCPU_ID_TARGET);
> > > +     return vm;
> > > +}
> > >
> > > -     get_reg(vm, VCPU_ID_TARGET, ARM64_SYS_REG(MPIDR_EL1), &target_mpidr);
> > > -     vcpu_args_set(vm, VCPU_ID_SOURCE, 1, target_mpidr & MPIDR_HWID_BITMASK);
> > > -     vcpu_run(vm, VCPU_ID_SOURCE);
> > > +static void enter_guest(struct kvm_vm *vm, uint32_t vcpuid)
> > > +{
> > > +     struct ucall uc;
> > >
> > > -     switch (get_ucall(vm, VCPU_ID_SOURCE, &uc)) {
> > > -     case UCALL_DONE:
> > > -             break;
> > > -     case UCALL_ABORT:
> > > +     vcpu_run(vm, vcpuid);
> > > +     if (get_ucall(vm, vcpuid, &uc) == UCALL_ABORT)
> > >               TEST_FAIL("%s at %s:%ld", (const char *)uc.args[0], __FILE__,
> > >                         uc.args[1]);
> > > -             break;
> > > -     default:
> > > -             TEST_FAIL("Unhandled ucall: %lu", uc.cmd);
> > > -     }
> > > +}
> > >
> > > -     get_reg(vm, VCPU_ID_TARGET, ARM64_CORE_REG(regs.pc), &obs_pc);
> > > -     get_reg(vm, VCPU_ID_TARGET, ARM64_CORE_REG(regs.regs[0]), &obs_x0);
> > > +static void assert_vcpu_reset(struct kvm_vm *vm, uint32_t vcpuid)
> > > +{
> > > +     uint64_t obs_pc, obs_x0;
> > > +
> > > +     get_reg(vm, vcpuid, ARM64_CORE_REG(regs.pc), &obs_pc);
> > > +     get_reg(vm, vcpuid, ARM64_CORE_REG(regs.regs[0]), &obs_x0);
> > >
> > >       TEST_ASSERT(obs_pc == CPU_ON_ENTRY_ADDR,
> > >                   "unexpected target cpu pc: %lx (expected: %lx)",
> > > @@ -115,7 +110,34 @@ int main(void)
> > >       TEST_ASSERT(obs_x0 == CPU_ON_CONTEXT_ID,
> > >                   "unexpected target context id: %lx (expected: %lx)",
> > >                   obs_x0, CPU_ON_CONTEXT_ID);
> > > +}
> > >
> > > +static void host_test_cpu_on(void)
> > > +{
> > > +     uint64_t target_mpidr;
> > > +     struct kvm_vm *vm;
> > > +     struct ucall uc;
> > > +
> > > +     vm = setup_vm(guest_test_cpu_on);
> > > +
> > > +     /*
> > > +      * make sure the target is already off when executing the test.
> > > +      */
> > > +     vcpu_power_off(vm, VCPU_ID_TARGET);
> > > +
> > > +     get_reg(vm, VCPU_ID_TARGET, ARM64_SYS_REG(MPIDR_EL1), &target_mpidr);
> > > +     vcpu_args_set(vm, VCPU_ID_SOURCE, 1, target_mpidr & MPIDR_HWID_BITMASK);
> > > +     enter_guest(vm, VCPU_ID_SOURCE);
> > > +
> > > +     if (get_ucall(vm, VCPU_ID_SOURCE, &uc) != UCALL_DONE)
> > > +             TEST_FAIL("Unhandled ucall: %lu", uc.cmd);
> > > +
> > > +     assert_vcpu_reset(vm, VCPU_ID_TARGET);
> > >       kvm_vm_free(vm);
> > > +}
> > > +
> > > +int main(void)
> > > +{
> > > +     host_test_cpu_on();
> > >       return 0;
> > >  }
> > > --
> > > 2.33.0.685.g46640cef36-goog
> > >
> >
> > Hard to read diff, but I think the refactoring comes out right.
> 
> Yeah, this one's nasty, sorry about that. Thanks for parsing it out, heh.
> 
> > Please do this refactoring before adding the new test in the next revision, though.
> >
> 
> This is 10/11 in the series, and the test is 11/11. I'm not seeing any
> context belonging to the last patch, but perhaps I'm missing something
> obvious.

It's not much, but nicer to have none.

Thanks,
drew

> 
> > Reviewed-by: Andrew Jones <drjones@redhat.com>
> 
> Thanks!
> 
> --
> Best,
> Oliver
> 

Re: [PATCH v2 03/11] KVM: arm64: Encapsulate reset request logic in a helper function

[Reiji Watanabe]

On Tue, Oct 5, 2021 at 12:02 PM Andrew Jones <drjones@redhat.com> wrote:
>
> On Tue, Oct 05, 2021 at 08:05:02AM -0700, Oliver Upton wrote:
> > Hi folks,
> >
> > On Tue, Oct 5, 2021 at 6:33 AM Andrew Jones <drjones@redhat.com> wrote:
> > >
> > > On Fri, Oct 01, 2021 at 09:10:14AM -0700, Oliver Upton wrote:
> > > > On Thu, Sep 30, 2021 at 11:05 PM Reiji Watanabe <reijiw@google.com> wrote:
> > > > >
> > > > > On Thu, Sep 23, 2021 at 12:16 PM Oliver Upton <oupton@google.com> wrote:
> > > > > >
> > > > > > In its implementation of the PSCI function, KVM needs to request that a
> > > > > > target vCPU resets before its next entry into the guest. Wrap the logic
> > > > > > for requesting a reset in a function for later use by other implemented
> > > > > > PSCI calls.
> > > > > >
> > > > > > No functional change intended.
> > > > > >
> > > > > > Signed-off-by: Oliver Upton <oupton@google.com>
> > > > > > ---
> > > > > >  arch/arm64/kvm/psci.c | 59 +++++++++++++++++++++++++------------------
> > > > > >  1 file changed, 35 insertions(+), 24 deletions(-)
> > > > > >
> > > > > > diff --git a/arch/arm64/kvm/psci.c b/arch/arm64/kvm/psci.c
> > > > > > index 310b9cb2b32b..bb59b692998b 100644
> > > > > > --- a/arch/arm64/kvm/psci.c
> > > > > > +++ b/arch/arm64/kvm/psci.c
> > > > > > @@ -64,9 +64,40 @@ static inline bool kvm_psci_valid_affinity(unsigned long affinity)
> > > > > >         return !(affinity & ~MPIDR_HWID_BITMASK);
> > > > > >  }
> > > > > >
> > > > > > -static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> > > > > > +static void kvm_psci_vcpu_request_reset(struct kvm_vcpu *vcpu,
> > > > > > +                                       unsigned long entry_addr,
> > > > > > +                                       unsigned long context_id,
> > > > > > +                                       bool big_endian)
> > > > > >  {
> > > > > >         struct vcpu_reset_state *reset_state;
> > > > > > +
> > > > > > +       lockdep_assert_held(&vcpu->kvm->lock);
> > > > > > +
> > > > > > +       reset_state = &vcpu->arch.reset_state;
> > > > > > +       reset_state->pc = entry_addr;
> > > > > > +
> > > > > > +       /* Propagate caller endianness */
> > > > > > +       reset_state->be = big_endian;
> > > > > > +
> > > > > > +       /*
> > > > > > +        * NOTE: We always update r0 (or x0) because for PSCI v0.1
> > > > > > +        * the general purpose registers are undefined upon CPU_ON.
> > > > > > +        */
> > > > > > +       reset_state->r0 = context_id;
> > > > > > +
> > > > > > +       WRITE_ONCE(reset_state->reset, true);
> > > > > > +       kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
> > > > > > +
> > > > > > +       /*
> > > > > > +        * Make sure the reset request is observed if the change to
> > > > > > +        * power_state is observed.
> > > > > > +        */
> > > > > > +       smp_wmb();
> > > > > > +       vcpu->arch.power_off = false;
> > > > > > +}
> > > > > > +
> > > > > > +static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> > > > > > +{
> > > > > >         struct kvm *kvm = source_vcpu->kvm;
> > > > > >         struct kvm_vcpu *vcpu = NULL;
> > > > > >         unsigned long cpu_id;
> > > > > > @@ -90,29 +121,9 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> > > > > >                         return PSCI_RET_INVALID_PARAMS;
> > > > > >         }
> > > > > >
> > > > > > -       reset_state = &vcpu->arch.reset_state;
> > > > > > -
> > > > > > -       reset_state->pc = smccc_get_arg2(source_vcpu);
> > > > > > -
> > > > > > -       /* Propagate caller endianness */
> > > > > > -       reset_state->be = kvm_vcpu_is_be(source_vcpu);
> > > > > > -
> > > > > > -       /*
> > > > > > -        * NOTE: We always update r0 (or x0) because for PSCI v0.1
> > > > > > -        * the general purpose registers are undefined upon CPU_ON.
> > > > > > -        */
> > > > > > -       reset_state->r0 = smccc_get_arg3(source_vcpu);
> > > > > > -
> > > > > > -       WRITE_ONCE(reset_state->reset, true);
> > > > > > -       kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
> > > > > > -
> > > > > > -       /*
> > > > > > -        * Make sure the reset request is observed if the change to
> > > > > > -        * power_state is observed.
> > > > > > -        */
> > > > > > -       smp_wmb();
> > > > > > -
> > > > > > -       vcpu->arch.power_off = false;
> > > > > > +       kvm_psci_vcpu_request_reset(vcpu, smccc_get_arg2(source_vcpu),
> > > > > > +                                   smccc_get_arg3(source_vcpu),
> > > > > > +                                   kvm_vcpu_is_be(source_vcpu));
> > > > > >         kvm_vcpu_wake_up(vcpu);
> > > > > >
> > > > > >         return PSCI_RET_SUCCESS;
> > > > > > --
> > > > > > 2.33.0.685.g46640cef36-goog
> > > > >
> > > > > Reviewed-by: Reiji Watanabe <reijiw@google.com>
> > > > >
> > > > > Not directly related to the patch, but the (original) code doesn't
> > > > > do any sanity checking for the entry address although the PSCI spec says:
> > > > >
> > > > > "INVALID_ADDRESS is returned when the entry point address is known
> > > > > by the implementation to be invalid, because it is in a range that
> > > > > is known not to be available to the caller."
> > > >
> > > > Right, I had noticed the same but was a tad too lazy to address in
> > > > this series :) Thanks for the review, Reji!
> > > >
> > >
> > > KVM doesn't reserve any subrange within [0 - max_ipa), afaik. So all
> > > we need to do is check 'entry_addr < max_ipa', right?
> > >
> >
> > We could be a bit more pedantic and check if the IPA exists in a
> > memory slot, seems like kvm_vcpu_is_visible_gfn() should do the trick.
> >
> > Thoughts?
>
> Are we sure that all emulated devices, nvram, etc. will always use a
> memslot for regions that contain executable code? If there's any doubt,
> then we can't be sure about non-memslot regions within the max_ipa range.
> That'd be up to userspace.

I'm sorry for the late response.
IMHO, I would prefer Andrew's suggestion (check with the max_ipa).

It looks like instructions must always be in memslot for KVM/ARM looking
at the current implementation (especially kvm_handle_guest_abort()).
But, it doesn't necessarily mean the address is not invalid for the
guest (could be valid for load/store) and it might be changed in
the future.


Thanks,
Reiji