[PATCH 0/7] Consolidate vcpu ioctl locking

[PATCH 0/7] Consolidate vcpu ioctl locking

[Avi Kivity]

Avi Kivity (7):
  KVM: PPC: Add missing vcpu_load()/vcpu_put() in vcpu ioctls
  KVM: x86: Add missing locking to arch specific vcpu ioctls
  KVM: move vcpu locking to dispatcher for generic vcpu ioctls
  KVM: x86: Lock arch specific vcpu ioctls centrally
  KVM: s390: Centrally lock arch specific vcpu ioctls
  KVM: PPC: Centralize locking of arch specific vcpu ioctls
  KVM: Consolidate arch specific vcpu ioctl locking

 arch/ia64/kvm/kvm-ia64.c   |   11 -------
 arch/powerpc/kvm/book3s.c  |   10 +-----
 arch/powerpc/kvm/booke.c   |    5 ++-
 arch/powerpc/kvm/powerpc.c |    7 +---
 arch/s390/kvm/kvm-s390.c   |   55 ++++++++++-------------------------
 arch/x86/kvm/x86.c         |   69 +------------------------------------------
 virt/kvm/kvm_main.c        |   13 ++++++++
 7 files changed, 39 insertions(+), 131 deletions(-)

[PATCH 1/7] KVM: PPC: Add missing vcpu_load()/vcpu_put() in vcpu ioctls

[Avi Kivity]

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/powerpc/kvm/book3s.c |   10 ++++++++++
 arch/powerpc/kvm/booke.c  |   15 ++++++++++++++-
 2 files changed, 24 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
index 11f226f..b998abf 100644
--- a/arch/powerpc/kvm/book3s.c
+++ b/arch/powerpc/kvm/book3s.c
@@ -1110,6 +1110,8 @@ int kvm_arch_vcpu_ioctl_get_sregs(struct kvm_vcpu *vcpu,
 	struct kvmppc_vcpu_book3s *vcpu3s = to_book3s(vcpu);
 	int i;
 
+	vcpu_load(vcpu);
+
 	sregs->pvr = vcpu->arch.pvr;
 
 	sregs->u.s.sdr1 = to_book3s(vcpu)->sdr1;
@@ -1128,6 +1130,9 @@ int kvm_arch_vcpu_ioctl_get_sregs(struct kvm_vcpu *vcpu,
 			sregs->u.s.ppc32.dbat[i] = vcpu3s->dbat[i].raw;
 		}
 	}
+
+	vcpu_put(vcpu);
+
 	return 0;
 }
 
@@ -1137,6 +1142,8 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
 	struct kvmppc_vcpu_book3s *vcpu3s = to_book3s(vcpu);
 	int i;
 
+	vcpu_load(vcpu);
+
 	kvmppc_set_pvr(vcpu, sregs->pvr);
 
 	vcpu3s->sdr1 = sregs->u.s.sdr1;
@@ -1163,6 +1170,9 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
 
 	/* Flush the MMU after messing with the segments */
 	kvmppc_mmu_pte_flush(vcpu, 0, 0);
+
+	vcpu_put(vcpu);
+
 	return 0;
 }
 
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index c922240..a33ab8c 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -485,6 +485,8 @@ int kvm_arch_vcpu_ioctl_get_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 {
 	int i;
 
+	vcpu_load(vcpu);
+
 	regs->pc = vcpu->arch.pc;
 	regs->cr = kvmppc_get_cr(vcpu);
 	regs->ctr = vcpu->arch.ctr;
@@ -505,6 +507,8 @@ int kvm_arch_vcpu_ioctl_get_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 	for (i = 0; i < ARRAY_SIZE(regs->gpr); i++)
 		regs->gpr[i] = kvmppc_get_gpr(vcpu, i);
 
+	vcpu_put(vcpu);
+
 	return 0;
 }
 
@@ -512,6 +516,8 @@ int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 {
 	int i;
 
+	vcpu_load(vcpu);
+
 	vcpu->arch.pc = regs->pc;
 	kvmppc_set_cr(vcpu, regs->cr);
 	vcpu->arch.ctr = regs->ctr;
@@ -531,6 +537,8 @@ int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 	for (i = 0; i < ARRAY_SIZE(regs->gpr); i++)
 		kvmppc_set_gpr(vcpu, i, regs->gpr[i]);
 
+	vcpu_put(vcpu);
+
 	return 0;
 }
 
@@ -559,7 +567,12 @@ int kvm_arch_vcpu_ioctl_set_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu)
 int kvm_arch_vcpu_ioctl_translate(struct kvm_vcpu *vcpu,
                                   struct kvm_translation *tr)
 {
-	return kvmppc_core_vcpu_translate(vcpu, tr);
+	int r;
+
+	vcpu_load(vcpu);
+	r = kvmppc_core_vcpu_translate(vcpu, tr);
+	vcpu_put(vcpu);
+	return r;
 }
 
 int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, struct kvm_dirty_log *log)
-- 
1.7.0.4

[PATCH 3/7] KVM: move vcpu locking to dispatcher for generic vcpu ioctls

[Avi Kivity]

All vcpu ioctls need to be locked, so instead of locking each one specifically
we lock at the generic dispatcher.

This patch only updates generic ioctls and leaves arch specific ioctls alone.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/ia64/kvm/kvm-ia64.c   |   11 -----------
 arch/powerpc/kvm/book3s.c  |   16 ----------------
 arch/powerpc/kvm/booke.c   |   10 ----------
 arch/powerpc/kvm/powerpc.c |    4 ----
 arch/s390/kvm/kvm-s390.c   |   16 ----------------
 arch/x86/kvm/x86.c         |   36 ++----------------------------------
 virt/kvm/kvm_main.c        |   15 +++++++++++++++
 7 files changed, 17 insertions(+), 91 deletions(-)

diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index d5f4e91..29f6075 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -724,8 +724,6 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 	int r;
 	sigset_t sigsaved;
 
-	vcpu_load(vcpu);
-
 	if (vcpu->sigset_active)
 		sigprocmask(SIG_SETMASK, &vcpu->sigset, &sigsaved);
 
@@ -747,7 +745,6 @@ out:
 	if (vcpu->sigset_active)
 		sigprocmask(SIG_SETMASK, &sigsaved, NULL);
 
-	vcpu_put(vcpu);
 	return r;
 }
 
@@ -882,8 +879,6 @@ int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 	struct vpd *vpd = to_host(vcpu->kvm, vcpu->arch.vpd);
 	int i;
 
-	vcpu_load(vcpu);
-
 	for (i = 0; i < 16; i++) {
 		vpd->vgr[i] = regs->vpd.vgr[i];
 		vpd->vbgr[i] = regs->vpd.vbgr[i];
@@ -930,8 +925,6 @@ int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 	vcpu->arch.itc_offset = regs->saved_itc - kvm_get_itc(vcpu);
 	set_bit(KVM_REQ_RESUME, &vcpu->requests);
 
-	vcpu_put(vcpu);
-
 	return 0;
 }
 
@@ -1966,9 +1959,7 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
 int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
 				    struct kvm_mp_state *mp_state)
 {
-	vcpu_load(vcpu);
 	mp_state->mp_state = vcpu->arch.mp_state;
-	vcpu_put(vcpu);
 	return 0;
 }
 
@@ -1999,10 +1990,8 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
 {
 	int r = 0;
 
-	vcpu_load(vcpu);
 	vcpu->arch.mp_state = mp_state->mp_state;
 	if (vcpu->arch.mp_state == KVM_MP_STATE_UNINITIALIZED)
 		r = vcpu_reset(vcpu);
-	vcpu_put(vcpu);
 	return r;
 }
diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
index b998abf..f6eac2f 100644
--- a/arch/powerpc/kvm/book3s.c
+++ b/arch/powerpc/kvm/book3s.c
@@ -1047,8 +1047,6 @@ int kvm_arch_vcpu_ioctl_get_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 {
 	int i;
 
-	vcpu_load(vcpu);
-
 	regs->pc = kvmppc_get_pc(vcpu);
 	regs->cr = kvmppc_get_cr(vcpu);
 	regs->ctr = kvmppc_get_ctr(vcpu);
@@ -1069,8 +1067,6 @@ int kvm_arch_vcpu_ioctl_get_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 	for (i = 0; i < ARRAY_SIZE(regs->gpr); i++)
 		regs->gpr[i] = kvmppc_get_gpr(vcpu, i);
 
-	vcpu_put(vcpu);
-
 	return 0;
 }
 
@@ -1078,8 +1074,6 @@ int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 {
 	int i;
 
-	vcpu_load(vcpu);
-
 	kvmppc_set_pc(vcpu, regs->pc);
 	kvmppc_set_cr(vcpu, regs->cr);
 	kvmppc_set_ctr(vcpu, regs->ctr);
@@ -1099,8 +1093,6 @@ int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 	for (i = 0; i < ARRAY_SIZE(regs->gpr); i++)
 		kvmppc_set_gpr(vcpu, i, regs->gpr[i]);
 
-	vcpu_put(vcpu);
-
 	return 0;
 }
 
@@ -1110,8 +1102,6 @@ int kvm_arch_vcpu_ioctl_get_sregs(struct kvm_vcpu *vcpu,
 	struct kvmppc_vcpu_book3s *vcpu3s = to_book3s(vcpu);
 	int i;
 
-	vcpu_load(vcpu);
-
 	sregs->pvr = vcpu->arch.pvr;
 
 	sregs->u.s.sdr1 = to_book3s(vcpu)->sdr1;
@@ -1131,8 +1121,6 @@ int kvm_arch_vcpu_ioctl_get_sregs(struct kvm_vcpu *vcpu,
 		}
 	}
 
-	vcpu_put(vcpu);
-
 	return 0;
 }
 
@@ -1142,8 +1130,6 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
 	struct kvmppc_vcpu_book3s *vcpu3s = to_book3s(vcpu);
 	int i;
 
-	vcpu_load(vcpu);
-
 	kvmppc_set_pvr(vcpu, sregs->pvr);
 
 	vcpu3s->sdr1 = sregs->u.s.sdr1;
@@ -1171,8 +1157,6 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
 	/* Flush the MMU after messing with the segments */
 	kvmppc_mmu_pte_flush(vcpu, 0, 0);
 
