KVM: arm/arm64: fix some races and allow userspace to set MPIDR

Make sure we don't leave vcpu requests we don't intend to handle later set in the request bitmap. If we don't clear them, then kvm_request_pending() may return true when we don't want it to.

Signed-off-by Andrew Jones <[email protected]>

---
 arch/arm/kvm/handle_exit.c   | 1 +
 arch/arm/kvm/psci.c          | 1 +
 arch/arm64/kvm/handle_exit.c | 1 +
 3 files changed, 3 insertions(+)

arch/arm/kvm/handle_exit.c +1 -0

index 4e40d1955e35..2ec31748fa0b 100644
@@ -72,6 +72,7 @@ static int kvm_handle_wfx(struct kvm_vcpu *vcpu, struct kvm_run *run)
 		trace_kvm_wfx(*vcpu_pc(vcpu), false);
 		vcpu->stat.wfi_exit_stat++;
 		kvm_vcpu_block(vcpu);
+		__kvm_request_clear(KVM_REQ_UNHALT, vcpu);
 	}
 
 	kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));

arch/arm/kvm/psci.c +1 -0

index c2b131527a64..fd7e381b13b2 100644
@@ -57,6 +57,7 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)
 	 * for KVM will preserve the register state.
 	 */
 	kvm_vcpu_block(vcpu);
+	__kvm_request_clear(KVM_REQ_UNHALT, vcpu);
 
 	return PSCI_RET_SUCCESS;
 }

arch/arm64/kvm/handle_exit.c +1 -0

index 1bfe30dfbfe7..b34971d7c30a 100644
@@ -89,6 +89,7 @@ static int kvm_handle_wfx(struct kvm_vcpu *vcpu, struct kvm_run *run)
 		trace_kvm_wfx_arm64(*vcpu_pc(vcpu), false);
 		vcpu->stat.wfi_exit_stat++;
 		kvm_vcpu_block(vcpu);
+		__kvm_request_clear(KVM_REQ_UNHALT, vcpu);
 	}
 
 	kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));

Signature · 1 line

2.9.3

This not only ensures visibility of changes to pause by using atomic ops, but also plugs a small race where a vcpu could get its pause state enabled just after its last check before entering the guest. With this patch, while the vcpu will still initially enter the guest, it will exit immediately due to the IPI sent by the vcpu kick issued after making the vcpu request.

We use __kvm_request_set, because we don't need the barrier kvm_request_set() provides. Additionally, kvm_request_test_and_clear is not used because, for pause, only the requester should do the clearing, i.e. testing and clearing need to be independent.

Signed-off-by Andrew Jones <[email protected]>

---
 arch/arm/include/asm/kvm_host.h   |  5 +----
 arch/arm/kvm/arm.c                | 26 +++++++++++++-------------
 arch/arm64/include/asm/kvm_host.h |  5 +----
 3 files changed, 15 insertions(+), 21 deletions(-)

arch/arm/include/asm/kvm_host.h +1 -4

index cc495d799c67..3b5d60611cac 100644
@@ -46,7 +46,7 @@
 #define KVM_MAX_VCPUS VGIC_V2_MAX_CPUS
 #endif
 
-#define KVM_REQ_VCPU_EXIT	8
+#define KVM_REQ_VCPU_PAUSE	8
 
 u32 *kvm_vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num, u32 mode);
 int __attribute_const__ kvm_target_cpu(void);

@@ -174,9 +174,6 @@ struct kvm_vcpu_arch {
 	/* vcpu power-off state */
 	bool power_off;
 
-	 /* Don't run the guest (internal implementation need) */
-	bool pause;
-
 	/* IO related fields */
 	struct kvm_decode mmio_decode;
 

arch/arm/kvm/arm.c +7 -11

index c9a2103faeb9..17d5f3fb33d9 100644
@@ -401,7 +401,8 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
 int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
 {
 	return ((!!v->arch.irq_lines || kvm_vgic_vcpu_pending_irq(v))
-		&& !v->arch.power_off && !v->arch.pause);
+		&& !v->arch.power_off
+		&& !__kvm_request_test(KVM_REQ_VCPU_PAUSE, v));
 }
 
 /* Just ensure a guest exit from a particular CPU */

