[PATCH 0/5] KVM: arm/arm64: Fix VCPU power management problems

[PATCH 0/5] KVM: arm/arm64: Fix VCPU power management problems

[Christoffer Dall]

This series fixes a number of issues:

 - When powering on and resetting VCPUs, we can be preempted in the
   middle which can lead to guest system register corruption.

 - We were missing support for PSCI ON_PENDING when multiple VCPUs try
   to turn on a target VCPU at the same time.

 - Powering off a VCPU could race with powering on the same VCPU.

 - We unnecesarily panic'ed if we found a non-initialized guest system
   register.

The main approach to fixing all these problems is by using VCPU
requests.

See the individual patches for more details.

Christoffer Dall (3):
  KVM: arm/arm64: Reset the VCPU without preemption and vcpu state
    loaded
  KVM: arm/arm64: Require VCPU threads to turn them self off
  KVM: arm/arm64: Implement PSCI ON_PENDING when turning on VCPUs

Marc Zyngier (2):
  arm/arm64: KVM: Allow a VCPU to fully reset itself
  arm/arm64: KVM: Don't panic on failure to properly reset system
    registers

 arch/arm/include/asm/kvm_host.h   | 22 ++++++++++-
 arch/arm/kvm/coproc.c             |  4 +-
 arch/arm/kvm/reset.c              | 24 ++++++++++++
 arch/arm64/include/asm/kvm_host.h | 23 ++++++++++-
 arch/arm64/kvm/reset.c            | 50 +++++++++++++++++++++++-
 arch/arm64/kvm/sys_regs.c         |  4 +-
 virt/kvm/arm/arm.c                | 40 ++++++++++++++-----
 virt/kvm/arm/psci.c               | 64 +++++++++++++++----------------
 8 files changed, 177 insertions(+), 54 deletions(-)

-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/5] KVM: arm/arm64: Reset the VCPU without preemption and vcpu state loaded

[Christoffer Dall]

Resetting the VCPU state modifies the system register state in memory,
but this may interact with vcpu_load/vcpu_put if running with preemption
disabled, which in turn may lead to corrupted system register state.

Address this by disabling preemption and doing put/load if required
around the reset logic.

Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kvm/reset.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
index b72a3dd56204..f21a2a575939 100644
--- a/arch/arm64/kvm/reset.c
+++ b/arch/arm64/kvm/reset.c
@@ -105,16 +105,33 @@ int kvm_arch_vm_ioctl_check_extension(struct kvm *kvm, long ext)
  * This function finds the right table above and sets the registers on
  * the virtual CPU struct to their architecturally defined reset
  * values.
+ *
+ * Note: This function can be called from two paths: The KVM_ARM_VCPU_INIT
+ * ioctl or as part of handling a request issued by another VCPU in the PSCI
+ * handling code.  In the first case, the VCPU will not be loaded, and in the
+ * second case the VCPU will be loaded.  Because this function operates purely
+ * on the memory-backed valus of system registers, we want to do a full put if
+ * we were loaded (handling a request) and load the values back at the end of
+ * the function.  Otherwise we leave the state alone.  In both cases, we
+ * disable preemption around the vcpu reset as we would otherwise race with
+ * preempt notifiers which also call put/load.
  */
 int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
 {
 	const struct kvm_regs *cpu_reset;
+	int ret = -EINVAL;
+	bool loaded;
+
+	preempt_disable();
+	loaded = (vcpu->cpu != -1);
+	if (loaded)
+		kvm_arch_vcpu_put(vcpu);
 
 	switch (vcpu->arch.target) {
 	default:
 		if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features)) {
 			if (!cpu_has_32bit_el1())
-				return -EINVAL;
+				goto out;
 			cpu_reset = &default_regs_reset32;
 		} else {
 			cpu_reset = &default_regs_reset;
@@ -137,7 +154,12 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
 		vcpu->arch.workaround_flags |= VCPU_WORKAROUND_2_FLAG;
 
 	/* Reset timer */
-	return kvm_timer_vcpu_reset(vcpu);
+	ret = kvm_timer_vcpu_reset(vcpu);
+out:
+	if (loaded)
+		kvm_arch_vcpu_load(vcpu, smp_processor_id());
+	preempt_enable();
+	return ret;
 }
 
 void kvm_set_ipa_limit(void)
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/5] arm/arm64: KVM: Allow a VCPU to fully reset itself

[Christoffer Dall]

From: Marc Zyngier <marc.zyngier@arm.com>

The current kvm_psci_vcpu_on implementation will directly try to
manipulate the state of the VCPU to reset it.  However, since this is
not done on the thread that runs the VCPU, we can end up in a strangely
corrupted state when the source and target VCPUs are running at the same
time.

Fix this by factoring out all reset logic from the PSCI implementation
and forwarding the required information along with a request to the
target VCPU.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
---
 arch/arm/include/asm/kvm_host.h   | 10 +++++++++
 arch/arm/kvm/reset.c              | 24 +++++++++++++++++++++
 arch/arm64/include/asm/kvm_host.h | 11 ++++++++++
 arch/arm64/kvm/reset.c            | 24 +++++++++++++++++++++
 virt/kvm/arm/arm.c                | 10 +++++++++
 virt/kvm/arm/psci.c               | 36 ++++++++++++++-----------------
 6 files changed, 95 insertions(+), 20 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index ca56537b61bc..50e89869178a 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -48,6 +48,7 @@
 #define KVM_REQ_SLEEP \
 	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
 #define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
+#define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
 
 DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
 
@@ -147,6 +148,13 @@ struct kvm_cpu_context {
 
 typedef struct kvm_cpu_context kvm_cpu_context_t;
 
+struct vcpu_reset_state {
+	unsigned long	pc;
+	unsigned long	r0;
+	bool		be;
+	bool		reset;
+};
+
 struct kvm_vcpu_arch {
 	struct kvm_cpu_context ctxt;
 
@@ -186,6 +194,8 @@ struct kvm_vcpu_arch {
 	/* Cache some mmu pages needed inside spinlock regions */
 	struct kvm_mmu_memory_cache mmu_page_cache;
 
+	struct vcpu_reset_state reset_state;
+
 	/* Detect first run of a vcpu */
 	bool has_run_once;
 };
diff --git a/arch/arm/kvm/reset.c b/arch/arm/kvm/reset.c
index 5ed0c3ee33d6..de41255eebcd 100644
--- a/arch/arm/kvm/reset.c
+++ b/arch/arm/kvm/reset.c
@@ -26,6 +26,7 @@
 #include <asm/cputype.h>
 #include <asm/kvm_arm.h>
 #include <asm/kvm_coproc.h>
+#include <asm/kvm_emulate.h>
 
 #include <kvm/arm_arch_timer.h>
 
@@ -69,6 +70,29 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
 	/* Reset CP15 registers */
 	kvm_reset_coprocs(vcpu);
 
+	/*
+	 * Additional reset state handling that PSCI may have imposed on us.
+	 * Must be done after all the sys_reg reset.
+	 */
+	if (vcpu->arch.reset_state.reset) {
+		unsigned long target_pc = vcpu->arch.reset_state.pc;
+
+		/* Gracefully handle Thumb2 entry point */
+		if (target_pc & 1) {
+			target_pc &= ~1UL;
+			vcpu_set_thumb(vcpu);
+		}
+
+		/* Propagate caller endianness */
+		if (vcpu->arch.reset_state.be)
+			kvm_vcpu_set_be(vcpu);
+
+		*vcpu_pc(vcpu) = target_pc;
+		vcpu_set_reg(vcpu, 0, vcpu->arch.reset_state.r0);
+
+		vcpu->arch.reset_state.reset = false;
+	}
+
 	/* Reset arch_timer context */
 	return kvm_timer_vcpu_reset(vcpu);
 }
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 7732d0ba4e60..da3fc7324d68 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -48,6 +48,7 @@
 #define KVM_REQ_SLEEP \
 	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
 #define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
+#define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
 
 DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
 
@@ -208,6 +209,13 @@ struct kvm_cpu_context {
 
 typedef struct kvm_cpu_context kvm_cpu_context_t;
 
+struct vcpu_reset_state {
+	unsigned long	pc;
+	unsigned long	r0;
+	bool		be;
+	bool		reset;
+};
+
 struct kvm_vcpu_arch {
 	struct kvm_cpu_context ctxt;
 
@@ -297,6 +305,9 @@ struct kvm_vcpu_arch {
 	/* Virtual SError ESR to restore when HCR_EL2.VSE is set */
 	u64 vsesr_el2;
 
+	/* Additional reset state */
+	struct vcpu_reset_state	reset_state;
+
 	/* True when deferrable sysregs are loaded on the physical CPU,
 	 * see kvm_vcpu_load_sysregs and kvm_vcpu_put_sysregs. */
 	bool sysregs_loaded_on_cpu;
diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
index f21a2a575939..f16a5f8ff2b4 100644
--- a/arch/arm64/kvm/reset.c
+++ b/arch/arm64/kvm/reset.c
@@ -32,6 +32,7 @@
 #include <asm/kvm_arm.h>
 #include <asm/kvm_asm.h>
 #include <asm/kvm_coproc.h>
+#include <asm/kvm_emulate.h>
 #include <asm/kvm_mmu.h>
 
 /* Maximum phys_shift supported for any VM on this host */
@@ -146,6 +147,29 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
 	/* Reset system registers */
 	kvm_reset_sys_regs(vcpu);
 
+	/*
+	 * Additional reset state handling that PSCI may have imposed on us.
+	 * Must be done after all the sys_reg reset.
+	 */
+	if (vcpu->arch.reset_state.reset) {
+		unsigned long target_pc = vcpu->arch.reset_state.pc;
+
+		/* Gracefully handle Thumb2 entry point */
+		if (vcpu_mode_is_32bit(vcpu) && (target_pc & 1)) {
+			target_pc &= ~1UL;
+			vcpu_set_thumb(vcpu);
+		}
+
+		/* Propagate caller endianness */
+		if (vcpu->arch.reset_state.be)
+			kvm_vcpu_set_be(vcpu);
+
+		*vcpu_pc(vcpu) = target_pc;
+		vcpu_set_reg(vcpu, 0, vcpu->arch.reset_state.r0);
+
+		vcpu->arch.reset_state.reset = false;
+	}
+
 	/* Reset PMU */
 	kvm_pmu_vcpu_reset(vcpu);
 
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 9e350fd34504..9c486fad3f9f 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -626,6 +626,13 @@ static void vcpu_req_sleep(struct kvm_vcpu *vcpu)
 		/* Awaken to handle a signal, request we sleep again later. */
 		kvm_make_request(KVM_REQ_SLEEP, vcpu);
 	}
+
+	/*
+	 * Make sure we will observe a potential reset request if we've
+	 * observed a change to the power state. Pairs with the smp_wmb() in
+	 * kvm_psci_vcpu_on().
+	 */
+	smp_rmb();
 }
 
 static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
@@ -639,6 +646,9 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
 		if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
 			vcpu_req_sleep(vcpu);
 
+		if (kvm_check_request(KVM_REQ_VCPU_RESET, vcpu))
+			kvm_reset_vcpu(vcpu);
+
 		/*
 		 * Clear IRQ_PENDING requests that were made to guarantee
 		 * that a VCPU sees new virtual interrupts.
diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
index 9b73d3ad918a..b9cff1d4b06d 100644
--- a/virt/kvm/arm/psci.c
+++ b/virt/kvm/arm/psci.c
@@ -104,12 +104,10 @@ static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu)
 
 static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
 {
+	struct vcpu_reset_state *reset_state;
 	struct kvm *kvm = source_vcpu->kvm;
 	struct kvm_vcpu *vcpu = NULL;
-	struct swait_queue_head *wq;
 	unsigned long cpu_id;
-	unsigned long context_id;
-	phys_addr_t target_pc;
 
 	cpu_id = smccc_get_arg1(source_vcpu) & MPIDR_HWID_BITMASK;
 	if (vcpu_mode_is_32bit(source_vcpu))
@@ -130,32 +128,30 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
 			return PSCI_RET_INVALID_PARAMS;
 	}
 
-	target_pc = smccc_get_arg2(source_vcpu);
-	context_id = smccc_get_arg3(source_vcpu);
+	reset_state = &vcpu->arch.reset_state;
 
-	kvm_reset_vcpu(vcpu);
-
-	/* Gracefully handle Thumb2 entry point */
-	if (vcpu_mode_is_32bit(vcpu) && (target_pc & 1)) {
-		target_pc &= ~((phys_addr_t) 1);
-		vcpu_set_thumb(vcpu);
-	}
+	reset_state->pc = smccc_get_arg2(source_vcpu);
 
 	/* Propagate caller endianness */
-	if (kvm_vcpu_is_be(source_vcpu))
-		kvm_vcpu_set_be(vcpu);
+	reset_state->be = kvm_vcpu_is_be(source_vcpu);
 
-	*vcpu_pc(vcpu) = target_pc;
 	/*
 	 * NOTE: We always update r0 (or x0) because for PSCI v0.1
 	 * the general puspose registers are undefined upon CPU_ON.
 	 */
-	smccc_set_retval(vcpu, context_id, 0, 0, 0);
-	vcpu->arch.power_off = false;
-	smp_mb();		/* Make sure the above is visible */
+	reset_state->r0 = smccc_get_arg3(source_vcpu);
+
+	reset_state->reset = true;
+	kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
 
-	wq = kvm_arch_vcpu_wq(vcpu);
-	swake_up_one(wq);
+	/*
+	 * Make sure the reset request is observed if the change to
+	 * power_state is observed.
+	 */
+	smp_wmb();
+
+	vcpu->arch.power_off = false;
+	kvm_vcpu_wake_up(vcpu);
 
 	return PSCI_RET_SUCCESS;
 }
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/5] KVM: arm/arm64: Require VCPU threads to turn them self off

[Christoffer Dall]

To avoid a race between turning VCPUs off and turning them on, make sure
that only the VCPU threat itself turns off the VCPU.  When other threads
want to turn of a VCPU, they now do this via a request.

Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/include/asm/kvm_host.h   |  2 ++
 arch/arm64/include/asm/kvm_host.h |  2 ++
 virt/kvm/arm/arm.c                |  8 ++++++--
 virt/kvm/arm/psci.c               | 11 ++---------
 4 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 50e89869178a..b1cfae222441 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -49,6 +49,8 @@
 	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
 #define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
 #define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
+#define KVM_REQ_VCPU_OFF \
+	KVM_ARCH_REQ_FLAGS(3, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
 
 DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
 
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index da3fc7324d68..d43b13421987 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -49,6 +49,8 @@
 	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
 #define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
 #define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
+#define KVM_REQ_VCPU_OFF \
+	KVM_ARCH_REQ_FLAGS(3, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
 
 DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
 
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 9c486fad3f9f..785076176814 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -404,8 +404,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 
 static void vcpu_power_off(struct kvm_vcpu *vcpu)
 {
-	vcpu->arch.power_off = true;
-	kvm_make_request(KVM_REQ_SLEEP, vcpu);
+	kvm_make_request(KVM_REQ_VCPU_OFF, vcpu);
 	kvm_vcpu_kick(vcpu);
 }
 
@@ -646,6 +645,11 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
 		if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
 			vcpu_req_sleep(vcpu);
 
+		if (kvm_check_request(KVM_REQ_VCPU_OFF, vcpu)) {
+			vcpu->arch.power_off = true;
+			vcpu_req_sleep(vcpu);
+		}
+
 		if (kvm_check_request(KVM_REQ_VCPU_RESET, vcpu))
 			kvm_reset_vcpu(vcpu);
 
diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
index b9cff1d4b06d..20255319e193 100644
--- a/virt/kvm/arm/psci.c
+++ b/virt/kvm/arm/psci.c
@@ -97,9 +97,7 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)
 
 static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu)
 {
-	vcpu->arch.power_off = true;
-	kvm_make_request(KVM_REQ_SLEEP, vcpu);
-	kvm_vcpu_kick(vcpu);
+	kvm_make_request(KVM_REQ_VCPU_OFF, vcpu);
 }
 
 static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
@@ -198,9 +196,6 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu)
 
 static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type)
 {
-	int i;
-	struct kvm_vcpu *tmp;
-
 	/*
 	 * The KVM ABI specifies that a system event exit may call KVM_RUN
 	 * again and may perform shutdown/reboot at a later time that when the
@@ -210,9 +205,7 @@ static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type)
 	 * after this call is handled and before the VCPUs have been
 	 * re-initialized.
 	 */
-	kvm_for_each_vcpu(i, tmp, vcpu->kvm)
-		tmp->arch.power_off = true;
-	kvm_make_all_cpus_request(vcpu->kvm, KVM_REQ_SLEEP);
+	kvm_make_all_cpus_request(vcpu->kvm, KVM_REQ_VCPU_OFF);
 
 	memset(&vcpu->run->system_event, 0, sizeof(vcpu->run->system_event));
 	vcpu->run->system_event.type = type;
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/5] KVM: arm/arm64: Implement PSCI ON_PENDING when turning on VCPUs

[Christoffer Dall]

We are currently not implementing the PSCI spec completely, as we do not
take handle the situation where two VCPUs are attempting to turn on a
third VCPU at the same time.  The PSCI implementation should make sure
that only one requesting VCPU wins the race and that the other receives
PSCI_RET_ON_PENDING.

Implement this by changing the VCPU power state to a tristate enum and
ensure only a single VCPU can turn on another VCPU at a given time using
a cmpxchg operation.

Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/include/asm/kvm_host.h   | 10 ++++++++--
 arch/arm64/include/asm/kvm_host.h | 10 ++++++++--
 virt/kvm/arm/arm.c                | 24 +++++++++++++++---------
 virt/kvm/arm/psci.c               | 21 ++++++++++++++-------
 4 files changed, 45 insertions(+), 20 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index b1cfae222441..4dc47fea1ac8 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -157,6 +157,12 @@ struct vcpu_reset_state {
 	bool		reset;
 };
 
+enum vcpu_power_state {
+	KVM_ARM_VCPU_OFF,
+	KVM_ARM_VCPU_ON_PENDING,
+	KVM_ARM_VCPU_ON,
+};
+
 struct kvm_vcpu_arch {
 	struct kvm_cpu_context ctxt;
 
@@ -184,8 +190,8 @@ struct kvm_vcpu_arch {
 	 * here.
 	 */
 
-	/* vcpu power-off state */
-	bool power_off;
+	/* vcpu power state */
+	enum vcpu_power_state power_state;
 
 	 /* Don't run the guest (internal implementation need) */
 	bool pause;
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index d43b13421987..0647a409657b 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -218,6 +218,12 @@ struct vcpu_reset_state {
 	bool		reset;
 };
 
+enum vcpu_power_state {
+	KVM_ARM_VCPU_OFF,
+	KVM_ARM_VCPU_ON_PENDING,
+	KVM_ARM_VCPU_ON,
+};
+
 struct kvm_vcpu_arch {
 	struct kvm_cpu_context ctxt;
 
@@ -285,8 +291,8 @@ struct kvm_vcpu_arch {
 		u32	mdscr_el1;
 	} guest_debug_preserved;
 
-	/* vcpu power-off state */
-	bool power_off;
+	/* vcpu power state */
+	enum vcpu_power_state power_state;
 
 	/* Don't run the guest (internal implementation need) */
 	bool pause;
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 785076176814..1e3195155860 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -411,7 +411,7 @@ static void vcpu_power_off(struct kvm_vcpu *vcpu)
 int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
 				    struct kvm_mp_state *mp_state)
 {
-	if (vcpu->arch.power_off)
+	if (vcpu->arch.power_state != KVM_ARM_VCPU_ON)
 		mp_state->mp_state = KVM_MP_STATE_STOPPED;
 	else
 		mp_state->mp_state = KVM_MP_STATE_RUNNABLE;
@@ -426,7 +426,7 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
 
 	switch (mp_state->mp_state) {
 	case KVM_MP_STATE_RUNNABLE:
-		vcpu->arch.power_off = false;
+		vcpu->arch.power_state = KVM_ARM_VCPU_ON;
 		break;
 	case KVM_MP_STATE_STOPPED:
 		vcpu_power_off(vcpu);
@@ -448,8 +448,9 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
 int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
 {
 	bool irq_lines = *vcpu_hcr(v) & (HCR_VI | HCR_VF);
-	return ((irq_lines || kvm_vgic_vcpu_pending_irq(v))
-		&& !v->arch.power_off && !v->arch.pause);
+	return (irq_lines || kvm_vgic_vcpu_pending_irq(v)) &&
+		v->arch.power_state == KVM_ARM_VCPU_ON &&
+		!v->arch.pause;
 }
 
 bool kvm_arch_vcpu_in_kernel(struct kvm_vcpu *vcpu)
@@ -614,14 +615,19 @@ void kvm_arm_resume_guest(struct kvm *kvm)
 	}
 }
 
+static bool vcpu_sleeping(struct kvm_vcpu *vcpu)
+{
+	return vcpu->arch.power_state != KVM_ARM_VCPU_ON ||
+		vcpu->arch.pause;
+}
+
 static void vcpu_req_sleep(struct kvm_vcpu *vcpu)
 {
 	struct swait_queue_head *wq = kvm_arch_vcpu_wq(vcpu);
 
-	swait_event_interruptible_exclusive(*wq, ((!vcpu->arch.power_off) &&
-				       (!vcpu->arch.pause)));
+	swait_event_interruptible_exclusive(*wq, !vcpu_sleeping(vcpu));
 
-	if (vcpu->arch.power_off || vcpu->arch.pause) {
+	if (vcpu_sleeping(vcpu)) {
 		/* Awaken to handle a signal, request we sleep again later. */
 		kvm_make_request(KVM_REQ_SLEEP, vcpu);
 	}
@@ -646,7 +652,7 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
 			vcpu_req_sleep(vcpu);
 
 		if (kvm_check_request(KVM_REQ_VCPU_OFF, vcpu)) {
-			vcpu->arch.power_off = true;
+			vcpu->arch.power_state = KVM_ARM_VCPU_OFF;
 			vcpu_req_sleep(vcpu);
 		}
 
@@ -1016,7 +1022,7 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
 	if (test_bit(KVM_ARM_VCPU_POWER_OFF, vcpu->arch.features))
 		vcpu_power_off(vcpu);
 	else
-		vcpu->arch.power_off = false;
+		vcpu->arch.power_state = KVM_ARM_VCPU_ON;
 
 	return 0;
 }
diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
index 20255319e193..5c2d9eeb810c 100644
--- a/virt/kvm/arm/psci.c
+++ b/virt/kvm/arm/psci.c
@@ -106,6 +106,7 @@ 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;
+	enum vcpu_power_state old_power_state;
 
 	cpu_id = smccc_get_arg1(source_vcpu) & MPIDR_HWID_BITMASK;
 	if (vcpu_mode_is_32bit(source_vcpu))
@@ -119,12 +120,18 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
 	 */
 	if (!vcpu)
 		return PSCI_RET_INVALID_PARAMS;
-	if (!vcpu->arch.power_off) {
-		if (kvm_psci_version(source_vcpu, kvm) != KVM_ARM_PSCI_0_1)
-			return PSCI_RET_ALREADY_ON;
-		else
+	old_power_state = cmpxchg(&vcpu->arch.power_state,
+				  KVM_ARM_VCPU_OFF,
+				  KVM_ARM_VCPU_ON_PENDING);
+
+	if (old_power_state != KVM_ARM_VCPU_OFF &&
+	    kvm_psci_version(source_vcpu, kvm) == KVM_ARM_PSCI_0_1)
 			return PSCI_RET_INVALID_PARAMS;
-	}
+
+	if (old_power_state == KVM_ARM_VCPU_ON_PENDING)
+			return PSCI_RET_ON_PENDING;
+	else if (old_power_state == KVM_ARM_VCPU_ON)
+			return PSCI_RET_ALREADY_ON;
 
 	reset_state = &vcpu->arch.reset_state;
 
@@ -148,7 +155,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
 	 */
 	smp_wmb();
 
-	vcpu->arch.power_off = false;
+	vcpu->arch.power_state = KVM_ARM_VCPU_ON;
 	kvm_vcpu_wake_up(vcpu);
 
 	return PSCI_RET_SUCCESS;
@@ -183,7 +190,7 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu)
 		mpidr = kvm_vcpu_get_mpidr_aff(tmp);
 		if ((mpidr & target_affinity_mask) == target_affinity) {
 			matching_cpus++;
-			if (!tmp->arch.power_off)
+			if (tmp->arch.power_state == KVM_ARM_VCPU_ON)
 				return PSCI_0_2_AFFINITY_LEVEL_ON;
 		}
 	}
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/5] arm/arm64: KVM: Don't panic on failure to properly reset system registers

[Christoffer Dall]

From: Marc Zyngier <marc.zyngier@arm.com>

Failing to properly reset system registers is pretty bad. But not
quite as bad as bringing the whole machine down... So warn loudly,
but slightly more gracefully.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Acked-by: Christoffer Dall <christoffer.dall@arm.com>
---
 arch/arm/kvm/coproc.c     | 4 ++--
 arch/arm64/kvm/sys_regs.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
index 222c1635bc7a..e8bd288fd5be 100644
--- a/arch/arm/kvm/coproc.c
+++ b/arch/arm/kvm/coproc.c
@@ -1450,6 +1450,6 @@ void kvm_reset_coprocs(struct kvm_vcpu *vcpu)
 	reset_coproc_regs(vcpu, table, num);
 
 	for (num = 1; num < NR_CP15_REGS; num++)
-		if (vcpu_cp15(vcpu, num) == 0x42424242)
-			panic("Didn't reset vcpu_cp15(vcpu, %zi)", num);
+		WARN(vcpu_cp15(vcpu, num) == 0x42424242,
+		     "Didn't reset vcpu_cp15(vcpu, %zi)", num);
 }
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 86096774abcd..4f067545c7d2 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2609,6 +2609,6 @@ void kvm_reset_sys_regs(struct kvm_vcpu *vcpu)
 	reset_sys_reg_descs(vcpu, table, num);
 
 	for (num = 1; num < NR_SYS_REGS; num++)
-		if (__vcpu_sys_reg(vcpu, num) == 0x4242424242424242)
-			panic("Didn't reset __vcpu_sys_reg(%zi)", num);
+		WARN(__vcpu_sys_reg(vcpu, num) == 0x4242424242424242,
+		     "Didn't reset __vcpu_sys_reg(%zi)\n", num);
 }
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/5] KVM: arm/arm64: Reset the VCPU without preemption and vcpu state loaded

[Andrew Jones]

Hi Christoffer,

On Fri, Jan 25, 2019 at 10:46:52AM +0100, Christoffer Dall wrote:
> Resetting the VCPU state modifies the system register state in memory,
> but this may interact with vcpu_load/vcpu_put if running with preemption
> disabled, which in turn may lead to corrupted system register state.
  ^ enabled
> 
> Address this by disabling preemption and doing put/load if required
> around the reset logic.

I'm having trouble understanding how disabling preemption helps here.
There shouldn't be an issue with the KVM_ARM_VCPU_INIT case, since the
target vcpu is guaranteed not to be loaded and therefore it doesn't
have preempt notifiers registered either. Also, KVM_ARM_VCPU_INIT holds
the vcpu mutex, so there's no chance for a load to occur until it's
complete.

For the PSCI case it makes sense to force a vcpu load after the reset,
otherwise the sleeping target vcpu won't have the correct state loaded.
The initial put also makes sense in case we're not resetting everything.
I don't understand how we're ensuring the target vcpu thread's preemption
is disabled though. This modified kvm_reset_vcpu would need to be run
from the target vcpu thread to work, but that's not how the PSCI code
currently does it.

> 
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/kvm/reset.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index b72a3dd56204..f21a2a575939 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -105,16 +105,33 @@ int kvm_arch_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>   * This function finds the right table above and sets the registers on
>   * the virtual CPU struct to their architecturally defined reset
>   * values.
> + *
> + * Note: This function can be called from two paths: The KVM_ARM_VCPU_INIT
> + * ioctl or as part of handling a request issued by another VCPU in the PSCI
> + * handling code.  In the first case, the VCPU will not be loaded, and in the
> + * second case the VCPU will be loaded.  Because this function operates purely
> + * on the memory-backed valus of system registers, we want to do a full put if
                           ^ values
> + * we were loaded (handling a request) and load the values back at the end of
> + * the function.  Otherwise we leave the state alone.  In both cases, we
> + * disable preemption around the vcpu reset as we would otherwise race with
> + * preempt notifiers which also call put/load.
>   */
>  int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>  {
>  	const struct kvm_regs *cpu_reset;
> +	int ret = -EINVAL;
> +	bool loaded;
> +
> +	preempt_disable();
> +	loaded = (vcpu->cpu != -1);
> +	if (loaded)
> +		kvm_arch_vcpu_put(vcpu);
>  
>  	switch (vcpu->arch.target) {
>  	default:
>  		if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features)) {
>  			if (!cpu_has_32bit_el1())
> -				return -EINVAL;
> +				goto out;
>  			cpu_reset = &default_regs_reset32;
>  		} else {
>  			cpu_reset = &default_regs_reset;
> @@ -137,7 +154,12 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>  		vcpu->arch.workaround_flags |= VCPU_WORKAROUND_2_FLAG;
>  
>  	/* Reset timer */
> -	return kvm_timer_vcpu_reset(vcpu);
> +	ret = kvm_timer_vcpu_reset(vcpu);
> +out:
> +	if (loaded)
> +		kvm_arch_vcpu_load(vcpu, smp_processor_id());
> +	preempt_enable();
> +	return ret;
>  }
>  
>  void kvm_set_ipa_limit(void)
> -- 
> 2.18.0
>

Thanks,
drew 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] arm/arm64: KVM: Allow a VCPU to fully reset itself

[Andrew Jones]

On Fri, Jan 25, 2019 at 10:46:53AM +0100, Christoffer Dall wrote:
> From: Marc Zyngier <marc.zyngier@arm.com>
> 
> The current kvm_psci_vcpu_on implementation will directly try to
> manipulate the state of the VCPU to reset it.  However, since this is
> not done on the thread that runs the VCPU, we can end up in a strangely
> corrupted state when the source and target VCPUs are running at the same
> time.
> 
> Fix this by factoring out all reset logic from the PSCI implementation
> and forwarding the required information along with a request to the
> target VCPU.

The last patch makes more sense, now that I see this one. I guess their
order should be swapped.

> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> ---
>  arch/arm/include/asm/kvm_host.h   | 10 +++++++++
>  arch/arm/kvm/reset.c              | 24 +++++++++++++++++++++
>  arch/arm64/include/asm/kvm_host.h | 11 ++++++++++
>  arch/arm64/kvm/reset.c            | 24 +++++++++++++++++++++
>  virt/kvm/arm/arm.c                | 10 +++++++++
>  virt/kvm/arm/psci.c               | 36 ++++++++++++++-----------------
>  6 files changed, 95 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index ca56537b61bc..50e89869178a 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -48,6 +48,7 @@
>  #define KVM_REQ_SLEEP \
>  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
>  #define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
> +#define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
>  
>  DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
>  
> @@ -147,6 +148,13 @@ struct kvm_cpu_context {
>  
>  typedef struct kvm_cpu_context kvm_cpu_context_t;
>  
> +struct vcpu_reset_state {
> +	unsigned long	pc;
> +	unsigned long	r0;
> +	bool		be;
> +	bool		reset;
> +};
> +
>  struct kvm_vcpu_arch {
>  	struct kvm_cpu_context ctxt;
>  
> @@ -186,6 +194,8 @@ struct kvm_vcpu_arch {
>  	/* Cache some mmu pages needed inside spinlock regions */
>  	struct kvm_mmu_memory_cache mmu_page_cache;
>  
> +	struct vcpu_reset_state reset_state;
> +
>  	/* Detect first run of a vcpu */
>  	bool has_run_once;
>  };
> diff --git a/arch/arm/kvm/reset.c b/arch/arm/kvm/reset.c
> index 5ed0c3ee33d6..de41255eebcd 100644
> --- a/arch/arm/kvm/reset.c
> +++ b/arch/arm/kvm/reset.c
> @@ -26,6 +26,7 @@
>  #include <asm/cputype.h>
>  #include <asm/kvm_arm.h>
>  #include <asm/kvm_coproc.h>
> +#include <asm/kvm_emulate.h>
>  
>  #include <kvm/arm_arch_timer.h>
>  
> @@ -69,6 +70,29 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>  	/* Reset CP15 registers */
>  	kvm_reset_coprocs(vcpu);
>  
> +	/*
> +	 * Additional reset state handling that PSCI may have imposed on us.
> +	 * Must be done after all the sys_reg reset.
> +	 */
> +	if (vcpu->arch.reset_state.reset) {
> +		unsigned long target_pc = vcpu->arch.reset_state.pc;
> +
> +		/* Gracefully handle Thumb2 entry point */
> +		if (target_pc & 1) {
> +			target_pc &= ~1UL;
> +			vcpu_set_thumb(vcpu);
> +		}
> +
> +		/* Propagate caller endianness */
> +		if (vcpu->arch.reset_state.be)
> +			kvm_vcpu_set_be(vcpu);
> +
> +		*vcpu_pc(vcpu) = target_pc;
> +		vcpu_set_reg(vcpu, 0, vcpu->arch.reset_state.r0);
> +
> +		vcpu->arch.reset_state.reset = false;
> +	}
> +
>  	/* Reset arch_timer context */
>  	return kvm_timer_vcpu_reset(vcpu);
>  }
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 7732d0ba4e60..da3fc7324d68 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -48,6 +48,7 @@
>  #define KVM_REQ_SLEEP \
>  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
>  #define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
> +#define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
>  
>  DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
>  
> @@ -208,6 +209,13 @@ struct kvm_cpu_context {
>  
>  typedef struct kvm_cpu_context kvm_cpu_context_t;
>  
> +struct vcpu_reset_state {
> +	unsigned long	pc;
> +	unsigned long	r0;
> +	bool		be;
> +	bool		reset;
> +};
> +
>  struct kvm_vcpu_arch {
>  	struct kvm_cpu_context ctxt;
>  
> @@ -297,6 +305,9 @@ struct kvm_vcpu_arch {
>  	/* Virtual SError ESR to restore when HCR_EL2.VSE is set */
>  	u64 vsesr_el2;
>  
> +	/* Additional reset state */
> +	struct vcpu_reset_state	reset_state;
> +
>  	/* True when deferrable sysregs are loaded on the physical CPU,
>  	 * see kvm_vcpu_load_sysregs and kvm_vcpu_put_sysregs. */
>  	bool sysregs_loaded_on_cpu;
> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index f21a2a575939..f16a5f8ff2b4 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -32,6 +32,7 @@
>  #include <asm/kvm_arm.h>
>  #include <asm/kvm_asm.h>
>  #include <asm/kvm_coproc.h>
> +#include <asm/kvm_emulate.h>
>  #include <asm/kvm_mmu.h>
>  
>  /* Maximum phys_shift supported for any VM on this host */
> @@ -146,6 +147,29 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>  	/* Reset system registers */
>  	kvm_reset_sys_regs(vcpu);
>  
> +	/*
> +	 * Additional reset state handling that PSCI may have imposed on us.
> +	 * Must be done after all the sys_reg reset.
> +	 */
> +	if (vcpu->arch.reset_state.reset) {
> +		unsigned long target_pc = vcpu->arch.reset_state.pc;
> +
> +		/* Gracefully handle Thumb2 entry point */
> +		if (vcpu_mode_is_32bit(vcpu) && (target_pc & 1)) {
> +			target_pc &= ~1UL;
> +			vcpu_set_thumb(vcpu);
> +		}
> +
> +		/* Propagate caller endianness */
> +		if (vcpu->arch.reset_state.be)
> +			kvm_vcpu_set_be(vcpu);
> +
> +		*vcpu_pc(vcpu) = target_pc;
> +		vcpu_set_reg(vcpu, 0, vcpu->arch.reset_state.r0);
> +
> +		vcpu->arch.reset_state.reset = false;
> +	}
> +
>  	/* Reset PMU */
>  	kvm_pmu_vcpu_reset(vcpu);
>  
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index 9e350fd34504..9c486fad3f9f 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -626,6 +626,13 @@ static void vcpu_req_sleep(struct kvm_vcpu *vcpu)
>  		/* Awaken to handle a signal, request we sleep again later. */
>  		kvm_make_request(KVM_REQ_SLEEP, vcpu);
>  	}
> +
> +	/*
> +	 * Make sure we will observe a potential reset request if we've
> +	 * observed a change to the power state. Pairs with the smp_wmb() in
> +	 * kvm_psci_vcpu_on().
> +	 */
> +	smp_rmb();

I don't believe this should be necessary, because...

>  }
>  
>  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
> @@ -639,6 +646,9 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
>  		if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
>  			vcpu_req_sleep(vcpu);
>  
> +		if (kvm_check_request(KVM_REQ_VCPU_RESET, vcpu))
> +			kvm_reset_vcpu(vcpu);

... we do kvm_check_request here before using the reset data, and we do...

> +
>  		/*
>  		 * Clear IRQ_PENDING requests that were made to guarantee
>  		 * that a VCPU sees new virtual interrupts.
> diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
> index 9b73d3ad918a..b9cff1d4b06d 100644
> --- a/virt/kvm/arm/psci.c
> +++ b/virt/kvm/arm/psci.c
> @@ -104,12 +104,10 @@ static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu)
>  
>  static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>  {
> +	struct vcpu_reset_state *reset_state;
>  	struct kvm *kvm = source_vcpu->kvm;
>  	struct kvm_vcpu *vcpu = NULL;
> -	struct swait_queue_head *wq;
>  	unsigned long cpu_id;
> -	unsigned long context_id;
> -	phys_addr_t target_pc;
>  
>  	cpu_id = smccc_get_arg1(source_vcpu) & MPIDR_HWID_BITMASK;
>  	if (vcpu_mode_is_32bit(source_vcpu))
> @@ -130,32 +128,30 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>  			return PSCI_RET_INVALID_PARAMS;
>  	}
>  
> -	target_pc = smccc_get_arg2(source_vcpu);
> -	context_id = smccc_get_arg3(source_vcpu);
> +	reset_state = &vcpu->arch.reset_state;
>  
> -	kvm_reset_vcpu(vcpu);
> -
> -	/* Gracefully handle Thumb2 entry point */
> -	if (vcpu_mode_is_32bit(vcpu) && (target_pc & 1)) {
> -		target_pc &= ~((phys_addr_t) 1);
> -		vcpu_set_thumb(vcpu);
> -	}
> +	reset_state->pc = smccc_get_arg2(source_vcpu);
>  
>  	/* Propagate caller endianness */
> -	if (kvm_vcpu_is_be(source_vcpu))
> -		kvm_vcpu_set_be(vcpu);
> +	reset_state->be = kvm_vcpu_is_be(source_vcpu);
>  
> -	*vcpu_pc(vcpu) = target_pc;
>  	/*
>  	 * NOTE: We always update r0 (or x0) because for PSCI v0.1
>  	 * the general puspose registers are undefined upon CPU_ON.
>  	 */
> -	smccc_set_retval(vcpu, context_id, 0, 0, 0);
> -	vcpu->arch.power_off = false;
> -	smp_mb();		/* Make sure the above is visible */
> +	reset_state->r0 = smccc_get_arg3(source_vcpu);
> +
> +	reset_state->reset = true;
> +	kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);

... this kvm_make_request() after writing the reset data. The
kvm_make_request/kvm_check_request embeds the necessary barriers.

>  
> -	wq = kvm_arch_vcpu_wq(vcpu);
> -	swake_up_one(wq);
> +	/*
> +	 * Make sure the reset request is observed if the change to
> +	 * power_state is observed.
> +	 */
> +	smp_wmb();
> +
> +	vcpu->arch.power_off = false;

Or you want to tie the reset data to the observability of the power_off
state? Why not just tie it to the KVM_REQ_VCPU_RESET request?

> +	kvm_vcpu_wake_up(vcpu);
>  
>  	return PSCI_RET_SUCCESS;
>  }
> -- 
> 2.18.0
> 

Thanks,
drew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/5] KVM: arm/arm64: Reset the VCPU without preemption and vcpu state loaded

[Marc Zyngier]

Hi Drew,

On Tue, 29 Jan 2019 15:48:58 +0000,
Andrew Jones <drjones@redhat.com> wrote:
> 
> Hi Christoffer,
> 
> On Fri, Jan 25, 2019 at 10:46:52AM +0100, Christoffer Dall wrote:
> > Resetting the VCPU state modifies the system register state in memory,
> > but this may interact with vcpu_load/vcpu_put if running with preemption
> > disabled, which in turn may lead to corrupted system register state.
>   ^ enabled
> > 
> > Address this by disabling preemption and doing put/load if required
> > around the reset logic.
> 
> I'm having trouble understanding how disabling preemption helps here.
> There shouldn't be an issue with the KVM_ARM_VCPU_INIT case, since the
> target vcpu is guaranteed not to be loaded and therefore it doesn't
> have preempt notifiers registered either. Also, KVM_ARM_VCPU_INIT holds
> the vcpu mutex, so there's no chance for a load to occur until it's
> complete.
> 
> For the PSCI case it makes sense to force a vcpu load after the reset,
> otherwise the sleeping target vcpu won't have the correct state loaded.
> The initial put also makes sense in case we're not resetting everything.
> I don't understand how we're ensuring the target vcpu thread's preemption
> is disabled though. This modified kvm_reset_vcpu would need to be run
> from the target vcpu thread to work, but that's not how the PSCI code
> currently does it.

And that's exactly where we're going with the following patch in the
series. Ultimately, we need a vcpu to reset itself, as we otherwise
have a window where a vcpu can be spuriously loaded whilst being
reset.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/5] KVM: arm/arm64: Require VCPU threads to turn them self off

[Andrew Jones]

Summary edit

> KVM: arm/arm64: Require VCPU threads to turn them self off

themselves

On Fri, Jan 25, 2019 at 10:46:54AM +0100, Christoffer Dall wrote:
> To avoid a race between turning VCPUs off and turning them on, make sure
> that only the VCPU threat itself turns off the VCPU.  When other threads
> want to turn of a VCPU, they now do this via a request.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm/include/asm/kvm_host.h   |  2 ++
>  arch/arm64/include/asm/kvm_host.h |  2 ++
>  virt/kvm/arm/arm.c                |  8 ++++++--
>  virt/kvm/arm/psci.c               | 11 ++---------
>  4 files changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 50e89869178a..b1cfae222441 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -49,6 +49,8 @@
>  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
>  #define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
>  #define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
> +#define KVM_REQ_VCPU_OFF \
> +	KVM_ARCH_REQ_FLAGS(3, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
>  
>  DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
>  
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index da3fc7324d68..d43b13421987 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -49,6 +49,8 @@
>  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
>  #define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
>  #define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
> +#define KVM_REQ_VCPU_OFF \
> +	KVM_ARCH_REQ_FLAGS(3, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
>  
>  DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
>  
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index 9c486fad3f9f..785076176814 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -404,8 +404,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>  
>  static void vcpu_power_off(struct kvm_vcpu *vcpu)
>  {
> -	vcpu->arch.power_off = true;
> -	kvm_make_request(KVM_REQ_SLEEP, vcpu);
> +	kvm_make_request(KVM_REQ_VCPU_OFF, vcpu);
>  	kvm_vcpu_kick(vcpu);
>  }

I think we should leave this function alone. Otherwise if userspace sets
the MP state to STOPPED and then queries the state before the vcpu
has a chance to manage its vcpu requests, the state will still indicate
RUNNBLE. The same goes for a query right after doing a vcpu init.

>  
> @@ -646,6 +645,11 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
>  		if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
>  			vcpu_req_sleep(vcpu);
>  
> +		if (kvm_check_request(KVM_REQ_VCPU_OFF, vcpu)) {
> +			vcpu->arch.power_off = true;
> +			vcpu_req_sleep(vcpu);
> +		}
> +
>  		if (kvm_check_request(KVM_REQ_VCPU_RESET, vcpu))
>  			kvm_reset_vcpu(vcpu);
>  
> diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
> index b9cff1d4b06d..20255319e193 100644
> --- a/virt/kvm/arm/psci.c
> +++ b/virt/kvm/arm/psci.c
> @@ -97,9 +97,7 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)
>  
>  static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu)
>  {
> -	vcpu->arch.power_off = true;
> -	kvm_make_request(KVM_REQ_SLEEP, vcpu);
> -	kvm_vcpu_kick(vcpu);
> +	kvm_make_request(KVM_REQ_VCPU_OFF, vcpu);
>  }

This was currently fine since it implements CPU_OFF which only applies to
the calling vcpu, but there's also no reason not to change it to be
consistent with the below change

>  
>  static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> @@ -198,9 +196,6 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu)
>  
>  static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type)
>  {
> -	int i;
> -	struct kvm_vcpu *tmp;
> -
>  	/*
>  	 * The KVM ABI specifies that a system event exit may call KVM_RUN
>  	 * again and may perform shutdown/reboot at a later time that when the
> @@ -210,9 +205,7 @@ static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type)
>  	 * after this call is handled and before the VCPUs have been
>  	 * re-initialized.
>  	 */
> -	kvm_for_each_vcpu(i, tmp, vcpu->kvm)
> -		tmp->arch.power_off = true;
> -	kvm_make_all_cpus_request(vcpu->kvm, KVM_REQ_SLEEP);
> +	kvm_make_all_cpus_request(vcpu->kvm, KVM_REQ_VCPU_OFF);
>  
>  	memset(&vcpu->run->system_event, 0, sizeof(vcpu->run->system_event));
>  	vcpu->run->system_event.type = type;
> -- 
> 2.18.0
>

Thanks,
drew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/5] KVM: arm/arm64: Implement PSCI ON_PENDING when turning on VCPUs

[Andrew Jones]

On Fri, Jan 25, 2019 at 10:46:55AM +0100, Christoffer Dall wrote:
> We are currently not implementing the PSCI spec completely, as we do not
> take handle the situation where two VCPUs are attempting to turn on a
> third VCPU at the same time.  The PSCI implementation should make sure
> that only one requesting VCPU wins the race and that the other receives
> PSCI_RET_ON_PENDING.
> 
> Implement this by changing the VCPU power state to a tristate enum and
> ensure only a single VCPU can turn on another VCPU at a given time using
> a cmpxchg operation.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm/include/asm/kvm_host.h   | 10 ++++++++--
>  arch/arm64/include/asm/kvm_host.h | 10 ++++++++--
>  virt/kvm/arm/arm.c                | 24 +++++++++++++++---------
>  virt/kvm/arm/psci.c               | 21 ++++++++++++++-------
>  4 files changed, 45 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index b1cfae222441..4dc47fea1ac8 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -157,6 +157,12 @@ struct vcpu_reset_state {
>  	bool		reset;
>  };
>  
> +enum vcpu_power_state {
> +	KVM_ARM_VCPU_OFF,
> +	KVM_ARM_VCPU_ON_PENDING,
> +	KVM_ARM_VCPU_ON,
> +};
> +
>  struct kvm_vcpu_arch {
>  	struct kvm_cpu_context ctxt;
>  
> @@ -184,8 +190,8 @@ struct kvm_vcpu_arch {
>  	 * here.
>  	 */
>  
> -	/* vcpu power-off state */
> -	bool power_off;
> +	/* vcpu power state */
> +	enum vcpu_power_state power_state;
>  
>  	 /* Don't run the guest (internal implementation need) */
>  	bool pause;
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index d43b13421987..0647a409657b 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -218,6 +218,12 @@ struct vcpu_reset_state {
>  	bool		reset;
>  };
>  
> +enum vcpu_power_state {
> +	KVM_ARM_VCPU_OFF,
> +	KVM_ARM_VCPU_ON_PENDING,
> +	KVM_ARM_VCPU_ON,
> +};
> +
>  struct kvm_vcpu_arch {
>  	struct kvm_cpu_context ctxt;
>  
> @@ -285,8 +291,8 @@ struct kvm_vcpu_arch {
>  		u32	mdscr_el1;
>  	} guest_debug_preserved;
>  
> -	/* vcpu power-off state */
> -	bool power_off;
> +	/* vcpu power state */
> +	enum vcpu_power_state power_state;
>  
>  	/* Don't run the guest (internal implementation need) */
>  	bool pause;
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index 785076176814..1e3195155860 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -411,7 +411,7 @@ static void vcpu_power_off(struct kvm_vcpu *vcpu)
>  int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
>  				    struct kvm_mp_state *mp_state)
>  {
> -	if (vcpu->arch.power_off)
> +	if (vcpu->arch.power_state != KVM_ARM_VCPU_ON)
>  		mp_state->mp_state = KVM_MP_STATE_STOPPED;
>  	else
>  		mp_state->mp_state = KVM_MP_STATE_RUNNABLE;
> @@ -426,7 +426,7 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
>  
>  	switch (mp_state->mp_state) {
>  	case KVM_MP_STATE_RUNNABLE:
> -		vcpu->arch.power_off = false;
> +		vcpu->arch.power_state = KVM_ARM_VCPU_ON;
>  		break;
>  	case KVM_MP_STATE_STOPPED:
>  		vcpu_power_off(vcpu);
> @@ -448,8 +448,9 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
>  int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
>  {
>  	bool irq_lines = *vcpu_hcr(v) & (HCR_VI | HCR_VF);
> -	return ((irq_lines || kvm_vgic_vcpu_pending_irq(v))
> -		&& !v->arch.power_off && !v->arch.pause);
> +	return (irq_lines || kvm_vgic_vcpu_pending_irq(v)) &&
> +		v->arch.power_state == KVM_ARM_VCPU_ON &&
> +		!v->arch.pause;
>  }
>  
>  bool kvm_arch_vcpu_in_kernel(struct kvm_vcpu *vcpu)
> @@ -614,14 +615,19 @@ void kvm_arm_resume_guest(struct kvm *kvm)
>  	}
>  }
>  
> +static bool vcpu_sleeping(struct kvm_vcpu *vcpu)
> +{
> +	return vcpu->arch.power_state != KVM_ARM_VCPU_ON ||
> +		vcpu->arch.pause;
> +}
> +
>  static void vcpu_req_sleep(struct kvm_vcpu *vcpu)
>  {
>  	struct swait_queue_head *wq = kvm_arch_vcpu_wq(vcpu);
>  
> -	swait_event_interruptible_exclusive(*wq, ((!vcpu->arch.power_off) &&
> -				       (!vcpu->arch.pause)));
> +	swait_event_interruptible_exclusive(*wq, !vcpu_sleeping(vcpu));
>  
> -	if (vcpu->arch.power_off || vcpu->arch.pause) {
> +	if (vcpu_sleeping(vcpu)) {
>  		/* Awaken to handle a signal, request we sleep again later. */
>  		kvm_make_request(KVM_REQ_SLEEP, vcpu);
>  	}
> @@ -646,7 +652,7 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
>  			vcpu_req_sleep(vcpu);
>  
>  		if (kvm_check_request(KVM_REQ_VCPU_OFF, vcpu)) {
> -			vcpu->arch.power_off = true;
> +			vcpu->arch.power_state = KVM_ARM_VCPU_OFF;
>  			vcpu_req_sleep(vcpu);
>  		}
>  
> @@ -1016,7 +1022,7 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
>  	if (test_bit(KVM_ARM_VCPU_POWER_OFF, vcpu->arch.features))
>  		vcpu_power_off(vcpu);
>  	else
> -		vcpu->arch.power_off = false;
> +		vcpu->arch.power_state = KVM_ARM_VCPU_ON;
>  
>  	return 0;
>  }
> diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
> index 20255319e193..5c2d9eeb810c 100644
> --- a/virt/kvm/arm/psci.c
> +++ b/virt/kvm/arm/psci.c
> @@ -106,6 +106,7 @@ 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;
> +	enum vcpu_power_state old_power_state;
>  
>  	cpu_id = smccc_get_arg1(source_vcpu) & MPIDR_HWID_BITMASK;
>  	if (vcpu_mode_is_32bit(source_vcpu))
> @@ -119,12 +120,18 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>  	 */
>  	if (!vcpu)
>  		return PSCI_RET_INVALID_PARAMS;
> -	if (!vcpu->arch.power_off) {
> -		if (kvm_psci_version(source_vcpu, kvm) != KVM_ARM_PSCI_0_1)
> -			return PSCI_RET_ALREADY_ON;
> -		else
> +	old_power_state = cmpxchg(&vcpu->arch.power_state,
> +				  KVM_ARM_VCPU_OFF,
> +				  KVM_ARM_VCPU_ON_PENDING);
> +
> +	if (old_power_state != KVM_ARM_VCPU_OFF &&
> +	    kvm_psci_version(source_vcpu, kvm) == KVM_ARM_PSCI_0_1)
>  			return PSCI_RET_INVALID_PARAMS;
> -	}
> +
> +	if (old_power_state == KVM_ARM_VCPU_ON_PENDING)
> +			return PSCI_RET_ON_PENDING;
> +	else if (old_power_state == KVM_ARM_VCPU_ON)
> +			return PSCI_RET_ALREADY_ON;
>  
>  	reset_state = &vcpu->arch.reset_state;
>  
> @@ -148,7 +155,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>  	 */
>  	smp_wmb();
>  
> -	vcpu->arch.power_off = false;
> +	vcpu->arch.power_state = KVM_ARM_VCPU_ON;
>  	kvm_vcpu_wake_up(vcpu);
>  
>  	return PSCI_RET_SUCCESS;
> @@ -183,7 +190,7 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu)
>  		mpidr = kvm_vcpu_get_mpidr_aff(tmp);
>  		if ((mpidr & target_affinity_mask) == target_affinity) {
>  			matching_cpus++;
> -			if (!tmp->arch.power_off)
> +			if (tmp->arch.power_state == KVM_ARM_VCPU_ON)
>  				return PSCI_0_2_AFFINITY_LEVEL_ON;
>  		}
>  	}
> -- 
> 2.18.0
>

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/5] arm/arm64: KVM: Don't panic on failure to properly reset system registers

[Andrew Jones]

On Fri, Jan 25, 2019 at 10:46:56AM +0100, Christoffer Dall wrote:
> From: Marc Zyngier <marc.zyngier@arm.com>
> 
> Failing to properly reset system registers is pretty bad. But not
> quite as bad as bringing the whole machine down... So warn loudly,
> but slightly more gracefully.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> Acked-by: Christoffer Dall <christoffer.dall@arm.com>
> ---
>  arch/arm/kvm/coproc.c     | 4 ++--
>  arch/arm64/kvm/sys_regs.c | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> index 222c1635bc7a..e8bd288fd5be 100644
> --- a/arch/arm/kvm/coproc.c
> +++ b/arch/arm/kvm/coproc.c
> @@ -1450,6 +1450,6 @@ void kvm_reset_coprocs(struct kvm_vcpu *vcpu)
>  	reset_coproc_regs(vcpu, table, num);
>  
>  	for (num = 1; num < NR_CP15_REGS; num++)
> -		if (vcpu_cp15(vcpu, num) == 0x42424242)
> -			panic("Didn't reset vcpu_cp15(vcpu, %zi)", num);
> +		WARN(vcpu_cp15(vcpu, num) == 0x42424242,
> +		     "Didn't reset vcpu_cp15(vcpu, %zi)", num);
>  }
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 86096774abcd..4f067545c7d2 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -2609,6 +2609,6 @@ void kvm_reset_sys_regs(struct kvm_vcpu *vcpu)
>  	reset_sys_reg_descs(vcpu, table, num);
>  
>  	for (num = 1; num < NR_SYS_REGS; num++)
> -		if (__vcpu_sys_reg(vcpu, num) == 0x4242424242424242)
> -			panic("Didn't reset __vcpu_sys_reg(%zi)", num);
> +		WARN(__vcpu_sys_reg(vcpu, num) == 0x4242424242424242,
> +		     "Didn't reset __vcpu_sys_reg(%zi)\n", num);
>  }
> -- 
> 2.18.0
>

If we only get halfway through resetting, then we'll get a warn splat,
complete with a backtrace, for each register. Should we do something
like the following instead?

  for (num = 1; num < NR_SYS_REGS; num++)
     if (__vcpu_sys_reg(vcpu, num) == 0x4242424242424242)
        failed++;
  WARN(failed, "Didn't reset %d system registers", failed);

Thanks,
drew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/5] KVM: arm/arm64: Reset the VCPU without preemption and vcpu state loaded

[Christoffer Dall]

On Tue, Jan 29, 2019 at 04:05:25PM +0000, Marc Zyngier wrote:
> Hi Drew,
> 
> On Tue, 29 Jan 2019 15:48:58 +0000,
> Andrew Jones <drjones@redhat.com> wrote:
> > 
> > Hi Christoffer,
> > 
> > On Fri, Jan 25, 2019 at 10:46:52AM +0100, Christoffer Dall wrote:
> > > Resetting the VCPU state modifies the system register state in memory,
> > > but this may interact with vcpu_load/vcpu_put if running with preemption
> > > disabled, which in turn may lead to corrupted system register state.
> >   ^ enabled
> > > 
> > > Address this by disabling preemption and doing put/load if required
> > > around the reset logic.
> > 
> > I'm having trouble understanding how disabling preemption helps here.
> > There shouldn't be an issue with the KVM_ARM_VCPU_INIT case, since the
> > target vcpu is guaranteed not to be loaded and therefore it doesn't
> > have preempt notifiers registered either. Also, KVM_ARM_VCPU_INIT holds
> > the vcpu mutex, so there's no chance for a load to occur until it's
> > complete.
> > 
> > For the PSCI case it makes sense to force a vcpu load after the reset,
> > otherwise the sleeping target vcpu won't have the correct state loaded.
> > The initial put also makes sense in case we're not resetting everything.
> > I don't understand how we're ensuring the target vcpu thread's preemption
> > is disabled though. This modified kvm_reset_vcpu would need to be run
> > from the target vcpu thread to work, but that's not how the PSCI code
> > currently does it.
> 
> And that's exactly where we're going with the following patch in the
> series. Ultimately, we need a vcpu to reset itself, as we otherwise
> have a window where a vcpu can be spuriously loaded whilst being
> reset.
> 
FWIW, I think the confusion here comes from having re-ordered the
patches compared to how the commit text was originally written.  We
should probably explain in the commit message that this is in
preparation for doing the reset from the VCPU itself.

Thanks,

    Christoffer

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] arm/arm64: KVM: Allow a VCPU to fully reset itself

[Christoffer Dall]