-	vcpu_put(vcpu);
-
 	return 0;
 }
 
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index a33ab8c..b687f43 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -485,8 +485,6 @@ int kvm_arch_vcpu_ioctl_get_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 {
 	int i;
 
-	vcpu_load(vcpu);
-
 	regs->pc = vcpu->arch.pc;
 	regs->cr = kvmppc_get_cr(vcpu);
 	regs->ctr = vcpu->arch.ctr;
@@ -507,8 +505,6 @@ int kvm_arch_vcpu_ioctl_get_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 	for (i = 0; i < ARRAY_SIZE(regs->gpr); i++)
 		regs->gpr[i] = kvmppc_get_gpr(vcpu, i);
 
-	vcpu_put(vcpu);
-
 	return 0;
 }
 
@@ -516,8 +512,6 @@ int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 {
 	int i;
 
-	vcpu_load(vcpu);
-
 	vcpu->arch.pc = regs->pc;
 	kvmppc_set_cr(vcpu, regs->cr);
 	vcpu->arch.ctr = regs->ctr;
@@ -537,8 +531,6 @@ int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 	for (i = 0; i < ARRAY_SIZE(regs->gpr); i++)
 		kvmppc_set_gpr(vcpu, i, regs->gpr[i]);
 
-	vcpu_put(vcpu);
-
 	return 0;
 }
 
@@ -569,9 +561,7 @@ int kvm_arch_vcpu_ioctl_translate(struct kvm_vcpu *vcpu,
 {
 	int r;
 
-	vcpu_load(vcpu);
 	r = kvmppc_core_vcpu_translate(vcpu, tr);
-	vcpu_put(vcpu);
 	return r;
 }
 
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 9b8683f..e0fae7a 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -423,8 +423,6 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	int r;
 	sigset_t sigsaved;
 
-	vcpu_load(vcpu);
-
 	if (vcpu->sigset_active)
 		sigprocmask(SIG_SETMASK, &vcpu->sigset, &sigsaved);
 
@@ -456,8 +454,6 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	if (vcpu->sigset_active)
 		sigprocmask(SIG_SETMASK, &sigsaved, NULL);
 
-	vcpu_put(vcpu);
-
 	return r;
 }
 
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 8093e6f..e80f55e 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -371,55 +371,43 @@ static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
 
 int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 {
-	vcpu_load(vcpu);
 	memcpy(&vcpu->arch.guest_gprs, &regs->gprs, sizeof(regs->gprs));
-	vcpu_put(vcpu);
 	return 0;
 }
 
 int kvm_arch_vcpu_ioctl_get_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 {
-	vcpu_load(vcpu);
 	memcpy(&regs->gprs, &vcpu->arch.guest_gprs, sizeof(regs->gprs));
-	vcpu_put(vcpu);
 	return 0;
 }
 
 int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
 				  struct kvm_sregs *sregs)
 {
-	vcpu_load(vcpu);
 	memcpy(&vcpu->arch.guest_acrs, &sregs->acrs, sizeof(sregs->acrs));
 	memcpy(&vcpu->arch.sie_block->gcr, &sregs->crs, sizeof(sregs->crs));
-	vcpu_put(vcpu);
 	return 0;
 }
 
 int kvm_arch_vcpu_ioctl_get_sregs(struct kvm_vcpu *vcpu,
 				  struct kvm_sregs *sregs)
 {
-	vcpu_load(vcpu);
 	memcpy(&sregs->acrs, &vcpu->arch.guest_acrs, sizeof(sregs->acrs));
 	memcpy(&sregs->crs, &vcpu->arch.sie_block->gcr, sizeof(sregs->crs));
-	vcpu_put(vcpu);
 	return 0;
 }
 
 int kvm_arch_vcpu_ioctl_set_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu)
 {
-	vcpu_load(vcpu);
 	memcpy(&vcpu->arch.guest_fpregs.fprs, &fpu->fprs, sizeof(fpu->fprs));
 	vcpu->arch.guest_fpregs.fpc = fpu->fpc;
-	vcpu_put(vcpu);
 	return 0;
 }
 
 int kvm_arch_vcpu_ioctl_get_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu)
 {
-	vcpu_load(vcpu);
 	memcpy(&fpu->fprs, &vcpu->arch.guest_fpregs.fprs, sizeof(fpu->fprs));
 	fpu->fpc = vcpu->arch.guest_fpregs.fpc;
-	vcpu_put(vcpu);
 	return 0;
 }
 
@@ -498,8 +486,6 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 	int rc;
 	sigset_t sigsaved;
 
-	vcpu_load(vcpu);
-
 rerun_vcpu:
 	if (vcpu->requests)
 		if (test_and_clear_bit(KVM_REQ_MMU_RELOAD, &vcpu->requests))
@@ -568,8 +554,6 @@ rerun_vcpu:
 	if (vcpu->sigset_active)
 		sigprocmask(SIG_SETMASK, &sigsaved, NULL);
 
-	vcpu_put(vcpu);
-
 	vcpu->stat.exit_userspace++;
 	return rc;
 }
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index f54ec24..eedb23b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4765,8 +4765,6 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 	int r;
 	sigset_t sigsaved;
 
-	vcpu_load(vcpu);
-
 	if (vcpu->sigset_active)
 		sigprocmask(SIG_SETMASK, &vcpu->sigset, &sigsaved);
 
@@ -4807,14 +4805,11 @@ out:
 	if (vcpu->sigset_active)
 		sigprocmask(SIG_SETMASK, &sigsaved, NULL);
 
-	vcpu_put(vcpu);
 	return r;
 }
 
 int kvm_arch_vcpu_ioctl_get_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 {
-	vcpu_load(vcpu);
-
 	regs->rax = kvm_register_read(vcpu, VCPU_REGS_RAX);
 	regs->rbx = kvm_register_read(vcpu, VCPU_REGS_RBX);
 	regs->rcx = kvm_register_read(vcpu, VCPU_REGS_RCX);
@@ -4837,15 +4832,11 @@ int kvm_arch_vcpu_ioctl_get_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 	regs->rip = kvm_rip_read(vcpu);
 	regs->rflags = kvm_get_rflags(vcpu);
 
-	vcpu_put(vcpu);
-
 	return 0;
 }
 
 int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 {
-	vcpu_load(vcpu);
-
 	kvm_register_write(vcpu, VCPU_REGS_RAX, regs->rax);
 	kvm_register_write(vcpu, VCPU_REGS_RBX, regs->rbx);
 	kvm_register_write(vcpu, VCPU_REGS_RCX, regs->rcx);
@@ -4870,8 +4861,6 @@ int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 
 	vcpu->arch.exception.pending = false;
 
-	vcpu_put(vcpu);
-
 	return 0;
 }
 
@@ -4890,8 +4879,6 @@ int kvm_arch_vcpu_ioctl_get_sregs(struct kvm_vcpu *vcpu,
 {
 	struct desc_ptr dt;
 
-	vcpu_load(vcpu);
-
 	kvm_get_segment(vcpu, &sregs->cs, VCPU_SREG_CS);
 	kvm_get_segment(vcpu, &sregs->ds, VCPU_SREG_DS);
 	kvm_get_segment(vcpu, &sregs->es, VCPU_SREG_ES);
@@ -4923,8 +4910,6 @@ int kvm_arch_vcpu_ioctl_get_sregs(struct kvm_vcpu *vcpu,
 		set_bit(vcpu->arch.interrupt.nr,
 			(unsigned long *)sregs->interrupt_bitmap);
 
-	vcpu_put(vcpu);
-
 	return 0;
 }
 
@@ -4988,8 +4973,6 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
 	int pending_vec, max_bits;
 	struct desc_ptr dt;
 
-	vcpu_load(vcpu);
-
 	dt.size = sregs->idt.limit;
 	dt.address = sregs->idt.base;
 	kvm_x86_ops->set_idt(vcpu, &dt);
@@ -5049,8 +5032,6 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
 	    !is_protmode(vcpu))
 		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
 
-	vcpu_put(vcpu);
-
 	return 0;
 }
 
@@ -5060,12 +5041,10 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
 	unsigned long rflags;
 	int i, r;
 
-	vcpu_load(vcpu);
-
 	if (dbg->control & (KVM_GUESTDBG_INJECT_DB | KVM_GUESTDBG_INJECT_BP)) {
 		r = -EBUSY;
 		if (vcpu->arch.exception.pending)
-			goto unlock_out;
+			goto out;
 		if (dbg->control & KVM_GUESTDBG_INJECT_DB)
 			kvm_queue_exception(vcpu, DB_VECTOR);
 		else
@@ -5107,8 +5086,7 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
 
 	r = 0;
 
-unlock_out:
-	vcpu_put(vcpu);
+out:
 
 	return r;
 }
@@ -5144,7 +5122,6 @@ int kvm_arch_vcpu_ioctl_translate(struct kvm_vcpu *vcpu,
 	gpa_t gpa;
 	int idx;
 
-	vcpu_load(vcpu);
 	idx = srcu_read_lock(&vcpu->kvm->srcu);
 	gpa = kvm_mmu_gva_to_gpa_system(vcpu, vaddr, NULL);
 	srcu_read_unlock(&vcpu->kvm->srcu, idx);
@@ -5152,7 +5129,6 @@ int kvm_arch_vcpu_ioctl_translate(struct kvm_vcpu *vcpu,
 	tr->valid = gpa != UNMAPPED_GVA;
 	tr->writeable = 1;
 	tr->usermode = 0;
-	vcpu_put(vcpu);
 
 	return 0;
 }
@@ -5161,8 +5137,6 @@ int kvm_arch_vcpu_ioctl_get_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu)
 {
 	struct fxsave *fxsave = (struct fxsave *)&vcpu->arch.guest_fx_image;
 
-	vcpu_load(vcpu);
-
 	memcpy(fpu->fpr, fxsave->st_space, 128);
 	fpu->fcw = fxsave->cwd;
 	fpu->fsw = fxsave->swd;
@@ -5172,8 +5146,6 @@ int kvm_arch_vcpu_ioctl_get_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu)
 	fpu->last_dp = fxsave->rdp;
 	memcpy(fpu->xmm, fxsave->xmm_space, sizeof fxsave->xmm_space);
 
-	vcpu_put(vcpu);
-
 	return 0;
 }
 