@@ -532,17 +533,12 @@ bool kvm_arch_intc_initialized(struct kvm *kvm)
 
 void kvm_arm_halt_guest(struct kvm *kvm)
 {
-	int i;
-	struct kvm_vcpu *vcpu;
-
-	kvm_for_each_vcpu(i, vcpu, kvm)
-		vcpu->arch.pause = true;
-	kvm_make_all_cpus_request(kvm, KVM_REQ_VCPU_EXIT);
+	kvm_make_all_cpus_request(kvm, KVM_REQ_VCPU_PAUSE);
 }
 
 void kvm_arm_halt_vcpu(struct kvm_vcpu *vcpu)
 {
-	vcpu->arch.pause = true;
+	__kvm_request_set(KVM_REQ_VCPU_PAUSE, vcpu);
 	kvm_vcpu_kick(vcpu);
 }
 

@@ -550,7 +546,7 @@ void kvm_arm_resume_vcpu(struct kvm_vcpu *vcpu)
 {
 	struct swait_queue_head *wq = kvm_arch_vcpu_wq(vcpu);
 
-	vcpu->arch.pause = false;
+	__kvm_request_clear(KVM_REQ_VCPU_PAUSE, vcpu);
 	swake_up(wq);
 }
 

@@ -568,7 +564,7 @@ static void vcpu_sleep(struct kvm_vcpu *vcpu)
 	struct swait_queue_head *wq = kvm_arch_vcpu_wq(vcpu);
 
 	swait_event_interruptible(*wq, ((!vcpu->arch.power_off) &&
-				       (!vcpu->arch.pause)));
+		(!__kvm_request_test(KVM_REQ_VCPU_PAUSE, vcpu))));
 }
 
 static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)

@@ -621,7 +617,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 
 		update_vttbr(vcpu->kvm);
 
-		if (vcpu->arch.power_off || vcpu->arch.pause)
+		if (vcpu->arch.power_off || __kvm_request_test(KVM_REQ_VCPU_PAUSE, vcpu))
 			vcpu_sleep(vcpu);
 
 		/*

arch/arm/kvm/arm.c +4 -2

+
 		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm) ||
-			vcpu->arch.power_off || vcpu->arch.pause) {
+			vcpu->arch.power_off || kvm_request_pending(vcpu)) {
+			vcpu->mode = OUTSIDE_GUEST_MODE;
+			smp_mb();
 			local_irq_enable();
 			kvm_pmu_sync_hwstate(vcpu);
 			kvm_timer_sync_hwstate(vcpu);

@@ -661,7 +662,6 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 		 */
 		trace_kvm_entry(*vcpu_pc(vcpu));
 		guest_enter_irqoff();
-		vcpu->mode = IN_GUEST_MODE;
 
 		ret = kvm_call_hyp(__kvm_vcpu_run, vcpu);
 

arch/arm64/include/asm/kvm_host.h +1 -4

index f21fd3894370..c03e1fc3bc34 100644
@@ -43,7 +43,7 @@
 
 #define KVM_VCPU_MAX_FEATURES 4
 
-#define KVM_REQ_VCPU_EXIT	8
+#define KVM_REQ_VCPU_PAUSE	8
 
 int __attribute_const__ kvm_target_cpu(void);
 int kvm_reset_vcpu(struct kvm_vcpu *vcpu);

@@ -257,9 +257,6 @@ struct kvm_vcpu_arch {
 	/* vcpu power-off state */
 	bool power_off;
 
-	/* Don't run the guest (internal implementation need) */
-	bool pause;
-
 	/* IO related fields */
 	struct kvm_decode mmio_decode;
 

Signature · 1 line

2.9.3

Quoted 14 lines

static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)