On Tue, Jan 29, 2019 at 05:03:47PM +0100, Andrew Jones wrote:
> On Fri, Jan 25, 2019 at 10:46:53AM +0100, Christoffer Dall wrote:
> > From: Marc Zyngier <marc.zyngier@arm.com>
> > 
> > The current kvm_psci_vcpu_on implementation will directly try to
> > manipulate the state of the VCPU to reset it.  However, since this is
> > not done on the thread that runs the VCPU, we can end up in a strangely
> > corrupted state when the source and target VCPUs are running at the same
> > time.
> > 
> > Fix this by factoring out all reset logic from the PSCI implementation
> > and forwarding the required information along with a request to the
> > target VCPU.
> 
> The last patch makes more sense, now that I see this one. I guess their
> order should be swapped.
> 
> > 
> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> > ---
> >  arch/arm/include/asm/kvm_host.h   | 10 +++++++++
> >  arch/arm/kvm/reset.c              | 24 +++++++++++++++++++++
> >  arch/arm64/include/asm/kvm_host.h | 11 ++++++++++
> >  arch/arm64/kvm/reset.c            | 24 +++++++++++++++++++++
> >  virt/kvm/arm/arm.c                | 10 +++++++++
> >  virt/kvm/arm/psci.c               | 36 ++++++++++++++-----------------
> >  6 files changed, 95 insertions(+), 20 deletions(-)
> > 
> > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> > index ca56537b61bc..50e89869178a 100644
> > --- a/arch/arm/include/asm/kvm_host.h
> > +++ b/arch/arm/include/asm/kvm_host.h
> > @@ -48,6 +48,7 @@
> >  #define KVM_REQ_SLEEP \
> >  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> >  #define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
> > +#define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
> >  
> >  DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
> >  
> > @@ -147,6 +148,13 @@ struct kvm_cpu_context {
> >  
> >  typedef struct kvm_cpu_context kvm_cpu_context_t;
> >  
> > +struct vcpu_reset_state {
> > +	unsigned long	pc;
> > +	unsigned long	r0;
> > +	bool		be;
> > +	bool		reset;
> > +};
> > +
> >  struct kvm_vcpu_arch {
> >  	struct kvm_cpu_context ctxt;
> >  
> > @@ -186,6 +194,8 @@ struct kvm_vcpu_arch {
> >  	/* Cache some mmu pages needed inside spinlock regions */
> >  	struct kvm_mmu_memory_cache mmu_page_cache;
> >  
> > +	struct vcpu_reset_state reset_state;
> > +
> >  	/* Detect first run of a vcpu */
> >  	bool has_run_once;
> >  };
> > diff --git a/arch/arm/kvm/reset.c b/arch/arm/kvm/reset.c
> > index 5ed0c3ee33d6..de41255eebcd 100644
> > --- a/arch/arm/kvm/reset.c
> > +++ b/arch/arm/kvm/reset.c
> > @@ -26,6 +26,7 @@
> >  #include <asm/cputype.h>
> >  #include <asm/kvm_arm.h>
> >  #include <asm/kvm_coproc.h>
> > +#include <asm/kvm_emulate.h>
> >  
> >  #include <kvm/arm_arch_timer.h>
> >  
> > @@ -69,6 +70,29 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> >  	/* Reset CP15 registers */
> >  	kvm_reset_coprocs(vcpu);
> >  
> > +	/*
> > +	 * Additional reset state handling that PSCI may have imposed on us.
> > +	 * Must be done after all the sys_reg reset.
> > +	 */
> > +	if (vcpu->arch.reset_state.reset) {
> > +		unsigned long target_pc = vcpu->arch.reset_state.pc;
> > +
> > +		/* Gracefully handle Thumb2 entry point */
> > +		if (target_pc & 1) {
> > +			target_pc &= ~1UL;
> > +			vcpu_set_thumb(vcpu);
> > +		}
> > +
> > +		/* Propagate caller endianness */
> > +		if (vcpu->arch.reset_state.be)
> > +			kvm_vcpu_set_be(vcpu);
> > +
> > +		*vcpu_pc(vcpu) = target_pc;
> > +		vcpu_set_reg(vcpu, 0, vcpu->arch.reset_state.r0);
> > +
> > +		vcpu->arch.reset_state.reset = false;
> > +	}
> > +
> >  	/* Reset arch_timer context */
> >  	return kvm_timer_vcpu_reset(vcpu);
> >  }
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index 7732d0ba4e60..da3fc7324d68 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -48,6 +48,7 @@
> >  #define KVM_REQ_SLEEP \
> >  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> >  #define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
> > +#define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
> >  
> >  DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
> >  
> > @@ -208,6 +209,13 @@ struct kvm_cpu_context {
> >  
> >  typedef struct kvm_cpu_context kvm_cpu_context_t;
> >  
> > +struct vcpu_reset_state {
> > +	unsigned long	pc;
> > +	unsigned long	r0;
> > +	bool		be;
> > +	bool		reset;
> > +};
> > +
> >  struct kvm_vcpu_arch {
> >  	struct kvm_cpu_context ctxt;
> >  
> > @@ -297,6 +305,9 @@ struct kvm_vcpu_arch {
> >  	/* Virtual SError ESR to restore when HCR_EL2.VSE is set */
> >  	u64 vsesr_el2;
> >  
> > +	/* Additional reset state */
> > +	struct vcpu_reset_state	reset_state;
> > +
> >  	/* True when deferrable sysregs are loaded on the physical CPU,
> >  	 * see kvm_vcpu_load_sysregs and kvm_vcpu_put_sysregs. */
> >  	bool sysregs_loaded_on_cpu;
> > diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> > index f21a2a575939..f16a5f8ff2b4 100644
> > --- a/arch/arm64/kvm/reset.c
> > +++ b/arch/arm64/kvm/reset.c
> > @@ -32,6 +32,7 @@
> >  #include <asm/kvm_arm.h>
> >  #include <asm/kvm_asm.h>
> >  #include <asm/kvm_coproc.h>
> > +#include <asm/kvm_emulate.h>
> >  #include <asm/kvm_mmu.h>
> >  
> >  /* Maximum phys_shift supported for any VM on this host */
> > @@ -146,6 +147,29 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> >  	/* Reset system registers */
> >  	kvm_reset_sys_regs(vcpu);
> >  
> > +	/*
> > +	 * Additional reset state handling that PSCI may have imposed on us.
> > +	 * Must be done after all the sys_reg reset.
> > +	 */
> > +	if (vcpu->arch.reset_state.reset) {
> > +		unsigned long target_pc = vcpu->arch.reset_state.pc;
> > +
> > +		/* Gracefully handle Thumb2 entry point */
> > +		if (vcpu_mode_is_32bit(vcpu) && (target_pc & 1)) {
> > +			target_pc &= ~1UL;
> > +			vcpu_set_thumb(vcpu);
> > +		}
> > +
> > +		/* Propagate caller endianness */
> > +		if (vcpu->arch.reset_state.be)
> > +			kvm_vcpu_set_be(vcpu);
> > +
> > +		*vcpu_pc(vcpu) = target_pc;
> > +		vcpu_set_reg(vcpu, 0, vcpu->arch.reset_state.r0);
> > +
> > +		vcpu->arch.reset_state.reset = false;
> > +	}
> > +
> >  	/* Reset PMU */
> >  	kvm_pmu_vcpu_reset(vcpu);
> >  
> > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> > index 9e350fd34504..9c486fad3f9f 100644
> > --- a/virt/kvm/arm/arm.c
> > +++ b/virt/kvm/arm/arm.c
> > @@ -626,6 +626,13 @@ static void vcpu_req_sleep(struct kvm_vcpu *vcpu)
> >  		/* Awaken to handle a signal, request we sleep again later. */
> >  		kvm_make_request(KVM_REQ_SLEEP, vcpu);
> >  	}
> > +
> > +	/*
> > +	 * Make sure we will observe a potential reset request if we've
> > +	 * observed a change to the power state. Pairs with the smp_wmb() in
> > +	 * kvm_psci_vcpu_on().
> > +	 */
> > +	smp_rmb();
> 
> I don't believe this should be necessary, because...
> 
> >  }
> >  
> >  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
> > @@ -639,6 +646,9 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
> >  		if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
> >  			vcpu_req_sleep(vcpu);
> >  
> > +		if (kvm_check_request(KVM_REQ_VCPU_RESET, vcpu))
> > +			kvm_reset_vcpu(vcpu);
> 
> ... we do kvm_check_request here before using the reset data, and we do...
> 
> > +
> >  		/*
> >  		 * Clear IRQ_PENDING requests that were made to guarantee
> >  		 * that a VCPU sees new virtual interrupts.
> > diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
> > index 9b73d3ad918a..b9cff1d4b06d 100644
> > --- a/virt/kvm/arm/psci.c
> > +++ b/virt/kvm/arm/psci.c
> > @@ -104,12 +104,10 @@ static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu)
> >  
> >  static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> >  {
> > +	struct vcpu_reset_state *reset_state;
> >  	struct kvm *kvm = source_vcpu->kvm;
> >  	struct kvm_vcpu *vcpu = NULL;
> > -	struct swait_queue_head *wq;
> >  	unsigned long cpu_id;
> > -	unsigned long context_id;
> > -	phys_addr_t target_pc;
> >  
> >  	cpu_id = smccc_get_arg1(source_vcpu) & MPIDR_HWID_BITMASK;
> >  	if (vcpu_mode_is_32bit(source_vcpu))
> > @@ -130,32 +128,30 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> >  			return PSCI_RET_INVALID_PARAMS;
> >  	}
> >  
> > -	target_pc = smccc_get_arg2(source_vcpu);
> > -	context_id = smccc_get_arg3(source_vcpu);
> > +	reset_state = &vcpu->arch.reset_state;
> >  
> > -	kvm_reset_vcpu(vcpu);
> > -
> > -	/* Gracefully handle Thumb2 entry point */
> > -	if (vcpu_mode_is_32bit(vcpu) && (target_pc & 1)) {
> > -		target_pc &= ~((phys_addr_t) 1);
> > -		vcpu_set_thumb(vcpu);
> > -	}
> > +	reset_state->pc = smccc_get_arg2(source_vcpu);
> >  
> >  	/* Propagate caller endianness */
> > -	if (kvm_vcpu_is_be(source_vcpu))
> > -		kvm_vcpu_set_be(vcpu);
> > +	reset_state->be = kvm_vcpu_is_be(source_vcpu);
> >  
> > -	*vcpu_pc(vcpu) = target_pc;
> >  	/*
> >  	 * NOTE: We always update r0 (or x0) because for PSCI v0.1
> >  	 * the general puspose registers are undefined upon CPU_ON.
> >  	 */
> > -	smccc_set_retval(vcpu, context_id, 0, 0, 0);
> > -	vcpu->arch.power_off = false;
> > -	smp_mb();		/* Make sure the above is visible */
> > +	reset_state->r0 = smccc_get_arg3(source_vcpu);
> > +
> > +	reset_state->reset = true;
> > +	kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
> 
> ... this kvm_make_request() after writing the reset data. The
> kvm_make_request/kvm_check_request embeds the necessary barriers.
> 
> >  
> > -	wq = kvm_arch_vcpu_wq(vcpu);
> > -	swake_up_one(wq);
> > +	/*
> > +	 * Make sure the reset request is observed if the change to
> > +	 * power_state is observed.
> > +	 */
> > +	smp_wmb();
> > +
> > +	vcpu->arch.power_off = false;
> 
> Or you want to tie the reset data to the observability of the power_off
> state? Why not just tie it to the KVM_REQ_VCPU_RESET request?
> 

The barrier in kvm_make_request is *before* the write to vcpu->requests,
so doesn't enforce any ordering of stores occurring *after* that write.

We don't want to wake up the target VCPU thread, have it observe that it
is powered on, yet not observe the reset request (my comment above
inversed).

I wrote this Litmus test (which is probably so appalingly obvious to
memory ordering folks that it is unnecessary) to convince myself the
barrier is needed:

----<8----
C vcpu-reset

{}

P0(int *power, int *reset)
{
	smp_wmb(); // from kvm_make_request
	WRITE_ONCE(*reset, 1); // kvm_psci_vcpu_on: kvm_make_request(KVM_REQ_RESET, vcpu);
	smp_wmb();
	WRITE_ONCE(*power, 1); // kvm_psci_vcpu_on: vcpu->arch.power_state = KVM_ARM_VCPU_ON;
}

P1(int *power, int *reset, spinlock_t *wqlock)
{
	int r0;
	int r1;

	spin_lock(wqlock);
	smp_mb(); // from prepare_to_wait -> set_current_state
	spin_unlock(wqlock);
	r0 = READ_ONCE(*power); // vcpu_req_sleep: if (vcpu_sleeping(vcpu))
	smp_rmb();
	r1 = READ_ONCE(*reset); // check_vcpu_requests: if (kvm_check_request(KVM_REQ_RESET, vcpu))
}

exists (1:r0=1 /\ 1:r1=0)
----<8----


Hope this helps,

    Christoffer

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/5] KVM: arm/arm64: Require VCPU threads to turn them self off

[Christoffer Dall]

On Tue, Jan 29, 2019 at 05:16:44PM +0100, Andrew Jones wrote:
> 
> Summary edit
> 
> > KVM: arm/arm64: Require VCPU threads to turn them self off
> 
> themselves
> 
> On Fri, Jan 25, 2019 at 10:46:54AM +0100, Christoffer Dall wrote:
> > To avoid a race between turning VCPUs off and turning them on, make sure
> > that only the VCPU threat itself turns off the VCPU.  When other threads
> > want to turn of a VCPU, they now do this via a request.
> > 
> > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> > Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> > ---
> >  arch/arm/include/asm/kvm_host.h   |  2 ++
> >  arch/arm64/include/asm/kvm_host.h |  2 ++
> >  virt/kvm/arm/arm.c                |  8 ++++++--
> >  virt/kvm/arm/psci.c               | 11 ++---------
> >  4 files changed, 12 insertions(+), 11 deletions(-)
> > 
> > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> > index 50e89869178a..b1cfae222441 100644
> > --- a/arch/arm/include/asm/kvm_host.h
> > +++ b/arch/arm/include/asm/kvm_host.h
> > @@ -49,6 +49,8 @@
> >  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> >  #define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
> >  #define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
> > +#define KVM_REQ_VCPU_OFF \
> > +	KVM_ARCH_REQ_FLAGS(3, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> >  
> >  DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
> >  
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index da3fc7324d68..d43b13421987 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -49,6 +49,8 @@
> >  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> >  #define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
> >  #define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
> > +#define KVM_REQ_VCPU_OFF \
> > +	KVM_ARCH_REQ_FLAGS(3, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> >  
> >  DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
> >  
> > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> > index 9c486fad3f9f..785076176814 100644
> > --- a/virt/kvm/arm/arm.c
> > +++ b/virt/kvm/arm/arm.c
> > @@ -404,8 +404,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
> >  
> >  static void vcpu_power_off(struct kvm_vcpu *vcpu)
> >  {
> > -	vcpu->arch.power_off = true;
> > -	kvm_make_request(KVM_REQ_SLEEP, vcpu);
> > +	kvm_make_request(KVM_REQ_VCPU_OFF, vcpu);
> >  	kvm_vcpu_kick(vcpu);
> >  }
> 
> I think we should leave this function alone. Otherwise if userspace sets
> the MP state to STOPPED and then queries the state before the vcpu
> has a chance to manage its vcpu requests, the state will still indicate
> RUNNBLE. The same goes for a query right after doing a vcpu init.
> 

We can't leave this alone, because that could lead to userspace racing
with two PSCI_VCPU_ON requests which could then both enter the critical
section gated only by the cmpxchg in kvm_psci_vcpu_on.

But we could do something like this (completely untested):


diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 1e3195155860..538b5eb9d920 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -404,6 +404,17 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 
 static void vcpu_power_off(struct kvm_vcpu *vcpu)
 {
+	enum vcpu_power_state old_power_state;
+
+	/*
+	 * Set power_state directly to reflect the power state back to user
+	 * space even when the VCPU thread has not had a chance to run, but
+	 * only if this doesn't accidentally allow interleaved PSCI_VCPU_ON
+	 * requests.
+	 */
+	old_power_state = cmpxchg(&vcpu->arch.power_state,
+				  KVM_ARM_VCPU_ON,
+				  KVM_ARM_VCPU_OFF);
 	kvm_make_request(KVM_REQ_VCPU_OFF, vcpu);
 	kvm_vcpu_kick(vcpu);
 }


> >  
> > @@ -646,6 +645,11 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
> >  		if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
> >  			vcpu_req_sleep(vcpu);
> >  
> > +		if (kvm_check_request(KVM_REQ_VCPU_OFF, vcpu)) {
> > +			vcpu->arch.power_off = true;
> > +			vcpu_req_sleep(vcpu);
> > +		}
> > +
> >  		if (kvm_check_request(KVM_REQ_VCPU_RESET, vcpu))
> >  			kvm_reset_vcpu(vcpu);
> >  
> > diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
> > index b9cff1d4b06d..20255319e193 100644
> > --- a/virt/kvm/arm/psci.c
> > +++ b/virt/kvm/arm/psci.c
> > @@ -97,9 +97,7 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)
> >  
> >  static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu)
> >  {
> > -	vcpu->arch.power_off = true;
> > -	kvm_make_request(KVM_REQ_SLEEP, vcpu);
> > -	kvm_vcpu_kick(vcpu);
> > +	kvm_make_request(KVM_REQ_VCPU_OFF, vcpu);
> >  }
> 
> This was currently fine since it implements CPU_OFF which only applies to
> the calling vcpu, but there's also no reason not to change it to be
> consistent with the below change
> 

Same problem as above, we don't want two VCPUs to be messing with a
target VCPU state at the same time.


Thanks,

    Christoffer

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/5] arm/arm64: KVM: Don't panic on failure to properly reset system registers

[Christoffer Dall]

On Tue, Jan 29, 2019 at 05:33:37PM +0100, Andrew Jones wrote:
> On Fri, Jan 25, 2019 at 10:46:56AM +0100, Christoffer Dall wrote:
> > From: Marc Zyngier <marc.zyngier@arm.com>
> > 
> > Failing to properly reset system registers is pretty bad. But not
> > quite as bad as bringing the whole machine down... So warn loudly,
> > but slightly more gracefully.
> > 
> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > Acked-by: Christoffer Dall <christoffer.dall@arm.com>
> > ---
> >  arch/arm/kvm/coproc.c     | 4 ++--
> >  arch/arm64/kvm/sys_regs.c | 4 ++--
> >  2 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> > index 222c1635bc7a..e8bd288fd5be 100644
> > --- a/arch/arm/kvm/coproc.c
> > +++ b/arch/arm/kvm/coproc.c
> > @@ -1450,6 +1450,6 @@ void kvm_reset_coprocs(struct kvm_vcpu *vcpu)
> >  	reset_coproc_regs(vcpu, table, num);
> >  
> >  	for (num = 1; num < NR_CP15_REGS; num++)
> > -		if (vcpu_cp15(vcpu, num) == 0x42424242)
> > -			panic("Didn't reset vcpu_cp15(vcpu, %zi)", num);
> > +		WARN(vcpu_cp15(vcpu, num) == 0x42424242,
> > +		     "Didn't reset vcpu_cp15(vcpu, %zi)", num);
> >  }
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 86096774abcd..4f067545c7d2 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -2609,6 +2609,6 @@ void kvm_reset_sys_regs(struct kvm_vcpu *vcpu)
> >  	reset_sys_reg_descs(vcpu, table, num);
> >  
> >  	for (num = 1; num < NR_SYS_REGS; num++)
> > -		if (__vcpu_sys_reg(vcpu, num) == 0x4242424242424242)
> > -			panic("Didn't reset __vcpu_sys_reg(%zi)", num);
> > +		WARN(__vcpu_sys_reg(vcpu, num) == 0x4242424242424242,
> > +		     "Didn't reset __vcpu_sys_reg(%zi)\n", num);
> >  }
> > -- 
> > 2.18.0
> >
> 
> If we only get halfway through resetting, then we'll get a warn splat,
> complete with a backtrace, for each register. Should we do something
> like the following instead?
> 
>   for (num = 1; num < NR_SYS_REGS; num++)
>      if (__vcpu_sys_reg(vcpu, num) == 0x4242424242424242)
>         failed++;
>   WARN(failed, "Didn't reset %d system registers", failed);
> 

I don't care stongly whichever way we do it, but when we actually saw
this, it seemed useful to see which system register was not initialized.

Thanks for the review.

    Christoffer

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/5] arm/arm64: KVM: Don't panic on failure to properly reset system registers

[Marc Zyngier]

On 29/01/2019 16:33, Andrew Jones wrote:
> On Fri, Jan 25, 2019 at 10:46:56AM +0100, Christoffer Dall wrote:
>> From: Marc Zyngier <marc.zyngier@arm.com>
>>
>> Failing to properly reset system registers is pretty bad. But not
>> quite as bad as bringing the whole machine down... So warn loudly,
>> but slightly more gracefully.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> Acked-by: Christoffer Dall <christoffer.dall@arm.com>
>> ---
>>  arch/arm/kvm/coproc.c     | 4 ++--
>>  arch/arm64/kvm/sys_regs.c | 4 ++--
>>  2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
>> index 222c1635bc7a..e8bd288fd5be 100644
>> --- a/arch/arm/kvm/coproc.c
>> +++ b/arch/arm/kvm/coproc.c
>> @@ -1450,6 +1450,6 @@ void kvm_reset_coprocs(struct kvm_vcpu *vcpu)
>>  	reset_coproc_regs(vcpu, table, num);
>>  
>>  	for (num = 1; num < NR_CP15_REGS; num++)
>> -		if (vcpu_cp15(vcpu, num) == 0x42424242)
>> -			panic("Didn't reset vcpu_cp15(vcpu, %zi)", num);
>> +		WARN(vcpu_cp15(vcpu, num) == 0x42424242,
>> +		     "Didn't reset vcpu_cp15(vcpu, %zi)", num);
>>  }
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 86096774abcd..4f067545c7d2 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -2609,6 +2609,6 @@ void kvm_reset_sys_regs(struct kvm_vcpu *vcpu)
>>  	reset_sys_reg_descs(vcpu, table, num);
>>  
>>  	for (num = 1; num < NR_SYS_REGS; num++)
>> -		if (__vcpu_sys_reg(vcpu, num) == 0x4242424242424242)
>> -			panic("Didn't reset __vcpu_sys_reg(%zi)", num);
>> +		WARN(__vcpu_sys_reg(vcpu, num) == 0x4242424242424242,
>> +		     "Didn't reset __vcpu_sys_reg(%zi)\n", num);
>>  }
>> -- 
>> 2.18.0
>>
> 
> If we only get halfway through resetting, then we'll get a warn splat,
> complete with a backtrace, for each register. Should we do something
> like the following instead?
> 
>   for (num = 1; num < NR_SYS_REGS; num++)
>      if (__vcpu_sys_reg(vcpu, num) == 0x4242424242424242)
>         failed++;
>   WARN(failed, "Didn't reset %d system registers", failed);

And doing so we'd loose the important bit of information, which is the
position in the table that doesn't get initialized.

The vcpu reset issue is rare enough that nobody noticed it yet (I only
reproduced it twice), and having it to scream 200 times is not really a
concern. What I want to catch is the case where someone has added a new
sysreg in the table, and has failed to provide a working init function.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] arm/arm64: KVM: Allow a VCPU to fully reset itself

[Andrew Jones]

On Wed, Jan 30, 2019 at 10:34:31AM +0100, Christoffer Dall wrote:
> On Tue, Jan 29, 2019 at 05:03:47PM +0100, Andrew Jones wrote:
> > On Fri, Jan 25, 2019 at 10:46:53AM +0100, Christoffer Dall wrote:
> > > From: Marc Zyngier <marc.zyngier@arm.com>
> > > 
> > > The current kvm_psci_vcpu_on implementation will directly try to
> > > manipulate the state of the VCPU to reset it.  However, since this is
> > > not done on the thread that runs the VCPU, we can end up in a strangely
> > > corrupted state when the source and target VCPUs are running at the same
> > > time.
> > > 
> > > Fix this by factoring out all reset logic from the PSCI implementation
> > > and forwarding the required information along with a request to the
> > > target VCPU.
> > 
> > The last patch makes more sense, now that I see this one. I guess their
> > order should be swapped.
> > 
> > > 
> > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> > > ---
> > >  arch/arm/include/asm/kvm_host.h   | 10 +++++++++
> > >  arch/arm/kvm/reset.c              | 24 +++++++++++++++++++++
> > >  arch/arm64/include/asm/kvm_host.h | 11 ++++++++++
> > >  arch/arm64/kvm/reset.c            | 24 +++++++++++++++++++++
> > >  virt/kvm/arm/arm.c                | 10 +++++++++
> > >  virt/kvm/arm/psci.c               | 36 ++++++++++++++-----------------
> > >  6 files changed, 95 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> > > index ca56537b61bc..50e89869178a 100644
> > > --- a/arch/arm/include/asm/kvm_host.h
> > > +++ b/arch/arm/include/asm/kvm_host.h
> > > @@ -48,6 +48,7 @@
> > >  #define KVM_REQ_SLEEP \
> > >  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > >  #define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
> > > +#define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
> > >  
> > >  DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
> > >  
> > > @@ -147,6 +148,13 @@ struct kvm_cpu_context {
> > >  
> > >  typedef struct kvm_cpu_context kvm_cpu_context_t;
> > >  
> > > +struct vcpu_reset_state {
> > > +	unsigned long	pc;
> > > +	unsigned long	r0;
> > > +	bool		be;
> > > +	bool		reset;
> > > +};
> > > +
> > >  struct kvm_vcpu_arch {
> > >  	struct kvm_cpu_context ctxt;
> > >  
> > > @@ -186,6 +194,8 @@ struct kvm_vcpu_arch {
> > >  	/* Cache some mmu pages needed inside spinlock regions */
> > >  	struct kvm_mmu_memory_cache mmu_page_cache;
> > >  
> > > +	struct vcpu_reset_state reset_state;
> > > +
> > >  	/* Detect first run of a vcpu */
> > >  	bool has_run_once;
> > >  };
> > > diff --git a/arch/arm/kvm/reset.c b/arch/arm/kvm/reset.c
> > > index 5ed0c3ee33d6..de41255eebcd 100644
> > > --- a/arch/arm/kvm/reset.c
> > > +++ b/arch/arm/kvm/reset.c
> > > @@ -26,6 +26,7 @@
> > >  #include <asm/cputype.h>
> > >  #include <asm/kvm_arm.h>
> > >  #include <asm/kvm_coproc.h>
> > > +#include <asm/kvm_emulate.h>
> > >  
> > >  #include <kvm/arm_arch_timer.h>
> > >  
> > > @@ -69,6 +70,29 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> > >  	/* Reset CP15 registers */
> > >  	kvm_reset_coprocs(vcpu);
> > >  
> > > +	/*
> > > +	 * Additional reset state handling that PSCI may have imposed on us.
> > > +	 * Must be done after all the sys_reg reset.
> > > +	 */
> > > +	if (vcpu->arch.reset_state.reset) {
> > > +		unsigned long target_pc = vcpu->arch.reset_state.pc;
> > > +
> > > +		/* Gracefully handle Thumb2 entry point */
> > > +		if (target_pc & 1) {
> > > +			target_pc &= ~1UL;
> > > +			vcpu_set_thumb(vcpu);
> > > +		}
> > > +
> > > +		/* Propagate caller endianness */
> > > +		if (vcpu->arch.reset_state.be)
> > > +			kvm_vcpu_set_be(vcpu);
> > > +
> > > +		*vcpu_pc(vcpu) = target_pc;
> > > +		vcpu_set_reg(vcpu, 0, vcpu->arch.reset_state.r0);
> > > +
> > > +		vcpu->arch.reset_state.reset = false;
> > > +	}
> > > +
> > >  	/* Reset arch_timer context */
> > >  	return kvm_timer_vcpu_reset(vcpu);
> > >  }
> > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > > index 7732d0ba4e60..da3fc7324d68 100644
> > > --- a/arch/arm64/include/asm/kvm_host.h
> > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > @@ -48,6 +48,7 @@
> > >  #define KVM_REQ_SLEEP \
> > >  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > >  #define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
> > > +#define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
> > >  
> > >  DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
> > >  
> > > @@ -208,6 +209,13 @@ struct kvm_cpu_context {
> > >  
> > >  typedef struct kvm_cpu_context kvm_cpu_context_t;
> > >  
> > > +struct vcpu_reset_state {
> > > +	unsigned long	pc;
> > > +	unsigned long	r0;
> > > +	bool		be;
> > > +	bool		reset;
> > > +};
> > > +
> > >  struct kvm_vcpu_arch {
> > >  	struct kvm_cpu_context ctxt;
> > >  
> > > @@ -297,6 +305,9 @@ struct kvm_vcpu_arch {
> > >  	/* Virtual SError ESR to restore when HCR_EL2.VSE is set */
> > >  	u64 vsesr_el2;
> > >  
> > > +	/* Additional reset state */
> > > +	struct vcpu_reset_state	reset_state;
> > > +
> > >  	/* True when deferrable sysregs are loaded on the physical CPU,
> > >  	 * see kvm_vcpu_load_sysregs and kvm_vcpu_put_sysregs. */
> > >  	bool sysregs_loaded_on_cpu;
> > > diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> > > index f21a2a575939..f16a5f8ff2b4 100644
> > > --- a/arch/arm64/kvm/reset.c
> > > +++ b/arch/arm64/kvm/reset.c
> > > @@ -32,6 +32,7 @@
> > >  #include <asm/kvm_arm.h>
> > >  #include <asm/kvm_asm.h>
> > >  #include <asm/kvm_coproc.h>
> > > +#include <asm/kvm_emulate.h>
> > >  #include <asm/kvm_mmu.h>
> > >  
> > >  /* Maximum phys_shift supported for any VM on this host */
> > > @@ -146,6 +147,29 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> > >  	/* Reset system registers */
> > >  	kvm_reset_sys_regs(vcpu);
> > >  
> > > +	/*
> > > +	 * Additional reset state handling that PSCI may have imposed on us.
> > > +	 * Must be done after all the sys_reg reset.
> > > +	 */
> > > +	if (vcpu->arch.reset_state.reset) {
> > > +		unsigned long target_pc = vcpu->arch.reset_state.pc;
> > > +
> > > +		/* Gracefully handle Thumb2 entry point */
> > > +		if (vcpu_mode_is_32bit(vcpu) && (target_pc & 1)) {
> > > +			target_pc &= ~1UL;
> > > +			vcpu_set_thumb(vcpu);
> > > +		}
> > > +
> > > +		/* Propagate caller endianness */
> > > +		if (vcpu->arch.reset_state.be)
> > > +			kvm_vcpu_set_be(vcpu);
> > > +
> > > +		*vcpu_pc(vcpu) = target_pc;
> > > +		vcpu_set_reg(vcpu, 0, vcpu->arch.reset_state.r0);
> > > +
> > > +		vcpu->arch.reset_state.reset = false;
> > > +	}
> > > +
> > >  	/* Reset PMU */
> > >  	kvm_pmu_vcpu_reset(vcpu);
> > >  
> > > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> > > index 9e350fd34504..9c486fad3f9f 100644
> > > --- a/virt/kvm/arm/arm.c
> > > +++ b/virt/kvm/arm/arm.c
> > > @@ -626,6 +626,13 @@ static void vcpu_req_sleep(struct kvm_vcpu *vcpu)
> > >  		/* Awaken to handle a signal, request we sleep again later. */
> > >  		kvm_make_request(KVM_REQ_SLEEP, vcpu);
> > >  	}
> > > +
> > > +	/*
> > > +	 * Make sure we will observe a potential reset request if we've
> > > +	 * observed a change to the power state. Pairs with the smp_wmb() in
> > > +	 * kvm_psci_vcpu_on().
> > > +	 */
> > > +	smp_rmb();
> > 
> > I don't believe this should be necessary, because...
> > 
> > >  }
> > >  
> > >  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
> > > @@ -639,6 +646,9 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
> > >  		if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
> > >  			vcpu_req_sleep(vcpu);
> > >  
> > > +		if (kvm_check_request(KVM_REQ_VCPU_RESET, vcpu))
> > > +			kvm_reset_vcpu(vcpu);
> > 
> > ... we do kvm_check_request here before using the reset data, and we do...
> > 
> > > +
> > >  		/*
> > >  		 * Clear IRQ_PENDING requests that were made to guarantee
> > >  		 * that a VCPU sees new virtual interrupts.
> > > diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
> > > index 9b73d3ad918a..b9cff1d4b06d 100644
> > > --- a/virt/kvm/arm/psci.c
> > > +++ b/virt/kvm/arm/psci.c
> > > @@ -104,12 +104,10 @@ static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu)
> > >  
> > >  static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> > >  {
> > > +	struct vcpu_reset_state *reset_state;
> > >  	struct kvm *kvm = source_vcpu->kvm;
> > >  	struct kvm_vcpu *vcpu = NULL;
> > > -	struct swait_queue_head *wq;
> > >  	unsigned long cpu_id;
> > > -	unsigned long context_id;
> > > -	phys_addr_t target_pc;
> > >  
> > >  	cpu_id = smccc_get_arg1(source_vcpu) & MPIDR_HWID_BITMASK;
> > >  	if (vcpu_mode_is_32bit(source_vcpu))
> > > @@ -130,32 +128,30 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> > >  			return PSCI_RET_INVALID_PARAMS;
> > >  	}
> > >  
> > > -	target_pc = smccc_get_arg2(source_vcpu);
> > > -	context_id = smccc_get_arg3(source_vcpu);
> > > +	reset_state = &vcpu->arch.reset_state;
> > >  
> > > -	kvm_reset_vcpu(vcpu);
> > > -
> > > -	/* Gracefully handle Thumb2 entry point */
> > > -	if (vcpu_mode_is_32bit(vcpu) && (target_pc & 1)) {
> > > -		target_pc &= ~((phys_addr_t) 1);
> > > -		vcpu_set_thumb(vcpu);
> > > -	}
> > > +	reset_state->pc = smccc_get_arg2(source_vcpu);
> > >  
> > >  	/* Propagate caller endianness */
> > > -	if (kvm_vcpu_is_be(source_vcpu))
> > > -		kvm_vcpu_set_be(vcpu);
> > > +	reset_state->be = kvm_vcpu_is_be(source_vcpu);
> > >  
> > > -	*vcpu_pc(vcpu) = target_pc;
> > >  	/*
> > >  	 * NOTE: We always update r0 (or x0) because for PSCI v0.1
> > >  	 * the general puspose registers are undefined upon CPU_ON.
> > >  	 */
> > > -	smccc_set_retval(vcpu, context_id, 0, 0, 0);
> > > -	vcpu->arch.power_off = false;
> > > -	smp_mb();		/* Make sure the above is visible */
> > > +	reset_state->r0 = smccc_get_arg3(source_vcpu);
> > > +
> > > +	reset_state->reset = true;
> > > +	kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
> > 
> > ... this kvm_make_request() after writing the reset data. The
> > kvm_make_request/kvm_check_request embeds the necessary barriers.
> > 
> > >  
> > > -	wq = kvm_arch_vcpu_wq(vcpu);
> > > -	swake_up_one(wq);
> > > +	/*
> > > +	 * Make sure the reset request is observed if the change to
> > > +	 * power_state is observed.
> > > +	 */
> > > +	smp_wmb();
> > > +
> > > +	vcpu->arch.power_off = false;
> > 
> > Or you want to tie the reset data to the observability of the power_off
> > state? Why not just tie it to the KVM_REQ_VCPU_RESET request?
> > 
> 
> The barrier in kvm_make_request is *before* the write to vcpu->requests,
> so doesn't enforce any ordering of stores occurring *after* that write.
> 
> We don't want to wake up the target VCPU thread, have it observe that it
> is powered on, yet not observe the reset request (my comment above
> inversed).
> 
> I wrote this Litmus test (which is probably so appalingly obvious to
> memory ordering folks that it is unnecessary) to convince myself the
> barrier is needed:
> 
> ----<8----
> C vcpu-reset
> 
> {}
> 
> P0(int *power, int *reset)
> {
> 	smp_wmb(); // from kvm_make_request
> 	WRITE_ONCE(*reset, 1); // kvm_psci_vcpu_on: kvm_make_request(KVM_REQ_RESET, vcpu);
> 	smp_wmb();
> 	WRITE_ONCE(*power, 1); // kvm_psci_vcpu_on: vcpu->arch.power_state = KVM_ARM_VCPU_ON;
> }
> 
> P1(int *power, int *reset, spinlock_t *wqlock)
> {
> 	int r0;
> 	int r1;
> 
> 	spin_lock(wqlock);
> 	smp_mb(); // from prepare_to_wait -> set_current_state
> 	spin_unlock(wqlock);
> 	r0 = READ_ONCE(*power); // vcpu_req_sleep: if (vcpu_sleeping(vcpu))
> 	smp_rmb();
> 	r1 = READ_ONCE(*reset); // check_vcpu_requests: if (kvm_check_request(KVM_REQ_RESET, vcpu))
> }
> 
> exists (1:r0=1 /\ 1:r1=0)
> ----<8----
> 
> 
> Hope this helps,

It certainly does, mostly because I didn't review closely enough and
thought the pair we were interested in was vcpu_reset_state and the
RESET vcpu request, which the vcpu request API handles. Now it's
clear we're worried about the pair power_off and the RESET vcpu
request. And we've reversed the usual "if we observe a request, then
we can observe its accompanying data" to "if we observe a change in
some data (power_off), then we need to also observe a request". I
agree the barriers are necessary to ensure that in the direct
sequence, but now I'm wondering if we need to care about the
direct sequence.

If we remove the barriers then the vcpu could have these possible
sequences

1)
  - wake up
  - observe power_off is false
  - observe the RESET vcpu request - do the reset
  - enter guest

That's the one this patch ensures, so it's definitely correct.

2)
  - wake up
  - the change to power_off not observed - the vcpu makes the
    SLEEP vcpu request on itself
  - observe the RESET vcpu request - do the reset
  - observe the SLEEP vcpu request (with or without the need for
    an IPI), attempt to sleep again, but now observe power_off
    is false
  - enter guest