@@ -5181,8 +5153,6 @@ int kvm_arch_vcpu_ioctl_set_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu)
 {
 	struct fxsave *fxsave = (struct fxsave *)&vcpu->arch.guest_fx_image;
 
-	vcpu_load(vcpu);
-
 	memcpy(fxsave->st_space, fpu->fpr, 128);
 	fxsave->cwd = fpu->fcw;
 	fxsave->swd = fpu->fsw;
@@ -5192,8 +5162,6 @@ int kvm_arch_vcpu_ioctl_set_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu)
 	fxsave->rdp = fpu->last_dp;
 	memcpy(fxsave->xmm_space, fpu->xmm, sizeof fxsave->xmm_space);
 
-	vcpu_put(vcpu);
-
 	return 0;
 }
 
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index f032806..08b2ccd 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1378,6 +1378,18 @@ static long kvm_vcpu_ioctl(struct file *filp,
 
 	if (vcpu->kvm->mm != current->mm)
 		return -EIO;
+
+#if defined(CONFIG_S390) || defined(CONFIG_PPC)
+	/*
+	 * Special cases: vcpu ioctls that are asynchronous to vcpu execution,
+	 * so vcpu_load() would break it.
+	 */
+	if (ioctl == KVM_S390_INTERRUPT || ioctl == KVM_INTERRUPT)
+		return kvm_arch_vcpu_ioctl(filp, ioctl, arg);
+#endif
+
+
+	vcpu_load(vcpu);
 	switch (ioctl) {
 	case KVM_RUN:
 		r = -EINVAL;
@@ -1552,9 +1564,12 @@ out_free2:
 		break;
 	}
 	default:
+		vcpu_put(vcpu);
 		r = kvm_arch_vcpu_ioctl(filp, ioctl, arg);
+		vcpu_load(vcpu);
 	}
 out:
+	vcpu_put(vcpu);
 	kfree(fpu);
 	kfree(kvm_sregs);
 	return r;
-- 
1.7.0.4

Re: [PATCH 3/7] KVM: move vcpu locking to dispatcher for generic vcpu ioctls

[Marcelo Tosatti]