arch/arm/kvm/arm.c +1 -1

@@ -621,7 +617,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 
 		update_vttbr(vcpu->kvm);
 
-		if (vcpu->arch.power_off || vcpu->arch.pause)
+		if (vcpu->arch.power_off || __kvm_request_test(KVM_REQ_VCPU_PAUSE, vcpu))
 			vcpu_sleep(vcpu);
 
 		/*

@@ -644,8 +640,13 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 			run->exit_reason = KVM_EXIT_INTR;
 		}
 

arch/arm/kvm/arm.c int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) · line 644

Quoted 2 lines

arch/arm/kvm/arm.c +2 -0

+		vcpu->mode = IN_GUEST_MODE;
+		smp_mb(); /* ensure vcpu->mode is visible to kvm_vcpu_kick */

I think this comment is misleading, because this smp_mb() is really about ensuring that with respect to other CPUs, the write to vcpu->mode is observable before the read of kvm_request_pending below, and the corresponding other barrier is the barrier implied in cmpxchg used by kvm_vcpu_exiting_guest_mode, which gets called by kvm_vcpu_kick(), which is called after __kvm_set_request.

So this also means that our direct use of kvm_vcpu_kick() without making a request is currently racy, so we should address that where appropriate as well.

Do you feel brave enough to take a look at that?

Quoted 14 lines · Christoffer Dall

Quoted 14 lines

static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)

arch/arm/kvm/arm.c +1 -1

@@ -621,7 +617,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 
 		update_vttbr(vcpu->kvm);
 
-		if (vcpu->arch.power_off || vcpu->arch.pause)
+		if (vcpu->arch.power_off || __kvm_request_test(KVM_REQ_VCPU_PAUSE, vcpu))
 			vcpu_sleep(vcpu);
 
 		/*

@@ -644,8 +640,13 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 			run->exit_reason = KVM_EXIT_INTR;
 		}
 

Quoted 9 lines

arch/arm/kvm/arm.c int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) · line 644

Quoted 2 lines

arch/arm/kvm/arm.c +2 -0

+		vcpu->mode = IN_GUEST_MODE;
+		smp_mb(); /* ensure vcpu->mode is visible to kvm_vcpu_kick */

I think this comment is misleading, because this smp_mb() is really about ensuring that with respect to other CPUs, the write to vcpu->mode is observable before the read of kvm_request_pending below, and the corresponding other barrier is the barrier implied in cmpxchg used by kvm_vcpu_exiting_guest_mode, which gets called by kvm_vcpu_kick(), which is called after __kvm_set_request.

Agreed

Quoted 25 lines · Andrew Jones

Quoted 23 lines · Christoffer Dall

Quoted 11 lines

static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)

arch/arm/kvm/arm.c +1 -1

@@ -621,7 +617,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 
 		update_vttbr(vcpu->kvm);
 
-		if (vcpu->arch.power_off || vcpu->arch.pause)
+		if (vcpu->arch.power_off || __kvm_request_test(KVM_REQ_VCPU_PAUSE, vcpu))
 			vcpu_sleep(vcpu);
 
 		/*

arch/arm/kvm/arm.c int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) · line 644

Quoted 5 lines

arch/arm/kvm/arm.c +2 -0

@@ -644,8 +640,13 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 			run->exit_reason = KVM_EXIT_INTR;
 		}
 
+		vcpu->mode = IN_GUEST_MODE;
+		smp_mb(); /* ensure vcpu->mode is visible to kvm_vcpu_kick */

I think this comment is misleading, because this smp_mb() is really about ensuring that with respect to other CPUs, the write to vcpu->mode is observable before the read of kvm_request_pending below, and the corresponding other barrier is the barrier implied in cmpxchg used by kvm_vcpu_exiting_guest_mode, which gets called by kvm_vcpu_kick(), which is called after __kvm_set_request.