I think that should be correct as well, even if less efficient.

3)
  - wake up
  - observe power_off is false
  - don't observe the RESET request yet, get closer to entering
    the guest
  - observe the RESET request a bit later (with or without the
    need for an IPI) - do the reset
  - enter guest

This is the same as (1), but we rely on the kvm_request_pending
stuff to make sure the vcpu request is seen before entering the
guest.

4)
  - wake up
  - the change to power_off not observed - the vcpu makes the
    SLEEP vcpu request on itself
  - don't observe the RESET request yet, get closer to entering
    the guest
  - observe the SLEEP vcpu request (with or without the need for
    an IPI), attempt to sleep again, but now observe power_off
    is false
  - observe the RESET request a bit later (with or without the
    need for an IPI) - do the reset
  - enter guest

The least efficient one, but should still be correct.

If you agree with this, then we get to remove the barriers,
simplifying things, and also we don't introduce the ordering
dependency in check_vcpu_requests(), where the vcpu_req_sleep()
calls must now come before checking the RESET request.

Thanks,
drew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/5] KVM: arm/arm64: Require VCPU threads to turn them self off

[Andrew Jones]

On Wed, Jan 30, 2019 at 10:49:09AM +0100, Christoffer Dall wrote:
> On Tue, Jan 29, 2019 at 05:16:44PM +0100, Andrew Jones wrote:
> > 
> > Summary edit
> > 
> > > KVM: arm/arm64: Require VCPU threads to turn them self off
> > 
> > themselves
> > 
> > On Fri, Jan 25, 2019 at 10:46:54AM +0100, Christoffer Dall wrote:
> > > To avoid a race between turning VCPUs off and turning them on, make sure
> > > that only the VCPU threat itself turns off the VCPU.  When other threads
> > > want to turn of a VCPU, they now do this via a request.
> > > 
> > > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> > > Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> > > ---
> > >  arch/arm/include/asm/kvm_host.h   |  2 ++
> > >  arch/arm64/include/asm/kvm_host.h |  2 ++
> > >  virt/kvm/arm/arm.c                |  8 ++++++--
> > >  virt/kvm/arm/psci.c               | 11 ++---------
> > >  4 files changed, 12 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> > > index 50e89869178a..b1cfae222441 100644
> > > --- a/arch/arm/include/asm/kvm_host.h
> > > +++ b/arch/arm/include/asm/kvm_host.h
> > > @@ -49,6 +49,8 @@
> > >  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > >  #define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
> > >  #define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
> > > +#define KVM_REQ_VCPU_OFF \
> > > +	KVM_ARCH_REQ_FLAGS(3, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > >  
> > >  DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
> > >  
> > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > > index da3fc7324d68..d43b13421987 100644
> > > --- a/arch/arm64/include/asm/kvm_host.h
> > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > @@ -49,6 +49,8 @@
> > >  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > >  #define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
> > >  #define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
> > > +#define KVM_REQ_VCPU_OFF \
> > > +	KVM_ARCH_REQ_FLAGS(3, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > >  
> > >  DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
> > >  
> > > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> > > index 9c486fad3f9f..785076176814 100644
> > > --- a/virt/kvm/arm/arm.c
> > > +++ b/virt/kvm/arm/arm.c
> > > @@ -404,8 +404,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
> > >  
> > >  static void vcpu_power_off(struct kvm_vcpu *vcpu)
> > >  {
> > > -	vcpu->arch.power_off = true;
> > > -	kvm_make_request(KVM_REQ_SLEEP, vcpu);
> > > +	kvm_make_request(KVM_REQ_VCPU_OFF, vcpu);
> > >  	kvm_vcpu_kick(vcpu);
> > >  }
> > 
> > I think we should leave this function alone. Otherwise if userspace sets
> > the MP state to STOPPED and then queries the state before the vcpu
> > has a chance to manage its vcpu requests, the state will still indicate
> > RUNNBLE. The same goes for a query right after doing a vcpu init.
> > 
> 
> We can't leave this alone, because that could lead to userspace racing
> with two PSCI_VCPU_ON requests which could then both enter the critical
> section gated only by the cmpxchg in kvm_psci_vcpu_on.

Right. I keep forgetting about that race.

> 
> But we could do something like this (completely untested):
> 
> 
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index 1e3195155860..538b5eb9d920 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -404,6 +404,17 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>  
>  static void vcpu_power_off(struct kvm_vcpu *vcpu)
>  {
> +	enum vcpu_power_state old_power_state;
> +
> +	/*
> +	 * Set power_state directly to reflect the power state back to user
> +	 * space even when the VCPU thread has not had a chance to run, but
> +	 * only if this doesn't accidentally allow interleaved PSCI_VCPU_ON
> +	 * requests.
> +	 */
> +	old_power_state = cmpxchg(&vcpu->arch.power_state,
> +				  KVM_ARM_VCPU_ON,
> +				  KVM_ARM_VCPU_OFF);
>  	kvm_make_request(KVM_REQ_VCPU_OFF, vcpu);
>  	kvm_vcpu_kick(vcpu);
>  }

Something like that sounds good to me.

> 
> 
> > >  
> > > @@ -646,6 +645,11 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
> > >  		if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
> > >  			vcpu_req_sleep(vcpu);
> > >  
> > > +		if (kvm_check_request(KVM_REQ_VCPU_OFF, vcpu)) {
> > > +			vcpu->arch.power_off = true;
> > > +			vcpu_req_sleep(vcpu);
> > > +		}
> > > +
> > >  		if (kvm_check_request(KVM_REQ_VCPU_RESET, vcpu))
> > >  			kvm_reset_vcpu(vcpu);
> > >  
> > > diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
> > > index b9cff1d4b06d..20255319e193 100644
> > > --- a/virt/kvm/arm/psci.c
> > > +++ b/virt/kvm/arm/psci.c
> > > @@ -97,9 +97,7 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)
> > >  
> > >  static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu)
> > >  {
> > > -	vcpu->arch.power_off = true;
> > > -	kvm_make_request(KVM_REQ_SLEEP, vcpu);
> > > -	kvm_vcpu_kick(vcpu);
> > > +	kvm_make_request(KVM_REQ_VCPU_OFF, vcpu);
> > >  }
> > 
> > This was currently fine since it implements CPU_OFF which only applies to
> > the calling vcpu, but there's also no reason not to change it to be
> > consistent with the below change
> > 
> 
> Same problem as above, we don't want two VCPUs to be messing with a
> target VCPU state at the same time.
>

Yup

Thanks,
drew 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/5] arm/arm64: KVM: Don't panic on failure to properly reset system registers

[Andrew Jones]

On Wed, Jan 30, 2019 at 10:56:19AM +0000, Marc Zyngier wrote:
> On 29/01/2019 16:33, Andrew Jones wrote:
> > On Fri, Jan 25, 2019 at 10:46:56AM +0100, Christoffer Dall wrote:
> >> From: Marc Zyngier <marc.zyngier@arm.com>
> >>
> >> Failing to properly reset system registers is pretty bad. But not
> >> quite as bad as bringing the whole machine down... So warn loudly,
> >> but slightly more gracefully.
> >>
> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> >> Acked-by: Christoffer Dall <christoffer.dall@arm.com>
> >> ---
> >>  arch/arm/kvm/coproc.c     | 4 ++--
> >>  arch/arm64/kvm/sys_regs.c | 4 ++--
> >>  2 files changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> >> index 222c1635bc7a..e8bd288fd5be 100644
> >> --- a/arch/arm/kvm/coproc.c
> >> +++ b/arch/arm/kvm/coproc.c
> >> @@ -1450,6 +1450,6 @@ void kvm_reset_coprocs(struct kvm_vcpu *vcpu)
> >>  	reset_coproc_regs(vcpu, table, num);
> >>  
> >>  	for (num = 1; num < NR_CP15_REGS; num++)
> >> -		if (vcpu_cp15(vcpu, num) == 0x42424242)
> >> -			panic("Didn't reset vcpu_cp15(vcpu, %zi)", num);
> >> +		WARN(vcpu_cp15(vcpu, num) == 0x42424242,
> >> +		     "Didn't reset vcpu_cp15(vcpu, %zi)", num);
> >>  }
> >> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> >> index 86096774abcd..4f067545c7d2 100644
> >> --- a/arch/arm64/kvm/sys_regs.c
> >> +++ b/arch/arm64/kvm/sys_regs.c
> >> @@ -2609,6 +2609,6 @@ void kvm_reset_sys_regs(struct kvm_vcpu *vcpu)
> >>  	reset_sys_reg_descs(vcpu, table, num);
> >>  
> >>  	for (num = 1; num < NR_SYS_REGS; num++)
> >> -		if (__vcpu_sys_reg(vcpu, num) == 0x4242424242424242)
> >> -			panic("Didn't reset __vcpu_sys_reg(%zi)", num);
> >> +		WARN(__vcpu_sys_reg(vcpu, num) == 0x4242424242424242,
> >> +		     "Didn't reset __vcpu_sys_reg(%zi)\n", num);
> >>  }
> >> -- 
> >> 2.18.0
> >>
> > 
> > If we only get halfway through resetting, then we'll get a warn splat,
> > complete with a backtrace, for each register. Should we do something
> > like the following instead?
> > 
> >   for (num = 1; num < NR_SYS_REGS; num++)
> >      if (__vcpu_sys_reg(vcpu, num) == 0x4242424242424242)
> >         failed++;
> >   WARN(failed, "Didn't reset %d system registers", failed);
> 
> And doing so we'd loose the important bit of information, which is the
> position in the table that doesn't get initialized.
> 
> The vcpu reset issue is rare enough that nobody noticed it yet (I only
> reproduced it twice), and having it to scream 200 times is not really a
> concern. What I want to catch is the case where someone has added a new
> sysreg in the table, and has failed to provide a working init function.

OK, Christoffer also said that information was useful. Would any following
registers also be useful? Or should it be something like

  for (num = 1; num < NR_SYS_REGS; num++) {
       WARN(__vcpu_sys_reg(vcpu, num) == 0x4242424242424242,
            "Didn't reset __vcpu_sys_reg(%zi)\n", num);
       break;
   }

to ensure the first one, the most important one, is there, and that it
doesn't get pushed out of the buffer by hundreds of more lines?

Thanks,
drew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] arm/arm64: KVM: Allow a VCPU to fully reset itself

[Christoffer Dall]

On Wed, Jan 30, 2019 at 04:27:21PM +0100, Andrew Jones wrote:
> On Wed, Jan 30, 2019 at 10:34:31AM +0100, Christoffer Dall wrote:
> > On Tue, Jan 29, 2019 at 05:03:47PM +0100, Andrew Jones wrote:
> > > On Fri, Jan 25, 2019 at 10:46:53AM +0100, Christoffer Dall wrote:
> > > > From: Marc Zyngier <marc.zyngier@arm.com>
> > > > 
> > > > The current kvm_psci_vcpu_on implementation will directly try to
> > > > manipulate the state of the VCPU to reset it.  However, since this is
> > > > not done on the thread that runs the VCPU, we can end up in a strangely
> > > > corrupted state when the source and target VCPUs are running at the same
> > > > time.
> > > > 
> > > > Fix this by factoring out all reset logic from the PSCI implementation
> > > > and forwarding the required information along with a request to the
> > > > target VCPU.
> > > 
> > > The last patch makes more sense, now that I see this one. I guess their
> > > order should be swapped.
> > > 
> > > > 
> > > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > > > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> > > > ---
> > > >  arch/arm/include/asm/kvm_host.h   | 10 +++++++++
> > > >  arch/arm/kvm/reset.c              | 24 +++++++++++++++++++++
> > > >  arch/arm64/include/asm/kvm_host.h | 11 ++++++++++
> > > >  arch/arm64/kvm/reset.c            | 24 +++++++++++++++++++++
> > > >  virt/kvm/arm/arm.c                | 10 +++++++++
> > > >  virt/kvm/arm/psci.c               | 36 ++++++++++++++-----------------
> > > >  6 files changed, 95 insertions(+), 20 deletions(-)
> > > > 
> > > > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> > > > index ca56537b61bc..50e89869178a 100644
> > > > --- a/arch/arm/include/asm/kvm_host.h
> > > > +++ b/arch/arm/include/asm/kvm_host.h
> > > > @@ -48,6 +48,7 @@
> > > >  #define KVM_REQ_SLEEP \
> > > >  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > > >  #define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
> > > > +#define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
> > > >  
> > > >  DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
> > > >  
> > > > @@ -147,6 +148,13 @@ struct kvm_cpu_context {
> > > >  
> > > >  typedef struct kvm_cpu_context kvm_cpu_context_t;
> > > >  
> > > > +struct vcpu_reset_state {
> > > > +	unsigned long	pc;
> > > > +	unsigned long	r0;
> > > > +	bool		be;
> > > > +	bool		reset;
> > > > +};
> > > > +
> > > >  struct kvm_vcpu_arch {
> > > >  	struct kvm_cpu_context ctxt;
> > > >  
> > > > @@ -186,6 +194,8 @@ struct kvm_vcpu_arch {
> > > >  	/* Cache some mmu pages needed inside spinlock regions */
> > > >  	struct kvm_mmu_memory_cache mmu_page_cache;
> > > >  
> > > > +	struct vcpu_reset_state reset_state;
> > > > +
> > > >  	/* Detect first run of a vcpu */
> > > >  	bool has_run_once;
> > > >  };
> > > > diff --git a/arch/arm/kvm/reset.c b/arch/arm/kvm/reset.c
> > > > index 5ed0c3ee33d6..de41255eebcd 100644
> > > > --- a/arch/arm/kvm/reset.c
> > > > +++ b/arch/arm/kvm/reset.c
> > > > @@ -26,6 +26,7 @@
> > > >  #include <asm/cputype.h>
> > > >  #include <asm/kvm_arm.h>
> > > >  #include <asm/kvm_coproc.h>
> > > > +#include <asm/kvm_emulate.h>
> > > >  
> > > >  #include <kvm/arm_arch_timer.h>
> > > >  
> > > > @@ -69,6 +70,29 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> > > >  	/* Reset CP15 registers */
> > > >  	kvm_reset_coprocs(vcpu);
> > > >  
> > > > +	/*
> > > > +	 * Additional reset state handling that PSCI may have imposed on us.
> > > > +	 * Must be done after all the sys_reg reset.
> > > > +	 */
> > > > +	if (vcpu->arch.reset_state.reset) {
> > > > +		unsigned long target_pc = vcpu->arch.reset_state.pc;
> > > > +
> > > > +		/* Gracefully handle Thumb2 entry point */
> > > > +		if (target_pc & 1) {
> > > > +			target_pc &= ~1UL;
> > > > +			vcpu_set_thumb(vcpu);
> > > > +		}
> > > > +
> > > > +		/* Propagate caller endianness */
> > > > +		if (vcpu->arch.reset_state.be)
> > > > +			kvm_vcpu_set_be(vcpu);
> > > > +
> > > > +		*vcpu_pc(vcpu) = target_pc;
> > > > +		vcpu_set_reg(vcpu, 0, vcpu->arch.reset_state.r0);
> > > > +
> > > > +		vcpu->arch.reset_state.reset = false;
> > > > +	}
> > > > +
> > > >  	/* Reset arch_timer context */
> > > >  	return kvm_timer_vcpu_reset(vcpu);
> > > >  }
> > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > > > index 7732d0ba4e60..da3fc7324d68 100644
> > > > --- a/arch/arm64/include/asm/kvm_host.h
> > > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > > @@ -48,6 +48,7 @@
> > > >  #define KVM_REQ_SLEEP \
> > > >  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > > >  #define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
> > > > +#define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
> > > >  
> > > >  DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
> > > >  
> > > > @@ -208,6 +209,13 @@ struct kvm_cpu_context {
> > > >  
> > > >  typedef struct kvm_cpu_context kvm_cpu_context_t;
> > > >  
> > > > +struct vcpu_reset_state {
> > > > +	unsigned long	pc;
> > > > +	unsigned long	r0;
> > > > +	bool		be;
> > > > +	bool		reset;
> > > > +};
> > > > +
> > > >  struct kvm_vcpu_arch {
> > > >  	struct kvm_cpu_context ctxt;
> > > >  
> > > > @@ -297,6 +305,9 @@ struct kvm_vcpu_arch {
> > > >  	/* Virtual SError ESR to restore when HCR_EL2.VSE is set */
> > > >  	u64 vsesr_el2;
> > > >  
> > > > +	/* Additional reset state */
> > > > +	struct vcpu_reset_state	reset_state;
> > > > +
> > > >  	/* True when deferrable sysregs are loaded on the physical CPU,
> > > >  	 * see kvm_vcpu_load_sysregs and kvm_vcpu_put_sysregs. */
> > > >  	bool sysregs_loaded_on_cpu;
> > > > diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> > > > index f21a2a575939..f16a5f8ff2b4 100644
> > > > --- a/arch/arm64/kvm/reset.c
> > > > +++ b/arch/arm64/kvm/reset.c
> > > > @@ -32,6 +32,7 @@
> > > >  #include <asm/kvm_arm.h>
> > > >  #include <asm/kvm_asm.h>
> > > >  #include <asm/kvm_coproc.h>
> > > > +#include <asm/kvm_emulate.h>
> > > >  #include <asm/kvm_mmu.h>
> > > >  
> > > >  /* Maximum phys_shift supported for any VM on this host */
> > > > @@ -146,6 +147,29 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> > > >  	/* Reset system registers */
> > > >  	kvm_reset_sys_regs(vcpu);
> > > >  
> > > > +	/*
> > > > +	 * Additional reset state handling that PSCI may have imposed on us.
> > > > +	 * Must be done after all the sys_reg reset.
> > > > +	 */
> > > > +	if (vcpu->arch.reset_state.reset) {
> > > > +		unsigned long target_pc = vcpu->arch.reset_state.pc;
> > > > +
> > > > +		/* Gracefully handle Thumb2 entry point */
> > > > +		if (vcpu_mode_is_32bit(vcpu) && (target_pc & 1)) {
> > > > +			target_pc &= ~1UL;
> > > > +			vcpu_set_thumb(vcpu);
> > > > +		}
> > > > +
> > > > +		/* Propagate caller endianness */
> > > > +		if (vcpu->arch.reset_state.be)
> > > > +			kvm_vcpu_set_be(vcpu);
> > > > +
> > > > +		*vcpu_pc(vcpu) = target_pc;
> > > > +		vcpu_set_reg(vcpu, 0, vcpu->arch.reset_state.r0);
> > > > +
> > > > +		vcpu->arch.reset_state.reset = false;
> > > > +	}
> > > > +
> > > >  	/* Reset PMU */
> > > >  	kvm_pmu_vcpu_reset(vcpu);
> > > >  
> > > > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> > > > index 9e350fd34504..9c486fad3f9f 100644
> > > > --- a/virt/kvm/arm/arm.c
> > > > +++ b/virt/kvm/arm/arm.c
> > > > @@ -626,6 +626,13 @@ static void vcpu_req_sleep(struct kvm_vcpu *vcpu)
> > > >  		/* Awaken to handle a signal, request we sleep again later. */
> > > >  		kvm_make_request(KVM_REQ_SLEEP, vcpu);
> > > >  	}
> > > > +
> > > > +	/*
> > > > +	 * Make sure we will observe a potential reset request if we've
> > > > +	 * observed a change to the power state. Pairs with the smp_wmb() in
> > > > +	 * kvm_psci_vcpu_on().
> > > > +	 */
> > > > +	smp_rmb();
> > > 
> > > I don't believe this should be necessary, because...
> > > 
> > > >  }
> > > >  
> > > >  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
> > > > @@ -639,6 +646,9 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
> > > >  		if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
> > > >  			vcpu_req_sleep(vcpu);
> > > >  
> > > > +		if (kvm_check_request(KVM_REQ_VCPU_RESET, vcpu))
> > > > +			kvm_reset_vcpu(vcpu);
> > > 
> > > ... we do kvm_check_request here before using the reset data, and we do...
> > > 
> > > > +
> > > >  		/*
> > > >  		 * Clear IRQ_PENDING requests that were made to guarantee
> > > >  		 * that a VCPU sees new virtual interrupts.
> > > > diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
> > > > index 9b73d3ad918a..b9cff1d4b06d 100644
> > > > --- a/virt/kvm/arm/psci.c
> > > > +++ b/virt/kvm/arm/psci.c
> > > > @@ -104,12 +104,10 @@ static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu)
> > > >  
> > > >  static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> > > >  {
> > > > +	struct vcpu_reset_state *reset_state;
> > > >  	struct kvm *kvm = source_vcpu->kvm;
> > > >  	struct kvm_vcpu *vcpu = NULL;
> > > > -	struct swait_queue_head *wq;
> > > >  	unsigned long cpu_id;
> > > > -	unsigned long context_id;
> > > > -	phys_addr_t target_pc;
> > > >  
> > > >  	cpu_id = smccc_get_arg1(source_vcpu) & MPIDR_HWID_BITMASK;
> > > >  	if (vcpu_mode_is_32bit(source_vcpu))
> > > > @@ -130,32 +128,30 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> > > >  			return PSCI_RET_INVALID_PARAMS;
> > > >  	}
> > > >  
> > > > -	target_pc = smccc_get_arg2(source_vcpu);
> > > > -	context_id = smccc_get_arg3(source_vcpu);
> > > > +	reset_state = &vcpu->arch.reset_state;
> > > >  
> > > > -	kvm_reset_vcpu(vcpu);
> > > > -
> > > > -	/* Gracefully handle Thumb2 entry point */
> > > > -	if (vcpu_mode_is_32bit(vcpu) && (target_pc & 1)) {
> > > > -		target_pc &= ~((phys_addr_t) 1);
> > > > -		vcpu_set_thumb(vcpu);
> > > > -	}
> > > > +	reset_state->pc = smccc_get_arg2(source_vcpu);
> > > >  
> > > >  	/* Propagate caller endianness */
> > > > -	if (kvm_vcpu_is_be(source_vcpu))
> > > > -		kvm_vcpu_set_be(vcpu);
> > > > +	reset_state->be = kvm_vcpu_is_be(source_vcpu);
> > > >  
> > > > -	*vcpu_pc(vcpu) = target_pc;
> > > >  	/*
> > > >  	 * NOTE: We always update r0 (or x0) because for PSCI v0.1
> > > >  	 * the general puspose registers are undefined upon CPU_ON.
> > > >  	 */
> > > > -	smccc_set_retval(vcpu, context_id, 0, 0, 0);
> > > > -	vcpu->arch.power_off = false;
> > > > -	smp_mb();		/* Make sure the above is visible */
> > > > +	reset_state->r0 = smccc_get_arg3(source_vcpu);
> > > > +
> > > > +	reset_state->reset = true;
> > > > +	kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
> > > 
> > > ... this kvm_make_request() after writing the reset data. The
> > > kvm_make_request/kvm_check_request embeds the necessary barriers.
> > > 
> > > >  
> > > > -	wq = kvm_arch_vcpu_wq(vcpu);
> > > > -	swake_up_one(wq);
> > > > +	/*
> > > > +	 * Make sure the reset request is observed if the change to
> > > > +	 * power_state is observed.
> > > > +	 */
> > > > +	smp_wmb();
> > > > +
> > > > +	vcpu->arch.power_off = false;
> > > 
> > > Or you want to tie the reset data to the observability of the power_off
> > > state? Why not just tie it to the KVM_REQ_VCPU_RESET request?
> > > 
> > 
> > The barrier in kvm_make_request is *before* the write to vcpu->requests,
> > so doesn't enforce any ordering of stores occurring *after* that write.
> > 
> > We don't want to wake up the target VCPU thread, have it observe that it
> > is powered on, yet not observe the reset request (my comment above
> > inversed).
> > 
> > I wrote this Litmus test (which is probably so appalingly obvious to
> > memory ordering folks that it is unnecessary) to convince myself the
> > barrier is needed:
> > 
> > ----<8----
> > C vcpu-reset
> > 
> > {}
> > 
> > P0(int *power, int *reset)
> > {
> > 	smp_wmb(); // from kvm_make_request
> > 	WRITE_ONCE(*reset, 1); // kvm_psci_vcpu_on: kvm_make_request(KVM_REQ_RESET, vcpu);
> > 	smp_wmb();
> > 	WRITE_ONCE(*power, 1); // kvm_psci_vcpu_on: vcpu->arch.power_state = KVM_ARM_VCPU_ON;
> > }
> > 
> > P1(int *power, int *reset, spinlock_t *wqlock)
> > {
> > 	int r0;
> > 	int r1;
> > 
> > 	spin_lock(wqlock);
> > 	smp_mb(); // from prepare_to_wait -> set_current_state
> > 	spin_unlock(wqlock);
> > 	r0 = READ_ONCE(*power); // vcpu_req_sleep: if (vcpu_sleeping(vcpu))
> > 	smp_rmb();
> > 	r1 = READ_ONCE(*reset); // check_vcpu_requests: if (kvm_check_request(KVM_REQ_RESET, vcpu))
> > }
> > 
> > exists (1:r0=1 /\ 1:r1=0)
> > ----<8----
> > 
> > 
> > Hope this helps,
> 
> It certainly does, mostly because I didn't review closely enough and
> thought the pair we were interested in was vcpu_reset_state and the
> RESET vcpu request, which the vcpu request API handles. Now it's
> clear we're worried about the pair power_off and the RESET vcpu
> request. And we've reversed the usual "if we observe a request, then
> we can observe its accompanying data" to "if we observe a change in
> some data (power_off), then we need to also observe a request". I
> agree the barriers are necessary to ensure that in the direct
> sequence, but now I'm wondering if we need to care about the
> direct sequence.
> 
> If we remove the barriers then the vcpu could have these possible
> sequences
> 
> 1)
>   - wake up
>   - observe power_off is false
>   - observe the RESET vcpu request - do the reset
>   - enter guest
> 
> That's the one this patch ensures, so it's definitely correct.
> 
> 2)
>   - wake up
>   - the change to power_off not observed - the vcpu makes the
>     SLEEP vcpu request on itself
>   - observe the RESET vcpu request - do the reset
>   - observe the SLEEP vcpu request (with or without the need for
>     an IPI), attempt to sleep again, but now observe power_off
>     is false
>   - enter guest
> 
> I think that should be correct as well, even if less efficient.
> 
> 3)
>   - wake up
>   - observe power_off is false
>   - don't observe the RESET request yet, get closer to entering
>     the guest
>   - observe the RESET request a bit later (with or without the
>     need for an IPI) - do the reset
>   - enter guest
> 
> This is the same as (1), but we rely on the kvm_request_pending
> stuff to make sure the vcpu request is seen before entering the
> guest.
> 
> 4)
>   - wake up
>   - the change to power_off not observed - the vcpu makes the
>     SLEEP vcpu request on itself
>   - don't observe the RESET request yet, get closer to entering
>     the guest
>   - observe the SLEEP vcpu request (with or without the need for
>     an IPI), attempt to sleep again, but now observe power_off
>     is false
>   - observe the RESET request a bit later (with or without the
>     need for an IPI) - do the reset
>   - enter guest
> 
> The least efficient one, but should still be correct.
> 
> If you agree with this, then we get to remove the barriers,
> simplifying things, and also we don't introduce the ordering
> dependency in check_vcpu_requests(), where the vcpu_req_sleep()
> calls must now come before checking the RESET request.
> 

I'm sorry, I'm not following what you want to achieve here?

Somehow delivering a series of possible interleaved executions doesn't
exactly convey anything meaningful to me.


Thanks,

    Christoffer

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] arm/arm64: KVM: Allow a VCPU to fully reset itself

[Andrew Jones]