On Thu, May 13, 2010 at 02:17:35PM +0300, Avi Kivity wrote:
> All vcpu ioctls need to be locked, so instead of locking each one specifically
> we lock at the generic dispatcher.
> 
> This patch only updates generic ioctls and leaves arch specific ioctls alone.
> 
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  arch/ia64/kvm/kvm-ia64.c   |   11 -----------
>  arch/powerpc/kvm/book3s.c  |   16 ----------------
>  arch/powerpc/kvm/booke.c   |   10 ----------
>  arch/powerpc/kvm/powerpc.c |    4 ----
>  arch/s390/kvm/kvm-s390.c   |   16 ----------------
>  arch/x86/kvm/x86.c         |   36 ++----------------------------------
>  virt/kvm/kvm_main.c        |   15 +++++++++++++++
>  7 files changed, 17 insertions(+), 91 deletions(-)
> 

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index f54ec24..eedb23b 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4890,8 +4879,6 @@ int kvm_arch_vcpu_ioctl_get_sregs(struct kvm_vcpu *vcpu,
>  {
>  	struct desc_ptr dt;
>  
> -	vcpu_load(vcpu);
> -
>  	kvm_get_segment(vcpu, &sregs->cs, VCPU_SREG_CS);
>  	kvm_get_segment(vcpu, &sregs->ds, VCPU_SREG_DS);
>  	kvm_get_segment(vcpu, &sregs->es, VCPU_SREG_ES);
> @@ -4923,8 +4910,6 @@ int kvm_arch_vcpu_ioctl_get_sregs(struct kvm_vcpu *vcpu,
>  		set_bit(vcpu->arch.interrupt.nr,
>  			(unsigned long *)sregs->interrupt_bitmap);
>  
> -	vcpu_put(vcpu);
> -
>  	return 0;
>  }

Forgot mp_state get/set.

Re: [PATCH 3/7] KVM: move vcpu locking to dispatcher for generic vcpu ioctls

[Avi Kivity]

On 05/15/2010 03:03 AM, Marcelo Tosatti wrote:
> On Thu, May 13, 2010 at 02:17:35PM +0300, Avi Kivity wrote:
>    
>> All vcpu ioctls need to be locked, so instead of locking each one specifically
>> we lock at the generic dispatcher.
>>
>> This patch only updates generic ioctls and leaves arch specific ioctls alone.
>>      
> Forgot mp_state get/set.
>
>    

Right, fixed and applied.

-- 
error compiling committee.c: too many arguments to function

[PATCH 2/7] KVM: x86: Add missing locking to arch specific vcpu ioctls

[Avi Kivity]

Signed-off-by: Avi Kivity <avi-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/x86/kvm/x86.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 4b1433f..f54ec24 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1844,6 +1844,7 @@ static int kvm_vcpu_ioctl_get_cpuid2(struct kvm_vcpu *vcpu,
 {
 	int r;
 
+	vcpu_load(vcpu);
 	r = -E2BIG;
 	if (cpuid->nent < vcpu->arch.cpuid_nent)
 		goto out;
@@ -1855,6 +1856,7 @@ static int kvm_vcpu_ioctl_get_cpuid2(struct kvm_vcpu *vcpu,
 
 out:
 	cpuid->nent = vcpu->arch.cpuid_nent;
+	vcpu_put(vcpu);
 	return r;
 }
 
@@ -2145,6 +2147,7 @@ static int kvm_vcpu_ioctl_x86_setup_mce(struct kvm_vcpu *vcpu,
 	int r;
 	unsigned bank_num = mcg_cap & 0xff, bank;
 
+	vcpu_load(vcpu);
 	r = -EINVAL;
 	if (!bank_num || bank_num >= KVM_MAX_MCE_BANKS)
 		goto out;
@@ -2159,6 +2162,7 @@ static int kvm_vcpu_ioctl_x86_setup_mce(struct kvm_vcpu *vcpu,
 	for (bank = 0; bank < bank_num; bank++)
 		vcpu->arch.mce_banks[bank*4] = ~(u64)0;
 out:
+	vcpu_put(vcpu);
 	return r;
 }
 
@@ -2467,7 +2471,9 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		r = -EFAULT;
 		if (copy_from_user(&mce, argp, sizeof mce))
 			goto out;
+		vcpu_load(vcpu);
 		r = kvm_vcpu_ioctl_x86_set_mce(vcpu, &mce);
+		vcpu_put(vcpu);
 		break;
 	}
 	case KVM_GET_VCPU_EVENTS: {
-- 
1.7.0.4

[PATCH 4/7] KVM: x86: Lock arch specific vcpu ioctls centrally

[Avi Kivity]

Signed-off-by: Avi Kivity <avi-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/x86/kvm/x86.c |   41 ++---------------------------------------
 1 files changed, 2 insertions(+), 39 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index eedb23b..8b9e5ec 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1531,16 +1531,12 @@ static int __msr_io(struct kvm_vcpu *vcpu, struct kvm_msrs *msrs,
 {
 	int i, idx;
 
-	vcpu_load(vcpu);
-
 	idx = srcu_read_lock(&vcpu->kvm->srcu);
 	for (i = 0; i < msrs->nmsrs; ++i)
 		if (do_msr(vcpu, entries[i].index, &entries[i].data))
 			break;
 	srcu_read_unlock(&vcpu->kvm->srcu, idx);
 
-	vcpu_put(vcpu);
-
 	return i;
 }
 
@@ -1788,7 +1784,6 @@ static int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
 	if (copy_from_user(cpuid_entries, entries,
 			   cpuid->nent * sizeof(struct kvm_cpuid_entry)))
 		goto out_free;
-	vcpu_load(vcpu);
 	for (i = 0; i < cpuid->nent; i++) {
 		vcpu->arch.cpuid_entries[i].function = cpuid_entries[i].function;
 		vcpu->arch.cpuid_entries[i].eax = cpuid_entries[i].eax;
@@ -1806,7 +1801,6 @@ static int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
 	r = 0;
 	kvm_apic_set_version(vcpu);
 	kvm_x86_ops->cpuid_update(vcpu);
-	vcpu_put(vcpu);
 
 out_free:
 	vfree(cpuid_entries);
@@ -1827,11 +1821,9 @@ static int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
 	if (copy_from_user(&vcpu->arch.cpuid_entries, entries,
 			   cpuid->nent * sizeof(struct kvm_cpuid_entry2)))
 		goto out;
-	vcpu_load(vcpu);
 	vcpu->arch.cpuid_nent = cpuid->nent;
 	kvm_apic_set_version(vcpu);
 	kvm_x86_ops->cpuid_update(vcpu);
-	vcpu_put(vcpu);
 	return 0;
 
 out:
@@ -1844,7 +1836,6 @@ static int kvm_vcpu_ioctl_get_cpuid2(struct kvm_vcpu *vcpu,
 {
 	int r;
 
-	vcpu_load(vcpu);
 	r = -E2BIG;
 	if (cpuid->nent < vcpu->arch.cpuid_nent)
 		goto out;
@@ -1856,7 +1847,6 @@ static int kvm_vcpu_ioctl_get_cpuid2(struct kvm_vcpu *vcpu,
 
 out:
 	cpuid->nent = vcpu->arch.cpuid_nent;
-	vcpu_put(vcpu);
 	return r;
 }
 
@@ -2088,9 +2078,7 @@ out:
 static int kvm_vcpu_ioctl_get_lapic(struct kvm_vcpu *vcpu,
 				    struct kvm_lapic_state *s)
 {
-	vcpu_load(vcpu);
 	memcpy(s->regs, vcpu->arch.apic->regs, sizeof *s);
-	vcpu_put(vcpu);
 
 	return 0;
 }
@@ -2098,11 +2086,9 @@ static int kvm_vcpu_ioctl_get_lapic(struct kvm_vcpu *vcpu,
 static int kvm_vcpu_ioctl_set_lapic(struct kvm_vcpu *vcpu,
 				    struct kvm_lapic_state *s)
 {
-	vcpu_load(vcpu);
 	memcpy(vcpu->arch.apic->regs, s->regs, sizeof *s);
 	kvm_apic_post_state_restore(vcpu);
 	update_cr8_intercept(vcpu);
-	vcpu_put(vcpu);
 
 	return 0;
 }
@@ -2114,20 +2100,15 @@ static int kvm_vcpu_ioctl_interrupt(struct kvm_vcpu *vcpu,
 		return -EINVAL;
 	if (irqchip_in_kernel(vcpu->kvm))
 		return -ENXIO;
-	vcpu_load(vcpu);
 
 	kvm_queue_interrupt(vcpu, irq->irq, false);
 
-	vcpu_put(vcpu);
-
 	return 0;
 }
 
 static int kvm_vcpu_ioctl_nmi(struct kvm_vcpu *vcpu)
 {
-	vcpu_load(vcpu);
 	kvm_inject_nmi(vcpu);
-	vcpu_put(vcpu);
 
 	return 0;
 }
@@ -2147,7 +2128,6 @@ static int kvm_vcpu_ioctl_x86_setup_mce(struct kvm_vcpu *vcpu,
 	int r;
 	unsigned bank_num = mcg_cap & 0xff, bank;
 
-	vcpu_load(vcpu);
 	r = -EINVAL;
 	if (!bank_num || bank_num >= KVM_MAX_MCE_BANKS)
 		goto out;
@@ -2162,7 +2142,6 @@ static int kvm_vcpu_ioctl_x86_setup_mce(struct kvm_vcpu *vcpu,
 	for (bank = 0; bank < bank_num; bank++)
 		vcpu->arch.mce_banks[bank*4] = ~(u64)0;
 out:
-	vcpu_put(vcpu);
 	return r;
 }
 
@@ -2220,8 +2199,6 @@ static int kvm_vcpu_ioctl_x86_set_mce(struct kvm_vcpu *vcpu,
 static void kvm_vcpu_ioctl_x86_get_vcpu_events(struct kvm_vcpu *vcpu,
 					       struct kvm_vcpu_events *events)
 {
-	vcpu_load(vcpu);
-
 	events->exception.injected =
 		vcpu->arch.exception.pending &&
 		!kvm_exception_is_soft(vcpu->arch.exception.nr);
@@ -2246,8 +2223,6 @@ static void kvm_vcpu_ioctl_x86_get_vcpu_events(struct kvm_vcpu *vcpu,
 	events->flags = (KVM_VCPUEVENT_VALID_NMI_PENDING
 			 | KVM_VCPUEVENT_VALID_SIPI_VECTOR
 			 | KVM_VCPUEVENT_VALID_SHADOW);
-
-	vcpu_put(vcpu);
 }
 
 static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
@@ -2258,8 +2233,6 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
 			      | KVM_VCPUEVENT_VALID_SHADOW))
 		return -EINVAL;
 
-	vcpu_load(vcpu);
-
 	vcpu->arch.exception.pending = events->exception.injected;
 	vcpu->arch.exception.nr = events->exception.nr;
 	vcpu->arch.exception.has_error_code = events->exception.has_error_code;
@@ -2282,22 +2255,16 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
 	if (events->flags & KVM_VCPUEVENT_VALID_SIPI_VECTOR)
 		vcpu->arch.sipi_vector = events->sipi_vector;
 
-	vcpu_put(vcpu);
-
 	return 0;
 }
 
 static void kvm_vcpu_ioctl_x86_get_debugregs(struct kvm_vcpu *vcpu,
 					     struct kvm_debugregs *dbgregs)
 {
-	vcpu_load(vcpu);
-
 	memcpy(dbgregs->db, vcpu->arch.db, sizeof(vcpu->arch.db));
 	dbgregs->dr6 = vcpu->arch.dr6;
 	dbgregs->dr7 = vcpu->arch.dr7;
 	dbgregs->flags = 0;
-
-	vcpu_put(vcpu);
 }
 
 static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,
@@ -2306,14 +2273,10 @@ static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,
 	if (dbgregs->flags)
 		return -EINVAL;
 
-	vcpu_load(vcpu);
-
 	memcpy(vcpu->arch.db, dbgregs->db, sizeof(vcpu->arch.db));
 	vcpu->arch.dr6 = dbgregs->dr6;
 	vcpu->arch.dr7 = dbgregs->dr7;
 
-	vcpu_put(vcpu);
-
 	return 0;
 }
 
@@ -2325,6 +2288,7 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 	int r;
 	struct kvm_lapic_state *lapic = NULL;
 
+	vcpu_load(vcpu);
 	switch (ioctl) {
 	case KVM_GET_LAPIC: {
 		r = -EINVAL;
@@ -2471,9 +2435,7 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		r = -EFAULT;
 		if (copy_from_user(&mce, argp, sizeof mce))
 			goto out;
-		vcpu_load(vcpu);
 		r = kvm_vcpu_ioctl_x86_set_mce(vcpu, &mce);
-		vcpu_put(vcpu);
 		break;
 	}
 	case KVM_GET_VCPU_EVENTS: {
@@ -2524,6 +2486,7 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		r = -EINVAL;
 	}
 out:
+	vcpu_put(vcpu);
 	kfree(lapic);
 	return r;
 }
-- 
1.7.0.4

[PATCH 5/7] KVM: s390: Centrally lock arch specific vcpu ioctls

[Avi Kivity]

Signed-off-by: Avi Kivity <avi-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/s390/kvm/kvm-s390.c |   40 +++++++++++++++++-----------------------
 1 files changed, 17 insertions(+), 23 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index e80f55e..28cd8fd 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -363,9 +363,7 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
 
 static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
 {
-	vcpu_load(vcpu);
 	kvm_s390_vcpu_initial_reset(vcpu);
-	vcpu_put(vcpu);
 	return 0;
 }
 
@@ -415,14 +413,12 @@ static int kvm_arch_vcpu_ioctl_set_initial_psw(struct kvm_vcpu *vcpu, psw_t psw)
 {
 	int rc = 0;
 
-	vcpu_load(vcpu);
 	if (atomic_read(&vcpu->arch.sie_block->cpuflags) & CPUSTAT_RUNNING)
 		rc = -EBUSY;
 	else {
 		vcpu->run->psw_mask = psw.mask;
 		vcpu->run->psw_addr = psw.addr;
 	}
-	vcpu_put(vcpu);
 	return rc;
 }
 
@@ -573,7 +569,7 @@ static int __guestcopy(struct kvm_vcpu *vcpu, u64 guestdest, const void *from,
  * KVM_S390_STORE_STATUS_NOADDR: -> 0x1200 on 64 bit
  * KVM_S390_STORE_STATUS_PREFIXED: -> prefix
  */
-int __kvm_s390_vcpu_store_status(struct kvm_vcpu *vcpu, unsigned long addr)
+static int kvm_s390_vcpu_store_status(struct kvm_vcpu *vcpu, unsigned long addr)
 {
 	const unsigned char archmode = 1;
 	int prefix;
@@ -635,45 +631,43 @@ int __kvm_s390_vcpu_store_status(struct kvm_vcpu *vcpu, unsigned long addr)
 	return 0;
 }
 
-static int kvm_s390_vcpu_store_status(struct kvm_vcpu *vcpu, unsigned long addr)
-{
-	int rc;
-
-	vcpu_load(vcpu);
-	rc = __kvm_s390_vcpu_store_status(vcpu, addr);
-	vcpu_put(vcpu);
-	return rc;
-}
-
 long kvm_arch_vcpu_ioctl(struct file *filp,
 			 unsigned int ioctl, unsigned long arg)
 {
 	struct kvm_vcpu *vcpu = filp->private_data;
 	void __user *argp = (void __user *)arg;
+	long r;
 
-	switch (ioctl) {
-	case KVM_S390_INTERRUPT: {
+	if (ioctl == KVM_S390_INTERRUPT) {
 		struct kvm_s390_interrupt s390int;
 
 		if (copy_from_user(&s390int, argp, sizeof(s390int)))
 			return -EFAULT;
 		return kvm_s390_inject_vcpu(vcpu, &s390int);
 	}
+
+	vcpu_load(vcpu);
+	switch (ioctl) {
 	case KVM_S390_STORE_STATUS:
-		return kvm_s390_vcpu_store_status(vcpu, arg);
+		r = kvm_s390_vcpu_store_status(vcpu, arg);
+		break;
 	case KVM_S390_SET_INITIAL_PSW: {
 		psw_t psw;
 
+		r = -EFAULT;
 		if (copy_from_user(&psw, argp, sizeof(psw)))
-			return -EFAULT;
-		return kvm_arch_vcpu_ioctl_set_initial_psw(vcpu, psw);
+			break;
+		r = kvm_arch_vcpu_ioctl_set_initial_psw(vcpu, psw);
+		break;
 	}
 	case KVM_S390_INITIAL_RESET:
-		return kvm_arch_vcpu_ioctl_initial_reset(vcpu);
+		r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
+		break;
 	default:
-		;
+		r = -EINVAL;
 	}
-	return -EINVAL;
+	vcpu_put(vcpu);
+	return r;
 }
 
 /* Section: memory related */
-- 
1.7.0.4

[PATCH 6/7] KVM: PPC: Centralize locking of arch specific vcpu ioctls

[Avi Kivity]

Signed-off-by: Avi Kivity <avi-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/powerpc/kvm/powerpc.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index e0fae7a..caeed7b 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -512,15 +512,17 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 	void __user *argp = (void __user *)arg;
 	long r;
 
-	switch (ioctl) {
-	case KVM_INTERRUPT: {
+	if (ioctl == KVM_INTERRUPT) {
 		struct kvm_interrupt irq;
 		r = -EFAULT;
 		if (copy_from_user(&irq, argp, sizeof(irq)))
-			goto out;
+			goto out_nolock;
 		r = kvm_vcpu_ioctl_interrupt(vcpu, &irq);
-		break;
+		goto out_nolock;
 	}
+
+	vcpu_load(vcpu);
+	switch (ioctl) {
 	case KVM_ENABLE_CAP:
 	{
 		struct kvm_enable_cap cap;
@@ -535,6 +537,8 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 	}
 
 out:
+	vcpu_put(vcpu);
+out_nolock:
 	return r;
 }
 
-- 
1.7.0.4

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Alexander Graf]

On 13.05.2010, at 13:17, Avi Kivity wrote:

> 
> 
> Avi Kivity (7):
>  KVM: PPC: Add missing vcpu_load()/vcpu_put() in vcpu ioctls
>  KVM: x86: Add missing locking to arch specific vcpu ioctls
>  KVM: move vcpu locking to dispatcher for generic vcpu ioctls
>  KVM: x86: Lock arch specific vcpu ioctls centrally
>  KVM: s390: Centrally lock arch specific vcpu ioctls
>  KVM: PPC: Centralize locking of arch specific vcpu ioctls
>  KVM: Consolidate arch specific vcpu ioctl locking

Mind to give a high level overview on where you're moving which locks?

Alex

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Avi Kivity]

On 05/13/2010 02:57 PM, Alexander Graf wrote:
>
> Mind to give a high level overview on where you're moving which locks?
>
>    

Um, looks like I forgot to fill in the patchset header.  Sorry.

The patches move all vcpu ioctl locking from the individual ioctl 
handlers (e.g. kvm_vcpu_ioctl_set_cpuid()) to the top-level vcpu ioctl 
handler (kvm_vcpu_ioctl()).  So tons of vcpu_load()/vcpu_put() pairs 
(some of the missing) get replaced by a single pair.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Avi Kivity]

On 05/13/2010 03:01 PM, Avi Kivity wrote:
> On 05/13/2010 02:57 PM, Alexander Graf wrote:
>>
>> Mind to give a high level overview on where you're moving which locks?
>>
>
> Um, looks like I forgot to fill in the patchset header.  Sorry.

Gar, I actually wrote it but forgot to save the file.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Avi Kivity]

On 05/13/2010 03:03 PM, Avi Kivity wrote:
> On 05/13/2010 03:01 PM, Avi Kivity wrote:
>> On 05/13/2010 02:57 PM, Alexander Graf wrote:
>>>
>>> Mind to give a high level overview on where you're moving which locks?
>>>
>>
>> Um, looks like I forgot to fill in the patchset header.  Sorry.
>
> Gar, I actually wrote it but forgot to save the file.
>

And it had some useful info:

[PATCH 0/7] Consolidate vcpu ioctl locking

In general, all vcpu ioctls need to take the vcpu mutex, but each one 
does it
(or not) individually.  This is cumbersome and error prone.

This patchset moves all locking to a central place.  This is complicated
by the fact that ppc's KVM_INTERRUPT and s390's KVM_S390_INTERRUPT break
the convention and need to run unlocked.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Alexander Graf]

On 13.05.2010, at 14:03, Avi Kivity wrote:

> On 05/13/2010 03:03 PM, Avi Kivity wrote:
>> On 05/13/2010 03:01 PM, Avi Kivity wrote:
>>> On 05/13/2010 02:57 PM, Alexander Graf wrote:
>>>> 
>>>> Mind to give a high level overview on where you're moving which locks?
>>>> 
>>> 
>>> Um, looks like I forgot to fill in the patchset header.  Sorry.
>> 
>> Gar, I actually wrote it but forgot to save the file.
>> 
> 
> And it had some useful info:
> 
> [PATCH 0/7] Consolidate vcpu ioctl locking
> 
> In general, all vcpu ioctls need to take the vcpu mutex, but each one does it
> (or not) individually.  This is cumbersome and error prone.
> 
> This patchset moves all locking to a central place.  This is complicated
> by the fact that ppc's KVM_INTERRUPT and s390's KVM_S390_INTERRUPT break
> the convention and need to run unlocked.

Why is the x86 non-kernel-pic path different?

Alex

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Avi Kivity]

On 05/13/2010 03:18 PM, Alexander Graf wrote:
>
>> [PATCH 0/7] Consolidate vcpu ioctl locking
>>
>> In general, all vcpu ioctls need to take the vcpu mutex, but each one does it
>> (or not) individually.  This is cumbersome and error prone.
>>
>> This patchset moves all locking to a central place.  This is complicated
>> by the fact that ppc's KVM_INTERRUPT and s390's KVM_S390_INTERRUPT break
>> the convention and need to run unlocked.
>>      
> Why is the x86 non-kernel-pic path different?
>    

Userspace issues the ioctl from a vcpu thread.

It has to, btw, since whether an interrupt can be injected or not 
depends on vcpu-synchronous registers: eflags.if and tpr/cr8.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Alexander Graf]

Am 13.05.2010 um 14:29 schrieb Avi Kivity <avi@redhat.com>:

> On 05/13/2010 03:18 PM, Alexander Graf wrote:
>>
>>> [PATCH 0/7] Consolidate vcpu ioctl locking
>>>
>>> In general, all vcpu ioctls need to take the vcpu mutex, but each  
>>> one does it
>>> (or not) individually.  This is cumbersome and error prone.
>>>
>>> This patchset moves all locking to a central place.  This is  
>>> complicated
>>> by the fact that ppc's KVM_INTERRUPT and s390's KVM_S390_INTERRUPT  
>>> break
>>> the convention and need to run unlocked.
>>>
>> Why is the x86 non-kernel-pic path different?
>>
>
> Userspace issues the ioctl from a vcpu thread.
>
> It has to, btw, since whether an interrupt can be injected or not  
> depends on vcpu-synchronous registers: eflags.if and tpr/cr8.

On ppc we don't have a tpr, but eflags.if is basically the same as  
msr.ee.

The major difference apparently is that on ppc we KVM_INTERRUPT pulls  
the interrupt line. On vcpu_run we then check whether msr.ee is set  
and if so, trigger the interrupt.

I wonder why we don't do the same for x86. The current limitation on  
userspace checking eflags and the tpr seems cumbersome.

Alex

>

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Avi Kivity]

On 05/13/2010 10:49 PM, Alexander Graf wrote:
>
> Am 13.05.2010 um 14:29 schrieb Avi Kivity <avi-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>
>> On 05/13/2010 03:18 PM, Alexander Graf wrote:
>>>
>>>> [PATCH 0/7] Consolidate vcpu ioctl locking
>>>>
>>>> In general, all vcpu ioctls need to take the vcpu mutex, but each 
>>>> one does it
>>>> (or not) individually.  This is cumbersome and error prone.
>>>>
>>>> This patchset moves all locking to a central place.  This is 
>>>> complicated
>>>> by the fact that ppc's KVM_INTERRUPT and s390's KVM_S390_INTERRUPT 
>>>> break
>>>> the convention and need to run unlocked.
>>>>
>>> Why is the x86 non-kernel-pic path different?
>>>
>>
>> Userspace issues the ioctl from a vcpu thread.
>>
>> It has to, btw, since whether an interrupt can be injected or not 
>> depends on vcpu-synchronous registers: eflags.if and tpr/cr8.
>
> On ppc we don't have a tpr, but eflags.if is basically the same as 
> msr.ee.
>
> The major difference apparently is that on ppc we KVM_INTERRUPT pulls 
> the interrupt line. On vcpu_run we then check whether msr.ee is set 
> and if so, trigger the interrupt.
>
> I wonder why we don't do the same for x86. The current limitation on 
> userspace checking eflags and the tpr seems cumbersome.

On x86 eflags.if is freely changeable by the guest, so if we want to 
queue an interrupt we have to IPI the vcpu to force it out of guest 
mode, so we can inspect eflags.  This means the vcpu thread has to be 
interrupted one way or another.

The tpr (really ppr) is even more problematic as it is maintained in 
userspace, not in the kernel (for non-kernel-irqchip).  It could in 
theory be inspected by another thread, but we wouldn't gain anything by 
it due to the requirement to IPI.

> void kvmppc_book3s_queue_irqprio(struct kvm_vcpu *vcpu, unsigned int vec)
> {
>     vcpu->stat.queue_intr++;
>
>     set_bit(kvmppc_book3s_vec2irqprio(vec),
> &vcpu->arch.pending_exceptions);
> #ifdef EXIT_DEBUG
>     printk(KERN_INFO "Queueing interrupt %x\n", vec);
> #endif
> }

Isn't this missing an IPI if the vcpu is in guest mode?


-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Alexander Graf]

On 15.05.2010, at 08:16, Avi Kivity wrote:

> On 05/13/2010 10:49 PM, Alexander Graf wrote:
>> 
>> Am 13.05.2010 um 14:29 schrieb Avi Kivity <avi-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>> 
>>> On 05/13/2010 03:18 PM, Alexander Graf wrote:
>>>> 
>>>>> [PATCH 0/7] Consolidate vcpu ioctl locking
>>>>> 
>>>>> In general, all vcpu ioctls need to take the vcpu mutex, but each one does it
>>>>> (or not) individually.  This is cumbersome and error prone.
>>>>> 
>>>>> This patchset moves all locking to a central place.  This is complicated
>>>>> by the fact that ppc's KVM_INTERRUPT and s390's KVM_S390_INTERRUPT break
>>>>> the convention and need to run unlocked.
>>>>> 
>>>> Why is the x86 non-kernel-pic path different?
>>>> 
>>> 
>>> Userspace issues the ioctl from a vcpu thread.
>>> 
>>> It has to, btw, since whether an interrupt can be injected or not depends on vcpu-synchronous registers: eflags.if and tpr/cr8.
>> 
>> On ppc we don't have a tpr, but eflags.if is basically the same as msr.ee.
>> 
>> The major difference apparently is that on ppc we KVM_INTERRUPT pulls the interrupt line. On vcpu_run we then check whether msr.ee is set and if so, trigger the interrupt.
>> 
>> I wonder why we don't do the same for x86. The current limitation on userspace checking eflags and the tpr seems cumbersome.
> 
> On x86 eflags.if is freely changeable by the guest, so if we want to queue an interrupt we have to IPI the vcpu to force it out of guest mode, so we can inspect eflags.  This means the vcpu thread has to be interrupted one way or another.
> 
> The tpr (really ppr) is even more problematic as it is maintained in userspace, not in the kernel (for non-kernel-irqchip).  It could in theory be inspected by another thread, but we wouldn't gain anything by it due to the requirement to IPI.

Hrm right. On PPC we trap on every MSR change, so we get notified when interrupts are enabled again. But isn't that what VINTR intercepts are there for? Just add the  lowest active TPR value to the KVM_INTERRUPT ioctl and then wait until the guest is ready to take it.

> 
>> void kvmppc_book3s_queue_irqprio(struct kvm_vcpu *vcpu, unsigned int vec)
>> {
>>    vcpu->stat.queue_intr++;
>> 
>>    set_bit(kvmppc_book3s_vec2irqprio(vec),
>> &vcpu->arch.pending_exceptions);
>> #ifdef EXIT_DEBUG
>>    printk(KERN_INFO "Queueing interrupt %x\n", vec);
>> #endif
>> }
> 
> Isn't this missing an IPI if the vcpu is in guest mode?

Yes, it is :). At least with qemu we're 100% sure we're not in VCPU_RUN when an interrupt gets injected, as the injection happens in kvm_arch_pre_run.


Alex

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Avi Kivity]

On 05/15/2010 09:21 AM, Alexander Graf wrote:
>
>> On x86 eflags.if is freely changeable by the guest, so if we want to queue an interrupt we have to IPI the vcpu to force it out of guest mode, so we can inspect eflags.  This means the vcpu thread has to be interrupted one way or another.
>>
>> The tpr (really ppr) is even more problematic as it is maintained in userspace, not in the kernel (for non-kernel-irqchip).  It could in theory be inspected by another thread, but we wouldn't gain anything by it due to the requirement to IPI.
>>      
> Hrm right. On PPC we trap on every MSR change, so we get notified when interrupts are enabled again. But isn't that what VINTR intercepts are there for? Just add the  lowest active TPR value to the KVM_INTERRUPT ioctl and then wait until the guest is ready to take it.
>    

Yes, and we do that when using in-kernel-irqchip.  With userspace apic, 
the tpr is maintained in userspace and the kernel has no visibility into it.

We could have changed moved the tpr/ppr into the kernel for userspace 
irqchip, but there's not much point now.

>>> void kvmppc_book3s_queue_irqprio(struct kvm_vcpu *vcpu, unsigned int vec)
>>> {
>>>     vcpu->stat.queue_intr++;
>>>
>>>     set_bit(kvmppc_book3s_vec2irqprio(vec),
>>> &vcpu->arch.pending_exceptions);
>>> #ifdef EXIT_DEBUG
>>>     printk(KERN_INFO "Queueing interrupt %x\n", vec);
>>> #endif
>>> }
>>>        
>> Isn't this missing an IPI if the vcpu is in guest mode?
>>      
> Yes, it is :). At least with qemu we're 100% sure we're not in VCPU_RUN when an interrupt gets injected, as the injection happens in kvm_arch_pre_run.
>    

That means you never inject an interrupt from the iothread (or from a 
different vcpu thread)?

If that's the case we might make it part of the API and require the 
ioctl to be issued from the vcpu thread.  We'd still be left with the 
s390 exception.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Alexander Graf]

On 15.05.2010, at 09:59, Avi Kivity wrote:

> On 05/15/2010 09:21 AM, Alexander Graf wrote:
>> 
>>> On x86 eflags.if is freely changeable by the guest, so if we want to queue an interrupt we have to IPI the vcpu to force it out of guest mode, so we can inspect eflags.  This means the vcpu thread has to be interrupted one way or another.
>>> 
>>> The tpr (really ppr) is even more problematic as it is maintained in userspace, not in the kernel (for non-kernel-irqchip).  It could in theory be inspected by another thread, but we wouldn't gain anything by it due to the requirement to IPI.
>>>     
>> Hrm right. On PPC we trap on every MSR change, so we get notified when interrupts are enabled again. But isn't that what VINTR intercepts are there for? Just add the  lowest active TPR value to the KVM_INTERRUPT ioctl and then wait until the guest is ready to take it.
>>   
> 
> Yes, and we do that when using in-kernel-irqchip.  With userspace apic, the tpr is maintained in userspace and the kernel has no visibility into it.
> 
> We could have changed moved the tpr/ppr into the kernel for userspace irqchip, but there's not much point now.
> 
>>>> void kvmppc_book3s_queue_irqprio(struct kvm_vcpu *vcpu, unsigned int vec)
>>>> {
>>>>    vcpu->stat.queue_intr++;
>>>> 
>>>>    set_bit(kvmppc_book3s_vec2irqprio(vec),
>>>> &vcpu->arch.pending_exceptions);
>>>> #ifdef EXIT_DEBUG
>>>>    printk(KERN_INFO "Queueing interrupt %x\n", vec);
>>>> #endif
>>>> }
>>>>       
>>> Isn't this missing an IPI if the vcpu is in guest mode?
>>>     
>> Yes, it is :). At least with qemu we're 100% sure we're not in VCPU_RUN when an interrupt gets injected, as the injection happens in kvm_arch_pre_run.
>>   
> 
> That means you never inject an interrupt from the iothread (or from a different vcpu thread)?
> 
> If that's the case we might make it part of the API and require the ioctl to be issued from the vcpu thread.  We'd still be left with the s390 exception.