Quoted 2 lines

Agreed

Just an adjustment to our conclusion from last week:

Will Deacon clarified that the cmpxchg doesn't have barrier semantics if the cmpxchg operation fails. My brain hurts trying to work out if we're still safe in that case. Thoughts?

Quoted 26 lines · Christoffer Dall

Quoted 25 lines · Andrew Jones

Quoted 23 lines · Christoffer Dall

Quoted 11 lines

static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)

arch/arm/kvm/arm.c +1 -1

@@ -621,7 +617,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 
 		update_vttbr(vcpu->kvm);
 
-		if (vcpu->arch.power_off || vcpu->arch.pause)
+		if (vcpu->arch.power_off || __kvm_request_test(KVM_REQ_VCPU_PAUSE, vcpu))
 			vcpu_sleep(vcpu);
 
 		/*

arch/arm/kvm/arm.c int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) · line 644

Quoted 5 lines

arch/arm/kvm/arm.c +2 -0

@@ -644,8 +640,13 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 			run->exit_reason = KVM_EXIT_INTR;
 		}
 
+		vcpu->mode = IN_GUEST_MODE;
+		smp_mb(); /* ensure vcpu->mode is visible to kvm_vcpu_kick */

I think this comment is misleading, because this smp_mb() is really about ensuring that with respect to other CPUs, the write to vcpu->mode is observable before the read of kvm_request_pending below, and the corresponding other barrier is the barrier implied in cmpxchg used by kvm_vcpu_exiting_guest_mode, which gets called by kvm_vcpu_kick(), which is called after __kvm_set_request.

Quoted 9 lines

Quoted 2 lines

Agreed

Just an adjustment to our conclusion from last week:

Will Deacon clarified that the cmpxchg doesn't have barrier semantics if the cmpxchg operation fails. My brain hurts trying to work out if we're still safe in that case. Thoughts?

OK, I just ran my brain through a blender, and then I grabbed Radim to bounce my thoughts off him first. I think he and I are in agreement that kvm_arch_vcpu_should_kick() should get a read barrier before the cmpxchg. That smp_rmb() would pair with the general barrier that I have poorly commented in the code above.

I'll refresh this series in the next couple days, adding that barrier, and some better comments.

QEMU would already prefer having control over the vcpu MPIDRs in order to correctly map vcpus to virtual numa nodes without having to first instantiate the vcpus. Also, when virtual cpu topologies are eventually describable, QEMU may need to adjust the MPIDR to reflect that topology. Finally, we need to cache the MPIDR in the vcpu structure to fix potential races that can arise between vcpu reset and the extraction of the MPIDR from the sys-reg array. So, while we're tinkering with MPIDR to provide it a cache, then we might as well extend its capabilities to allowing user setting too.

Cc Ashok Kumar <[email protected]>

Cc Igor Mammedov <[email protected]>

Signed-off-by Andrew Jones <[email protected]>

---
 arch/arm/include/asm/kvm_emulate.h   |  2 +-
 arch/arm/include/asm/kvm_host.h      |  3 ++
 arch/arm/kvm/arm.c                   |  1 +
 arch/arm/kvm/coproc.c                | 61 +++++++++++++++++++++++++++++-----
 arch/arm/kvm/coproc.h                |  6 ++++
 arch/arm64/include/asm/kvm_emulate.h |  2 +-
 arch/arm64/include/asm/kvm_host.h    |  3 ++
 arch/arm64/kvm/sys_regs.c            | 64 ++++++++++++++++++++++++++++--------
 include/uapi/linux/kvm.h             |  1 +
 9 files changed, 119 insertions(+), 24 deletions(-)

arch/arm/include/asm/kvm_emulate.h +1 -1