On Thu, Jan 31, 2019 at 08:43:53AM +0100, Christoffer Dall wrote:
> On Wed, Jan 30, 2019 at 04:27:21PM +0100, Andrew Jones wrote:
> > On Wed, Jan 30, 2019 at 10:34:31AM +0100, Christoffer Dall wrote:
> > > On Tue, Jan 29, 2019 at 05:03:47PM +0100, Andrew Jones wrote:
> > > > On Fri, Jan 25, 2019 at 10:46:53AM +0100, Christoffer Dall wrote:
> > > > > From: Marc Zyngier <marc.zyngier@arm.com>
> > > > > 
> > > > > The current kvm_psci_vcpu_on implementation will directly try to
> > > > > manipulate the state of the VCPU to reset it.  However, since this is
> > > > > not done on the thread that runs the VCPU, we can end up in a strangely
> > > > > corrupted state when the source and target VCPUs are running at the same
> > > > > time.
> > > > > 
> > > > > Fix this by factoring out all reset logic from the PSCI implementation
> > > > > and forwarding the required information along with a request to the
> > > > > target VCPU.
> > > > 
> > > > The last patch makes more sense, now that I see this one. I guess their
> > > > order should be swapped.
> > > > 
> > > > > 
> > > > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > > > > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> > > > > ---
> > > > >  arch/arm/include/asm/kvm_host.h   | 10 +++++++++
> > > > >  arch/arm/kvm/reset.c              | 24 +++++++++++++++++++++
> > > > >  arch/arm64/include/asm/kvm_host.h | 11 ++++++++++
> > > > >  arch/arm64/kvm/reset.c            | 24 +++++++++++++++++++++
> > > > >  virt/kvm/arm/arm.c                | 10 +++++++++
> > > > >  virt/kvm/arm/psci.c               | 36 ++++++++++++++-----------------
> > > > >  6 files changed, 95 insertions(+), 20 deletions(-)
> > > > > 
> > > > > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> > > > > index ca56537b61bc..50e89869178a 100644
> > > > > --- a/arch/arm/include/asm/kvm_host.h
> > > > > +++ b/arch/arm/include/asm/kvm_host.h
> > > > > @@ -48,6 +48,7 @@
> > > > >  #define KVM_REQ_SLEEP \
> > > > >  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > > > >  #define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
> > > > > +#define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
> > > > >  
> > > > >  DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
> > > > >  
> > > > > @@ -147,6 +148,13 @@ struct kvm_cpu_context {
> > > > >  
> > > > >  typedef struct kvm_cpu_context kvm_cpu_context_t;
> > > > >  
> > > > > +struct vcpu_reset_state {
> > > > > +	unsigned long	pc;
> > > > > +	unsigned long	r0;
> > > > > +	bool		be;
> > > > > +	bool		reset;
> > > > > +};
> > > > > +
> > > > >  struct kvm_vcpu_arch {
> > > > >  	struct kvm_cpu_context ctxt;
> > > > >  
> > > > > @@ -186,6 +194,8 @@ struct kvm_vcpu_arch {
> > > > >  	/* Cache some mmu pages needed inside spinlock regions */
> > > > >  	struct kvm_mmu_memory_cache mmu_page_cache;
> > > > >  
> > > > > +	struct vcpu_reset_state reset_state;
> > > > > +
> > > > >  	/* Detect first run of a vcpu */
> > > > >  	bool has_run_once;
> > > > >  };
> > > > > diff --git a/arch/arm/kvm/reset.c b/arch/arm/kvm/reset.c
> > > > > index 5ed0c3ee33d6..de41255eebcd 100644
> > > > > --- a/arch/arm/kvm/reset.c
> > > > > +++ b/arch/arm/kvm/reset.c
> > > > > @@ -26,6 +26,7 @@
> > > > >  #include <asm/cputype.h>
> > > > >  #include <asm/kvm_arm.h>
> > > > >  #include <asm/kvm_coproc.h>
> > > > > +#include <asm/kvm_emulate.h>
> > > > >  
> > > > >  #include <kvm/arm_arch_timer.h>
> > > > >  
> > > > > @@ -69,6 +70,29 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> > > > >  	/* Reset CP15 registers */
> > > > >  	kvm_reset_coprocs(vcpu);
> > > > >  
> > > > > +	/*
> > > > > +	 * Additional reset state handling that PSCI may have imposed on us.
> > > > > +	 * Must be done after all the sys_reg reset.
> > > > > +	 */
> > > > > +	if (vcpu->arch.reset_state.reset) {
> > > > > +		unsigned long target_pc = vcpu->arch.reset_state.pc;
> > > > > +
> > > > > +		/* Gracefully handle Thumb2 entry point */
> > > > > +		if (target_pc & 1) {
> > > > > +			target_pc &= ~1UL;
> > > > > +			vcpu_set_thumb(vcpu);
> > > > > +		}
> > > > > +
> > > > > +		/* Propagate caller endianness */
> > > > > +		if (vcpu->arch.reset_state.be)
> > > > > +			kvm_vcpu_set_be(vcpu);
> > > > > +
> > > > > +		*vcpu_pc(vcpu) = target_pc;
> > > > > +		vcpu_set_reg(vcpu, 0, vcpu->arch.reset_state.r0);
> > > > > +
> > > > > +		vcpu->arch.reset_state.reset = false;
> > > > > +	}
> > > > > +
> > > > >  	/* Reset arch_timer context */
> > > > >  	return kvm_timer_vcpu_reset(vcpu);
> > > > >  }
> > > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > > > > index 7732d0ba4e60..da3fc7324d68 100644
> > > > > --- a/arch/arm64/include/asm/kvm_host.h
> > > > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > > > @@ -48,6 +48,7 @@
> > > > >  #define KVM_REQ_SLEEP \
> > > > >  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > > > >  #define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
> > > > > +#define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
> > > > >  
> > > > >  DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
> > > > >  
> > > > > @@ -208,6 +209,13 @@ struct kvm_cpu_context {
> > > > >  
> > > > >  typedef struct kvm_cpu_context kvm_cpu_context_t;
> > > > >  
> > > > > +struct vcpu_reset_state {
> > > > > +	unsigned long	pc;
> > > > > +	unsigned long	r0;
> > > > > +	bool		be;
> > > > > +	bool		reset;
> > > > > +};
> > > > > +
> > > > >  struct kvm_vcpu_arch {
> > > > >  	struct kvm_cpu_context ctxt;
> > > > >  
> > > > > @@ -297,6 +305,9 @@ struct kvm_vcpu_arch {
> > > > >  	/* Virtual SError ESR to restore when HCR_EL2.VSE is set */
> > > > >  	u64 vsesr_el2;
> > > > >  
> > > > > +	/* Additional reset state */
> > > > > +	struct vcpu_reset_state	reset_state;
> > > > > +
> > > > >  	/* True when deferrable sysregs are loaded on the physical CPU,
> > > > >  	 * see kvm_vcpu_load_sysregs and kvm_vcpu_put_sysregs. */
> > > > >  	bool sysregs_loaded_on_cpu;
> > > > > diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> > > > > index f21a2a575939..f16a5f8ff2b4 100644
> > > > > --- a/arch/arm64/kvm/reset.c
> > > > > +++ b/arch/arm64/kvm/reset.c
> > > > > @@ -32,6 +32,7 @@
> > > > >  #include <asm/kvm_arm.h>
> > > > >  #include <asm/kvm_asm.h>
> > > > >  #include <asm/kvm_coproc.h>
> > > > > +#include <asm/kvm_emulate.h>
> > > > >  #include <asm/kvm_mmu.h>
> > > > >  
> > > > >  /* Maximum phys_shift supported for any VM on this host */
> > > > > @@ -146,6 +147,29 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> > > > >  	/* Reset system registers */
> > > > >  	kvm_reset_sys_regs(vcpu);
> > > > >  
> > > > > +	/*
> > > > > +	 * Additional reset state handling that PSCI may have imposed on us.
> > > > > +	 * Must be done after all the sys_reg reset.
> > > > > +	 */
> > > > > +	if (vcpu->arch.reset_state.reset) {
> > > > > +		unsigned long target_pc = vcpu->arch.reset_state.pc;
> > > > > +
> > > > > +		/* Gracefully handle Thumb2 entry point */
> > > > > +		if (vcpu_mode_is_32bit(vcpu) && (target_pc & 1)) {
> > > > > +			target_pc &= ~1UL;
> > > > > +			vcpu_set_thumb(vcpu);
> > > > > +		}
> > > > > +
> > > > > +		/* Propagate caller endianness */
> > > > > +		if (vcpu->arch.reset_state.be)
> > > > > +			kvm_vcpu_set_be(vcpu);
> > > > > +
> > > > > +		*vcpu_pc(vcpu) = target_pc;
> > > > > +		vcpu_set_reg(vcpu, 0, vcpu->arch.reset_state.r0);
> > > > > +
> > > > > +		vcpu->arch.reset_state.reset = false;
> > > > > +	}
> > > > > +
> > > > >  	/* Reset PMU */
> > > > >  	kvm_pmu_vcpu_reset(vcpu);
> > > > >  
> > > > > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> > > > > index 9e350fd34504..9c486fad3f9f 100644
> > > > > --- a/virt/kvm/arm/arm.c
> > > > > +++ b/virt/kvm/arm/arm.c
> > > > > @@ -626,6 +626,13 @@ static void vcpu_req_sleep(struct kvm_vcpu *vcpu)
> > > > >  		/* Awaken to handle a signal, request we sleep again later. */
> > > > >  		kvm_make_request(KVM_REQ_SLEEP, vcpu);
> > > > >  	}
> > > > > +
> > > > > +	/*
> > > > > +	 * Make sure we will observe a potential reset request if we've
> > > > > +	 * observed a change to the power state. Pairs with the smp_wmb() in
> > > > > +	 * kvm_psci_vcpu_on().
> > > > > +	 */
> > > > > +	smp_rmb();
> > > > 
> > > > I don't believe this should be necessary, because...
> > > > 
> > > > >  }
> > > > >  
> > > > >  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
> > > > > @@ -639,6 +646,9 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
> > > > >  		if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
> > > > >  			vcpu_req_sleep(vcpu);
> > > > >  
> > > > > +		if (kvm_check_request(KVM_REQ_VCPU_RESET, vcpu))
> > > > > +			kvm_reset_vcpu(vcpu);
> > > > 
> > > > ... we do kvm_check_request here before using the reset data, and we do...
> > > > 
> > > > > +
> > > > >  		/*
> > > > >  		 * Clear IRQ_PENDING requests that were made to guarantee
> > > > >  		 * that a VCPU sees new virtual interrupts.
> > > > > diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
> > > > > index 9b73d3ad918a..b9cff1d4b06d 100644
> > > > > --- a/virt/kvm/arm/psci.c
> > > > > +++ b/virt/kvm/arm/psci.c
> > > > > @@ -104,12 +104,10 @@ static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu)
> > > > >  
> > > > >  static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> > > > >  {
> > > > > +	struct vcpu_reset_state *reset_state;
> > > > >  	struct kvm *kvm = source_vcpu->kvm;
> > > > >  	struct kvm_vcpu *vcpu = NULL;
> > > > > -	struct swait_queue_head *wq;
> > > > >  	unsigned long cpu_id;
> > > > > -	unsigned long context_id;
> > > > > -	phys_addr_t target_pc;
> > > > >  
> > > > >  	cpu_id = smccc_get_arg1(source_vcpu) & MPIDR_HWID_BITMASK;
> > > > >  	if (vcpu_mode_is_32bit(source_vcpu))
> > > > > @@ -130,32 +128,30 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> > > > >  			return PSCI_RET_INVALID_PARAMS;
> > > > >  	}
> > > > >  
> > > > > -	target_pc = smccc_get_arg2(source_vcpu);
> > > > > -	context_id = smccc_get_arg3(source_vcpu);
> > > > > +	reset_state = &vcpu->arch.reset_state;
> > > > >  
> > > > > -	kvm_reset_vcpu(vcpu);
> > > > > -
> > > > > -	/* Gracefully handle Thumb2 entry point */
> > > > > -	if (vcpu_mode_is_32bit(vcpu) && (target_pc & 1)) {
> > > > > -		target_pc &= ~((phys_addr_t) 1);
> > > > > -		vcpu_set_thumb(vcpu);
> > > > > -	}
> > > > > +	reset_state->pc = smccc_get_arg2(source_vcpu);
> > > > >  
> > > > >  	/* Propagate caller endianness */
> > > > > -	if (kvm_vcpu_is_be(source_vcpu))
> > > > > -		kvm_vcpu_set_be(vcpu);
> > > > > +	reset_state->be = kvm_vcpu_is_be(source_vcpu);
> > > > >  
> > > > > -	*vcpu_pc(vcpu) = target_pc;
> > > > >  	/*
> > > > >  	 * NOTE: We always update r0 (or x0) because for PSCI v0.1
> > > > >  	 * the general puspose registers are undefined upon CPU_ON.
> > > > >  	 */
> > > > > -	smccc_set_retval(vcpu, context_id, 0, 0, 0);
> > > > > -	vcpu->arch.power_off = false;
> > > > > -	smp_mb();		/* Make sure the above is visible */
> > > > > +	reset_state->r0 = smccc_get_arg3(source_vcpu);
> > > > > +
> > > > > +	reset_state->reset = true;
> > > > > +	kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
> > > > 
> > > > ... this kvm_make_request() after writing the reset data. The
> > > > kvm_make_request/kvm_check_request embeds the necessary barriers.
> > > > 
> > > > >  
> > > > > -	wq = kvm_arch_vcpu_wq(vcpu);
> > > > > -	swake_up_one(wq);
> > > > > +	/*
> > > > > +	 * Make sure the reset request is observed if the change to
> > > > > +	 * power_state is observed.
> > > > > +	 */
> > > > > +	smp_wmb();
> > > > > +
> > > > > +	vcpu->arch.power_off = false;
> > > > 
> > > > Or you want to tie the reset data to the observability of the power_off
> > > > state? Why not just tie it to the KVM_REQ_VCPU_RESET request?
> > > > 
> > > 
> > > The barrier in kvm_make_request is *before* the write to vcpu->requests,
> > > so doesn't enforce any ordering of stores occurring *after* that write.
> > > 
> > > We don't want to wake up the target VCPU thread, have it observe that it
> > > is powered on, yet not observe the reset request (my comment above
> > > inversed).
> > > 
> > > I wrote this Litmus test (which is probably so appalingly obvious to
> > > memory ordering folks that it is unnecessary) to convince myself the
> > > barrier is needed:
> > > 
> > > ----<8----
> > > C vcpu-reset
> > > 
> > > {}
> > > 
> > > P0(int *power, int *reset)
> > > {
> > > 	smp_wmb(); // from kvm_make_request
> > > 	WRITE_ONCE(*reset, 1); // kvm_psci_vcpu_on: kvm_make_request(KVM_REQ_RESET, vcpu);
> > > 	smp_wmb();
> > > 	WRITE_ONCE(*power, 1); // kvm_psci_vcpu_on: vcpu->arch.power_state = KVM_ARM_VCPU_ON;
> > > }
> > > 
> > > P1(int *power, int *reset, spinlock_t *wqlock)
> > > {
> > > 	int r0;
> > > 	int r1;
> > > 
> > > 	spin_lock(wqlock);
> > > 	smp_mb(); // from prepare_to_wait -> set_current_state
> > > 	spin_unlock(wqlock);
> > > 	r0 = READ_ONCE(*power); // vcpu_req_sleep: if (vcpu_sleeping(vcpu))
> > > 	smp_rmb();
> > > 	r1 = READ_ONCE(*reset); // check_vcpu_requests: if (kvm_check_request(KVM_REQ_RESET, vcpu))
> > > }
> > > 
> > > exists (1:r0=1 /\ 1:r1=0)
> > > ----<8----
> > > 
> > > 
> > > Hope this helps,
> > 
> > It certainly does, mostly because I didn't review closely enough and
> > thought the pair we were interested in was vcpu_reset_state and the
> > RESET vcpu request, which the vcpu request API handles. Now it's
> > clear we're worried about the pair power_off and the RESET vcpu
> > request. And we've reversed the usual "if we observe a request, then
> > we can observe its accompanying data" to "if we observe a change in
> > some data (power_off), then we need to also observe a request". I
> > agree the barriers are necessary to ensure that in the direct
> > sequence, but now I'm wondering if we need to care about the
> > direct sequence.
> > 
> > If we remove the barriers then the vcpu could have these possible
> > sequences
> > 
> > 1)
> >   - wake up
> >   - observe power_off is false
> >   - observe the RESET vcpu request - do the reset
> >   - enter guest
> > 
> > That's the one this patch ensures, so it's definitely correct.
> > 
> > 2)
> >   - wake up
> >   - the change to power_off not observed - the vcpu makes the
> >     SLEEP vcpu request on itself
> >   - observe the RESET vcpu request - do the reset
> >   - observe the SLEEP vcpu request (with or without the need for
> >     an IPI), attempt to sleep again, but now observe power_off
> >     is false
> >   - enter guest
> > 
> > I think that should be correct as well, even if less efficient.
> > 
> > 3)
> >   - wake up
> >   - observe power_off is false
> >   - don't observe the RESET request yet, get closer to entering
> >     the guest
> >   - observe the RESET request a bit later (with or without the
> >     need for an IPI) - do the reset
> >   - enter guest
> > 
> > This is the same as (1), but we rely on the kvm_request_pending
> > stuff to make sure the vcpu request is seen before entering the
> > guest.
> > 
> > 4)
> >   - wake up
> >   - the change to power_off not observed - the vcpu makes the
> >     SLEEP vcpu request on itself
> >   - don't observe the RESET request yet, get closer to entering
> >     the guest
> >   - observe the SLEEP vcpu request (with or without the need for
> >     an IPI), attempt to sleep again, but now observe power_off
> >     is false
> >   - observe the RESET request a bit later (with or without the
> >     need for an IPI) - do the reset
> >   - enter guest
> > 
> > The least efficient one, but should still be correct.
> > 
> > If you agree with this, then we get to remove the barriers,
> > simplifying things, and also we don't introduce the ordering
> > dependency in check_vcpu_requests(), where the vcpu_req_sleep()
> > calls must now come before checking the RESET request.
> > 
> 
> I'm sorry, I'm not following what you want to achieve here?
> 
> Somehow delivering a series of possible interleaved executions doesn't
> exactly convey anything meaningful to me.
>

This patch introduces an unconventional use of vcpu requests, which is
to ensure a change to vcpu->requests is observed if a change to
arch.power_off is observed. That unconventional use requires additional
barriers and an ordering requirement in check_vcpu_requests(), i.e.
anything that calls vcpu_req_sleep() must come before the check for
the RESET vcpu request. I'm stating, and attempting to prove with the
list of possible executions, that those barriers and ordering requirements
are not necessary.

Assuming the objective is to ensure a vcpu reset is done before entering
guest mode after a vcpu power on, then the RESET vcpu request is
sufficient by itself. vcpu requests are guaranteed to be seen by a vcpu
before entering guest mode.

Additionally, I'm pretty sure that even leaving this patch as it is,
all the above execution scenarios are possible except (3), depending on
the timing of a signal delivery.

Is there a problem with (3) that I'm missing? Or am I missing something
else?

Thanks,
drew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/5] arm/arm64: KVM: Don't panic on failure to properly reset system registers

[Marc Zyngier]

On 30/01/2019 15:36, Andrew Jones wrote:
> On Wed, Jan 30, 2019 at 10:56:19AM +0000, Marc Zyngier wrote:
>> On 29/01/2019 16:33, Andrew Jones wrote:

[...]

>> If we only get halfway through resetting, then we'll get a warn splat,
>>> complete with a backtrace, for each register. Should we do something
>>> like the following instead?
>>>
>>>   for (num = 1; num < NR_SYS_REGS; num++)
>>>      if (__vcpu_sys_reg(vcpu, num) == 0x4242424242424242)
>>>         failed++;
>>>   WARN(failed, "Didn't reset %d system registers", failed);
>>
>> And doing so we'd loose the important bit of information, which is the
>> position in the table that doesn't get initialized.
>>
>> The vcpu reset issue is rare enough that nobody noticed it yet (I only
>> reproduced it twice), and having it to scream 200 times is not really a
>> concern. What I want to catch is the case where someone has added a new
>> sysreg in the table, and has failed to provide a working init function.
> 
> OK, Christoffer also said that information was useful. Would any following
> registers also be useful? Or should it be something like
> 
>   for (num = 1; num < NR_SYS_REGS; num++) {
>        WARN(__vcpu_sys_reg(vcpu, num) == 0x4242424242424242,
>             "Didn't reset __vcpu_sys_reg(%zi)\n", num);
>        break;
>    }
> 
> to ensure the first one, the most important one, is there, and that it
> doesn't get pushed out of the buffer by hundreds of more lines?

Fair enough.

	M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] arm/arm64: KVM: Allow a VCPU to fully reset itself

[Christoffer Dall]

On Thu, Jan 31, 2019 at 11:12:54AM +0100, Andrew Jones wrote:
> On Thu, Jan 31, 2019 at 08:43:53AM +0100, Christoffer Dall wrote:
> > On Wed, Jan 30, 2019 at 04:27:21PM +0100, Andrew Jones wrote:
> > > On Wed, Jan 30, 2019 at 10:34:31AM +0100, Christoffer Dall wrote:
> > > > On Tue, Jan 29, 2019 at 05:03:47PM +0100, Andrew Jones wrote:
> > > > > On Fri, Jan 25, 2019 at 10:46:53AM +0100, Christoffer Dall wrote:
> > > > > > From: Marc Zyngier <marc.zyngier@arm.com>
> > > > > > 
> > > > > > The current kvm_psci_vcpu_on implementation will directly try to
> > > > > > manipulate the state of the VCPU to reset it.  However, since this is
> > > > > > not done on the thread that runs the VCPU, we can end up in a strangely
> > > > > > corrupted state when the source and target VCPUs are running at the same
> > > > > > time.
> > > > > > 
> > > > > > Fix this by factoring out all reset logic from the PSCI implementation
> > > > > > and forwarding the required information along with a request to the
> > > > > > target VCPU.
> > > > > 
> > > > > The last patch makes more sense, now that I see this one. I guess their
> > > > > order should be swapped.
> > > > > 
> > > > > > 
> > > > > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > > > > > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> > > > > > ---
> > > > > >  arch/arm/include/asm/kvm_host.h   | 10 +++++++++
> > > > > >  arch/arm/kvm/reset.c              | 24 +++++++++++++++++++++
> > > > > >  arch/arm64/include/asm/kvm_host.h | 11 ++++++++++
> > > > > >  arch/arm64/kvm/reset.c            | 24 +++++++++++++++++++++
> > > > > >  virt/kvm/arm/arm.c                | 10 +++++++++
> > > > > >  virt/kvm/arm/psci.c               | 36 ++++++++++++++-----------------
> > > > > >  6 files changed, 95 insertions(+), 20 deletions(-)
> > > > > > 
> > > > > > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> > > > > > index ca56537b61bc..50e89869178a 100644
> > > > > > --- a/arch/arm/include/asm/kvm_host.h
> > > > > > +++ b/arch/arm/include/asm/kvm_host.h
> > > > > > @@ -48,6 +48,7 @@
> > > > > >  #define KVM_REQ_SLEEP \
> > > > > >  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > > > > >  #define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
> > > > > > +#define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
> > > > > >  
> > > > > >  DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
> > > > > >  
> > > > > > @@ -147,6 +148,13 @@ struct kvm_cpu_context {
> > > > > >  
> > > > > >  typedef struct kvm_cpu_context kvm_cpu_context_t;
> > > > > >  
> > > > > > +struct vcpu_reset_state {
> > > > > > +	unsigned long	pc;
> > > > > > +	unsigned long	r0;
> > > > > > +	bool		be;
> > > > > > +	bool		reset;
> > > > > > +};
> > > > > > +
> > > > > >  struct kvm_vcpu_arch {
> > > > > >  	struct kvm_cpu_context ctxt;
> > > > > >  
> > > > > > @@ -186,6 +194,8 @@ struct kvm_vcpu_arch {
> > > > > >  	/* Cache some mmu pages needed inside spinlock regions */
> > > > > >  	struct kvm_mmu_memory_cache mmu_page_cache;
> > > > > >  
> > > > > > +	struct vcpu_reset_state reset_state;
> > > > > > +
> > > > > >  	/* Detect first run of a vcpu */
> > > > > >  	bool has_run_once;
> > > > > >  };
> > > > > > diff --git a/arch/arm/kvm/reset.c b/arch/arm/kvm/reset.c
> > > > > > index 5ed0c3ee33d6..de41255eebcd 100644
> > > > > > --- a/arch/arm/kvm/reset.c
> > > > > > +++ b/arch/arm/kvm/reset.c
> > > > > > @@ -26,6 +26,7 @@
> > > > > >  #include <asm/cputype.h>
> > > > > >  #include <asm/kvm_arm.h>
> > > > > >  #include <asm/kvm_coproc.h>
> > > > > > +#include <asm/kvm_emulate.h>
> > > > > >  
> > > > > >  #include <kvm/arm_arch_timer.h>
> > > > > >  
> > > > > > @@ -69,6 +70,29 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> > > > > >  	/* Reset CP15 registers */
> > > > > >  	kvm_reset_coprocs(vcpu);
> > > > > >  
> > > > > > +	/*
> > > > > > +	 * Additional reset state handling that PSCI may have imposed on us.
> > > > > > +	 * Must be done after all the sys_reg reset.
> > > > > > +	 */
> > > > > > +	if (vcpu->arch.reset_state.reset) {
> > > > > > +		unsigned long target_pc = vcpu->arch.reset_state.pc;
> > > > > > +
> > > > > > +		/* Gracefully handle Thumb2 entry point */
> > > > > > +		if (target_pc & 1) {
> > > > > > +			target_pc &= ~1UL;
> > > > > > +			vcpu_set_thumb(vcpu);
> > > > > > +		}
> > > > > > +
> > > > > > +		/* Propagate caller endianness */
> > > > > > +		if (vcpu->arch.reset_state.be)
> > > > > > +			kvm_vcpu_set_be(vcpu);
> > > > > > +
> > > > > > +		*vcpu_pc(vcpu) = target_pc;
> > > > > > +		vcpu_set_reg(vcpu, 0, vcpu->arch.reset_state.r0);
> > > > > > +
> > > > > > +		vcpu->arch.reset_state.reset = false;
> > > > > > +	}
> > > > > > +
> > > > > >  	/* Reset arch_timer context */
> > > > > >  	return kvm_timer_vcpu_reset(vcpu);
> > > > > >  }
> > > > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > > > > > index 7732d0ba4e60..da3fc7324d68 100644
> > > > > > --- a/arch/arm64/include/asm/kvm_host.h
> > > > > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > > > > @@ -48,6 +48,7 @@
> > > > > >  #define KVM_REQ_SLEEP \
> > > > > >  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > > > > >  #define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
> > > > > > +#define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
> > > > > >  
> > > > > >  DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
> > > > > >  
> > > > > > @@ -208,6 +209,13 @@ struct kvm_cpu_context {
> > > > > >  
> > > > > >  typedef struct kvm_cpu_context kvm_cpu_context_t;
> > > > > >  
> > > > > > +struct vcpu_reset_state {
> > > > > > +	unsigned long	pc;
> > > > > > +	unsigned long	r0;
> > > > > > +	bool		be;
> > > > > > +	bool		reset;
> > > > > > +};
> > > > > > +
> > > > > >  struct kvm_vcpu_arch {
> > > > > >  	struct kvm_cpu_context ctxt;
> > > > > >  
> > > > > > @@ -297,6 +305,9 @@ struct kvm_vcpu_arch {
> > > > > >  	/* Virtual SError ESR to restore when HCR_EL2.VSE is set */
> > > > > >  	u64 vsesr_el2;
> > > > > >  
> > > > > > +	/* Additional reset state */
> > > > > > +	struct vcpu_reset_state	reset_state;
> > > > > > +
> > > > > >  	/* True when deferrable sysregs are loaded on the physical CPU,
> > > > > >  	 * see kvm_vcpu_load_sysregs and kvm_vcpu_put_sysregs. */
> > > > > >  	bool sysregs_loaded_on_cpu;
> > > > > > diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> > > > > > index f21a2a575939..f16a5f8ff2b4 100644
> > > > > > --- a/arch/arm64/kvm/reset.c
> > > > > > +++ b/arch/arm64/kvm/reset.c
> > > > > > @@ -32,6 +32,7 @@
> > > > > >  #include <asm/kvm_arm.h>
> > > > > >  #include <asm/kvm_asm.h>
> > > > > >  #include <asm/kvm_coproc.h>
> > > > > > +#include <asm/kvm_emulate.h>
> > > > > >  #include <asm/kvm_mmu.h>
> > > > > >  
> > > > > >  /* Maximum phys_shift supported for any VM on this host */
> > > > > > @@ -146,6 +147,29 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> > > > > >  	/* Reset system registers */
> > > > > >  	kvm_reset_sys_regs(vcpu);
> > > > > >  
> > > > > > +	/*
> > > > > > +	 * Additional reset state handling that PSCI may have imposed on us.
> > > > > > +	 * Must be done after all the sys_reg reset.
> > > > > > +	 */
> > > > > > +	if (vcpu->arch.reset_state.reset) {
> > > > > > +		unsigned long target_pc = vcpu->arch.reset_state.pc;
> > > > > > +
> > > > > > +		/* Gracefully handle Thumb2 entry point */
> > > > > > +		if (vcpu_mode_is_32bit(vcpu) && (target_pc & 1)) {
> > > > > > +			target_pc &= ~1UL;
> > > > > > +			vcpu_set_thumb(vcpu);
> > > > > > +		}
> > > > > > +
> > > > > > +		/* Propagate caller endianness */
> > > > > > +		if (vcpu->arch.reset_state.be)
> > > > > > +			kvm_vcpu_set_be(vcpu);
> > > > > > +
> > > > > > +		*vcpu_pc(vcpu) = target_pc;
> > > > > > +		vcpu_set_reg(vcpu, 0, vcpu->arch.reset_state.r0);
> > > > > > +
> > > > > > +		vcpu->arch.reset_state.reset = false;
> > > > > > +	}
> > > > > > +
> > > > > >  	/* Reset PMU */
> > > > > >  	kvm_pmu_vcpu_reset(vcpu);
> > > > > >  
> > > > > > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> > > > > > index 9e350fd34504..9c486fad3f9f 100644
> > > > > > --- a/virt/kvm/arm/arm.c
> > > > > > +++ b/virt/kvm/arm/arm.c
> > > > > > @@ -626,6 +626,13 @@ static void vcpu_req_sleep(struct kvm_vcpu *vcpu)
> > > > > >  		/* Awaken to handle a signal, request we sleep again later. */
> > > > > >  		kvm_make_request(KVM_REQ_SLEEP, vcpu);
> > > > > >  	}
> > > > > > +
> > > > > > +	/*
> > > > > > +	 * Make sure we will observe a potential reset request if we've
> > > > > > +	 * observed a change to the power state. Pairs with the smp_wmb() in
> > > > > > +	 * kvm_psci_vcpu_on().
> > > > > > +	 */
> > > > > > +	smp_rmb();
> > > > > 
> > > > > I don't believe this should be necessary, because...
> > > > > 
> > > > > >  }
> > > > > >  
> > > > > >  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
> > > > > > @@ -639,6 +646,9 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
> > > > > >  		if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
> > > > > >  			vcpu_req_sleep(vcpu);
> > > > > >  
> > > > > > +		if (kvm_check_request(KVM_REQ_VCPU_RESET, vcpu))
> > > > > > +			kvm_reset_vcpu(vcpu);
> > > > > 
> > > > > ... we do kvm_check_request here before using the reset data, and we do...
> > > > > 
> > > > > > +
> > > > > >  		/*
> > > > > >  		 * Clear IRQ_PENDING requests that were made to guarantee
> > > > > >  		 * that a VCPU sees new virtual interrupts.
> > > > > > diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
> > > > > > index 9b73d3ad918a..b9cff1d4b06d 100644
> > > > > > --- a/virt/kvm/arm/psci.c
> > > > > > +++ b/virt/kvm/arm/psci.c
> > > > > > @@ -104,12 +104,10 @@ static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu)
> > > > > >  
> > > > > >  static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> > > > > >  {
> > > > > > +	struct vcpu_reset_state *reset_state;
> > > > > >  	struct kvm *kvm = source_vcpu->kvm;
> > > > > >  	struct kvm_vcpu *vcpu = NULL;
> > > > > > -	struct swait_queue_head *wq;
> > > > > >  	unsigned long cpu_id;
> > > > > > -	unsigned long context_id;
> > > > > > -	phys_addr_t target_pc;
> > > > > >  
> > > > > >  	cpu_id = smccc_get_arg1(source_vcpu) & MPIDR_HWID_BITMASK;
> > > > > >  	if (vcpu_mode_is_32bit(source_vcpu))
> > > > > > @@ -130,32 +128,30 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> > > > > >  			return PSCI_RET_INVALID_PARAMS;
> > > > > >  	}
> > > > > >  
> > > > > > -	target_pc = smccc_get_arg2(source_vcpu);
> > > > > > -	context_id = smccc_get_arg3(source_vcpu);
> > > > > > +	reset_state = &vcpu->arch.reset_state;
> > > > > >  
> > > > > > -	kvm_reset_vcpu(vcpu);
> > > > > > -
> > > > > > -	/* Gracefully handle Thumb2 entry point */
> > > > > > -	if (vcpu_mode_is_32bit(vcpu) && (target_pc & 1)) {
> > > > > > -		target_pc &= ~((phys_addr_t) 1);
> > > > > > -		vcpu_set_thumb(vcpu);
> > > > > > -	}
> > > > > > +	reset_state->pc = smccc_get_arg2(source_vcpu);
> > > > > >  
> > > > > >  	/* Propagate caller endianness */
> > > > > > -	if (kvm_vcpu_is_be(source_vcpu))
> > > > > > -		kvm_vcpu_set_be(vcpu);
> > > > > > +	reset_state->be = kvm_vcpu_is_be(source_vcpu);
> > > > > >  
> > > > > > -	*vcpu_pc(vcpu) = target_pc;
> > > > > >  	/*
> > > > > >  	 * NOTE: We always update r0 (or x0) because for PSCI v0.1
> > > > > >  	 * the general puspose registers are undefined upon CPU_ON.
> > > > > >  	 */
> > > > > > -	smccc_set_retval(vcpu, context_id, 0, 0, 0);
> > > > > > -	vcpu->arch.power_off = false;
> > > > > > -	smp_mb();		/* Make sure the above is visible */
> > > > > > +	reset_state->r0 = smccc_get_arg3(source_vcpu);
> > > > > > +
> > > > > > +	reset_state->reset = true;
> > > > > > +	kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
> > > > > 
> > > > > ... this kvm_make_request() after writing the reset data. The
> > > > > kvm_make_request/kvm_check_request embeds the necessary barriers.
> > > > > 
> > > > > >  
> > > > > > -	wq = kvm_arch_vcpu_wq(vcpu);
> > > > > > -	swake_up_one(wq);
> > > > > > +	/*
> > > > > > +	 * Make sure the reset request is observed if the change to
> > > > > > +	 * power_state is observed.
> > > > > > +	 */
> > > > > > +	smp_wmb();
> > > > > > +
> > > > > > +	vcpu->arch.power_off = false;
> > > > > 
> > > > > Or you want to tie the reset data to the observability of the power_off
> > > > > state? Why not just tie it to the KVM_REQ_VCPU_RESET request?
> > > > > 
> > > > 
> > > > The barrier in kvm_make_request is *before* the write to vcpu->requests,
> > > > so doesn't enforce any ordering of stores occurring *after* that write.
> > > > 
> > > > We don't want to wake up the target VCPU thread, have it observe that it
> > > > is powered on, yet not observe the reset request (my comment above
> > > > inversed).
> > > > 
> > > > I wrote this Litmus test (which is probably so appalingly obvious to
> > > > memory ordering folks that it is unnecessary) to convince myself the
> > > > barrier is needed:
> > > > 
> > > > ----<8----
> > > > C vcpu-reset
> > > > 
> > > > {}
> > > > 
> > > > P0(int *power, int *reset)
> > > > {
> > > > 	smp_wmb(); // from kvm_make_request
> > > > 	WRITE_ONCE(*reset, 1); // kvm_psci_vcpu_on: kvm_make_request(KVM_REQ_RESET, vcpu);
> > > > 	smp_wmb();
> > > > 	WRITE_ONCE(*power, 1); // kvm_psci_vcpu_on: vcpu->arch.power_state = KVM_ARM_VCPU_ON;
> > > > }
> > > > 
> > > > P1(int *power, int *reset, spinlock_t *wqlock)
> > > > {
> > > > 	int r0;
> > > > 	int r1;
> > > > 
> > > > 	spin_lock(wqlock);
> > > > 	smp_mb(); // from prepare_to_wait -> set_current_state
> > > > 	spin_unlock(wqlock);
> > > > 	r0 = READ_ONCE(*power); // vcpu_req_sleep: if (vcpu_sleeping(vcpu))
> > > > 	smp_rmb();
> > > > 	r1 = READ_ONCE(*reset); // check_vcpu_requests: if (kvm_check_request(KVM_REQ_RESET, vcpu))
> > > > }
> > > > 
> > > > exists (1:r0=1 /\ 1:r1=0)
> > > > ----<8----
> > > > 
> > > > 
> > > > Hope this helps,
> > > 
> > > It certainly does, mostly because I didn't review closely enough and
> > > thought the pair we were interested in was vcpu_reset_state and the
> > > RESET vcpu request, which the vcpu request API handles. Now it's
> > > clear we're worried about the pair power_off and the RESET vcpu
> > > request. And we've reversed the usual "if we observe a request, then
> > > we can observe its accompanying data" to "if we observe a change in
> > > some data (power_off), then we need to also observe a request". I
> > > agree the barriers are necessary to ensure that in the direct
> > > sequence, but now I'm wondering if we need to care about the
> > > direct sequence.
> > > 
> > > If we remove the barriers then the vcpu could have these possible
> > > sequences
> > > 
> > > 1)
> > >   - wake up
> > >   - observe power_off is false
> > >   - observe the RESET vcpu request - do the reset
> > >   - enter guest
> > > 
> > > That's the one this patch ensures, so it's definitely correct.
> > > 
> > > 2)
> > >   - wake up
> > >   - the change to power_off not observed - the vcpu makes the
> > >     SLEEP vcpu request on itself
> > >   - observe the RESET vcpu request - do the reset
> > >   - observe the SLEEP vcpu request (with or without the need for
> > >     an IPI), attempt to sleep again, but now observe power_off
> > >     is false
> > >   - enter guest
> > > 
> > > I think that should be correct as well, even if less efficient.
> > > 
> > > 3)
> > >   - wake up
> > >   - observe power_off is false
> > >   - don't observe the RESET request yet, get closer to entering
> > >     the guest
> > >   - observe the RESET request a bit later (with or without the
> > >     need for an IPI) - do the reset
> > >   - enter guest
> > > 
> > > This is the same as (1), but we rely on the kvm_request_pending
> > > stuff to make sure the vcpu request is seen before entering the
> > > guest.
> > > 
> > > 4)
> > >   - wake up
> > >   - the change to power_off not observed - the vcpu makes the
> > >     SLEEP vcpu request on itself
> > >   - don't observe the RESET request yet, get closer to entering
> > >     the guest
> > >   - observe the SLEEP vcpu request (with or without the need for
> > >     an IPI), attempt to sleep again, but now observe power_off
> > >     is false
> > >   - observe the RESET request a bit later (with or without the
> > >     need for an IPI) - do the reset
> > >   - enter guest
> > > 
> > > The least efficient one, but should still be correct.
> > > 
> > > If you agree with this, then we get to remove the barriers,
> > > simplifying things, and also we don't introduce the ordering
> > > dependency in check_vcpu_requests(), where the vcpu_req_sleep()
> > > calls must now come before checking the RESET request.
> > > 
> > 
> > I'm sorry, I'm not following what you want to achieve here?
> > 
> > Somehow delivering a series of possible interleaved executions doesn't
> > exactly convey anything meaningful to me.
> >
> 
> This patch introduces an unconventional use of vcpu requests, which is
> to ensure a change to vcpu->requests is observed if a change to
> arch.power_off is observed. That unconventional use requires additional
> barriers and an ordering requirement in check_vcpu_requests(), i.e.
> anything that calls vcpu_req_sleep() must come before the check for
> the RESET vcpu request. I'm stating, and attempting to prove with the
> list of possible executions, that those barriers and ordering requirements
> are not necessary.
> 
> Assuming the objective is to ensure a vcpu reset is done before entering
> guest mode after a vcpu power on, then the RESET vcpu request is
> sufficient by itself. vcpu requests are guaranteed to be seen by a vcpu
> before entering guest mode.
> 
> Additionally, I'm pretty sure that even leaving this patch as it is,
> all the above execution scenarios are possible except (3), depending on
> the timing of a signal delivery.
> 
> Is there a problem with (3) that I'm missing? Or am I missing something
> else?