Well I'm not sure that's guaranteed for MOL or Dolphin, but I guess the user base is small enough to ignore them.

Either way, I'm actually rather unhappy with the way interrupts work right now. We're only injecting interrupts when in the main loop, which is rare if we did our homework right. So what I'd really like to see is that the MPIC on ppc directly invokes KVM_INTERRUPT to pull (and losen) the interrupt line. That way we can't just accidently miss interrupts.

What happens now with ppc64 guests that have EE lazily disabled is:

* device in userspace wants to trigger an interrupt
* mpic pulls external interrupt line
* kvm_arch_pre_run sees that external interrupt line is pulled, issues ioctl(KVM_INTERRUPT), kernel space sets trigger_interrupt=1
* we enter the guest
* we inject an external interrupt, set trigger_interrupt=0
* guest gets the interrupt, sees it's lazily disabled, sets msr.ee=0
* guest moves on, sets msr.ee=1 again later
*** guest expects the interrupt to trigger again, but we don't know that it's still pending
-> we need to exit to userspace to realize that the interrupt is still active

This is fundamentally broken. What I'd like to see is:

* device in userspace wants to trigger an interrupt
* mpic pulls external interrupt line, automatically issues ioctl(KVM_INTERRUPT)
* we enter the guest
* we inject the external interrupt
* guest gets the interrupt, sees it's lazily disabled, sets msr.ee=0
* guest moves on, sets msr.ee=1 again later
* we inject the external interrupt again, since it's still active
* guest acknowledges the interrupt with the mpic, issues ioctl(KVM_INTERRUPT, disable)
-> all is great