index 9a8a45aaf19a..1b922de46785 100644
@@ -213,7 +213,7 @@ static inline u32 kvm_vcpu_hvc_get_imm(struct kvm_vcpu *vcpu)
 
 static inline unsigned long kvm_vcpu_get_mpidr_aff(struct kvm_vcpu *vcpu)
 {
-	return vcpu_cp15(vcpu, c0_MPIDR) & MPIDR_HWID_BITMASK;
+	return vcpu->arch.vmpidr & MPIDR_HWID_BITMASK;
 }
 
 static inline void kvm_vcpu_set_be(struct kvm_vcpu *vcpu)

arch/arm/include/asm/kvm_host.h +3 -0

index 73f61773e9c2..6427cac77204 100644
@@ -151,6 +151,9 @@ struct kvm_vcpu_arch {
 	/* The CPU type we expose to the VM */
 	u32 midr;
 
+	/* vcpu MPIDR */
+	u32 vmpidr;
+
 	/* HYP trapping configuration */
 	u32 hcr;
 

arch/arm/kvm/arm.c +1 -0

index aa68354a0bf8..899528b884d9 100644
@@ -207,6 +207,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_READONLY_MEM:
 	case KVM_CAP_MP_STATE:
 	case KVM_CAP_IMMEDIATE_EXIT:
+	case KVM_CAP_ARM_USER_MPIDR:
 		r = 1;
 		break;
 	case KVM_CAP_COALESCED_MMIO:

arch/arm/kvm/coproc.c +18 -7

index 3e5e4194ef86..30d35dd407a6 100644
@@ -99,16 +99,55 @@ int kvm_handle_cp14_access(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	return 1;
 }
 
-static void reset_mpidr(struct kvm_vcpu *vcpu, const struct coproc_reg *r)
+static int set_mpidr(struct kvm_vcpu *vcpu, const struct coproc_reg *rd,
+		     const struct kvm_one_reg *reg, void __user *uaddr)
 {
+	struct kvm_vcpu *v;
+	__u32 mpidr;
+	int i;
+
+	if (copy_from_user(&mpidr, uaddr, KVM_REG_SIZE(reg->id)))
+		return -EFAULT;
+
 	/*
-	 * Compute guest MPIDR. We build a virtual cluster out of the
-	 * vcpu_id, but we read the 'U' bit from the underlying
-	 * hardware directly.
+	 * All ARMv7 processors that support KVM include the
+	 * Multiprocessing Extensions. Furthermore, without at
+	 * least one bit set in the MPIDR we won't be able to
+	 * distinguish a user set MPIDR from an unset MPIDR in
+	 * reset_mpidr. Thus we force bit 31 on.
 	 */
-	vcpu_cp15(vcpu, c0_MPIDR) = ((read_cpuid_mpidr() & MPIDR_SMP_BITMASK) |
-				     ((vcpu->vcpu_id >> 2) << MPIDR_LEVEL_BITS) |
-				     (vcpu->vcpu_id & 3));
+	mpidr |= (1UL << 31);
+
+	/* Ensure flag consistency and ID uniqueness */
+	kvm_for_each_vcpu(i, v, vcpu->kvm) {

arch/arm/kvm/coproc.c +25 -1

+
+	return 0;
+}
+
+static void reset_mpidr(struct kvm_vcpu *vcpu, const struct coproc_reg *r)
+{
+	if (!vcpu->arch.vmpidr) {
+		/*
+		 * Compute guest MPIDR. We build a virtual cluster out of the
+		 * vcpu_id, but we read the 'U' bit from the underlying
+		 * hardware directly.
+		 */
+		u32 mpidr = ((read_cpuid_mpidr() & MPIDR_SMP_BITMASK) |
+			     ((vcpu->vcpu_id >> 2) << MPIDR_LEVEL_BITS) |
+			     (vcpu->vcpu_id & 3));
+		vcpu->arch.vmpidr = mpidr;
+	}
+	vcpu_cp15(vcpu, c0_MPIDR) = vcpu->arch.vmpidr;
 }
 
 /* TRM entries A7:4.3.31 A15:4.3.28 - RO WI */

@@ -300,7 +339,7 @@ static bool pm_fake(struct kvm_vcpu *vcpu,
 static const struct coproc_reg cp15_regs[] = {
 	/* MPIDR: we use VMPIDR for guest access. */
 	{ CRn( 0), CRm( 0), Op1( 0), Op2( 5), is32,
-			NULL, reset_mpidr, c0_MPIDR },
+			NULL, reset_mpidr, c0_MPIDR, 0, NULL, set_mpidr },
 
 	/* CSSELR: swapped by interrupt.S. */
 	{ CRn( 0), CRm( 0), Op1( 2), Op2( 0), is32,

@@ -1072,6 +1111,9 @@ int kvm_arm_coproc_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 	if (!r)
 		return get_invariant_cp15(reg->id, uaddr);
 
+	if (r->get_user)
+		return (r->get_user)(vcpu, r, reg, uaddr);
+
 	ret = -ENOENT;
 	if (KVM_REG_SIZE(reg->id) == 8) {
 		u64 val;

@@ -1101,6 +1143,9 @@ int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 	if (!r)
 		return set_invariant_cp15(reg->id, uaddr);
 
+	if (r->set_user)
+		return (r->set_user)(vcpu, r, reg, uaddr);
+
 	ret = -ENOENT;
 	if (KVM_REG_SIZE(reg->id) == 8) {
 		u64 val;

arch/arm/kvm/coproc.h +6 -0

index eef1759c2b65..7107f19cac5e 100644
@@ -52,6 +52,12 @@ struct coproc_reg {
 
 	/* Value (usually reset value) */
 	u64 val;
+
+	/* Custom get/set_user functions, fallback to generic if NULL */
+	int (*get_user)(struct kvm_vcpu *vcpu, const struct coproc_reg *rd,
+			const struct kvm_one_reg *reg, void __user *uaddr);
+	int (*set_user)(struct kvm_vcpu *vcpu, const struct coproc_reg *rd,
+			const struct kvm_one_reg *reg, void __user *uaddr);
 };
 
 static inline void print_cp_instr(const struct coproc_params *p)

arch/arm64/include/asm/kvm_emulate.h +1 -1

index f5ea0ba70f07..c138bb15b507 100644
@@ -242,7 +242,7 @@ static inline u8 kvm_vcpu_trap_get_fault_type(const struct kvm_vcpu *vcpu)
 
 static inline unsigned long kvm_vcpu_get_mpidr_aff(struct kvm_vcpu *vcpu)
 {
-	return vcpu_sys_reg(vcpu, MPIDR_EL1) & MPIDR_HWID_BITMASK;
+	return vcpu->arch.vmpidr_el2 & MPIDR_HWID_BITMASK;
 }
 
 static inline void kvm_vcpu_set_be(struct kvm_vcpu *vcpu)

arch/arm64/include/asm/kvm_host.h +3 -0

index d1e4b5d962eb..31aed40a3724 100644
@@ -198,6 +198,9 @@ typedef struct kvm_cpu_context kvm_cpu_context_t;
 struct kvm_vcpu_arch {
 	struct kvm_cpu_context ctxt;
 
+	/* vcpu MPIDR */
+	u64 vmpidr_el2;
+
 	/* HYP configuration */
 	u64 hcr_el2;
 	u32 mdcr_el2;

arch/arm64/kvm/sys_regs.c +50 -14

index 0e26f8c2b56f..2b558ac034ce 100644
@@ -429,21 +429,57 @@ static void reset_amair_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 	vcpu_sys_reg(vcpu, AMAIR_EL1) = read_sysreg(amair_el1);
 }
 
-static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
+static int set_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
+		     const struct kvm_one_reg *reg, void __user *uaddr)
 {
-	u64 mpidr;
+	struct kvm_vcpu *v;
+	__u64 mpidr;
+	int i;
 
-	/*
-	 * Map the vcpu_id into the first three affinity level fields of
-	 * the MPIDR. We limit the number of VCPUs in level 0 due to a
-	 * limitation to 16 CPUs in that level in the ICC_SGIxR registers
-	 * of the GICv3 to be able to address each CPU directly when
-	 * sending IPIs.
-	 */
-	mpidr = (vcpu->vcpu_id & 0x0f) << MPIDR_LEVEL_SHIFT(0);
-	mpidr |= ((vcpu->vcpu_id >> 4) & 0xff) << MPIDR_LEVEL_SHIFT(1);
-	mpidr |= ((vcpu->vcpu_id >> 12) & 0xff) << MPIDR_LEVEL_SHIFT(2);
-	vcpu_sys_reg(vcpu, MPIDR_EL1) = (1ULL << 31) | mpidr;
+	if (copy_from_user(&mpidr, uaddr, KVM_REG_SIZE(reg->id)))
+		return -EFAULT;
+
+	/* Ensure RES1 bits are set */
+	mpidr |= (1ULL << 31);
+
+	/* Ensure no RES0 bits are set */
+	if ((mpidr & (GENMASK_ULL(63, 40) | GENMASK(29, 25))) ||
+	    (vcpu_mode_is_32bit(vcpu) && (mpidr & GENMASK_ULL(39, 32))))
+		return -EINVAL;
+
+	/* Ensure flag consistency and ID uniqueness */
+	kvm_for_each_vcpu(i, v, vcpu->kvm) {
+		if (v == vcpu)
+			continue;
+		if (((v->arch.vmpidr_el2 | mpidr) & MPIDR_UP_BITMASK)
+		    || ((mpidr & MPIDR_MT_BITMASK) && !(v->arch.vmpidr_el2 & MPIDR_MT_BITMASK))
+		    || v->arch.vmpidr_el2 == mpidr)
+			return -EINVAL;
+	}
+
+	vcpu->arch.vmpidr_el2 = mpidr;
+	vcpu_sys_reg(vcpu, MPIDR_EL1) = mpidr;
+
+	return 0;
+}
+
+static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
+{
+	if (!vcpu->arch.vmpidr_el2) {
+		/*
+		 * Userspace didn't provide us an MPIDR, so we construct one
+		 * from the vcpu_id by mapping it into the first three affinity
+		 * levels. We limit the number of VCPUs in level 0 due to a
+		 * limitation of 16 CPUs in that level in the ICC_SGIxR
+		 * registers of the GICv3, which are used to address each CPU
+		 * directly when sending IPIs.
+		 */
+		u64 mpidr = (vcpu->vcpu_id & 0x0f) << MPIDR_LEVEL_SHIFT(0);
+		mpidr |= ((vcpu->vcpu_id >> 4) & 0xff) << MPIDR_LEVEL_SHIFT(1);
+		mpidr |= ((vcpu->vcpu_id >> 12) & 0xff) << MPIDR_LEVEL_SHIFT(2);
+		vcpu->arch.vmpidr_el2 = (1ULL << 31) | mpidr;
+	}
+	vcpu_sys_reg(vcpu, MPIDR_EL1) = vcpu->arch.vmpidr_el2;
 }
 
 static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)

@@ -961,7 +997,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 
 	/* MPIDR_EL1 */
 	{ Op0(0b11), Op1(0b000), CRn(0b0000), CRm(0b0000), Op2(0b101),
-	  NULL, reset_mpidr, MPIDR_EL1 },
+	  NULL, reset_mpidr, MPIDR_EL1, 0, NULL, set_mpidr },
 	/* SCTLR_EL1 */
 	{ Op0(0b11), Op1(0b000), CRn(0b0001), CRm(0b0000), Op2(0b000),
 	  access_vm_reg, reset_val, SCTLR_EL1, 0x00C50078 },

include/uapi/linux/kvm.h +1 -0

index f51d5082a377..c20b58fe84cd 100644
@@ -883,6 +883,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_PPC_MMU_RADIX 134
 #define KVM_CAP_PPC_MMU_HASH_V3 135
 #define KVM_CAP_IMMEDIATE_EXIT 136
+#define KVM_CAP_ARM_USER_MPIDR 137
 
 #ifdef KVM_CAP_IRQ_ROUTING
 

Signature · 1 line

2.9.3

Isn't this all super racy? What prevents two VCPUs from getting the same MPIDR at the same time?

From a userspace API point of view, I was expecting this to be

Quoted 2 lines · Christoffer Dall

Isn't this all super racy? What prevents two VCPUs from getting the same MPIDR at the same time?

From a userspace API point of view, I was expecting this to be "just don't do that then" territory, ie it's userspace's job to set the MPIDRs to something that makes sense. Does KVM actually care internally what the MPIDR presented to the guest is? (In theory I don't see why you shouldn't be able to present the guest with a bit of bogus hardware that claims the same MPIDR for all cores.)

Quoted 8 lines · Peter Maydell

From a userspace API point of view, I was expecting this to be "just don't do that then" territory, ie it's userspace's job to set the MPIDRs to something that makes sense. Does KVM actually care internally what the MPIDR presented to the guest is? (In theory I don't see why you shouldn't be able to present the guest with a bit of bogus hardware that claims the same MPIDR for all cores.)

I think it's important that we maintain a strict mapping between an MPIDR and a VCPU at any given time, because otherwise, we'll have problems knowing which redestributor belongs to which VCPU and making other qualified decisions in the KVM/ARM implementation.

This series fixes four races. Two are easy to produce with a kvm-unit-test test[1], but the other two would be quite hard. I didn't even try to produce those. The two hard to produce races are addressed by changing vcpu->arch.pause and vcpu->arch.power_off to vcpu requests. The two easy to produce races are addressed in two different ways: the first takes advantage of power_off having been changed to a vcpu request, the second caches vcpu MPIDRs in order to avoid extracting them from sys_regs. When introducing the MPIDR cache we also introduce a new feature (userspace settable MPIDRs).

Support for userspace settable MPIDRs was already posted once[2], but rejected due to not having a use case. We have one now, which is to satisfy QEMU's need for the MPDIR information very early, before vcpu-init has even run. While the original posting author wasn't me, I've taken authorship now, as I've changed the patch substantially. If anybody disagrees with that, then feel free to suggest alternatives. The QEMU counterpart has been posted[3].

This series is based on Radim's recent posting[4] that improves the vcpu-request framework. I've tested the series on a couple AArch64 platforms and compile-tested the arm bits.

Hi Drew,

Quoted 18 lines · Andrew Jones

This series fixes four races. Two are easy to produce with a kvm-unit-test test[1], but the other two would be quite hard. I didn't even try to produce those. The two hard to produce races are addressed by changing vcpu->arch.pause and vcpu->arch.power_off to vcpu requests. The two easy to produce races are addressed in two different ways: the first takes advantage of power_off having been changed to a vcpu request, the second caches vcpu MPIDRs in order to avoid extracting them from sys_regs. When introducing the MPIDR cache we also introduce a new feature (userspace settable MPIDRs).

Support for userspace settable MPIDRs was already posted once[2], but rejected due to not having a use case. We have one now, which is to satisfy QEMU's need for the MPDIR information very early, before vcpu-init has even run. While the original posting author wasn't me, I've taken authorship now, as I've changed the patch substantially. If anybody disagrees with that, then feel free to suggest alternatives. The QEMU counterpart has been posted[3].

Quoted 3 lines

This series is based on Radim's recent posting[4] that improves the vcpu-request framework. I've tested the series on a couple AArch64 platforms and compile-tested the arm bits.

So as we discussed during lunch today, it would be awesome if you could split this up into a series that fixes the identified races and a separate series for the introduction of supporting setting the MPIDR from user space.

Thanks a lot!
-Christoffer