There's not a problem with (3), but the question is flawed; you've shown
me a benign execution sequence and somehow you're arguing that the
execution sequence is always correct?

I don't think there's anything very unconventional here.

Let's try this:  If you have a better way of implementing this, how
about you write a patch?


Thanks,

    Christoffer

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] arm/arm64: KVM: Allow a VCPU to fully reset itself

[Andrew Jones]

On Thu, Jan 31, 2019 at 12:51:56PM +0100, Christoffer Dall wrote:
> On Thu, Jan 31, 2019 at 11:12:54AM +0100, Andrew Jones wrote:
> > On Thu, Jan 31, 2019 at 08:43:53AM +0100, Christoffer Dall wrote:
> > > On Wed, Jan 30, 2019 at 04:27:21PM +0100, Andrew Jones wrote:
> > > > On Wed, Jan 30, 2019 at 10:34:31AM +0100, Christoffer Dall wrote:
> > > > > On Tue, Jan 29, 2019 at 05:03:47PM +0100, Andrew Jones wrote:
> > > > > > On Fri, Jan 25, 2019 at 10:46:53AM +0100, Christoffer Dall wrote:
> > > > > > > From: Marc Zyngier <marc.zyngier@arm.com>
> > > > > > > 
> > > > > > > The current kvm_psci_vcpu_on implementation will directly try to
> > > > > > > manipulate the state of the VCPU to reset it.  However, since this is
> > > > > > > not done on the thread that runs the VCPU, we can end up in a strangely
> > > > > > > corrupted state when the source and target VCPUs are running at the same
> > > > > > > time.
> > > > > > > 
> > > > > > > Fix this by factoring out all reset logic from the PSCI implementation
> > > > > > > and forwarding the required information along with a request to the
> > > > > > > target VCPU.
> > > > > > 
> > > > > > The last patch makes more sense, now that I see this one. I guess their
> > > > > > order should be swapped.
> > > > > > 
> > > > > > > 
> > > > > > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > > > > > > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> > > > > > > ---
> > > > > > >  arch/arm/include/asm/kvm_host.h   | 10 +++++++++
> > > > > > >  arch/arm/kvm/reset.c              | 24 +++++++++++++++++++++
> > > > > > >  arch/arm64/include/asm/kvm_host.h | 11 ++++++++++
> > > > > > >  arch/arm64/kvm/reset.c            | 24 +++++++++++++++++++++
> > > > > > >  virt/kvm/arm/arm.c                | 10 +++++++++
> > > > > > >  virt/kvm/arm/psci.c               | 36 ++++++++++++++-----------------
> > > > > > >  6 files changed, 95 insertions(+), 20 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> > > > > > > index ca56537b61bc..50e89869178a 100644
> > > > > > > --- a/arch/arm/include/asm/kvm_host.h
> > > > > > > +++ b/arch/arm/include/asm/kvm_host.h
> > > > > > > @@ -48,6 +48,7 @@
> > > > > > >  #define KVM_REQ_SLEEP \
> > > > > > >  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > > > > > >  #define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
> > > > > > > +#define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
> > > > > > >  
> > > > > > >  DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
> > > > > > >  
> > > > > > > @@ -147,6 +148,13 @@ struct kvm_cpu_context {
> > > > > > >  
> > > > > > >  typedef struct kvm_cpu_context kvm_cpu_context_t;
> > > > > > >  
> > > > > > > +struct vcpu_reset_state {
> > > > > > > +	unsigned long	pc;
> > > > > > > +	unsigned long	r0;
> > > > > > > +	bool		be;
> > > > > > > +	bool		reset;
> > > > > > > +};
> > > > > > > +
> > > > > > >  struct kvm_vcpu_arch {
> > > > > > >  	struct kvm_cpu_context ctxt;
> > > > > > >  
> > > > > > > @@ -186,6 +194,8 @@ struct kvm_vcpu_arch {
> > > > > > >  	/* Cache some mmu pages needed inside spinlock regions */
> > > > > > >  	struct kvm_mmu_memory_cache mmu_page_cache;
> > > > > > >  
> > > > > > > +	struct vcpu_reset_state reset_state;
> > > > > > > +
> > > > > > >  	/* Detect first run of a vcpu */
> > > > > > >  	bool has_run_once;
> > > > > > >  };
> > > > > > > diff --git a/arch/arm/kvm/reset.c b/arch/arm/kvm/reset.c
> > > > > > > index 5ed0c3ee33d6..de41255eebcd 100644
> > > > > > > --- a/arch/arm/kvm/reset.c
> > > > > > > +++ b/arch/arm/kvm/reset.c
> > > > > > > @@ -26,6 +26,7 @@
> > > > > > >  #include <asm/cputype.h>
> > > > > > >  #include <asm/kvm_arm.h>
> > > > > > >  #include <asm/kvm_coproc.h>
> > > > > > > +#include <asm/kvm_emulate.h>
> > > > > > >  
> > > > > > >  #include <kvm/arm_arch_timer.h>
> > > > > > >  
> > > > > > > @@ -69,6 +70,29 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> > > > > > >  	/* Reset CP15 registers */
> > > > > > >  	kvm_reset_coprocs(vcpu);
> > > > > > >  
> > > > > > > +	/*
> > > > > > > +	 * Additional reset state handling that PSCI may have imposed on us.
> > > > > > > +	 * Must be done after all the sys_reg reset.
> > > > > > > +	 */
> > > > > > > +	if (vcpu->arch.reset_state.reset) {
> > > > > > > +		unsigned long target_pc = vcpu->arch.reset_state.pc;
> > > > > > > +
> > > > > > > +		/* Gracefully handle Thumb2 entry point */
> > > > > > > +		if (target_pc & 1) {
> > > > > > > +			target_pc &= ~1UL;
> > > > > > > +			vcpu_set_thumb(vcpu);
> > > > > > > +		}
> > > > > > > +
> > > > > > > +		/* Propagate caller endianness */
> > > > > > > +		if (vcpu->arch.reset_state.be)
> > > > > > > +			kvm_vcpu_set_be(vcpu);
> > > > > > > +
> > > > > > > +		*vcpu_pc(vcpu) = target_pc;
> > > > > > > +		vcpu_set_reg(vcpu, 0, vcpu->arch.reset_state.r0);
> > > > > > > +
> > > > > > > +		vcpu->arch.reset_state.reset = false;
> > > > > > > +	}
> > > > > > > +
> > > > > > >  	/* Reset arch_timer context */
> > > > > > >  	return kvm_timer_vcpu_reset(vcpu);
> > > > > > >  }
> > > > > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > > > > > > index 7732d0ba4e60..da3fc7324d68 100644
> > > > > > > --- a/arch/arm64/include/asm/kvm_host.h
> > > > > > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > > > > > @@ -48,6 +48,7 @@
> > > > > > >  #define KVM_REQ_SLEEP \
> > > > > > >  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > > > > > >  #define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
> > > > > > > +#define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
> > > > > > >  
> > > > > > >  DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
> > > > > > >  
> > > > > > > @@ -208,6 +209,13 @@ struct kvm_cpu_context {
> > > > > > >  
> > > > > > >  typedef struct kvm_cpu_context kvm_cpu_context_t;
> > > > > > >  
> > > > > > > +struct vcpu_reset_state {
> > > > > > > +	unsigned long	pc;
> > > > > > > +	unsigned long	r0;
> > > > > > > +	bool		be;
> > > > > > > +	bool		reset;
> > > > > > > +};
> > > > > > > +
> > > > > > >  struct kvm_vcpu_arch {
> > > > > > >  	struct kvm_cpu_context ctxt;
> > > > > > >  
> > > > > > > @@ -297,6 +305,9 @@ struct kvm_vcpu_arch {
> > > > > > >  	/* Virtual SError ESR to restore when HCR_EL2.VSE is set */
> > > > > > >  	u64 vsesr_el2;
> > > > > > >  
> > > > > > > +	/* Additional reset state */
> > > > > > > +	struct vcpu_reset_state	reset_state;
> > > > > > > +
> > > > > > >  	/* True when deferrable sysregs are loaded on the physical CPU,
> > > > > > >  	 * see kvm_vcpu_load_sysregs and kvm_vcpu_put_sysregs. */
> > > > > > >  	bool sysregs_loaded_on_cpu;
> > > > > > > diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> > > > > > > index f21a2a575939..f16a5f8ff2b4 100644
> > > > > > > --- a/arch/arm64/kvm/reset.c
> > > > > > > +++ b/arch/arm64/kvm/reset.c
> > > > > > > @@ -32,6 +32,7 @@
> > > > > > >  #include <asm/kvm_arm.h>
> > > > > > >  #include <asm/kvm_asm.h>
> > > > > > >  #include <asm/kvm_coproc.h>
> > > > > > > +#include <asm/kvm_emulate.h>
> > > > > > >  #include <asm/kvm_mmu.h>
> > > > > > >  
> > > > > > >  /* Maximum phys_shift supported for any VM on this host */
> > > > > > > @@ -146,6 +147,29 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> > > > > > >  	/* Reset system registers */
> > > > > > >  	kvm_reset_sys_regs(vcpu);
> > > > > > >  
> > > > > > > +	/*
> > > > > > > +	 * Additional reset state handling that PSCI may have imposed on us.
> > > > > > > +	 * Must be done after all the sys_reg reset.
> > > > > > > +	 */
> > > > > > > +	if (vcpu->arch.reset_state.reset) {
> > > > > > > +		unsigned long target_pc = vcpu->arch.reset_state.pc;
> > > > > > > +
> > > > > > > +		/* Gracefully handle Thumb2 entry point */
> > > > > > > +		if (vcpu_mode_is_32bit(vcpu) && (target_pc & 1)) {
> > > > > > > +			target_pc &= ~1UL;
> > > > > > > +			vcpu_set_thumb(vcpu);
> > > > > > > +		}
> > > > > > > +
> > > > > > > +		/* Propagate caller endianness */
> > > > > > > +		if (vcpu->arch.reset_state.be)
> > > > > > > +			kvm_vcpu_set_be(vcpu);
> > > > > > > +
> > > > > > > +		*vcpu_pc(vcpu) = target_pc;
> > > > > > > +		vcpu_set_reg(vcpu, 0, vcpu->arch.reset_state.r0);
> > > > > > > +
> > > > > > > +		vcpu->arch.reset_state.reset = false;
> > > > > > > +	}
> > > > > > > +
> > > > > > >  	/* Reset PMU */
> > > > > > >  	kvm_pmu_vcpu_reset(vcpu);
> > > > > > >  
> > > > > > > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> > > > > > > index 9e350fd34504..9c486fad3f9f 100644
> > > > > > > --- a/virt/kvm/arm/arm.c
> > > > > > > +++ b/virt/kvm/arm/arm.c
> > > > > > > @@ -626,6 +626,13 @@ static void vcpu_req_sleep(struct kvm_vcpu *vcpu)
> > > > > > >  		/* Awaken to handle a signal, request we sleep again later. */
> > > > > > >  		kvm_make_request(KVM_REQ_SLEEP, vcpu);
> > > > > > >  	}
> > > > > > > +
> > > > > > > +	/*
> > > > > > > +	 * Make sure we will observe a potential reset request if we've
> > > > > > > +	 * observed a change to the power state. Pairs with the smp_wmb() in
> > > > > > > +	 * kvm_psci_vcpu_on().
> > > > > > > +	 */
> > > > > > > +	smp_rmb();
> > > > > > 
> > > > > > I don't believe this should be necessary, because...
> > > > > > 
> > > > > > >  }
> > > > > > >  
> > > > > > >  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
> > > > > > > @@ -639,6 +646,9 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
> > > > > > >  		if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
> > > > > > >  			vcpu_req_sleep(vcpu);
> > > > > > >  
> > > > > > > +		if (kvm_check_request(KVM_REQ_VCPU_RESET, vcpu))
> > > > > > > +			kvm_reset_vcpu(vcpu);
> > > > > > 
> > > > > > ... we do kvm_check_request here before using the reset data, and we do...
> > > > > > 
> > > > > > > +
> > > > > > >  		/*
> > > > > > >  		 * Clear IRQ_PENDING requests that were made to guarantee
> > > > > > >  		 * that a VCPU sees new virtual interrupts.
> > > > > > > diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
> > > > > > > index 9b73d3ad918a..b9cff1d4b06d 100644
> > > > > > > --- a/virt/kvm/arm/psci.c
> > > > > > > +++ b/virt/kvm/arm/psci.c
> > > > > > > @@ -104,12 +104,10 @@ static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu)
> > > > > > >  
> > > > > > >  static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> > > > > > >  {
> > > > > > > +	struct vcpu_reset_state *reset_state;
> > > > > > >  	struct kvm *kvm = source_vcpu->kvm;
> > > > > > >  	struct kvm_vcpu *vcpu = NULL;
> > > > > > > -	struct swait_queue_head *wq;
> > > > > > >  	unsigned long cpu_id;
> > > > > > > -	unsigned long context_id;
> > > > > > > -	phys_addr_t target_pc;
> > > > > > >  
> > > > > > >  	cpu_id = smccc_get_arg1(source_vcpu) & MPIDR_HWID_BITMASK;
> > > > > > >  	if (vcpu_mode_is_32bit(source_vcpu))
> > > > > > > @@ -130,32 +128,30 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> > > > > > >  			return PSCI_RET_INVALID_PARAMS;
> > > > > > >  	}
> > > > > > >  
> > > > > > > -	target_pc = smccc_get_arg2(source_vcpu);
> > > > > > > -	context_id = smccc_get_arg3(source_vcpu);
> > > > > > > +	reset_state = &vcpu->arch.reset_state;
> > > > > > >  
> > > > > > > -	kvm_reset_vcpu(vcpu);
> > > > > > > -
> > > > > > > -	/* Gracefully handle Thumb2 entry point */
> > > > > > > -	if (vcpu_mode_is_32bit(vcpu) && (target_pc & 1)) {
> > > > > > > -		target_pc &= ~((phys_addr_t) 1);
> > > > > > > -		vcpu_set_thumb(vcpu);
> > > > > > > -	}
> > > > > > > +	reset_state->pc = smccc_get_arg2(source_vcpu);
> > > > > > >  
> > > > > > >  	/* Propagate caller endianness */
> > > > > > > -	if (kvm_vcpu_is_be(source_vcpu))
> > > > > > > -		kvm_vcpu_set_be(vcpu);
> > > > > > > +	reset_state->be = kvm_vcpu_is_be(source_vcpu);
> > > > > > >  
> > > > > > > -	*vcpu_pc(vcpu) = target_pc;
> > > > > > >  	/*
> > > > > > >  	 * NOTE: We always update r0 (or x0) because for PSCI v0.1
> > > > > > >  	 * the general puspose registers are undefined upon CPU_ON.
> > > > > > >  	 */
> > > > > > > -	smccc_set_retval(vcpu, context_id, 0, 0, 0);
> > > > > > > -	vcpu->arch.power_off = false;
> > > > > > > -	smp_mb();		/* Make sure the above is visible */
> > > > > > > +	reset_state->r0 = smccc_get_arg3(source_vcpu);
> > > > > > > +
> > > > > > > +	reset_state->reset = true;
> > > > > > > +	kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
> > > > > > 
> > > > > > ... this kvm_make_request() after writing the reset data. The
> > > > > > kvm_make_request/kvm_check_request embeds the necessary barriers.
> > > > > > 
> > > > > > >  
> > > > > > > -	wq = kvm_arch_vcpu_wq(vcpu);
> > > > > > > -	swake_up_one(wq);
> > > > > > > +	/*
> > > > > > > +	 * Make sure the reset request is observed if the change to
> > > > > > > +	 * power_state is observed.
> > > > > > > +	 */
> > > > > > > +	smp_wmb();
> > > > > > > +
> > > > > > > +	vcpu->arch.power_off = false;
> > > > > > 
> > > > > > Or you want to tie the reset data to the observability of the power_off
> > > > > > state? Why not just tie it to the KVM_REQ_VCPU_RESET request?
> > > > > > 
> > > > > 
> > > > > The barrier in kvm_make_request is *before* the write to vcpu->requests,
> > > > > so doesn't enforce any ordering of stores occurring *after* that write.
> > > > > 
> > > > > We don't want to wake up the target VCPU thread, have it observe that it
> > > > > is powered on, yet not observe the reset request (my comment above
> > > > > inversed).
> > > > > 
> > > > > I wrote this Litmus test (which is probably so appalingly obvious to
> > > > > memory ordering folks that it is unnecessary) to convince myself the
> > > > > barrier is needed:
> > > > > 
> > > > > ----<8----
> > > > > C vcpu-reset
> > > > > 
> > > > > {}
> > > > > 
> > > > > P0(int *power, int *reset)
> > > > > {
> > > > > 	smp_wmb(); // from kvm_make_request
> > > > > 	WRITE_ONCE(*reset, 1); // kvm_psci_vcpu_on: kvm_make_request(KVM_REQ_RESET, vcpu);
> > > > > 	smp_wmb();
> > > > > 	WRITE_ONCE(*power, 1); // kvm_psci_vcpu_on: vcpu->arch.power_state = KVM_ARM_VCPU_ON;
> > > > > }
> > > > > 
> > > > > P1(int *power, int *reset, spinlock_t *wqlock)
> > > > > {
> > > > > 	int r0;
> > > > > 	int r1;
> > > > > 
> > > > > 	spin_lock(wqlock);
> > > > > 	smp_mb(); // from prepare_to_wait -> set_current_state
> > > > > 	spin_unlock(wqlock);
> > > > > 	r0 = READ_ONCE(*power); // vcpu_req_sleep: if (vcpu_sleeping(vcpu))
> > > > > 	smp_rmb();
> > > > > 	r1 = READ_ONCE(*reset); // check_vcpu_requests: if (kvm_check_request(KVM_REQ_RESET, vcpu))
> > > > > }
> > > > > 
> > > > > exists (1:r0=1 /\ 1:r1=0)
> > > > > ----<8----
> > > > > 
> > > > > 
> > > > > Hope this helps,
> > > > 
> > > > It certainly does, mostly because I didn't review closely enough and
> > > > thought the pair we were interested in was vcpu_reset_state and the
> > > > RESET vcpu request, which the vcpu request API handles. Now it's
> > > > clear we're worried about the pair power_off and the RESET vcpu
> > > > request. And we've reversed the usual "if we observe a request, then
> > > > we can observe its accompanying data" to "if we observe a change in
> > > > some data (power_off), then we need to also observe a request". I
> > > > agree the barriers are necessary to ensure that in the direct
> > > > sequence, but now I'm wondering if we need to care about the
> > > > direct sequence.
> > > > 
> > > > If we remove the barriers then the vcpu could have these possible
> > > > sequences
> > > > 
> > > > 1)
> > > >   - wake up
> > > >   - observe power_off is false
> > > >   - observe the RESET vcpu request - do the reset
> > > >   - enter guest
> > > > 
> > > > That's the one this patch ensures, so it's definitely correct.
> > > > 
> > > > 2)
> > > >   - wake up
> > > >   - the change to power_off not observed - the vcpu makes the
> > > >     SLEEP vcpu request on itself
> > > >   - observe the RESET vcpu request - do the reset
> > > >   - observe the SLEEP vcpu request (with or without the need for
> > > >     an IPI), attempt to sleep again, but now observe power_off
> > > >     is false
> > > >   - enter guest
> > > > 
> > > > I think that should be correct as well, even if less efficient.
> > > > 
> > > > 3)
> > > >   - wake up
> > > >   - observe power_off is false
> > > >   - don't observe the RESET request yet, get closer to entering
> > > >     the guest
> > > >   - observe the RESET request a bit later (with or without the
> > > >     need for an IPI) - do the reset
> > > >   - enter guest
> > > > 
> > > > This is the same as (1), but we rely on the kvm_request_pending
> > > > stuff to make sure the vcpu request is seen before entering the
> > > > guest.
> > > > 
> > > > 4)
> > > >   - wake up
> > > >   - the change to power_off not observed - the vcpu makes the
> > > >     SLEEP vcpu request on itself
> > > >   - don't observe the RESET request yet, get closer to entering
> > > >     the guest
> > > >   - observe the SLEEP vcpu request (with or without the need for
> > > >     an IPI), attempt to sleep again, but now observe power_off
> > > >     is false
> > > >   - observe the RESET request a bit later (with or without the
> > > >     need for an IPI) - do the reset
> > > >   - enter guest
> > > > 
> > > > The least efficient one, but should still be correct.
> > > > 
> > > > If you agree with this, then we get to remove the barriers,
> > > > simplifying things, and also we don't introduce the ordering
> > > > dependency in check_vcpu_requests(), where the vcpu_req_sleep()
> > > > calls must now come before checking the RESET request.
> > > > 
> > > 
> > > I'm sorry, I'm not following what you want to achieve here?
> > > 
> > > Somehow delivering a series of possible interleaved executions doesn't
> > > exactly convey anything meaningful to me.
> > >
> > 
> > This patch introduces an unconventional use of vcpu requests, which is
> > to ensure a change to vcpu->requests is observed if a change to
> > arch.power_off is observed. That unconventional use requires additional
> > barriers and an ordering requirement in check_vcpu_requests(), i.e.
> > anything that calls vcpu_req_sleep() must come before the check for
> > the RESET vcpu request. I'm stating, and attempting to prove with the
> > list of possible executions, that those barriers and ordering requirements
> > are not necessary.
> > 
> > Assuming the objective is to ensure a vcpu reset is done before entering
> > guest mode after a vcpu power on, then the RESET vcpu request is
> > sufficient by itself. vcpu requests are guaranteed to be seen by a vcpu
> > before entering guest mode.
> > 
> > Additionally, I'm pretty sure that even leaving this patch as it is,
> > all the above execution scenarios are possible except (3), depending on
> > the timing of a signal delivery.
> > 
> > Is there a problem with (3) that I'm missing? Or am I missing something
> > else?
> 
> There's not a problem with (3), but the question is flawed; you've shown
> me a benign execution sequence and somehow you're arguing that the
> execution sequence is always correct?

Since you agree the execution sequence is fine, then I can't see why we
would want these additional barriers. Did I miss the execution sequence
you're concerned with?

> 
> I don't think there's anything very unconventional here.