For that to work we need to enable triggering KVM_INTERRUPT from a non-vcpu thread.


On S390, I'm also still sceptical if the implementation we have really works. A device injects an S390_INTERRUPT with its address and on the next vcpu_run, an according interrupt is issued. But what happens if two devices trigger an S390_INTERRUPT before the vcpu_run? We'd have lost an interrupt by then...


Alex

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Avi Kivity]

On 05/15/2010 11:26 AM, Alexander Graf wrote:
>
>> That means you never inject an interrupt from the iothread (or from a different vcpu thread)?
>>
>> If that's the case we might make it part of the API and require the ioctl to be issued from the vcpu thread.  We'd still be left with the s390 exception.
>>      
> Well I'm not sure that's guaranteed for MOL or Dolphin, but I guess the user base is small enough to ignore them.
>
> Either way, I'm actually rather unhappy with the way interrupts work right now. We're only injecting interrupts when in the main loop, which is rare if we did our homework right. So what I'd really like to see is that the MPIC on ppc directly invokes KVM_INTERRUPT to pull (and losen) the interrupt line. That way we can't just accidently miss interrupts.
>    

on x86 we signal the vcpu thread to pull it out of the main loop and 
poll the apic.

> What happens now with ppc64 guests that have EE lazily disabled is:
>
> * device in userspace wants to trigger an interrupt
> * mpic pulls external interrupt line
>    

<signal>

> * kvm_arch_pre_run sees that external interrupt line is pulled, issues ioctl(KVM_INTERRUPT), kernel space sets trigger_interrupt=1
> * we enter the guest
> * we inject an external interrupt, set trigger_interrupt=0
> * guest gets the interrupt, sees it's lazily disabled, sets msr.ee=0
> * guest moves on, sets msr.ee=1 again later
> *** guest expects the interrupt to trigger again, but we don't know that it's still pending
>    

Why does the guest expect the interrupt to trigger again?  Is it level 
triggered until acknowledged?

That's exactly why x86 has run->request_interrupt_window, 
run->ready_for_interrupt_injection, and run->if_flag.

> ->  we need to exit to userspace to realize that the interrupt is still active
>
> This is fundamentally broken. What I'd like to see is:
>
> * device in userspace wants to trigger an interrupt
> * mpic pulls external interrupt line, automatically issues ioctl(KVM_INTERRUPT)
> * we enter the guest
> * we inject the external interrupt
> * guest gets the interrupt, sees it's lazily disabled, sets msr.ee=0
> * guest moves on, sets msr.ee=1 again later
> * we inject the external interrupt again, since it's still active
> * guest acknowledges the interrupt with the mpic, issues ioctl(KVM_INTERRUPT, disable)
> ->  all is great
>    

This is similar to KVM_IRQ_LINE.

> For that to work we need to enable triggering KVM_INTERRUPT from a non-vcpu thread.
>    

KVM_IRQ_LINE is a vm ioctl instead of a vcpu ioctl.

The motivation for the strict 'issue vcpu ioctls from vcpu thread' rule 
is to prepare the way for a syscall (instead of ioctl) API.  Then we 
lost the fd argument, and choosing the vcpu is done by associating it 
with the current task.  That allows us to get rid of vcpu->mutex and 
fget_light() (as well as the ioctl dispatch).

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Alexander Graf]

On 15.05.2010, at 19:30, Avi Kivity wrote:

> On 05/15/2010 11:26 AM, Alexander Graf wrote:
>> 
>>> That means you never inject an interrupt from the iothread (or from a different vcpu thread)?
>>> 
>>> If that's the case we might make it part of the API and require the ioctl to be issued from the vcpu thread.  We'd still be left with the s390 exception.
>>>     
>> Well I'm not sure that's guaranteed for MOL or Dolphin, but I guess the user base is small enough to ignore them.
>> 
>> Either way, I'm actually rather unhappy with the way interrupts work right now. We're only injecting interrupts when in the main loop, which is rare if we did our homework right. So what I'd really like to see is that the MPIC on ppc directly invokes KVM_INTERRUPT to pull (and losen) the interrupt line. That way we can't just accidently miss interrupts.
>>   
> 
> on x86 we signal the vcpu thread to pull it out of the main loop and poll the apic.