Normally if a thread observes a change to vcpu->requests, then we ensure a
change to some accompanying data is also observable. We're reversing that
here, which adds a need for additional barriers and a strict request
checking order.

> 
> Let's try this:  If you have a better way of implementing this, how
> about you write a patch?

It would just be this patch minus the unnecessary barriers. I can send it
if you like, but I wouldn't want to change the authorship for such a small
change.

drew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] arm/arm64: KVM: Allow a VCPU to fully reset itself

[Christoffer Dall]

On Thu, Jan 31, 2019 at 01:57:12PM +0100, Andrew Jones wrote:
> On Thu, Jan 31, 2019 at 12:51:56PM +0100, Christoffer Dall wrote:
> > On Thu, Jan 31, 2019 at 11:12:54AM +0100, Andrew Jones wrote:
> > > On Thu, Jan 31, 2019 at 08:43:53AM +0100, Christoffer Dall wrote:
> > > > On Wed, Jan 30, 2019 at 04:27:21PM +0100, Andrew Jones wrote:
> > > > > On Wed, Jan 30, 2019 at 10:34:31AM +0100, Christoffer Dall wrote:
> > > > > > On Tue, Jan 29, 2019 at 05:03:47PM +0100, Andrew Jones wrote:
> > > > > > > On Fri, Jan 25, 2019 at 10:46:53AM +0100, Christoffer Dall wrote:
> > > > > > > > From: Marc Zyngier <marc.zyngier@arm.com>
> > > > > > > > 
> > > > > > > > The current kvm_psci_vcpu_on implementation will directly try to
> > > > > > > > manipulate the state of the VCPU to reset it.  However, since this is
> > > > > > > > not done on the thread that runs the VCPU, we can end up in a strangely
> > > > > > > > corrupted state when the source and target VCPUs are running at the same
> > > > > > > > time.
> > > > > > > > 
> > > > > > > > Fix this by factoring out all reset logic from the PSCI implementation
> > > > > > > > and forwarding the required information along with a request to the
> > > > > > > > target VCPU.
> > > > > > > 
> > > > > > > The last patch makes more sense, now that I see this one. I guess their
> > > > > > > order should be swapped.
> > > > > > > 
> > > > > > > > 
> > > > > > > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > > > > > > > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> > > > > > > > ---
> > > > > > > >  arch/arm/include/asm/kvm_host.h   | 10 +++++++++
> > > > > > > >  arch/arm/kvm/reset.c              | 24 +++++++++++++++++++++
> > > > > > > >  arch/arm64/include/asm/kvm_host.h | 11 ++++++++++
> > > > > > > >  arch/arm64/kvm/reset.c            | 24 +++++++++++++++++++++
> > > > > > > >  virt/kvm/arm/arm.c                | 10 +++++++++
> > > > > > > >  virt/kvm/arm/psci.c               | 36 ++++++++++++++-----------------
> > > > > > > >  6 files changed, 95 insertions(+), 20 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> > > > > > > > index ca56537b61bc..50e89869178a 100644
> > > > > > > > --- a/arch/arm/include/asm/kvm_host.h
> > > > > > > > +++ b/arch/arm/include/asm/kvm_host.h
> > > > > > > > @@ -48,6 +48,7 @@
> > > > > > > >  #define KVM_REQ_SLEEP \
> > > > > > > >  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > > > > > > >  #define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
> > > > > > > > +#define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
> > > > > > > >  
> > > > > > > >  DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
> > > > > > > >  
> > > > > > > > @@ -147,6 +148,13 @@ struct kvm_cpu_context {
> > > > > > > >  
> > > > > > > >  typedef struct kvm_cpu_context kvm_cpu_context_t;
> > > > > > > >  
> > > > > > > > +struct vcpu_reset_state {
> > > > > > > > +	unsigned long	pc;
> > > > > > > > +	unsigned long	r0;
> > > > > > > > +	bool		be;
> > > > > > > > +	bool		reset;
> > > > > > > > +};
> > > > > > > > +
> > > > > > > >  struct kvm_vcpu_arch {
> > > > > > > >  	struct kvm_cpu_context ctxt;
> > > > > > > >  
> > > > > > > > @@ -186,6 +194,8 @@ struct kvm_vcpu_arch {
> > > > > > > >  	/* Cache some mmu pages needed inside spinlock regions */
> > > > > > > >  	struct kvm_mmu_memory_cache mmu_page_cache;
> > > > > > > >  
> > > > > > > > +	struct vcpu_reset_state reset_state;
> > > > > > > > +
> > > > > > > >  	/* Detect first run of a vcpu */
> > > > > > > >  	bool has_run_once;
> > > > > > > >  };
> > > > > > > > diff --git a/arch/arm/kvm/reset.c b/arch/arm/kvm/reset.c
> > > > > > > > index 5ed0c3ee33d6..de41255eebcd 100644
> > > > > > > > --- a/arch/arm/kvm/reset.c
> > > > > > > > +++ b/arch/arm/kvm/reset.c
> > > > > > > > @@ -26,6 +26,7 @@
> > > > > > > >  #include <asm/cputype.h>
> > > > > > > >  #include <asm/kvm_arm.h>
> > > > > > > >  #include <asm/kvm_coproc.h>
> > > > > > > > +#include <asm/kvm_emulate.h>
> > > > > > > >  
> > > > > > > >  #include <kvm/arm_arch_timer.h>
> > > > > > > >  
> > > > > > > > @@ -69,6 +70,29 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> > > > > > > >  	/* Reset CP15 registers */
> > > > > > > >  	kvm_reset_coprocs(vcpu);
> > > > > > > >  
> > > > > > > > +	/*
> > > > > > > > +	 * Additional reset state handling that PSCI may have imposed on us.
> > > > > > > > +	 * Must be done after all the sys_reg reset.
> > > > > > > > +	 */
> > > > > > > > +	if (vcpu->arch.reset_state.reset) {
> > > > > > > > +		unsigned long target_pc = vcpu->arch.reset_state.pc;
> > > > > > > > +
> > > > > > > > +		/* Gracefully handle Thumb2 entry point */
> > > > > > > > +		if (target_pc & 1) {
> > > > > > > > +			target_pc &= ~1UL;
> > > > > > > > +			vcpu_set_thumb(vcpu);
> > > > > > > > +		}
> > > > > > > > +
> > > > > > > > +		/* Propagate caller endianness */
> > > > > > > > +		if (vcpu->arch.reset_state.be)
> > > > > > > > +			kvm_vcpu_set_be(vcpu);
> > > > > > > > +
> > > > > > > > +		*vcpu_pc(vcpu) = target_pc;
> > > > > > > > +		vcpu_set_reg(vcpu, 0, vcpu->arch.reset_state.r0);
> > > > > > > > +
> > > > > > > > +		vcpu->arch.reset_state.reset = false;
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > >  	/* Reset arch_timer context */
> > > > > > > >  	return kvm_timer_vcpu_reset(vcpu);
> > > > > > > >  }
> > > > > > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > > > > > > > index 7732d0ba4e60..da3fc7324d68 100644
> > > > > > > > --- a/arch/arm64/include/asm/kvm_host.h
> > > > > > > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > > > > > > @@ -48,6 +48,7 @@
> > > > > > > >  #define KVM_REQ_SLEEP \
> > > > > > > >  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > > > > > > >  #define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
> > > > > > > > +#define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
> > > > > > > >  
> > > > > > > >  DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
> > > > > > > >  
> > > > > > > > @@ -208,6 +209,13 @@ struct kvm_cpu_context {
> > > > > > > >  
> > > > > > > >  typedef struct kvm_cpu_context kvm_cpu_context_t;
> > > > > > > >  
> > > > > > > > +struct vcpu_reset_state {
> > > > > > > > +	unsigned long	pc;
> > > > > > > > +	unsigned long	r0;
> > > > > > > > +	bool		be;
> > > > > > > > +	bool		reset;
> > > > > > > > +};
> > > > > > > > +
> > > > > > > >  struct kvm_vcpu_arch {
> > > > > > > >  	struct kvm_cpu_context ctxt;
> > > > > > > >  
> > > > > > > > @@ -297,6 +305,9 @@ struct kvm_vcpu_arch {
> > > > > > > >  	/* Virtual SError ESR to restore when HCR_EL2.VSE is set */
> > > > > > > >  	u64 vsesr_el2;
> > > > > > > >  
> > > > > > > > +	/* Additional reset state */
> > > > > > > > +	struct vcpu_reset_state	reset_state;
> > > > > > > > +
> > > > > > > >  	/* True when deferrable sysregs are loaded on the physical CPU,
> > > > > > > >  	 * see kvm_vcpu_load_sysregs and kvm_vcpu_put_sysregs. */
> > > > > > > >  	bool sysregs_loaded_on_cpu;
> > > > > > > > diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> > > > > > > > index f21a2a575939..f16a5f8ff2b4 100644
> > > > > > > > --- a/arch/arm64/kvm/reset.c
> > > > > > > > +++ b/arch/arm64/kvm/reset.c
> > > > > > > > @@ -32,6 +32,7 @@
> > > > > > > >  #include <asm/kvm_arm.h>
> > > > > > > >  #include <asm/kvm_asm.h>
> > > > > > > >  #include <asm/kvm_coproc.h>
> > > > > > > > +#include <asm/kvm_emulate.h>
> > > > > > > >  #include <asm/kvm_mmu.h>
> > > > > > > >  
> > > > > > > >  /* Maximum phys_shift supported for any VM on this host */
> > > > > > > > @@ -146,6 +147,29 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> > > > > > > >  	/* Reset system registers */
> > > > > > > >  	kvm_reset_sys_regs(vcpu);
> > > > > > > >  
> > > > > > > > +	/*
> > > > > > > > +	 * Additional reset state handling that PSCI may have imposed on us.
> > > > > > > > +	 * Must be done after all the sys_reg reset.
> > > > > > > > +	 */
> > > > > > > > +	if (vcpu->arch.reset_state.reset) {
> > > > > > > > +		unsigned long target_pc = vcpu->arch.reset_state.pc;
> > > > > > > > +
> > > > > > > > +		/* Gracefully handle Thumb2 entry point */
> > > > > > > > +		if (vcpu_mode_is_32bit(vcpu) && (target_pc & 1)) {
> > > > > > > > +			target_pc &= ~1UL;
> > > > > > > > +			vcpu_set_thumb(vcpu);
> > > > > > > > +		}
> > > > > > > > +
> > > > > > > > +		/* Propagate caller endianness */
> > > > > > > > +		if (vcpu->arch.reset_state.be)
> > > > > > > > +			kvm_vcpu_set_be(vcpu);
> > > > > > > > +
> > > > > > > > +		*vcpu_pc(vcpu) = target_pc;
> > > > > > > > +		vcpu_set_reg(vcpu, 0, vcpu->arch.reset_state.r0);
> > > > > > > > +
> > > > > > > > +		vcpu->arch.reset_state.reset = false;
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > >  	/* Reset PMU */
> > > > > > > >  	kvm_pmu_vcpu_reset(vcpu);
> > > > > > > >  
> > > > > > > > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> > > > > > > > index 9e350fd34504..9c486fad3f9f 100644
> > > > > > > > --- a/virt/kvm/arm/arm.c
> > > > > > > > +++ b/virt/kvm/arm/arm.c
> > > > > > > > @@ -626,6 +626,13 @@ static void vcpu_req_sleep(struct kvm_vcpu *vcpu)
> > > > > > > >  		/* Awaken to handle a signal, request we sleep again later. */
> > > > > > > >  		kvm_make_request(KVM_REQ_SLEEP, vcpu);
> > > > > > > >  	}
> > > > > > > > +
> > > > > > > > +	/*
> > > > > > > > +	 * Make sure we will observe a potential reset request if we've
> > > > > > > > +	 * observed a change to the power state. Pairs with the smp_wmb() in
> > > > > > > > +	 * kvm_psci_vcpu_on().
> > > > > > > > +	 */
> > > > > > > > +	smp_rmb();
> > > > > > > 
> > > > > > > I don't believe this should be necessary, because...
> > > > > > > 
> > > > > > > >  }
> > > > > > > >  
> > > > > > > >  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
> > > > > > > > @@ -639,6 +646,9 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
> > > > > > > >  		if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
> > > > > > > >  			vcpu_req_sleep(vcpu);
> > > > > > > >  
> > > > > > > > +		if (kvm_check_request(KVM_REQ_VCPU_RESET, vcpu))
> > > > > > > > +			kvm_reset_vcpu(vcpu);
> > > > > > > 
> > > > > > > ... we do kvm_check_request here before using the reset data, and we do...
> > > > > > > 
> > > > > > > > +
> > > > > > > >  		/*
> > > > > > > >  		 * Clear IRQ_PENDING requests that were made to guarantee
> > > > > > > >  		 * that a VCPU sees new virtual interrupts.
> > > > > > > > diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
> > > > > > > > index 9b73d3ad918a..b9cff1d4b06d 100644
> > > > > > > > --- a/virt/kvm/arm/psci.c
> > > > > > > > +++ b/virt/kvm/arm/psci.c
> > > > > > > > @@ -104,12 +104,10 @@ static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu)
> > > > > > > >  
> > > > > > > >  static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> > > > > > > >  {
> > > > > > > > +	struct vcpu_reset_state *reset_state;
> > > > > > > >  	struct kvm *kvm = source_vcpu->kvm;
> > > > > > > >  	struct kvm_vcpu *vcpu = NULL;
> > > > > > > > -	struct swait_queue_head *wq;
> > > > > > > >  	unsigned long cpu_id;
> > > > > > > > -	unsigned long context_id;
> > > > > > > > -	phys_addr_t target_pc;
> > > > > > > >  
> > > > > > > >  	cpu_id = smccc_get_arg1(source_vcpu) & MPIDR_HWID_BITMASK;
> > > > > > > >  	if (vcpu_mode_is_32bit(source_vcpu))
> > > > > > > > @@ -130,32 +128,30 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> > > > > > > >  			return PSCI_RET_INVALID_PARAMS;
> > > > > > > >  	}
> > > > > > > >  
> > > > > > > > -	target_pc = smccc_get_arg2(source_vcpu);
> > > > > > > > -	context_id = smccc_get_arg3(source_vcpu);
> > > > > > > > +	reset_state = &vcpu->arch.reset_state;
> > > > > > > >  
> > > > > > > > -	kvm_reset_vcpu(vcpu);
> > > > > > > > -
> > > > > > > > -	/* Gracefully handle Thumb2 entry point */
> > > > > > > > -	if (vcpu_mode_is_32bit(vcpu) && (target_pc & 1)) {
> > > > > > > > -		target_pc &= ~((phys_addr_t) 1);
> > > > > > > > -		vcpu_set_thumb(vcpu);
> > > > > > > > -	}
> > > > > > > > +	reset_state->pc = smccc_get_arg2(source_vcpu);
> > > > > > > >  
> > > > > > > >  	/* Propagate caller endianness */
> > > > > > > > -	if (kvm_vcpu_is_be(source_vcpu))
> > > > > > > > -		kvm_vcpu_set_be(vcpu);
> > > > > > > > +	reset_state->be = kvm_vcpu_is_be(source_vcpu);
> > > > > > > >  
> > > > > > > > -	*vcpu_pc(vcpu) = target_pc;
> > > > > > > >  	/*
> > > > > > > >  	 * NOTE: We always update r0 (or x0) because for PSCI v0.1
> > > > > > > >  	 * the general puspose registers are undefined upon CPU_ON.
> > > > > > > >  	 */
> > > > > > > > -	smccc_set_retval(vcpu, context_id, 0, 0, 0);
> > > > > > > > -	vcpu->arch.power_off = false;
> > > > > > > > -	smp_mb();		/* Make sure the above is visible */
> > > > > > > > +	reset_state->r0 = smccc_get_arg3(source_vcpu);
> > > > > > > > +
> > > > > > > > +	reset_state->reset = true;
> > > > > > > > +	kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
> > > > > > > 
> > > > > > > ... this kvm_make_request() after writing the reset data. The
> > > > > > > kvm_make_request/kvm_check_request embeds the necessary barriers.
> > > > > > > 
> > > > > > > >  
> > > > > > > > -	wq = kvm_arch_vcpu_wq(vcpu);
> > > > > > > > -	swake_up_one(wq);
> > > > > > > > +	/*
> > > > > > > > +	 * Make sure the reset request is observed if the change to
> > > > > > > > +	 * power_state is observed.
> > > > > > > > +	 */
> > > > > > > > +	smp_wmb();
> > > > > > > > +
> > > > > > > > +	vcpu->arch.power_off = false;
> > > > > > > 
> > > > > > > Or you want to tie the reset data to the observability of the power_off
> > > > > > > state? Why not just tie it to the KVM_REQ_VCPU_RESET request?
> > > > > > > 
> > > > > > 
> > > > > > The barrier in kvm_make_request is *before* the write to vcpu->requests,
> > > > > > so doesn't enforce any ordering of stores occurring *after* that write.
> > > > > > 
> > > > > > We don't want to wake up the target VCPU thread, have it observe that it
> > > > > > is powered on, yet not observe the reset request (my comment above
> > > > > > inversed).
> > > > > > 
> > > > > > I wrote this Litmus test (which is probably so appalingly obvious to
> > > > > > memory ordering folks that it is unnecessary) to convince myself the
> > > > > > barrier is needed:
> > > > > > 
> > > > > > ----<8----
> > > > > > C vcpu-reset
> > > > > > 
> > > > > > {}
> > > > > > 
> > > > > > P0(int *power, int *reset)
> > > > > > {
> > > > > > 	smp_wmb(); // from kvm_make_request
> > > > > > 	WRITE_ONCE(*reset, 1); // kvm_psci_vcpu_on: kvm_make_request(KVM_REQ_RESET, vcpu);
> > > > > > 	smp_wmb();
> > > > > > 	WRITE_ONCE(*power, 1); // kvm_psci_vcpu_on: vcpu->arch.power_state = KVM_ARM_VCPU_ON;
> > > > > > }
> > > > > > 
> > > > > > P1(int *power, int *reset, spinlock_t *wqlock)
> > > > > > {
> > > > > > 	int r0;
> > > > > > 	int r1;
> > > > > > 
> > > > > > 	spin_lock(wqlock);
> > > > > > 	smp_mb(); // from prepare_to_wait -> set_current_state
> > > > > > 	spin_unlock(wqlock);
> > > > > > 	r0 = READ_ONCE(*power); // vcpu_req_sleep: if (vcpu_sleeping(vcpu))
> > > > > > 	smp_rmb();
> > > > > > 	r1 = READ_ONCE(*reset); // check_vcpu_requests: if (kvm_check_request(KVM_REQ_RESET, vcpu))
> > > > > > }
> > > > > > 
> > > > > > exists (1:r0=1 /\ 1:r1=0)
> > > > > > ----<8----
> > > > > > 
> > > > > > 
> > > > > > Hope this helps,
> > > > > 
> > > > > It certainly does, mostly because I didn't review closely enough and
> > > > > thought the pair we were interested in was vcpu_reset_state and the
> > > > > RESET vcpu request, which the vcpu request API handles. Now it's
> > > > > clear we're worried about the pair power_off and the RESET vcpu
> > > > > request. And we've reversed the usual "if we observe a request, then
> > > > > we can observe its accompanying data" to "if we observe a change in
> > > > > some data (power_off), then we need to also observe a request". I
> > > > > agree the barriers are necessary to ensure that in the direct
> > > > > sequence, but now I'm wondering if we need to care about the
> > > > > direct sequence.
> > > > > 
> > > > > If we remove the barriers then the vcpu could have these possible
> > > > > sequences
> > > > > 
> > > > > 1)
> > > > >   - wake up
> > > > >   - observe power_off is false
> > > > >   - observe the RESET vcpu request - do the reset
> > > > >   - enter guest
> > > > > 
> > > > > That's the one this patch ensures, so it's definitely correct.
> > > > > 
> > > > > 2)
> > > > >   - wake up
> > > > >   - the change to power_off not observed - the vcpu makes the
> > > > >     SLEEP vcpu request on itself
> > > > >   - observe the RESET vcpu request - do the reset
> > > > >   - observe the SLEEP vcpu request (with or without the need for
> > > > >     an IPI), attempt to sleep again, but now observe power_off
> > > > >     is false
> > > > >   - enter guest
> > > > > 
> > > > > I think that should be correct as well, even if less efficient.
> > > > > 
> > > > > 3)
> > > > >   - wake up
> > > > >   - observe power_off is false
> > > > >   - don't observe the RESET request yet, get closer to entering
> > > > >     the guest
> > > > >   - observe the RESET request a bit later (with or without the
> > > > >     need for an IPI) - do the reset
> > > > >   - enter guest
> > > > > 
> > > > > This is the same as (1), but we rely on the kvm_request_pending
> > > > > stuff to make sure the vcpu request is seen before entering the
> > > > > guest.
> > > > > 
> > > > > 4)
> > > > >   - wake up
> > > > >   - the change to power_off not observed - the vcpu makes the
> > > > >     SLEEP vcpu request on itself
> > > > >   - don't observe the RESET request yet, get closer to entering
> > > > >     the guest
> > > > >   - observe the SLEEP vcpu request (with or without the need for
> > > > >     an IPI), attempt to sleep again, but now observe power_off
> > > > >     is false
> > > > >   - observe the RESET request a bit later (with or without the
> > > > >     need for an IPI) - do the reset
> > > > >   - enter guest
> > > > > 
> > > > > The least efficient one, but should still be correct.
> > > > > 
> > > > > If you agree with this, then we get to remove the barriers,
> > > > > simplifying things, and also we don't introduce the ordering
> > > > > dependency in check_vcpu_requests(), where the vcpu_req_sleep()
> > > > > calls must now come before checking the RESET request.
> > > > > 
> > > > 
> > > > I'm sorry, I'm not following what you want to achieve here?
> > > > 
> > > > Somehow delivering a series of possible interleaved executions doesn't
> > > > exactly convey anything meaningful to me.
> > > >
> > > 
> > > This patch introduces an unconventional use of vcpu requests, which is
> > > to ensure a change to vcpu->requests is observed if a change to
> > > arch.power_off is observed. That unconventional use requires additional
> > > barriers and an ordering requirement in check_vcpu_requests(), i.e.
> > > anything that calls vcpu_req_sleep() must come before the check for
> > > the RESET vcpu request. I'm stating, and attempting to prove with the
> > > list of possible executions, that those barriers and ordering requirements
> > > are not necessary.
> > > 
> > > Assuming the objective is to ensure a vcpu reset is done before entering
> > > guest mode after a vcpu power on, then the RESET vcpu request is
> > > sufficient by itself. vcpu requests are guaranteed to be seen by a vcpu
> > > before entering guest mode.
> > > 
> > > Additionally, I'm pretty sure that even leaving this patch as it is,
> > > all the above execution scenarios are possible except (3), depending on
> > > the timing of a signal delivery.
> > > 
> > > Is there a problem with (3) that I'm missing? Or am I missing something
> > > else?
> > 
> > There's not a problem with (3), but the question is flawed; you've shown
> > me a benign execution sequence and somehow you're arguing that the
> > execution sequence is always correct?
> 
> Since you agree the execution sequence is fine, then I can't see why we
> would want these additional barriers. Did I miss the execution sequence
> you're concerned with?
> 

Yes, clearly.  See my litmus test.

The fact that you show a benign execution doesn't mean that all
executions are benign.

I claim that there is an execution where the reset request is not
observed but the power state change is observed when the VCPU wakes up.

Show me why that is not the case.  As far as I can tell, you haven't
done that, you've just showed me that there are benign executions.

[...]

> > 
> > Let's try this:  If you have a better way of implementing this, how
> > about you write a patch?
> 
> It would just be this patch minus the unnecessary barriers. I can send it
> if you like, but I wouldn't want to change the authorship for such a small
> change.
> 

Try removing the barriers in the litmus test and run the test, it will
fail.

    Christoffer

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] arm/arm64: KVM: Allow a VCPU to fully reset itself

[Marc Zyngier]

On 31/01/2019 12:57, Andrew Jones wrote:
> On Thu, Jan 31, 2019 at 12:51:56PM +0100, Christoffer Dall wrote:

[...]

>> I don't think there's anything very unconventional here.
> 
> Normally if a thread observes a change to vcpu->requests, then we ensure a
> change to some accompanying data is also observable. We're reversing that
> here, which adds a need for additional barriers and a strict request
> checking order.
> 
>>
>> Let's try this:  If you have a better way of implementing this, how
>> about you write a patch?
> 
> It would just be this patch minus the unnecessary barriers. I can send it
> if you like, but I wouldn't want to change the authorship for such a small
> change.

Having these barriers makes it explicit (at least to me) what data we
expect to be visible in other threads and in which order. You keep
saying that order doesn't matter and we disagree on this. Yes, you've
listed cases where we can survive things coming in out of order, but
that's not a proof that we don't need them.

So at the end of the day, and unless you can prove that the barriers are
not necessary by providing the same form of validation tool, I'm
inclined to go with the verified approach.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] arm/arm64: KVM: Allow a VCPU to fully reset itself

[Andrew Jones]

On Thu, Jan 31, 2019 at 02:52:11PM +0000, Marc Zyngier wrote:
> On 31/01/2019 12:57, Andrew Jones wrote:
> > On Thu, Jan 31, 2019 at 12:51:56PM +0100, Christoffer Dall wrote:
> 
> [...]
> 
> >> I don't think there's anything very unconventional here.
> > 
> > Normally if a thread observes a change to vcpu->requests, then we ensure a
> > change to some accompanying data is also observable. We're reversing that
> > here, which adds a need for additional barriers and a strict request
> > checking order.
> > 
> >>
> >> Let's try this:  If you have a better way of implementing this, how
> >> about you write a patch?
> > 
> > It would just be this patch minus the unnecessary barriers. I can send it
> > if you like, but I wouldn't want to change the authorship for such a small
> > change.
> 
> Having these barriers makes it explicit (at least to me) what data we
> expect to be visible in other threads and in which order. You keep
> saying that order doesn't matter and we disagree on this. Yes, you've
> listed cases where we can survive things coming in out of order, but
> that's not a proof that we don't need them.
> 
> So at the end of the day, and unless you can prove that the barriers are
> not necessary by providing the same form of validation tool, I'm
> inclined to go with the verified approach.

I don't know how to compile and run the litmus test, but I'd be happy to
try if given some pointers. If I did know how, I would add vcpu->mode to
the P1 inputs and some additional lines that look similar to what's in
"Ensuring Requests Are Seen" of Documentation/virtual/kvm/vcpu-requests.rst
Even without the litmus test please allow me to try again to describe why
I think the barriers may be removed.

Any vcpu we're attempting to power on must be on its way to sleep with a
SLEEP request, or already be sleeping. This means that it's outside guest
mode, or will be shortly. If the vcpu observes power_off=false in
vcpu_req_sleep(), whether it was awaken or never even got to sleep, we
know that observation is taking place with vcpu->mode != IN_GUEST_MODE.

We now no longer need to be concerned with the relationship between
power_off and the RESET vcpu request. Now we just need to be certain
that if another thread made a RESET vcpu request at some point since
the powering off of the vcpu (the requesting for it to sleep), then
there's no way to miss the reset request before the vcpu reenters guest
mode. And of that we are certain.

Thanks,
drew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] arm/arm64: KVM: Allow a VCPU to fully reset itself

[Christoffer Dall]

On Thu, Jan 31, 2019 at 06:06:09PM +0100, Andrew Jones wrote:
> On Thu, Jan 31, 2019 at 02:52:11PM +0000, Marc Zyngier wrote:
> > On 31/01/2019 12:57, Andrew Jones wrote:
> > > On Thu, Jan 31, 2019 at 12:51:56PM +0100, Christoffer Dall wrote:
> > 
> > [...]
> > 
> > >> I don't think there's anything very unconventional here.
> > > 
> > > Normally if a thread observes a change to vcpu->requests, then we ensure a
> > > change to some accompanying data is also observable. We're reversing that
> > > here, which adds a need for additional barriers and a strict request
> > > checking order.
> > > 
> > >>
> > >> Let's try this:  If you have a better way of implementing this, how
> > >> about you write a patch?
> > > 
> > > It would just be this patch minus the unnecessary barriers. I can send it
> > > if you like, but I wouldn't want to change the authorship for such a small
> > > change.
> > 
> > Having these barriers makes it explicit (at least to me) what data we
> > expect to be visible in other threads and in which order. You keep
> > saying that order doesn't matter and we disagree on this. Yes, you've
> > listed cases where we can survive things coming in out of order, but
> > that's not a proof that we don't need them.
> > 
> > So at the end of the day, and unless you can prove that the barriers are
> > not necessary by providing the same form of validation tool, I'm
> > inclined to go with the verified approach.
> 
> I don't know how to compile and run the litmus test, but I'd be happy to
> try if given some pointers.

You can look in tools/memory-model/README as a start.


> If I did know how, I would add vcpu->mode to
> the P1 inputs and some additional lines that look similar to what's in
> "Ensuring Requests Are Seen" of Documentation/virtual/kvm/vcpu-requests.rst
> Even without the litmus test please allow me to try again to describe why
> I think the barriers may be removed.
> 
> Any vcpu we're attempting to power on must be on its way to sleep with a
> SLEEP request, or already be sleeping. This means that it's outside guest
> mode, or will be shortly. If the vcpu observes power_off=false in
> vcpu_req_sleep(), whether it was awaken or never even got to sleep, we
> know that observation is taking place with vcpu->mode != IN_GUEST_MODE.
> 
> We now no longer need to be concerned with the relationship between
> power_off and the RESET vcpu request. 

I disagree.  That argument requires more explanation.

If you set power_off = false before posting the reset
request, then if the VCPU thread is awoken (for any reason) it can run
the VCPU without observing the reset request and that's the problem.

If you are making assumptions about only being woken up as a result of a
reset request, or the interaction with the pause flag, or setting the
sleep request to prevent the guest from executing again, that is a more
complex argument (which you haven't made yet!) and I add that it's a
brittle construction.

What we have here are three pieces of state:

  reset_state->reset
  vcpu->requests
  vcpu->arch.power_state

They must be written to, and the writes must be observed, in that
particular order without any additional assumptions.

You keep arguing that you can enforce an ordering between these three
states with a single barrier which is clearly not possible.

So this boils down to you making additional assumptions (see above,
brittle) without explaining what they are.  I suspect you want this to
fit in your mental model of how vcpu requests solve the world, otherwise
I'm not sure what your concern with this patch, which we all agree is
correct, really is.


Thanks,

    Christoffer

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] arm/arm64: KVM: Allow a VCPU to fully reset itself

[Andrew Jones]