Hrm, makes sense. Though it's additional overhead of a task switch. Why take the burden if you don't have to?

> 
>> What happens now with ppc64 guests that have EE lazily disabled is:
>> 
>> * device in userspace wants to trigger an interrupt
>> * mpic pulls external interrupt line
>>   
> 
> <signal>
> 
>> * kvm_arch_pre_run sees that external interrupt line is pulled, issues ioctl(KVM_INTERRUPT), kernel space sets trigger_interrupt=1
>> * we enter the guest
>> * we inject an external interrupt, set trigger_interrupt=0
>> * guest gets the interrupt, sees it's lazily disabled, sets msr.ee=0
>> * guest moves on, sets msr.ee=1 again later
>> *** guest expects the interrupt to trigger again, but we don't know that it's still pending
>>   
> 
> Why does the guest expect the interrupt to trigger again?  Is it level triggered until acknowledged?

IIUC, yes.

> That's exactly why x86 has run->request_interrupt_window, run->ready_for_interrupt_injection, and run->if_flag.

So how does x86 userspace get notified when it has an interrupt pending but couldn't inject it? Without a notification, we delay interrupts by quite some time.

> 
>> ->  we need to exit to userspace to realize that the interrupt is still active
>> 
>> This is fundamentally broken. What I'd like to see is:
>> 
>> * device in userspace wants to trigger an interrupt
>> * mpic pulls external interrupt line, automatically issues ioctl(KVM_INTERRUPT)
>> * we enter the guest
>> * we inject the external interrupt
>> * guest gets the interrupt, sees it's lazily disabled, sets msr.ee=0
>> * guest moves on, sets msr.ee=1 again later
>> * we inject the external interrupt again, since it's still active
>> * guest acknowledges the interrupt with the mpic, issues ioctl(KVM_INTERRUPT, disable)
>> ->  all is great
>>   
> 
> This is similar to KVM_IRQ_LINE.

Well, KVM_IRQ_LINE would be the in-kernel pic, no? That'd mean I'd have to reimplement the mpic in kernel space - which might eventually be a good idea when going for SMP, but I'd first like to see if I can keep the current interrupt injection path efficient.

> 
>> For that to work we need to enable triggering KVM_INTERRUPT from a non-vcpu thread.
>>   
> 
> KVM_IRQ_LINE is a vm ioctl instead of a vcpu ioctl.
> 
> The motivation for the strict 'issue vcpu ioctls from vcpu thread' rule is to prepare the way for a syscall (instead of ioctl) API.  Then we lost the fd argument, and choosing the vcpu is done by associating it with the current task.  That allows us to get rid of vcpu->mutex and fget_light() (as well as the ioctl dispatch).

If we define the API to only work on the current vcpu with a few excetions where we're safe (KVM_INTERRUPT), that'd get rid of the mutex too, no?
What does fget_light do?
And does the ioctl dispatch cost that much? I like the flexibility of it to be honest.


Alex

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Avi Kivity]

On 05/16/2010 04:00 AM, Alexander Graf wrote:
> On 15.05.2010, at 19:30, Avi Kivity wrote:
>
>    
>> On 05/15/2010 11:26 AM, Alexander Graf wrote:
>>      
>>>        
>>>> That means you never inject an interrupt from the iothread (or from a different vcpu thread)?
>>>>
>>>> If that's the case we might make it part of the API and require the ioctl to be issued from the vcpu thread.  We'd still be left with the s390 exception.
>>>>
>>>>          
>>> Well I'm not sure that's guaranteed for MOL or Dolphin, but I guess the user base is small enough to ignore them.
>>>
>>> Either way, I'm actually rather unhappy with the way interrupts work right now. We're only injecting interrupts when in the main loop, which is rare if we did our homework right. So what I'd really like to see is that the MPIC on ppc directly invokes KVM_INTERRUPT to pull (and losen) the interrupt line. That way we can't just accidently miss interrupts.
>>>
>>>        
>> on x86 we signal the vcpu thread to pull it out of the main loop and poll the apic.
>>      
> Hrm, makes sense. Though it's additional overhead of a task switch. Why take the burden if you don't have to?
>    

That's what the world looked like in 2006.

We could change it, but there's not much point, since having the local 
apic in the kernel is pretty much a requirement for reasonable performance.

>> That's exactly why x86 has run->request_interrupt_window, run->ready_for_interrupt_injection, and run->if_flag.
>>      
> So how does x86 userspace get notified when it has an interrupt pending but couldn't inject it? Without a notification, we delay interrupts by quite some time.
>    

run->ready_for_interrupt_injection and run->request_irq_window.

>>> ->   we need to exit to userspace to realize that the interrupt is still active
>>>
>>> This is fundamentally broken. What I'd like to see is:
>>>
>>> * device in userspace wants to trigger an interrupt
>>> * mpic pulls external interrupt line, automatically issues ioctl(KVM_INTERRUPT)
>>> * we enter the guest
>>> * we inject the external interrupt
>>> * guest gets the interrupt, sees it's lazily disabled, sets msr.ee=0
>>> * guest moves on, sets msr.ee=1 again later
>>> * we inject the external interrupt again, since it's still active
>>> * guest acknowledges the interrupt with the mpic, issues ioctl(KVM_INTERRUPT, disable)
>>> ->   all is great
>>>
>>>        
>> This is similar to KVM_IRQ_LINE.
>>      
> Well, KVM_IRQ_LINE would be the in-kernel pic, no? That'd mean I'd have to reimplement the mpic in kernel space - which might eventually be a good idea when going for SMP, but I'd first like to see if I can keep the current interrupt injection path efficient.
>    

Sure.

>>> For that to work we need to enable triggering KVM_INTERRUPT from a non-vcpu thread.
>>>
>>>        
>> KVM_IRQ_LINE is a vm ioctl instead of a vcpu ioctl.
>>
>> The motivation for the strict 'issue vcpu ioctls from vcpu thread' rule is to prepare the way for a syscall (instead of ioctl) API.  Then we lost the fd argument, and choosing the vcpu is done by associating it with the current task.  That allows us to get rid of vcpu->mutex and fget_light() (as well as the ioctl dispatch).
>>      
> If we define the API to only work on the current vcpu with a few excetions where we're safe (KVM_INTERRUPT), that'd get rid of the mutex too, no?
>    

Yes.  Need to document it though.

> What does fget_light do?
>    

Make sure that the vcpu fd doesn't disappear.

> And does the ioctl dispatch cost that much? I like the flexibility of it to be honest.
>    

Not much, which is why there's no movement in that direction.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Alexander Graf]

On 16.05.2010, at 10:23, Avi Kivity wrote:

> On 05/16/2010 04:00 AM, Alexander Graf wrote:
>> On 15.05.2010, at 19:30, Avi Kivity wrote:
>> 
>>   
>>> On 05/15/2010 11:26 AM, Alexander Graf wrote:
>>>     
>>>>       
>>>>> That means you never inject an interrupt from the iothread (or from a different vcpu thread)?
>>>>> 
>>>>> If that's the case we might make it part of the API and require the ioctl to be issued from the vcpu thread.  We'd still be left with the s390 exception.
>>>>> 
>>>>>         
>>>> Well I'm not sure that's guaranteed for MOL or Dolphin, but I guess the user base is small enough to ignore them.
>>>> 
>>>> Either way, I'm actually rather unhappy with the way interrupts work right now. We're only injecting interrupts when in the main loop, which is rare if we did our homework right. So what I'd really like to see is that the MPIC on ppc directly invokes KVM_INTERRUPT to pull (and losen) the interrupt line. That way we can't just accidently miss interrupts.
>>>> 
>>>>       
>>> on x86 we signal the vcpu thread to pull it out of the main loop and poll the apic.
>>>     
>> Hrm, makes sense. Though it's additional overhead of a task switch. Why take the burden if you don't have to?
>>   
> 
> That's what the world looked like in 2006.
> 
> We could change it, but there's not much point, since having the local apic in the kernel is pretty much a requirement for reasonable performance.

Well, I'm not convinced yet that's the case for PPC as well. The timer is in-cpu anyways and I don't see why IPIs should be slow with a userspace pic - if we keep the overhead low.

So let me think this through. With remote interrupt injection we have.

* thread 1 does vcpu_run
* thread 2 triggers KVM_INTERRUPT on fd
* thread 2 signals thread 1 so we're sure the interrupt gets injected
* thread 1 exits into qemu
* thread 1 goes back into the vcpu, triggering an interrupt

Without we have:

* thread 1 does vcpu_run
* thread 2 wants to trigger an an interrupt, sets the qemu internal bit
* thread 2 signals thread 1 so we're sure the interrupt gets processed
* thread 1 exits into qemu
* thread 1 triggers KVM_INTERRUPT on fd
* thread 1 goes into the vcpu

So we don't really buy anything from doing the remote injection. Hrm.

What's somewhat striking me here though is - why do we need KVM_INTERRUPT when there's all those kvm_run fields? Can't we just do interrupt injection by setting run->trigger_interrupt? There's only a single "interrupt line" on the CPU anyways. That way we'd save the ioctl and get rid of the locking problem altogether.

Alex

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Avi Kivity]

On 05/16/2010 12:01 PM, Alexander Graf wrote:
>
>> That's what the world looked like in 2006.
>>
>> We could change it, but there's not much point, since having the local apic in the kernel is pretty much a requirement for reasonable performance.
>>      
> Well, I'm not convinced yet that's the case for PPC as well. The timer is in-cpu anyways and I don't see why IPIs should be slow with a userspace pic - if we keep the overhead low.
>    

If it's at all possible keep the mpic out.  I am _not_ advocating 
pushing ppc's mpic into the kernel.

> So let me think this through. With remote interrupt injection we have.
>
> * thread 1 does vcpu_run
> * thread 2 triggers KVM_INTERRUPT on fd
> * thread 2 signals thread 1 so we're sure the interrupt gets injected
> * thread 1 exits into qemu
>    

This doesn't seem necessary.  The kernel can own the interrupt line, so 
it remembers it from the last KVM_INTERRUPT.

> * thread 1 goes back into the vcpu, triggering an interrupt
>
> Without we have:
>
> * thread 1 does vcpu_run
> * thread 2 wants to trigger an an interrupt, sets the qemu internal bit
> * thread 2 signals thread 1 so we're sure the interrupt gets processed
> * thread 1 exits into qemu
> * thread 1 triggers KVM_INTERRUPT on fd
> * thread 1 goes into the vcpu
>
> So we don't really buy anything from doing the remote injection. Hrm.
>    

Not if you make interrupt injection a lightweight exit.

> What's somewhat striking me here though is - why do we need KVM_INTERRUPT when there's all those kvm_run fields? Can't we just do interrupt injection by setting run->trigger_interrupt? There's only a single "interrupt line" on the CPU anyways. That way we'd save the ioctl and get rid of the locking problem altogether.
>    

That's what x86 does.  However, it's synchronous.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Alexander Graf]

On 16.05.2010, at 11:09, Avi Kivity wrote:

> On 05/16/2010 12:01 PM, Alexander Graf wrote:
>> 
>>> That's what the world looked like in 2006.
>>> 
>>> We could change it, but there's not much point, since having the local apic in the kernel is pretty much a requirement for reasonable performance.
>>>     
>> Well, I'm not convinced yet that's the case for PPC as well. The timer is in-cpu anyways and I don't see why IPIs should be slow with a userspace pic - if we keep the overhead low.
>>   
> 
> If it's at all possible keep the mpic out.  I am _not_ advocating pushing ppc's mpic into the kernel.
> 
>> So let me think this through. With remote interrupt injection we have.
>> 
>> * thread 1 does vcpu_run
>> * thread 2 triggers KVM_INTERRUPT on fd
>> * thread 2 signals thread 1 so we're sure the interrupt gets injected
>> * thread 1 exits into qemu
>>   
> 
> This doesn't seem necessary.  The kernel can own the interrupt line, so it remembers it from the last KVM_INTERRUPT.

It's not? On signals we always exit to userspace, no?

> 
>> * thread 1 goes back into the vcpu, triggering an interrupt
>> 
>> Without we have:
>> 
>> * thread 1 does vcpu_run
>> * thread 2 wants to trigger an an interrupt, sets the qemu internal bit
>> * thread 2 signals thread 1 so we're sure the interrupt gets processed
>> * thread 1 exits into qemu
>> * thread 1 triggers KVM_INTERRUPT on fd
>> * thread 1 goes into the vcpu
>> 
>> So we don't really buy anything from doing the remote injection. Hrm.
>>   
> 
> Not if you make interrupt injection a lightweight exit.

Please elaborate.

> 
>> What's somewhat striking me here though is - why do we need KVM_INTERRUPT when there's all those kvm_run fields? Can't we just do interrupt injection by setting run->trigger_interrupt? There's only a single "interrupt line" on the CPU anyways. That way we'd save the ioctl and get rid of the locking problem altogether.
>>   
> 
> That's what x86 does.  However, it's synchronous.

For everyone except for the vcpu thread executing the interrupt, it's asynchronous, right? The same applies to an in-kernel pic.


Alex

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Avi Kivity]

On 05/16/2010 12:35 PM, Alexander Graf wrote:
>
>>
>>> So let me think this through. With remote interrupt injection we have.
>>>
>>> * thread 1 does vcpu_run
>>> * thread 2 triggers KVM_INTERRUPT on fd
>>> * thread 2 signals thread 1 so we're sure the interrupt gets injected
>>> * thread 1 exits into qemu
>>>
>>>        
>> This doesn't seem necessary.  The kernel can own the interrupt line, so it remembers it from the last KVM_INTERRUPT.
>>      
> It's not?

With s/signals/IPIs/.

> On signals we always exit to userspace, no?
>    

Yes (if the signal isn't blocked).

>>> * thread 1 goes back into the vcpu, triggering an interrupt
>>>
>>> Without we have:
>>>
>>> * thread 1 does vcpu_run
>>> * thread 2 wants to trigger an an interrupt, sets the qemu internal bit
>>> * thread 2 signals thread 1 so we're sure the interrupt gets processed
>>> * thread 1 exits into qemu
>>> * thread 1 triggers KVM_INTERRUPT on fd
>>> * thread 1 goes into the vcpu
>>>
>>> So we don't really buy anything from doing the remote injection. Hrm.
>>>
>>>        
>> Not if you make interrupt injection a lightweight exit.
>>      
> Please elaborate.
>    

1: vcpu_run
2: KVM_INTERRUPT
2k: sets flag, if msr.ee IPIs 1 or wakes up 1 if halted
1k: notices flag, if msr.ee injects interrupt
...
1g: acks
1k: forwards ack to userspace
1: completes interrupt


>>> What's somewhat striking me here though is - why do we need KVM_INTERRUPT when there's all those kvm_run fields? Can't we just do interrupt injection by setting run->trigger_interrupt? There's only a single "interrupt line" on the CPU anyways. That way we'd save the ioctl and get rid of the locking problem altogether.
>>>
>>>        
>> That's what x86 does.  However, it's synchronous.
>>      
> For everyone except for the vcpu thread executing the interrupt, it's asynchronous, right?

For everyone other than the vcpu thread, it's off limits.  kvm_run is 
only read on KVM_RUN entries and written on KVM_RUN exits.

> The same applies to an in-kernel pic.
>    

The in-kernel pic doesn't use kvm_run.


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Alexander Graf]

On 16.05.2010, at 11:47, Avi Kivity wrote:

> 1: vcpu_run
> 2: KVM_INTERRUPT
> 2k: sets flag, if msr.ee IPIs 1 or wakes up 1 if halted

Doesn't that break when we have a while(1) loop in the guest with msr.ee=0 while no timer is scheduled on the host? But then again with msr.ee=0 we don't get interrupts in the guest and to set msr.ee=1 we trap. Yeah, that would work.

> 1k: notices flag, if msr.ee injects interrupt
> ...
> 1g: acks

The ack is done in userspace by the mpic, so we can just complete the interrupt there.

> 1k: forwards ack to userspace
> 1: completes interrupt


So if I just have a field kvm_run->external_active I could set that to =1 on KVM_INTERRUPT including the above logic. To acknowledge it userspace would then do something like this in kvm_arch_pre_run:

    if (kvm_run->external_active &&
        !((env->interrupt_request & CPU_INTERRUPT_HARD) &&
          (env->irq_input_state & (1<<PPC_INPUT_INT))))
    {
        kvm_run->external_active = 0;
    }

The big question is how to make such a change backwards compatible. But I guess I could just reuse the feature enabling framework. Well, sounds like we're getting closer.


Alex

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Carsten Otte]

On 15.05.2010 10:26, Alexander Graf wrote:
> On S390, I'm also still sceptical if the implementation we have really works. A device injects an S390_INTERRUPT with its address and on the next vcpu_run, an according interrupt is issued. But what happens if two devices trigger an S390_INTERRUPT before the vcpu_run? We'd have lost an interrupt by then...
We're safe on that: the interrupt info field in both struct kvm (for 
floating interrupts) and struct vcpu (for cpu local interrupts) have 
their own locking and can queue up interrupts.

cheers,
Carsten

[PATCH 7/7] KVM: Consolidate arch specific vcpu ioctl locking

[Avi Kivity]

Now that all arch specific ioctls have centralized locking, it is easy to
move it to the central dispatcher.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/powerpc/kvm/powerpc.c |   11 ++++-------
 arch/s390/kvm/kvm-s390.c   |   13 ++++++-------
 arch/x86/kvm/x86.c         |    2 --
 virt/kvm/kvm_main.c        |    2 --
 4 files changed, 10 insertions(+), 18 deletions(-)

diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index caeed7b..a1d8750 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -512,17 +512,16 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 	void __user *argp = (void __user *)arg;
 	long r;
 
-	if (ioctl == KVM_INTERRUPT) {
+	switch (ioctl) {
+	case KVM_INTERRUPT: {
 		struct kvm_interrupt irq;
 		r = -EFAULT;
 		if (copy_from_user(&irq, argp, sizeof(irq)))
-			goto out_nolock;
+			goto out;
 		r = kvm_vcpu_ioctl_interrupt(vcpu, &irq);
-		goto out_nolock;
+		goto out;
 	}
 
-	vcpu_load(vcpu);
-	switch (ioctl) {
 	case KVM_ENABLE_CAP:
 	{
 		struct kvm_enable_cap cap;
@@ -537,8 +536,6 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 	}
 
 out:
-	vcpu_put(vcpu);
-out_nolock:
 	return r;
 }
 
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 28cd8fd..fad1024 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -638,16 +638,16 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 	void __user *argp = (void __user *)arg;
 	long r;
 
-	if (ioctl == KVM_S390_INTERRUPT) {
+	switch (ioctl) {
+	case KVM_S390_INTERRUPT: {
 		struct kvm_s390_interrupt s390int;
 
+		r = -EFAULT;
 		if (copy_from_user(&s390int, argp, sizeof(s390int)))
-			return -EFAULT;
-		return kvm_s390_inject_vcpu(vcpu, &s390int);
+			break;
+		r = kvm_s390_inject_vcpu(vcpu, &s390int);
+		break;
 	}
-
-	vcpu_load(vcpu);
-	switch (ioctl) {
 	case KVM_S390_STORE_STATUS:
 		r = kvm_s390_vcpu_store_status(vcpu, arg);
 		break;
@@ -666,7 +666,6 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 	default:
 		r = -EINVAL;
 	}
-	vcpu_put(vcpu);
 	return r;
 }
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 8b9e5ec..3a763de 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2288,7 +2288,6 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 	int r;
 	struct kvm_lapic_state *lapic = NULL;
 
-	vcpu_load(vcpu);
 	switch (ioctl) {
 	case KVM_GET_LAPIC: {
 		r = -EINVAL;
@@ -2486,7 +2485,6 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		r = -EINVAL;
 	}
 out:
-	vcpu_put(vcpu);
 	kfree(lapic);
 	return r;
 }
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 08b2ccd..5ee558c 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1564,9 +1564,7 @@ out_free2:
 		break;
 	}
 	default:
-		vcpu_put(vcpu);
 		r = kvm_arch_vcpu_ioctl(filp, ioctl, arg);
-		vcpu_load(vcpu);
 	}
 out:
 	vcpu_put(vcpu);
-- 
1.7.0.4