On Fri, Feb 01, 2019 at 08:58:34AM +0100, Christoffer Dall wrote:
> On Thu, Jan 31, 2019 at 06:06:09PM +0100, Andrew Jones wrote:
> > On Thu, Jan 31, 2019 at 02:52:11PM +0000, Marc Zyngier wrote:
> > > On 31/01/2019 12:57, Andrew Jones wrote:
> > > > On Thu, Jan 31, 2019 at 12:51:56PM +0100, Christoffer Dall wrote:
> > > 
> > > [...]
> > > 
> > > >> I don't think there's anything very unconventional here.
> > > > 
> > > > Normally if a thread observes a change to vcpu->requests, then we ensure a
> > > > change to some accompanying data is also observable. We're reversing that
> > > > here, which adds a need for additional barriers and a strict request
> > > > checking order.
> > > > 
> > > >>
> > > >> Let's try this:  If you have a better way of implementing this, how
> > > >> about you write a patch?
> > > > 
> > > > It would just be this patch minus the unnecessary barriers. I can send it
> > > > if you like, but I wouldn't want to change the authorship for such a small
> > > > change.
> > > 
> > > Having these barriers makes it explicit (at least to me) what data we
> > > expect to be visible in other threads and in which order. You keep
> > > saying that order doesn't matter and we disagree on this. Yes, you've
> > > listed cases where we can survive things coming in out of order, but
> > > that's not a proof that we don't need them.
> > > 
> > > So at the end of the day, and unless you can prove that the barriers are
> > > not necessary by providing the same form of validation tool, I'm
> > > inclined to go with the verified approach.
> > 
> > I don't know how to compile and run the litmus test, but I'd be happy to
> > try if given some pointers.
> 
> You can look in tools/memory-model/README as a start.

Thanks. Neat tool.

> 
> 
> > If I did know how, I would add vcpu->mode to
> > the P1 inputs and some additional lines that look similar to what's in
> > "Ensuring Requests Are Seen" of Documentation/virtual/kvm/vcpu-requests.rst
> > Even without the litmus test please allow me to try again to describe why
> > I think the barriers may be removed.
> > 
> > Any vcpu we're attempting to power on must be on its way to sleep with a
> > SLEEP request, or already be sleeping. This means that it's outside guest
> > mode, or will be shortly. If the vcpu observes power_off=false in
> > vcpu_req_sleep(), whether it was awaken or never even got to sleep, we
> > know that observation is taking place with vcpu->mode != IN_GUEST_MODE.
> > 
> > We now no longer need to be concerned with the relationship between
> > power_off and the RESET vcpu request. 
> 
> I disagree.  That argument requires more explanation.
> 
> If you set power_off = false before posting the reset
> request, then if the VCPU thread is awoken (for any reason) it can run
> the VCPU without observing the reset request and that's the problem.
> 
> If you are making assumptions about only being woken up as a result of a
> reset request, or the interaction with the pause flag, or setting the
> sleep request to prevent the guest from executing again, that is a more
> complex argument (which you haven't made yet!) and I add that it's a
> brittle construction.

Yes, I was attempting to integrate more of the expected state of the
is / will be sleeping vcpu into the analysis. I was hoping that it was
provable to make stronger statements about the use of vcpu requests.
I failed to do so though, both with logical arguments and I couldn't
come up with a way to model outside-guest mode with the litmus test.

> 
> What we have here are three pieces of state:
> 
>   reset_state->reset
>   vcpu->requests
>   vcpu->arch.power_state
> 
> They must be written to, and the writes must be observed, in that
> particular order without any additional assumptions.
> 
> You keep arguing that you can enforce an ordering between these three
> states with a single barrier which is clearly not possible.

There's also the mode state and barriers in place that ensure the order
of observation for that and requests, but as I said I couldn't model it
well enough to prove anything.

> 
> So this boils down to you making additional assumptions (see above,
> brittle) without explaining what they are.  I suspect you want this to
> fit in your mental model of how vcpu requests solve the world, otherwise
> I'm not sure what your concern with this patch, which we all agree is
> correct, really is.

Well, I don't expect the vcpu requests to solve the world (if only!),
but I was hoping that they could be used with a simpler pattern. I.e.
on the sending side the vcpu request is fired off (probably with a kick
too) and then forgotten. On the receiving side path only a check and
handler need to be added, with no concern for the order of the handlers
nor of other data that we cannot observe without also observing the
request. I surrender though, because I do agree there's nothing wrong
with the additional barriers in this patch, only that it no longer fits
my opinion of the simplest pattern.

Anyway, thanks for your patience and sorry for the noise.

drew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] arm/arm64: KVM: Allow a VCPU to fully reset itself

[Andrew Jones]

On Fri, Jan 25, 2019 at 10:46:53AM +0100, Christoffer Dall wrote:
> From: Marc Zyngier <marc.zyngier@arm.com>
> 
> The current kvm_psci_vcpu_on implementation will directly try to
> manipulate the state of the VCPU to reset it.  However, since this is
> not done on the thread that runs the VCPU, we can end up in a strangely
> corrupted state when the source and target VCPUs are running at the same
> time.
> 
> Fix this by factoring out all reset logic from the PSCI implementation
> and forwarding the required information along with a request to the
> target VCPU.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> ---
>  arch/arm/include/asm/kvm_host.h   | 10 +++++++++
>  arch/arm/kvm/reset.c              | 24 +++++++++++++++++++++
>  arch/arm64/include/asm/kvm_host.h | 11 ++++++++++
>  arch/arm64/kvm/reset.c            | 24 +++++++++++++++++++++
>  virt/kvm/arm/arm.c                | 10 +++++++++
>  virt/kvm/arm/psci.c               | 36 ++++++++++++++-----------------
>  6 files changed, 95 insertions(+), 20 deletions(-)
>

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/5] KVM: arm/arm64: Reset the VCPU without preemption and vcpu state loaded

[Andrew Jones]

On Fri, Jan 25, 2019 at 10:46:52AM +0100, Christoffer Dall wrote:
> Resetting the VCPU state modifies the system register state in memory,
> but this may interact with vcpu_load/vcpu_put if running with preemption
> disabled, which in turn may lead to corrupted system register state.
> 
> Address this by disabling preemption and doing put/load if required
> around the reset logic.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/kvm/reset.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
>

I believe this should come after the next patch, but anyway

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/5] KVM: arm/arm64: Reset the VCPU without preemption and vcpu state loaded

[Dave Martin]

On Fri, Jan 25, 2019 at 10:46:52AM +0100, Christoffer Dall wrote:
> Resetting the VCPU state modifies the system register state in memory,
> but this may interact with vcpu_load/vcpu_put if running with preemption
> disabled, which in turn may lead to corrupted system register state.

Should this be "enabled"?

Too late now, but I want to make sure I understand this right for
patches that will go on top.

> Address this by disabling preemption and doing put/load if required
> around the reset logic.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/kvm/reset.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index b72a3dd56204..f21a2a575939 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -105,16 +105,33 @@ int kvm_arch_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>   * This function finds the right table above and sets the registers on
>   * the virtual CPU struct to their architecturally defined reset
>   * values.
> + *
> + * Note: This function can be called from two paths: The KVM_ARM_VCPU_INIT
> + * ioctl or as part of handling a request issued by another VCPU in the PSCI
> + * handling code.  In the first case, the VCPU will not be loaded, and in the
> + * second case the VCPU will be loaded.  Because this function operates purely
> + * on the memory-backed valus of system registers, we want to do a full put if
> + * we were loaded (handling a request) and load the values back at the end of
> + * the function.  Otherwise we leave the state alone.  In both cases, we
> + * disable preemption around the vcpu reset as we would otherwise race with
> + * preempt notifiers which also call put/load.
>   */
>  int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>  {
>  	const struct kvm_regs *cpu_reset;
> +	int ret = -EINVAL;
> +	bool loaded;
> +
> +	preempt_disable();
> +	loaded = (vcpu->cpu != -1);
> +	if (loaded)
> +		kvm_arch_vcpu_put(vcpu);
>  
>  	switch (vcpu->arch.target) {
>  	default:
>  		if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features)) {
>  			if (!cpu_has_32bit_el1())
> -				return -EINVAL;
> +				goto out;
>  			cpu_reset = &default_regs_reset32;
>  		} else {
>  			cpu_reset = &default_regs_reset;
> @@ -137,7 +154,12 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>  		vcpu->arch.workaround_flags |= VCPU_WORKAROUND_2_FLAG;
>  
>  	/* Reset timer */
> -	return kvm_timer_vcpu_reset(vcpu);
> +	ret = kvm_timer_vcpu_reset(vcpu);
> +out:
> +	if (loaded)
> +		kvm_arch_vcpu_load(vcpu, smp_processor_id());
> +	preempt_enable();
> +	return ret;
>  }
>  
>  void kvm_set_ipa_limit(void)

I was really confused by this: as far as I can see, we don't really need
to disable preemption here once kvm_arch_vcpu_put() is complete -- at
least not for the purpose of avoiding corruption of the reg state.  But
we _do_ need to disable the preempt notifier so that it doesn't fire
before we are ready.

It actually seems a bit surprising for a powered-off CPU to sit with the
VM regs live and preempt notifier armed, when the vcpu thread is
heading to interruptible sleep anyway until someone turns it on.
Perhaps an alternative approach would be to nobble the preempt notifier
and stick an explicit vcpu_put()...vcpu_load() around the
swait_event_interruptible_exclusive() call in vcpu_req_sleep().  This
is not fast path.


Any, with the code as-is, it looks like the SVE regs resetting should
go in the preempt_disable() region, after the kvm_arch_vcpu_put() call.

Does it sound like I've understood that right?

Cheers
---Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/5] KVM: arm/arm64: Reset the VCPU without preemption and vcpu state loaded

[Marc Zyngier]

On Wed, 20 Feb 2019 19:14:53 +0000
Dave Martin <Dave.Martin@arm.com> wrote:

> On Fri, Jan 25, 2019 at 10:46:52AM +0100, Christoffer Dall wrote:
> > Resetting the VCPU state modifies the system register state in memory,
> > but this may interact with vcpu_load/vcpu_put if running with preemption
> > disabled, which in turn may lead to corrupted system register state.  
> 
> Should this be "enabled"?

Yup. Never mind. The patches are firmly in mainline now.

> 
> Too late now, but I want to make sure I understand this right for
> patches that will go on top.
> 
> > Address this by disabling preemption and doing put/load if required
> > around the reset logic.
> > 
> > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > ---
> >  arch/arm64/kvm/reset.c | 26 ++++++++++++++++++++++++--
> >  1 file changed, 24 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> > index b72a3dd56204..f21a2a575939 100644
> > --- a/arch/arm64/kvm/reset.c
> > +++ b/arch/arm64/kvm/reset.c
> > @@ -105,16 +105,33 @@ int kvm_arch_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> >   * This function finds the right table above and sets the registers on
> >   * the virtual CPU struct to their architecturally defined reset
> >   * values.
> > + *
> > + * Note: This function can be called from two paths: The KVM_ARM_VCPU_INIT
> > + * ioctl or as part of handling a request issued by another VCPU in the PSCI
> > + * handling code.  In the first case, the VCPU will not be loaded, and in the
> > + * second case the VCPU will be loaded.  Because this function operates purely
> > + * on the memory-backed valus of system registers, we want to do a full put if
> > + * we were loaded (handling a request) and load the values back at the end of
> > + * the function.  Otherwise we leave the state alone.  In both cases, we
> > + * disable preemption around the vcpu reset as we would otherwise race with
> > + * preempt notifiers which also call put/load.
> >   */
> >  int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> >  {
> >  	const struct kvm_regs *cpu_reset;
> > +	int ret = -EINVAL;
> > +	bool loaded;
> > +
> > +	preempt_disable();
> > +	loaded = (vcpu->cpu != -1);
> > +	if (loaded)
> > +		kvm_arch_vcpu_put(vcpu);
> >  
> >  	switch (vcpu->arch.target) {
> >  	default:
> >  		if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features)) {
> >  			if (!cpu_has_32bit_el1())
> > -				return -EINVAL;
> > +				goto out;
> >  			cpu_reset = &default_regs_reset32;
> >  		} else {
> >  			cpu_reset = &default_regs_reset;
> > @@ -137,7 +154,12 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> >  		vcpu->arch.workaround_flags |= VCPU_WORKAROUND_2_FLAG;
> >  
> >  	/* Reset timer */
> > -	return kvm_timer_vcpu_reset(vcpu);
> > +	ret = kvm_timer_vcpu_reset(vcpu);
> > +out:
> > +	if (loaded)
> > +		kvm_arch_vcpu_load(vcpu, smp_processor_id());
> > +	preempt_enable();
> > +	return ret;
> >  }
> >  
> >  void kvm_set_ipa_limit(void)  
> 
> I was really confused by this: as far as I can see, we don't really need
> to disable preemption here once kvm_arch_vcpu_put() is complete -- at
> least not for the purpose of avoiding corruption of the reg state.  But
> we _do_ need to disable the preempt notifier so that it doesn't fire
> before we are ready.

Just reading this patch alone won't help. You need to read it in
conjunction with the following patch, which resets the vcpu from a
preemptible section.

> It actually seems a bit surprising for a powered-off CPU to sit with the
> VM regs live and preempt notifier armed, when the vcpu thread is
> heading to interruptible sleep anyway until someone turns it on.

All it takes is a signal for that vcpu to wake up, power-off or not.

> Perhaps an alternative approach would be to nobble the preempt notifier
> and stick an explicit vcpu_put()...vcpu_load() around the
> swait_event_interruptible_exclusive() call in vcpu_req_sleep().  This
> is not fast path.

What does it buy us? The problem we're solving here is a powered-off,
spuriously woken-up vcpu racing against the reset performed from
another vcpu. I don't see what adding more put/load would solve.

> Any, with the code as-is, it looks like the SVE regs resetting should
> go in the preempt_disable() region, after the kvm_arch_vcpu_put() call.

All resets must go there. This is the only safe location.

> Does it sound like I've understood that right?

I'm not sure. Your analysis of what we're trying to achieve with this
series seems a bit off. Or I'm completely misunderstanding what you're
suggesting, which is the most likely explanation.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/5] KVM: arm/arm64: Reset the VCPU without preemption and vcpu state loaded

[Christoffer Dall]

On Wed, Feb 20, 2019 at 07:14:53PM +0000, Dave Martin wrote:
> On Fri, Jan 25, 2019 at 10:46:52AM +0100, Christoffer Dall wrote:
> > Resetting the VCPU state modifies the system register state in memory,
> > but this may interact with vcpu_load/vcpu_put if running with preemption
> > disabled, which in turn may lead to corrupted system register state.
> 
> Should this be "enabled"?
> 
> Too late now, but I want to make sure I understand this right for
> patches that will go on top.
> 
> > Address this by disabling preemption and doing put/load if required
> > around the reset logic.
> > 
> > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > ---
> >  arch/arm64/kvm/reset.c | 26 ++++++++++++++++++++++++--
> >  1 file changed, 24 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> > index b72a3dd56204..f21a2a575939 100644
> > --- a/arch/arm64/kvm/reset.c
> > +++ b/arch/arm64/kvm/reset.c
> > @@ -105,16 +105,33 @@ int kvm_arch_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> >   * This function finds the right table above and sets the registers on
> >   * the virtual CPU struct to their architecturally defined reset
> >   * values.
> > + *
> > + * Note: This function can be called from two paths: The KVM_ARM_VCPU_INIT
> > + * ioctl or as part of handling a request issued by another VCPU in the PSCI
> > + * handling code.  In the first case, the VCPU will not be loaded, and in the
> > + * second case the VCPU will be loaded.  Because this function operates purely
> > + * on the memory-backed valus of system registers, we want to do a full put if
> > + * we were loaded (handling a request) and load the values back at the end of
> > + * the function.  Otherwise we leave the state alone.  In both cases, we
> > + * disable preemption around the vcpu reset as we would otherwise race with
> > + * preempt notifiers which also call put/load.
> >   */
> >  int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> >  {
> >  	const struct kvm_regs *cpu_reset;
> > +	int ret = -EINVAL;
> > +	bool loaded;
> > +
> > +	preempt_disable();
> > +	loaded = (vcpu->cpu != -1);
> > +	if (loaded)
> > +		kvm_arch_vcpu_put(vcpu);
> >  
> >  	switch (vcpu->arch.target) {
> >  	default:
> >  		if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features)) {
> >  			if (!cpu_has_32bit_el1())
> > -				return -EINVAL;
> > +				goto out;
> >  			cpu_reset = &default_regs_reset32;
> >  		} else {
> >  			cpu_reset = &default_regs_reset;
> > @@ -137,7 +154,12 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> >  		vcpu->arch.workaround_flags |= VCPU_WORKAROUND_2_FLAG;
> >  
> >  	/* Reset timer */
> > -	return kvm_timer_vcpu_reset(vcpu);
> > +	ret = kvm_timer_vcpu_reset(vcpu);
> > +out:
> > +	if (loaded)
> > +		kvm_arch_vcpu_load(vcpu, smp_processor_id());
> > +	preempt_enable();
> > +	return ret;
> >  }
> >  
> >  void kvm_set_ipa_limit(void)
> 
> I was really confused by this: as far as I can see, we don't really need
> to disable preemption here once kvm_arch_vcpu_put() is complete -- at
> least not for the purpose of avoiding corruption of the reg state.  But
> we _do_ need to disable the preempt notifier so that it doesn't fire
> before we are ready.
> 
> It actually seems a bit surprising for a powered-off CPU to sit with the
> VM regs live and preempt notifier armed, when the vcpu thread is
> heading to interruptible sleep anyway until someone turns it on.
> Perhaps an alternative approach would be to nobble the preempt notifier
> and stick an explicit vcpu_put()...vcpu_load() around the
> swait_event_interruptible_exclusive() call in vcpu_req_sleep().  This
> is not fast path.
> 
> 

I think you've understood the problem correctly, and the thing here is
that we (sort-of) "abuse" disabling preemption as a way to disable
preempt notifiers, which I don't think we have.  So we could add that,
and do something like:

  preempt_disable();
  vcpu_put(vcpu);
  disable_preempt_notifiers(vcpu);
  preempt_disable();
  funky_stuff();
  vcpu_load();
  preempt_enable();

But I think that's additional complexity to get a slightly shorter
section with disabled preemption.

We could also re-architect a lot of the vcpu_load/vpcu_put functionality
more drastically, but that is difficult and requires understanding of
how the other architectures work, so at the end of the day we just use
this pattern in multiple places, which is:

  preempt_disable();
  vcpu_put();
  modify_vcpu_state_in_memory();
  vcpu_load();
  preempt_enable();

Does that help?


Thanks,

    Christoffer

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/5] KVM: arm/arm64: Reset the VCPU without preemption and vcpu state loaded

[Dave Martin]

On Wed, Feb 20, 2019 at 07:41:43PM +0000, Marc Zyngier wrote:
> On Wed, 20 Feb 2019 19:14:53 +0000
> Dave Martin <Dave.Martin@arm.com> wrote:
> 
> > On Fri, Jan 25, 2019 at 10:46:52AM +0100, Christoffer Dall wrote:
> > > Resetting the VCPU state modifies the system register state in memory,
> > > but this may interact with vcpu_load/vcpu_put if running with preemption
> > > disabled, which in turn may lead to corrupted system register state.  
> > 
> > Should this be "enabled"?
> 
> Yup. Never mind. The patches are firmly in mainline now.

Understood.  Just wanted to check my intuition.

> > 
> > Too late now, but I want to make sure I understand this right for
> > patches that will go on top.
> > 
> > > Address this by disabling preemption and doing put/load if required
> > > around the reset logic.
> > > 
> > > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > > ---
> > >  arch/arm64/kvm/reset.c | 26 ++++++++++++++++++++++++--
> > >  1 file changed, 24 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> > > index b72a3dd56204..f21a2a575939 100644
> > > --- a/arch/arm64/kvm/reset.c
> > > +++ b/arch/arm64/kvm/reset.c
> > > @@ -105,16 +105,33 @@ int kvm_arch_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> > >   * This function finds the right table above and sets the registers on
> > >   * the virtual CPU struct to their architecturally defined reset
> > >   * values.
> > > + *
> > > + * Note: This function can be called from two paths: The KVM_ARM_VCPU_INIT
> > > + * ioctl or as part of handling a request issued by another VCPU in the PSCI
> > > + * handling code.  In the first case, the VCPU will not be loaded, and in the
> > > + * second case the VCPU will be loaded.  Because this function operates purely
> > > + * on the memory-backed valus of system registers, we want to do a full put if
> > > + * we were loaded (handling a request) and load the values back at the end of
> > > + * the function.  Otherwise we leave the state alone.  In both cases, we
> > > + * disable preemption around the vcpu reset as we would otherwise race with
> > > + * preempt notifiers which also call put/load.
> > >   */
> > >  int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> > >  {
> > >  	const struct kvm_regs *cpu_reset;
> > > +	int ret = -EINVAL;
> > > +	bool loaded;
> > > +
> > > +	preempt_disable();
> > > +	loaded = (vcpu->cpu != -1);
> > > +	if (loaded)
> > > +		kvm_arch_vcpu_put(vcpu);
> > >  
> > >  	switch (vcpu->arch.target) {
> > >  	default:
> > >  		if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features)) {
> > >  			if (!cpu_has_32bit_el1())
> > > -				return -EINVAL;
> > > +				goto out;
> > >  			cpu_reset = &default_regs_reset32;
> > >  		} else {
> > >  			cpu_reset = &default_regs_reset;
> > > @@ -137,7 +154,12 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> > >  		vcpu->arch.workaround_flags |= VCPU_WORKAROUND_2_FLAG;
> > >  
> > >  	/* Reset timer */
> > > -	return kvm_timer_vcpu_reset(vcpu);
> > > +	ret = kvm_timer_vcpu_reset(vcpu);
> > > +out:
> > > +	if (loaded)
> > > +		kvm_arch_vcpu_load(vcpu, smp_processor_id());
> > > +	preempt_enable();
> > > +	return ret;
> > >  }
> > >  
> > >  void kvm_set_ipa_limit(void)  
> > 
> > I was really confused by this: as far as I can see, we don't really need
> > to disable preemption here once kvm_arch_vcpu_put() is complete -- at
> > least not for the purpose of avoiding corruption of the reg state.  But
> > we _do_ need to disable the preempt notifier so that it doesn't fire
> > before we are ready.
> 
> Just reading this patch alone won't help. You need to read it in
> conjunction with the following patch, which resets the vcpu from a
> preemptible section.
> 
> > It actually seems a bit surprising for a powered-off CPU to sit with the
> > VM regs live and preempt notifier armed, when the vcpu thread is
> > heading to interruptible sleep anyway until someone turns it on.
> 
> All it takes is a signal for that vcpu to wake up, power-off or not.

Sure, but the vcpu was scheduled out in the meantim, so we may end up
having to do all the loading twice:  not a problem, because this is a
rare, slow path.  But I wanted to make sure I wasn't more confused than
necessary.

> > Perhaps an alternative approach would be to nobble the preempt notifier
> > and stick an explicit vcpu_put()...vcpu_load() around the
> > swait_event_interruptible_exclusive() call in vcpu_req_sleep().  This
> > is not fast path.
> 
> What does it buy us? The problem we're solving here is a powered-off,
> spuriously woken-up vcpu racing against the reset performed from
> another vcpu. I don't see what adding more put/load would solve.

Not a lot.  Changes in this area would allow some code to move outside
preempt_disable(), but only at the expense of extra cost elsewhere...

> > Any, with the code as-is, it looks like the SVE regs resetting should
> > go in the preempt_disable() region, after the kvm_arch_vcpu_put() call.
> 
> All resets must go there. This is the only safe location.

OK, that's what I wanted to be sure of.

> > Does it sound like I've understood that right?
> 
> I'm not sure. Your analysis of what we're trying to achieve with this
> series seems a bit off. Or I'm completely misunderstanding what you're
> suggesting, which is the most likely explanation.

I'm not trying to achieve anything except rebase the SVE series
correctly on top of this patch (which I've now done -- thanks for
your confirmation above of where the extra resetting should go).

My suggestions above were only intended as thought experiments to check
my understanding of what's going on, *not* proposals for changing the
code.  I guess I didn't make that very clear.

[...]

Cheers
---Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/5] KVM: arm/arm64: Reset the VCPU without preemption and vcpu state loaded

[Dave Martin]

On Tue, Feb 26, 2019 at 01:34:49PM +0100, Christoffer Dall wrote:
> On Wed, Feb 20, 2019 at 07:14:53PM +0000, Dave Martin wrote:
> > On Fri, Jan 25, 2019 at 10:46:52AM +0100, Christoffer Dall wrote:
> > > Resetting the VCPU state modifies the system register state in memory,
> > > but this may interact with vcpu_load/vcpu_put if running with preemption
> > > disabled, which in turn may lead to corrupted system register state.
> > 
> > Should this be "enabled"?
> > 
> > Too late now, but I want to make sure I understand this right for
> > patches that will go on top.
> > 
> > > Address this by disabling preemption and doing put/load if required
> > > around the reset logic.
> > > 
> > > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > > ---
> > >  arch/arm64/kvm/reset.c | 26 ++++++++++++++++++++++++--
> > >  1 file changed, 24 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> > > index b72a3dd56204..f21a2a575939 100644
> > > --- a/arch/arm64/kvm/reset.c
> > > +++ b/arch/arm64/kvm/reset.c
> > > @@ -105,16 +105,33 @@ int kvm_arch_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> > >   * This function finds the right table above and sets the registers on
> > >   * the virtual CPU struct to their architecturally defined reset
> > >   * values.
> > > + *
> > > + * Note: This function can be called from two paths: The KVM_ARM_VCPU_INIT
> > > + * ioctl or as part of handling a request issued by another VCPU in the PSCI
> > > + * handling code.  In the first case, the VCPU will not be loaded, and in the
> > > + * second case the VCPU will be loaded.  Because this function operates purely
> > > + * on the memory-backed valus of system registers, we want to do a full put if
> > > + * we were loaded (handling a request) and load the values back at the end of
> > > + * the function.  Otherwise we leave the state alone.  In both cases, we
> > > + * disable preemption around the vcpu reset as we would otherwise race with
> > > + * preempt notifiers which also call put/load.
> > >   */
> > >  int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> > >  {
> > >  	const struct kvm_regs *cpu_reset;
> > > +	int ret = -EINVAL;
> > > +	bool loaded;
> > > +
> > > +	preempt_disable();
> > > +	loaded = (vcpu->cpu != -1);
> > > +	if (loaded)
> > > +		kvm_arch_vcpu_put(vcpu);
> > >  
> > >  	switch (vcpu->arch.target) {
> > >  	default:
> > >  		if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features)) {
> > >  			if (!cpu_has_32bit_el1())
> > > -				return -EINVAL;
> > > +				goto out;
> > >  			cpu_reset = &default_regs_reset32;
> > >  		} else {
> > >  			cpu_reset = &default_regs_reset;
> > > @@ -137,7 +154,12 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> > >  		vcpu->arch.workaround_flags |= VCPU_WORKAROUND_2_FLAG;
> > >  
> > >  	/* Reset timer */
> > > -	return kvm_timer_vcpu_reset(vcpu);
> > > +	ret = kvm_timer_vcpu_reset(vcpu);
> > > +out:
> > > +	if (loaded)
> > > +		kvm_arch_vcpu_load(vcpu, smp_processor_id());
> > > +	preempt_enable();
> > > +	return ret;
> > >  }
> > >  
> > >  void kvm_set_ipa_limit(void)
> > 
> > I was really confused by this: as far as I can see, we don't really need
> > to disable preemption here once kvm_arch_vcpu_put() is complete -- at
> > least not for the purpose of avoiding corruption of the reg state.  But
> > we _do_ need to disable the preempt notifier so that it doesn't fire
> > before we are ready.
> > 
> > It actually seems a bit surprising for a powered-off CPU to sit with the
> > VM regs live and preempt notifier armed, when the vcpu thread is
> > heading to interruptible sleep anyway until someone turns it on.
> > Perhaps an alternative approach would be to nobble the preempt notifier
> > and stick an explicit vcpu_put()...vcpu_load() around the
> > swait_event_interruptible_exclusive() call in vcpu_req_sleep().  This
> > is not fast path.
> > 
> > 
> 
> I think you've understood the problem correctly, and the thing here is
> that we (sort-of) "abuse" disabling preemption as a way to disable
> preempt notifiers, which I don't think we have.  So we could add that,
> and do something like:
> 
>   preempt_disable();
>   vcpu_put(vcpu);
>   disable_preempt_notifiers(vcpu);
>   preempt_disable();
>   funky_stuff();
>   vcpu_load();
>   preempt_enable();

Did you mean

	preempt_disable();
	disable_preempt_notifiers(vcpu);
	vcpu_put(vcpu);
	preempt_enable();

	funky_stuff();

	preempt_disable();
	vcpu_load(vcpu);
	enable_preempt_notifiers(vcpu);
	preempt_enable();

> But I think that's additional complexity to get a slightly shorter
> section with disabled preemption.

Agreed.

disable/enable_preempt_notifiers() may do little more than toggle a flag
that the preempt notifiers check, but I totally agree that it's unlikely
to be worth it just to be preemptible in this small region.

> We could also re-architect a lot of the vcpu_load/vpcu_put functionality
> more drastically, but that is difficult and requires understanding of
> how the other architectures work, so at the end of the day we just use
> this pattern in multiple places, which is:
> 
>   preempt_disable();
>   vcpu_put();

We still have the anomaly that we save all the state we're about to
reset here.

(i.e., we have no pure "invalidate" operation for the loaded CPU regs;
vcpu_put() is more "clean and invalidate", but we may not care much
about the "clean" part in this particular sequence).

Again, this only makes a difference to a rare slow path, so I don't
see it as a problem.

>   modify_vcpu_state_in_memory();
>   vcpu_load();
>   preempt_enable();
> 
> Does that help?

Yes, I think I've understood correctly what's going on here now.

The code is fine as it is.

Cheers
---Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/5] KVM: arm/arm64: Reset the VCPU without preemption and vcpu state loaded

[Dave Martin]

On Tue, Feb 26, 2019 at 01:53:52PM +0000, Dave Martin wrote:
> On Wed, Feb 20, 2019 at 07:41:43PM +0000, Marc Zyngier wrote:
> > On Wed, 20 Feb 2019 19:14:53 +0000
> > Dave Martin <Dave.Martin@arm.com> wrote:

[...]

> > > Perhaps an alternative approach would be to nobble the preempt notifier
> > > and stick an explicit vcpu_put()...vcpu_load() around the
> > > swait_event_interruptible_exclusive() call in vcpu_req_sleep().  This
> > > is not fast path.
> > 
> > What does it buy us? The problem we're solving here is a powered-off,
> > spuriously woken-up vcpu racing against the reset performed from
> > another vcpu. I don't see what adding more put/load would solve.
> 
> Not a lot.  Changes in this area would allow some code to move outside
> preempt_disable(), but only at the expense of extra cost elsewhere...
> 
> > > Any, with the code as-is, it looks like the SVE regs resetting should
> > > go in the preempt_disable() region, after the kvm_arch_vcpu_put() call.
> > 
> > All resets must go there. This is the only safe location.
> 
> OK, that's what I wanted to be sure of.

Hmmm, it turns out this is no good because we may need to kzalloc()
the vcpu->arch.sve_state storage here in the new-vcpu case.

Rather than create additional mess in this code, I will move that
allocation to be done more eagerly rather than deferred until the last
possible moment.

With the current API flow, this may mean krealloc()'ing the buffer if
the set of vector lengths for the vcpu is modified from the default
before the vcpu is run, via a user write to KVM_REG_ARM64_SVE_VLS --
though we would anyway get away with not doing it, since the default
buffer size will be the maximum that the hardware could possibly
require anyway.  We could waste a few K per vcpu, but that's almost not
worth caring about.

I'd rather not churn the API at this point, but I'll experiment to see
which approach under the hood looks cleanest.

Thoughts?

Cheers
---Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel