[PATCH 0/4] Fix book3s-pr KVM with preemption

[PATCH 0/4] Fix book3s-pr KVM with preemption

[Alexander Graf]

So far we got away with not implementing preemption properly. However,
recently users emerged who wanted to run PREEMPT_xxx kernels, running
into issues with KVM on there.

This patch set fixes all preempt issues I've found so far with Book3S
PR KVM.

Alexander Graf (4):
  KVM: PPC: Book3s: PR: Disable preemption in vcpu_run
  KVM: PPC: Book3s: PR: No irq_disable in vcpu_run
  KVM: PPC: Use get/set for to_svcpu to help preemption
  KVM: PPC: align vcpu_kick with x86

 arch/powerpc/include/asm/kvm_book3s.h    |   76 +++++++++++++++++++++++-------
 arch/powerpc/include/asm/kvm_book3s_32.h |    6 ++-
 arch/powerpc/include/asm/kvm_book3s_64.h |    8 +++-
 arch/powerpc/kvm/book3s_32_mmu_host.c    |   21 ++++++--
 arch/powerpc/kvm/book3s_64_mmu_host.c    |   66 +++++++++++++++++---------
 arch/powerpc/kvm/book3s_emulate.c        |    8 ++-
 arch/powerpc/kvm/book3s_pr.c             |   71 +++++++++++++++++++---------
 arch/powerpc/kvm/powerpc.c               |    7 ++-
 arch/powerpc/kvm/trace.h                 |    5 ++-
 9 files changed, 194 insertions(+), 74 deletions(-)

[PATCH 1/4] KVM: PPC: Book3s: PR: Disable preemption in vcpu_run

[Alexander Graf]

When entering the guest, we want to make sure we're not getting preempted
away, so let's disable preemption on entry, but enable it again while handling
guest exits.

Reported-by: Jörg Sommer <joerg@alea.gnuu.de>
Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/kvm/book3s_pr.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
index 726512b..8e4f800 100644
--- a/arch/powerpc/kvm/book3s_pr.c
+++ b/arch/powerpc/kvm/book3s_pr.c
@@ -519,6 +519,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
 	run->ready_for_interrupt_injection = 1;
 
 	trace_kvm_book3s_exit(exit_nr, vcpu);
+	preempt_enable();
 	kvm_resched(vcpu);
 	switch (exit_nr) {
 	case BOOK3S_INTERRUPT_INST_STORAGE:
@@ -763,6 +764,8 @@ program_interrupt:
 			run->exit_reason = KVM_EXIT_INTR;
 			r = -EINTR;
 		} else {
+			preempt_disable();
+
 			/* In case an interrupt came in that was triggered
 			 * from userspace (like DEC), we need to check what
 			 * to inject now! */
@@ -925,6 +928,8 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
 #endif
 	ulong ext_msr;
 
+	preempt_disable();
+
 	/* Check if we can run the vcpu at all */
 	if (!vcpu->arch.sane) {
 		kvm_run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
@@ -1006,6 +1011,8 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
 	current->thread.used_vsr = used_vsr;
 #endif
 
+	preempt_enable();
+
 	return ret;
 }
 
-- 
1.6.0.2

[PATCH 2/4] KVM: PPC: Book3s: PR: No irq_disable in vcpu_run

[Alexander Graf]

Somewhere during merges we ended up from

  local_irq_enable()
  foo();
  local_irq_disable()

to always keeping irqs enabled during that part. However, we now
have the following code:

  foo();
  local_irq_disable()

which disables interrupts without the surrounding code enabling them
again! So let's remove that disable and be happy.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/kvm/book3s_pr.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
index 8e4f800..40ce940 100644
--- a/arch/powerpc/kvm/book3s_pr.c
+++ b/arch/powerpc/kvm/book3s_pr.c
@@ -983,8 +983,6 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
 
 	kvm_guest_exit();
 
-	local_irq_disable();
-
 	current->thread.regs->msr = ext_msr;
 
 	/* Make sure we save the guest FPU/Altivec/VSX state */
-- 
1.6.0.2

[PATCH 3/4] KVM: PPC: Use get/set for to_svcpu to help preemption

[Alexander Graf]

When running the 64-bit Book3s PR code without CONFIG_PREEMPT_NONE, we were
doing a few things wrong, most notably access to PACA fields without making
sure that the pointers stay stable accross the access (preempt_disable()).

This patch moves to_svcpu towards a get/put model which allows us to disable
preemption while accessing the shadow vcpu fields in the PACA. That way we
can run preemptible and everyone's happy!

Reported-by: Jörg Sommer <joerg@alea.gnuu.de>
Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/include/asm/kvm_book3s.h    |   76 +++++++++++++++++++++++-------
 arch/powerpc/include/asm/kvm_book3s_32.h |    6 ++-
 arch/powerpc/include/asm/kvm_book3s_64.h |    8 +++-
 arch/powerpc/kvm/book3s_32_mmu_host.c    |   21 ++++++--
 arch/powerpc/kvm/book3s_64_mmu_host.c    |   66 +++++++++++++++++---------
 arch/powerpc/kvm/book3s_emulate.c        |    8 ++-
 arch/powerpc/kvm/book3s_pr.c             |   62 ++++++++++++++++--------
 arch/powerpc/kvm/trace.h                 |    5 ++-
 8 files changed, 181 insertions(+), 71 deletions(-)

diff --git a/arch/powerpc/include/asm/kvm_book3s.h b/arch/powerpc/include/asm/kvm_book3s.h
index deb8a4e..e8c78ac 100644
--- a/arch/powerpc/include/asm/kvm_book3s.h
+++ b/arch/powerpc/include/asm/kvm_book3s.h
@@ -185,7 +185,9 @@ static inline void kvmppc_update_int_pending(struct kvm_vcpu *vcpu,
 static inline void kvmppc_set_gpr(struct kvm_vcpu *vcpu, int num, ulong val)
 {
 	if ( num < 14 ) {
-		to_svcpu(vcpu)->gpr[num] = val;
+		struct kvmppc_book3s_shadow_vcpu *svcpu = svcpu_get(vcpu);
+		svcpu->gpr[num] = val;
+		svcpu_put(svcpu);
 		to_book3s(vcpu)->shadow_vcpu->gpr[num] = val;
 	} else
 		vcpu->arch.gpr[num] = val;
@@ -193,80 +195,120 @@ static inline void kvmppc_set_gpr(struct kvm_vcpu *vcpu, int num, ulong val)
 
 static inline ulong kvmppc_get_gpr(struct kvm_vcpu *vcpu, int num)
 {
-	if ( num < 14 )
-		return to_svcpu(vcpu)->gpr[num];
-	else
+	if ( num < 14 ) {
+		struct kvmppc_book3s_shadow_vcpu *svcpu = svcpu_get(vcpu);
+		ulong r = svcpu->gpr[num];
+		svcpu_put(svcpu);
+		return r;
+	} else
 		return vcpu->arch.gpr[num];
 }
 
 static inline void kvmppc_set_cr(struct kvm_vcpu *vcpu, u32 val)
 {
-	to_svcpu(vcpu)->cr = val;
+	struct kvmppc_book3s_shadow_vcpu *svcpu = svcpu_get(vcpu);
+	svcpu->cr = val;
+	svcpu_put(svcpu);
 	to_book3s(vcpu)->shadow_vcpu->cr = val;
 }
 
 static inline u32 kvmppc_get_cr(struct kvm_vcpu *vcpu)
 {
-	return to_svcpu(vcpu)->cr;
+	struct kvmppc_book3s_shadow_vcpu *svcpu = svcpu_get(vcpu);
+	u32 r;
+	r = svcpu->cr;
+	svcpu_put(svcpu);
+	return r;
 }
 
 static inline void kvmppc_set_xer(struct kvm_vcpu *vcpu, u32 val)
 {
-	to_svcpu(vcpu)->xer = val;
+	struct kvmppc_book3s_shadow_vcpu *svcpu = svcpu_get(vcpu);
+	svcpu->xer = val;
 	to_book3s(vcpu)->shadow_vcpu->xer = val;
+	svcpu_put(svcpu);
 }
 
 static inline u32 kvmppc_get_xer(struct kvm_vcpu *vcpu)
 {
-	return to_svcpu(vcpu)->xer;
+	struct kvmppc_book3s_shadow_vcpu *svcpu = svcpu_get(vcpu);
+	u32 r;
+	r = svcpu->xer;
+	svcpu_put(svcpu);
+	return r;
 }
 
 static inline void kvmppc_set_ctr(struct kvm_vcpu *vcpu, ulong val)
 {
-	to_svcpu(vcpu)->ctr = val;
+	struct kvmppc_book3s_shadow_vcpu *svcpu = svcpu_get(vcpu);
+	svcpu->ctr = val;
+	svcpu_put(svcpu);
 }
 
 static inline ulong kvmppc_get_ctr(struct kvm_vcpu *vcpu)
 {
-	return to_svcpu(vcpu)->ctr;
+	struct kvmppc_book3s_shadow_vcpu *svcpu = svcpu_get(vcpu);
+	ulong r;
+	r = svcpu->ctr;
+	svcpu_put(svcpu);
+	return r;
 }
 
 static inline void kvmppc_set_lr(struct kvm_vcpu *vcpu, ulong val)
 {
-	to_svcpu(vcpu)->lr = val;
+	struct kvmppc_book3s_shadow_vcpu *svcpu = svcpu_get(vcpu);
+	svcpu->lr = val;
+	svcpu_put(svcpu);
 }
 
 static inline ulong kvmppc_get_lr(struct kvm_vcpu *vcpu)
 {
-	return to_svcpu(vcpu)->lr;
+	struct kvmppc_book3s_shadow_vcpu *svcpu = svcpu_get(vcpu);
+	ulong r;
+	r = svcpu->lr;
+	svcpu_put(svcpu);
+	return r;
 }
 
 static inline void kvmppc_set_pc(struct kvm_vcpu *vcpu, ulong val)
 {
-	to_svcpu(vcpu)->pc = val;
+	struct kvmppc_book3s_shadow_vcpu *svcpu = svcpu_get(vcpu);
+	svcpu->pc = val;
+	svcpu_put(svcpu);
 }
 
 static inline ulong kvmppc_get_pc(struct kvm_vcpu *vcpu)
 {
-	return to_svcpu(vcpu)->pc;
+	struct kvmppc_book3s_shadow_vcpu *svcpu = svcpu_get(vcpu);
+	ulong r;
+	r = svcpu->pc;
+	svcpu_put(svcpu);
+	return r;
 }
 
 static inline u32 kvmppc_get_last_inst(struct kvm_vcpu *vcpu)
 {
 	ulong pc = kvmppc_get_pc(vcpu);
-	struct kvmppc_book3s_shadow_vcpu *svcpu = to_svcpu(vcpu);
+	struct kvmppc_book3s_shadow_vcpu *svcpu = svcpu_get(vcpu);
+	u32 r;
 
 	/* Load the instruction manually if it failed to do so in the
 	 * exit path */
 	if (svcpu->last_inst == KVM_INST_FETCH_FAILED)
 		kvmppc_ld(vcpu, &pc, sizeof(u32), &svcpu->last_inst, false);
 
-	return svcpu->last_inst;
+	r = svcpu->last_inst;
+	svcpu_put(svcpu);
+	return r;
 }
 
 static inline ulong kvmppc_get_fault_dar(struct kvm_vcpu *vcpu)
 {
-	return to_svcpu(vcpu)->fault_dar;
+	struct kvmppc_book3s_shadow_vcpu *svcpu = svcpu_get(vcpu);
+	ulong r;
+	r = svcpu->fault_dar;
+	svcpu_put(svcpu);
+	return r;
 }
 
 static inline bool kvmppc_critical_section(struct kvm_vcpu *vcpu)
diff --git a/arch/powerpc/include/asm/kvm_book3s_32.h b/arch/powerpc/include/asm/kvm_book3s_32.h
index de604db..38040ff 100644
--- a/arch/powerpc/include/asm/kvm_book3s_32.h
+++ b/arch/powerpc/include/asm/kvm_book3s_32.h
@@ -20,11 +20,15 @@
 #ifndef __ASM_KVM_BOOK3S_32_H__
 #define __ASM_KVM_BOOK3S_32_H__
 
-static inline struct kvmppc_book3s_shadow_vcpu *to_svcpu(struct kvm_vcpu *vcpu)
+static inline struct kvmppc_book3s_shadow_vcpu *svcpu_get(struct kvm_vcpu *vcpu)
 {
 	return to_book3s(vcpu)->shadow_vcpu;
 }
 
+static inline void svcpu_put(struct kvmppc_book3s_shadow_vcpu *svcpu)
+{
+}
+
 #define PTE_SIZE	12
 #define VSID_ALL	0
 #define SR_INVALID	0x00000001	/* VSID 1 should always be unused */
diff --git a/arch/powerpc/include/asm/kvm_book3s_64.h b/arch/powerpc/include/asm/kvm_book3s_64.h
index d0ac94f..2054e47 100644
--- a/arch/powerpc/include/asm/kvm_book3s_64.h
+++ b/arch/powerpc/include/asm/kvm_book3s_64.h
@@ -21,10 +21,16 @@
 #define __ASM_KVM_BOOK3S_64_H__
 
 #ifdef CONFIG_KVM_BOOK3S_PR
-static inline struct kvmppc_book3s_shadow_vcpu *to_svcpu(struct kvm_vcpu *vcpu)
+static inline struct kvmppc_book3s_shadow_vcpu *svcpu_get(struct kvm_vcpu *vcpu)
 {
+	preempt_disable();
 	return &get_paca()->shadow_vcpu;
 }
+
+static inline void svcpu_put(struct kvmppc_book3s_shadow_vcpu *svcpu)
+{
+	preempt_enable();
+}
 #endif
 
 #define SPAPR_TCE_SHIFT		12
diff --git a/arch/powerpc/kvm/book3s_32_mmu_host.c b/arch/powerpc/kvm/book3s_32_mmu_host.c
index 9fecbfb..f922c29 100644
--- a/arch/powerpc/kvm/book3s_32_mmu_host.c
+++ b/arch/powerpc/kvm/book3s_32_mmu_host.c
@@ -151,13 +151,15 @@ int kvmppc_mmu_map_page(struct kvm_vcpu *vcpu, struct kvmppc_pte *orig_pte)
 	bool primary = false;
 	bool evict = false;
 	struct hpte_cache *pte;
+	int r = 0;
 
 	/* Get host physical address for gpa */
 	hpaddr = kvmppc_gfn_to_pfn(vcpu, orig_pte->raddr >> PAGE_SHIFT);
 	if (is_error_pfn(hpaddr)) {
 		printk(KERN_INFO "Couldn't get guest page for gfn %lx!\n",
 				 orig_pte->eaddr);
-		return -EINVAL;
+		r = -EINVAL;
+		goto out;
 	}
 	hpaddr <<= PAGE_SHIFT;
 
@@ -249,7 +251,8 @@ next_pteg:
 
 	kvmppc_mmu_hpte_cache_map(vcpu, pte);
 
-	return 0;
+out:
+	return r;
 }
 
 static struct kvmppc_sid_map *create_sid_map(struct kvm_vcpu *vcpu, u64 gvsid)
@@ -297,12 +300,14 @@ int kvmppc_mmu_map_segment(struct kvm_vcpu *vcpu, ulong eaddr)
 	u64 gvsid;
 	u32 sr;
 	struct kvmppc_sid_map *map;
-	struct kvmppc_book3s_shadow_vcpu *svcpu = to_svcpu(vcpu);
+	struct kvmppc_book3s_shadow_vcpu *svcpu = svcpu_get(vcpu);
+	int r = 0;
 
 	if (vcpu->arch.mmu.esid_to_vsid(vcpu, esid, &gvsid)) {
 		/* Invalidate an entry */
 		svcpu->sr[esid] = SR_INVALID;
-		return -ENOENT;
+		r = -ENOENT;
+		goto out;
 	}
 
 	map = find_sid_vsid(vcpu, gvsid);
@@ -315,17 +320,21 @@ int kvmppc_mmu_map_segment(struct kvm_vcpu *vcpu, ulong eaddr)
 
 	dprintk_sr("MMU: mtsr %d, 0x%x\n", esid, sr);
 
-	return 0;
+out:
+	svcpu_put(svcpu);
+	return r;
 }
 
 void kvmppc_mmu_flush_segments(struct kvm_vcpu *vcpu)
 {
 	int i;
-	struct kvmppc_book3s_shadow_vcpu *svcpu = to_svcpu(vcpu);
+	struct kvmppc_book3s_shadow_vcpu *svcpu = svcpu_get(vcpu);
 
 	dprintk_sr("MMU: flushing all segments (%d)\n", ARRAY_SIZE(svcpu->sr));
 	for (i = 0; i < ARRAY_SIZE(svcpu->sr); i++)
 		svcpu->sr[i] = SR_INVALID;
+
+	svcpu_put(svcpu);
 }
 
 void kvmppc_mmu_destroy(struct kvm_vcpu *vcpu)
diff --git a/arch/powerpc/kvm/book3s_64_mmu_host.c b/arch/powerpc/kvm/book3s_64_mmu_host.c
index fa2f084..6f87f39 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_host.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_host.c
@@ -88,12 +88,14 @@ int kvmppc_mmu_map_page(struct kvm_vcpu *vcpu, struct kvmppc_pte *orig_pte)
 	int vflags = 0;
 	int attempt = 0;
 	struct kvmppc_sid_map *map;
+	int r = 0;
 
 	/* Get host physical address for gpa */
 	hpaddr = kvmppc_gfn_to_pfn(vcpu, orig_pte->raddr >> PAGE_SHIFT);
 	if (is_error_pfn(hpaddr)) {
 		printk(KERN_INFO "Couldn't get guest page for gfn %lx!\n", orig_pte->eaddr);
-		return -EINVAL;
+		r = -EINVAL;
+		goto out;
 	}
 	hpaddr <<= PAGE_SHIFT;
 	hpaddr |= orig_pte->raddr & (~0xfffULL & ~PAGE_MASK);
@@ -110,7 +112,8 @@ int kvmppc_mmu_map_page(struct kvm_vcpu *vcpu, struct kvmppc_pte *orig_pte)
 		printk(KERN_ERR "KVM: Segment map for 0x%llx (0x%lx) failed\n",
 				vsid, orig_pte->eaddr);
 		WARN_ON(true);
-		return -EINVAL;
+		r = -EINVAL;
+		goto out;
 	}
 
 	vsid = map->host_vsid;
@@ -131,8 +134,10 @@ map_again:
 
 	/* In case we tried normal mapping already, let's nuke old entries */
 	if (attempt > 1)
-		if (ppc_md.hpte_remove(hpteg) < 0)
-			return -1;
+		if (ppc_md.hpte_remove(hpteg) < 0) {
+			r = -1;
+			goto out;
+		}
 
 	ret = ppc_md.hpte_insert(hpteg, va, hpaddr, rflags, vflags, MMU_PAGE_4K, MMU_SEGSIZE_256M);
 
@@ -162,7 +167,8 @@ map_again:
 		kvmppc_mmu_hpte_cache_map(vcpu, pte);
 	}
 
-	return 0;
+out:
+	return r;
 }
 
 static struct kvmppc_sid_map *create_sid_map(struct kvm_vcpu *vcpu, u64 gvsid)
@@ -207,25 +213,30 @@ static struct kvmppc_sid_map *create_sid_map(struct kvm_vcpu *vcpu, u64 gvsid)
 
 static int kvmppc_mmu_next_segment(struct kvm_vcpu *vcpu, ulong esid)
 {
+	struct kvmppc_book3s_shadow_vcpu *svcpu = svcpu_get(vcpu);
 	int i;
 	int max_slb_size = 64;
 	int found_inval = -1;
 	int r;
 
-	if (!to_svcpu(vcpu)->slb_max)
-		to_svcpu(vcpu)->slb_max = 1;
+	if (!svcpu->slb_max)
+		svcpu->slb_max = 1;
 
 	/* Are we overwriting? */
-	for (i = 1; i < to_svcpu(vcpu)->slb_max; i++) {
-		if (!(to_svcpu(vcpu)->slb[i].esid & SLB_ESID_V))
+	for (i = 1; i < svcpu->slb_max; i++) {
+		if (!(svcpu->slb[i].esid & SLB_ESID_V))
 			found_inval = i;
-		else if ((to_svcpu(vcpu)->slb[i].esid & ESID_MASK) == esid)
-			return i;
+		else if ((svcpu->slb[i].esid & ESID_MASK) == esid) {
+			r = i;
+			goto out;
+		}
 	}
 
 	/* Found a spare entry that was invalidated before */
-	if (found_inval > 0)
-		return found_inval;
+	if (found_inval > 0) {
+		r = found_inval;
+		goto out;
+	}
 
 	/* No spare invalid entry, so create one */
 
@@ -233,30 +244,35 @@ static int kvmppc_mmu_next_segment(struct kvm_vcpu *vcpu, ulong esid)
 		max_slb_size = mmu_slb_size;
 
 	/* Overflowing -> purge */
-	if ((to_svcpu(vcpu)->slb_max) == max_slb_size)
+	if ((svcpu->slb_max) == max_slb_size)
 		kvmppc_mmu_flush_segments(vcpu);
 
-	r = to_svcpu(vcpu)->slb_max;
-	to_svcpu(vcpu)->slb_max++;
+	r = svcpu->slb_max;
+	svcpu->slb_max++;
 
+out:
+	svcpu_put(svcpu);
 	return r;
 }
 
 int kvmppc_mmu_map_segment(struct kvm_vcpu *vcpu, ulong eaddr)
 {
+	struct kvmppc_book3s_shadow_vcpu *svcpu = svcpu_get(vcpu);
 	u64 esid = eaddr >> SID_SHIFT;
 	u64 slb_esid = (eaddr & ESID_MASK) | SLB_ESID_V;
 	u64 slb_vsid = SLB_VSID_USER;
 	u64 gvsid;
 	int slb_index;
 	struct kvmppc_sid_map *map;
+	int r = 0;
 
 	slb_index = kvmppc_mmu_next_segment(vcpu, eaddr & ESID_MASK);
 
 	if (vcpu->arch.mmu.esid_to_vsid(vcpu, esid, &gvsid)) {
 		/* Invalidate an entry */
-		to_svcpu(vcpu)->slb[slb_index].esid = 0;
-		return -ENOENT;
+		svcpu->slb[slb_index].esid = 0;
+		r = -ENOENT;
+		goto out;
 	}
 
 	map = find_sid_vsid(vcpu, gvsid);
@@ -269,18 +285,22 @@ int kvmppc_mmu_map_segment(struct kvm_vcpu *vcpu, ulong eaddr)
 	slb_vsid &= ~SLB_VSID_KP;
 	slb_esid |= slb_index;
 
-	to_svcpu(vcpu)->slb[slb_index].esid = slb_esid;
-	to_svcpu(vcpu)->slb[slb_index].vsid = slb_vsid;
+	svcpu->slb[slb_index].esid = slb_esid;
+	svcpu->slb[slb_index].vsid = slb_vsid;
 
 	trace_kvm_book3s_slbmte(slb_vsid, slb_esid);
 
-	return 0;
+out:
+	svcpu_put(svcpu);
+	return r;
 }
 
 void kvmppc_mmu_flush_segments(struct kvm_vcpu *vcpu)
 {
-	to_svcpu(vcpu)->slb_max = 1;
-	to_svcpu(vcpu)->slb[0].esid = 0;
+	struct kvmppc_book3s_shadow_vcpu *svcpu = svcpu_get(vcpu);
+	svcpu->slb_max = 1;
+	svcpu->slb[0].esid = 0;
+	svcpu_put(svcpu);
 }
 
 void kvmppc_mmu_destroy(struct kvm_vcpu *vcpu)
diff --git a/arch/powerpc/kvm/book3s_emulate.c b/arch/powerpc/kvm/book3s_emulate.c
index 0c9dc62..f1950d1 100644
--- a/arch/powerpc/kvm/book3s_emulate.c
+++ b/arch/powerpc/kvm/book3s_emulate.c
@@ -230,9 +230,12 @@ int kvmppc_core_emulate_op(struct kvm_run *run, struct kvm_vcpu *vcpu,
 
 			r = kvmppc_st(vcpu, &addr, 32, zeros, true);
 			if ((r == -ENOENT) || (r == -EPERM)) {
+				struct kvmppc_book3s_shadow_vcpu *svcpu;
+
+				svcpu = svcpu_get(vcpu);
 				*advance = 0;
 				vcpu->arch.shared->dar = vaddr;
-				to_svcpu(vcpu)->fault_dar = vaddr;
+				svcpu->fault_dar = vaddr;
 
 				dsisr = DSISR_ISSTORE;
 				if (r == -ENOENT)
@@ -241,7 +244,8 @@ int kvmppc_core_emulate_op(struct kvm_run *run, struct kvm_vcpu *vcpu,
 					dsisr |= DSISR_PROTFAULT;
 
 				vcpu->arch.shared->dsisr = dsisr;
-				to_svcpu(vcpu)->fault_dsisr = dsisr;
+				svcpu->fault_dsisr = dsisr;
+				svcpu_put(svcpu);
 
 				kvmppc_book3s_queue_irqprio(vcpu,
 					BOOK3S_INTERRUPT_DATA_STORAGE);
diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
index 40ce940..ae6a034 100644
--- a/arch/powerpc/kvm/book3s_pr.c
+++ b/arch/powerpc/kvm/book3s_pr.c
@@ -56,10 +56,12 @@ static int kvmppc_handle_ext(struct kvm_vcpu *vcpu, unsigned int exit_nr,
 void kvmppc_core_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 {
 #ifdef CONFIG_PPC_BOOK3S_64
-	memcpy(to_svcpu(vcpu)->slb, to_book3s(vcpu)->slb_shadow, sizeof(to_svcpu(vcpu)->slb));
+	struct kvmppc_book3s_shadow_vcpu *svcpu = svcpu_get(vcpu);
+	memcpy(svcpu->slb, to_book3s(vcpu)->slb_shadow, sizeof(svcpu->slb));
 	memcpy(&get_paca()->shadow_vcpu, to_book3s(vcpu)->shadow_vcpu,
 	       sizeof(get_paca()->shadow_vcpu));
-	to_svcpu(vcpu)->slb_max = to_book3s(vcpu)->slb_shadow_max;
+	svcpu->slb_max = to_book3s(vcpu)->slb_shadow_max;
+	svcpu_put(svcpu);
 #endif
 
 #ifdef CONFIG_PPC_BOOK3S_32
@@ -70,10 +72,12 @@ void kvmppc_core_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 void kvmppc_core_vcpu_put(struct kvm_vcpu *vcpu)
 {
 #ifdef CONFIG_PPC_BOOK3S_64
-	memcpy(to_book3s(vcpu)->slb_shadow, to_svcpu(vcpu)->slb, sizeof(to_svcpu(vcpu)->slb));
+	struct kvmppc_book3s_shadow_vcpu *svcpu = svcpu_get(vcpu);
+	memcpy(to_book3s(vcpu)->slb_shadow, svcpu->slb, sizeof(svcpu->slb));
 	memcpy(to_book3s(vcpu)->shadow_vcpu, &get_paca()->shadow_vcpu,
 	       sizeof(get_paca()->shadow_vcpu));
-	to_book3s(vcpu)->slb_shadow_max = to_svcpu(vcpu)->slb_max;
+	to_book3s(vcpu)->slb_shadow_max = svcpu->slb_max;
+	svcpu_put(svcpu);
 #endif
 
 	kvmppc_giveup_ext(vcpu, MSR_FP);
@@ -310,19 +314,22 @@ int kvmppc_handle_pagefault(struct kvm_run *run, struct kvm_vcpu *vcpu,
 
 	if (page_found == -ENOENT) {
 		/* Page not found in guest PTE entries */
+		struct kvmppc_book3s_shadow_vcpu *svcpu = svcpu_get(vcpu);
 		vcpu->arch.shared->dar = kvmppc_get_fault_dar(vcpu);
-		vcpu->arch.shared->dsisr = to_svcpu(vcpu)->fault_dsisr;
+		vcpu->arch.shared->dsisr = svcpu->fault_dsisr;
 		vcpu->arch.shared->msr |=
-			(to_svcpu(vcpu)->shadow_srr1 & 0x00000000f8000000ULL);
+			(svcpu->shadow_srr1 & 0x00000000f8000000ULL);
+		svcpu_put(svcpu);
 		kvmppc_book3s_queue_irqprio(vcpu, vec);
 	} else if (page_found == -EPERM) {
 		/* Storage protection */
+		struct kvmppc_book3s_shadow_vcpu *svcpu = svcpu_get(vcpu);
 		vcpu->arch.shared->dar = kvmppc_get_fault_dar(vcpu);
-		vcpu->arch.shared->dsisr =
-			to_svcpu(vcpu)->fault_dsisr & ~DSISR_NOHPTE;
+		vcpu->arch.shared->dsisr = svcpu->fault_dsisr & ~DSISR_NOHPTE;
 		vcpu->arch.shared->dsisr |= DSISR_PROTFAULT;
 		vcpu->arch.shared->msr |=
-			(to_svcpu(vcpu)->shadow_srr1 & 0x00000000f8000000ULL);
+			svcpu->shadow_srr1 & 0x00000000f8000000ULL;
+		svcpu_put(svcpu);
 		kvmppc_book3s_queue_irqprio(vcpu, vec);
 	} else if (page_found == -EINVAL) {
 		/* Page not found in guest SLB */
@@ -523,21 +530,25 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
 	kvm_resched(vcpu);
 	switch (exit_nr) {
 	case BOOK3S_INTERRUPT_INST_STORAGE:
+	{
+		struct kvmppc_book3s_shadow_vcpu *svcpu = svcpu_get(vcpu);
+		ulong shadow_srr1 = svcpu->shadow_srr1;
 		vcpu->stat.pf_instruc++;
 
 #ifdef CONFIG_PPC_BOOK3S_32
 		/* We set segments as unused segments when invalidating them. So
 		 * treat the respective fault as segment fault. */
-		if (to_svcpu(vcpu)->sr[kvmppc_get_pc(vcpu) >> SID_SHIFT]
-		    == SR_INVALID) {
+		if (svcpu->sr[kvmppc_get_pc(vcpu) >> SID_SHIFT] == SR_INVALID) {
 			kvmppc_mmu_map_segment(vcpu, kvmppc_get_pc(vcpu));
 			r = RESUME_GUEST;
+			svcpu_put(svcpu);
 			break;
 		}
 #endif
+		svcpu_put(svcpu);
 
 		/* only care about PTEG not found errors, but leave NX alone */
-		if (to_svcpu(vcpu)->shadow_srr1 & 0x40000000) {
+		if (shadow_srr1 & 0x40000000) {
 			r = kvmppc_handle_pagefault(run, vcpu, kvmppc_get_pc(vcpu), exit_nr);
 			vcpu->stat.sp_instruc++;
 		} else if (vcpu->arch.mmu.is_dcbz32(vcpu) &&
@@ -550,33 +561,37 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
 			kvmppc_mmu_pte_flush(vcpu, kvmppc_get_pc(vcpu), ~0xFFFUL);
 			r = RESUME_GUEST;
 		} else {
-			vcpu->arch.shared->msr |=
-				to_svcpu(vcpu)->shadow_srr1 & 0x58000000;
+			vcpu->arch.shared->msr |= shadow_srr1 & 0x58000000;
 			kvmppc_book3s_queue_irqprio(vcpu, exit_nr);
 			r = RESUME_GUEST;
 		}
 		break;
+	}
 	case BOOK3S_INTERRUPT_DATA_STORAGE:
 	{
 		ulong dar = kvmppc_get_fault_dar(vcpu);
+		struct kvmppc_book3s_shadow_vcpu *svcpu = svcpu_get(vcpu);
+		u32 fault_dsisr = svcpu->fault_dsisr;
 		vcpu->stat.pf_storage++;
 
 #ifdef CONFIG_PPC_BOOK3S_32
 		/* We set segments as unused segments when invalidating them. So
 		 * treat the respective fault as segment fault. */
-		if ((to_svcpu(vcpu)->sr[dar >> SID_SHIFT]) == SR_INVALID) {
+		if ((svcpu->sr[dar >> SID_SHIFT]) == SR_INVALID) {
 			kvmppc_mmu_map_segment(vcpu, dar);
 			r = RESUME_GUEST;
+			svcpu_put(svcpu);
 			break;
 		}
 #endif
+		svcpu_put(svcpu);
 
 		/* The only case we need to handle is missing shadow PTEs */
-		if (to_svcpu(vcpu)->fault_dsisr & DSISR_NOHPTE) {
+		if (fault_dsisr & DSISR_NOHPTE) {
 			r = kvmppc_handle_pagefault(run, vcpu, dar, exit_nr);
 		} else {
 			vcpu->arch.shared->dar = dar;
-			vcpu->arch.shared->dsisr = to_svcpu(vcpu)->fault_dsisr;
+			vcpu->arch.shared->dsisr = fault_dsisr;
 			kvmppc_book3s_queue_irqprio(vcpu, exit_nr);
 			r = RESUME_GUEST;
 		}
@@ -612,10 +627,13 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
 	case BOOK3S_INTERRUPT_PROGRAM:
 	{
 		enum emulation_result er;
+		struct kvmppc_book3s_shadow_vcpu *svcpu;
 		ulong flags;
 
 program_interrupt:
-		flags = to_svcpu(vcpu)->shadow_srr1 & 0x1f0000ull;
+		svcpu = svcpu_get(vcpu);
+		flags = svcpu->shadow_srr1 & 0x1f0000ull;
+		svcpu_put(svcpu);
 
 		if (vcpu->arch.shared->msr & MSR_PR) {
 #ifdef EXIT_DEBUG
@@ -743,14 +761,18 @@ program_interrupt:
 		r = RESUME_GUEST;
 		break;
 	default:
+	{
+		struct kvmppc_book3s_shadow_vcpu *svcpu = svcpu_get(vcpu);
+		ulong shadow_srr1 = svcpu->shadow_srr1;
+		svcpu_put(svcpu);
 		/* Ugh - bork here! What did we get? */
 		printk(KERN_EMERG "exit_nr=0x%x | pc=0x%lx | msr=0x%lx\n",
-			exit_nr, kvmppc_get_pc(vcpu), to_svcpu(vcpu)->shadow_srr1);
+			exit_nr, kvmppc_get_pc(vcpu), shadow_srr1);
 		r = RESUME_HOST;
 		BUG();
 		break;
 	}
-
+	}
 
 	if (!(r & RESUME_HOST)) {
 		/* To avoid clobbering exit_reason, only check for signals if
diff --git a/arch/powerpc/kvm/trace.h b/arch/powerpc/kvm/trace.h
index b135d3d..609d8bf 100644
--- a/arch/powerpc/kvm/trace.h
+++ b/arch/powerpc/kvm/trace.h
@@ -118,11 +118,14 @@ TRACE_EVENT(kvm_book3s_exit,
 	),
 
 	TP_fast_assign(
+		struct kvmppc_book3s_shadow_vcpu *svcpu;
 		__entry->exit_nr	= exit_nr;
 		__entry->pc		= kvmppc_get_pc(vcpu);
 		__entry->dar		= kvmppc_get_fault_dar(vcpu);
 		__entry->msr		= vcpu->arch.shared->msr;
-		__entry->srr1		= to_svcpu(vcpu)->shadow_srr1;
+		svcpu = svcpu_get(vcpu);
+		__entry->srr1		= svcpu->shadow_srr1;
+		svcpu_put(svcpu);
 	),
 
 	TP_printk("exit=0x%x | pc=0x%lx | msr=0x%lx | dar=0x%lx | srr1=0x%lx",
-- 
1.6.0.2

[PATCH 4/4] KVM: PPC: align vcpu_kick with x86

[Alexander Graf]

Our vcpu kick implementation differs a bit from x86 which resulted in us not
disabling preemption during the kick. Get it a bit closer to what x86 does.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/kvm/powerpc.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index c952f13..ef8c990 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -557,12 +557,17 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 
 void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
 {
+        int me;
+        int cpu = vcpu->cpu;
+
+        me = get_cpu();
 	if (waitqueue_active(&vcpu->wq)) {
 		wake_up_interruptible(vcpu->arch.wqp);
 		vcpu->stat.halt_wakeup++;
-	} else if (vcpu->cpu != -1) {
+	} else if (cpu != me && cpu != -1) {
 		smp_send_reschedule(vcpu->cpu);
 	}
+        put_cpu();
 }
 
 int kvm_vcpu_ioctl_interrupt(struct kvm_vcpu *vcpu, struct kvm_interrupt *irq)
-- 
1.6.0.2

Re: [PATCH 4/4] KVM: PPC: align vcpu_kick with x86

[Scott Wood]

On 12/09/2011 09:26 AM, Alexander Graf wrote:
> Our vcpu kick implementation differs a bit from x86 which resulted in us not
> disabling preemption during the kick. Get it a bit closer to what x86 does.

Disabling preemption only matters due to the other bit of functionality
you brought over -- avoiding kicking the current CPU.

Probably doesn't even matter all that much with it, since avoiding that
is just an optimization, and any race that causes us to fail to
reschedule a vcpu of a different thread means that thread just
rescheduled anyway.  Not something I feel any great need to rely on,
though. :-)

> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  arch/powerpc/kvm/powerpc.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index c952f13..ef8c990 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -557,12 +557,17 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  
>  void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
>  {
> +        int me;
> +        int cpu = vcpu->cpu;
> +
> +        me = get_cpu();
>  	if (waitqueue_active(&vcpu->wq)) {
>  		wake_up_interruptible(vcpu->arch.wqp);
>  		vcpu->stat.halt_wakeup++;
> -	} else if (vcpu->cpu != -1) {
> +	} else if (cpu != me && cpu != -1) {
>  		smp_send_reschedule(vcpu->cpu);
>  	}
> +        put_cpu();
>  }

Whitespace.

-Scott

Re: [PATCH 1/4] KVM: PPC: Book3s: PR: Disable preemption in vcpu_run

[Scott Wood]

On 12/09/2011 09:26 AM, Alexander Graf wrote:
> When entering the guest, we want to make sure we're not getting preempted
> away, so let's disable preemption on entry, but enable it again while handling
> guest exits.
> 
> Reported-by: Jörg Sommer <joerg@alea.gnuu.de>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  arch/powerpc/kvm/book3s_pr.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
> index 726512b..8e4f800 100644
> --- a/arch/powerpc/kvm/book3s_pr.c
> +++ b/arch/powerpc/kvm/book3s_pr.c
> @@ -519,6 +519,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
>  	run->ready_for_interrupt_injection = 1;
>  
>  	trace_kvm_book3s_exit(exit_nr, vcpu);
> +	preempt_enable();
>  	kvm_resched(vcpu);
>  	switch (exit_nr) {
>  	case BOOK3S_INTERRUPT_INST_STORAGE:
> @@ -763,6 +764,8 @@ program_interrupt:
>  			run->exit_reason = KVM_EXIT_INTR;
>  			r = -EINTR;
>  		} else {
> +			preempt_disable();
> +
>  			/* In case an interrupt came in that was triggered
>  			 * from userspace (like DEC), we need to check what
>  			 * to inject now! */

Shouldn't you really have interrupts disabled here, as booke does?

Otherwise an interrupt (including an IPI kick) could send you a signal
or guest exception after you check.

Likewise for other guest entry points.

-Scott

Re: [PATCH 4/4] KVM: PPC: align vcpu_kick with x86

[Alexander Graf]

On 09.12.2011, at 19:19, Scott Wood <scottwood@freescale.com> wrote:

> On 12/09/2011 09:26 AM, Alexander Graf wrote:
>> Our vcpu kick implementation differs a bit from x86 which resulted in us not
>> disabling preemption during the kick. Get it a bit closer to what x86 does.
> 
> Disabling preemption only matters due to the other bit of functionality
> you brought over -- avoiding kicking the current CPU.

Nope, I had BUG_ON warnings in the kick code because preemption was on. get_cpu() disables preemption indirectly though, getting rid of the oops :)

Alex

Re: [PATCH 4/4] KVM: PPC: align vcpu_kick with x86

[Scott Wood]

On 12/09/2011 01:10 PM, Alexander Graf wrote:
> 
> On 09.12.2011, at 19:19, Scott Wood <scottwood@freescale.com> wrote:
> 
>> On 12/09/2011 09:26 AM, Alexander Graf wrote:
>>> Our vcpu kick implementation differs a bit from x86 which resulted in us not
>>> disabling preemption during the kick. Get it a bit closer to what x86 does.
>>
>> Disabling preemption only matters due to the other bit of functionality
>> you brought over -- avoiding kicking the current CPU.
> 
> Nope, I had BUG_ON warnings in the kick code because preemption was on.

Coming from where?

-Scott

Re: [PATCH 4/4] KVM: PPC: align vcpu_kick with x86

[Alexander Graf]

On 09.12.2011, at 20:15, Scott Wood wrote:

> On 12/09/2011 01:10 PM, Alexander Graf wrote:
>> 
>> On 09.12.2011, at 19:19, Scott Wood <scottwood@freescale.com> wrote:
>> 
>>> On 12/09/2011 09:26 AM, Alexander Graf wrote:
>>>> Our vcpu kick implementation differs a bit from x86 which resulted in us not
>>>> disabling preemption during the kick. Get it a bit closer to what x86 does.
>>> 
>>> Disabling preemption only matters due to the other bit of functionality
>>> you brought over -- avoiding kicking the current CPU.
>> 
>> Nope, I had BUG_ON warnings in the kick code because preemption was on.
> 
> Coming from where?

>From here:

BUG: using smp_processor_id() in preemptible [00000000] code: qemu-system-ppc/17448
caller is .smp_mpic_message_pass+0x88/0x10c
Call Trace:
[c000000078d83600] [c000000000013e70] .show_stack+0x6c/0x16c (unreliable)
[c000000078d836b0] [c00000000037b614] .debug_smp_processor_id+0xe4/0x11c
[c000000078d83740] [c000000000048988] .smp_mpic_message_pass+0x88/0x10c
[c000000078d837d0] [c00000000002f2b4] .smp_send_reschedule+0x4c/0x80
[c000000078d83850] [d000000005e68984] .kvm_vcpu_kick+0x5c/0x74 [kvm]
[c000000078d838d0] [d000000005e689d8] .kvm_vcpu_ioctl_interrupt+0x3c/0x54 [kvm]
[c000000078d83950] [d000000005e68a8c] .kvm_arch_vcpu_ioctl+0x9c/0x21c [kvm]
[c000000078d83b60] [d000000005e64520] .kvm_vcpu_ioctl+0x7c/0x6b8 [kvm]
[c000000078d83c20] [d000000005e64c2c] .kvm_vcpu_compat_ioctl+0xd0/0xfc [kvm]
[c000000078d83cd0] [c0000000001be750] .compat_sys_ioctl+0x160/0x1864
[c000000078d83e30] [c00000000000988c] syscall_exit+0x0/0x40


Alex

Re: [PATCH 1/4] KVM: PPC: Book3s: PR: Disable preemption in vcpu_run

[Alexander Graf]

On 09.12.2011, at 19:19, Scott Wood wrote:

> On 12/09/2011 09:26 AM, Alexander Graf wrote:
>> When entering the guest, we want to make sure we're not getting preempted
>> away, so let's disable preemption on entry, but enable it again while handling
>> guest exits.
>> 
>> Reported-by: Jörg Sommer <joerg@alea.gnuu.de>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> arch/powerpc/kvm/book3s_pr.c |    7 +++++++
>> 1 files changed, 7 insertions(+), 0 deletions(-)
>> 
>> diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
>> index 726512b..8e4f800 100644
>> --- a/arch/powerpc/kvm/book3s_pr.c
>> +++ b/arch/powerpc/kvm/book3s_pr.c
>> @@ -519,6 +519,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
>> 	run->ready_for_interrupt_injection = 1;
>> 
>> 	trace_kvm_book3s_exit(exit_nr, vcpu);
>> +	preempt_enable();
>> 	kvm_resched(vcpu);
>> 	switch (exit_nr) {
>> 	case BOOK3S_INTERRUPT_INST_STORAGE:
>> @@ -763,6 +764,8 @@ program_interrupt:
>> 			run->exit_reason = KVM_EXIT_INTR;
>> 			r = -EINTR;
>> 		} else {
>> +			preempt_disable();
>> +
>> 			/* In case an interrupt came in that was triggered
>> 			 * from userspace (like DEC), we need to check what
>> 			 * to inject now! */
> 
> Shouldn't you really have interrupts disabled here, as booke does?

Ah, thanks for the reminder. Yeah, we probably want to disable interrupts in parallel to checking for signals (basically from one signal check point to world switch). I'm just not 100% sure how to easily sync the C and asm code on the first entry though. Doing local_irq_disable in C and undoing it in asm could become ugly with lazy interrupt disabling.


Alex

Re: [PATCH 4/4] KVM: PPC: align vcpu_kick with x86

[Scott Wood]

On 12/09/2011 05:15 PM, Alexander Graf wrote:
> 
> On 09.12.2011, at 20:15, Scott Wood wrote:
> 
>> On 12/09/2011 01:10 PM, Alexander Graf wrote:
>>>
>>> On 09.12.2011, at 19:19, Scott Wood <scottwood@freescale.com> wrote:
>>>
>>>> On 12/09/2011 09:26 AM, Alexander Graf wrote:
>>>>> Our vcpu kick implementation differs a bit from x86 which resulted in us not
>>>>> disabling preemption during the kick. Get it a bit closer to what x86 does.
>>>>
>>>> Disabling preemption only matters due to the other bit of functionality
>>>> you brought over -- avoiding kicking the current CPU.
>>>
>>> Nope, I had BUG_ON warnings in the kick code because preemption was on.
>>
>> Coming from where?
> 
> From here:
> 
> BUG: using smp_processor_id() in preemptible [00000000] code: qemu-system-ppc/17448
> caller is .smp_mpic_message_pass+0x88/0x10c
> Call Trace:
> [c000000078d83600] [c000000000013e70] .show_stack+0x6c/0x16c (unreliable)
> [c000000078d836b0] [c00000000037b614] .debug_smp_processor_id+0xe4/0x11c
> [c000000078d83740] [c000000000048988] .smp_mpic_message_pass+0x88/0x10c
> [c000000078d837d0] [c00000000002f2b4] .smp_send_reschedule+0x4c/0x80
> [c000000078d83850] [d000000005e68984] .kvm_vcpu_kick+0x5c/0x74 [kvm]
> [c000000078d838d0] [d000000005e689d8] .kvm_vcpu_ioctl_interrupt+0x3c/0x54 [kvm]
> [c000000078d83950] [d000000005e68a8c] .kvm_arch_vcpu_ioctl+0x9c/0x21c [kvm]
> [c000000078d83b60] [d000000005e64520] .kvm_vcpu_ioctl+0x7c/0x6b8 [kvm]
> [c000000078d83c20] [d000000005e64c2c] .kvm_vcpu_compat_ioctl+0xd0/0xfc [kvm]
> [c000000078d83cd0] [c0000000001be750] .compat_sys_ioctl+0x160/0x1864
> [c000000078d83e30] [c00000000000988c] syscall_exit+0x0/0x40

Hmm, unless smp_send_reschedule must be called with preemption disabled
(doesn't seem like an obvious requirement, even if callers commonly have
a reason to do so, and the only documentation of it I see is on the ia64
implementation), that seems like a bug in the MPIC IPI implementation.

It wouldn't be an issue (and would make the MPIC driver slightly faster,
too) if the MPIC driver were to use the magic CPU region instead of
manually indexing the non-magic array of per-CPU regions.  Maybe it
doesn't work on all hardware the driver supports?

Not that it matters much here -- we want this patch anyway, because we
want to check that it's not the current CPU.

-Scott

Re: [PATCH 1/4] KVM: PPC: Book3s: PR: Disable preemption in vcpu_run

[Scott Wood]

On 12/09/2011 05:18 PM, Alexander Graf wrote:
> 
> On 09.12.2011, at 19:19, Scott Wood wrote:
> 
>> Shouldn't you really have interrupts disabled here, as booke does?
> 
> Ah, thanks for the reminder. Yeah, we probably want to disable
> interrupts in parallel to checking for signals (basically from one
> signal check point to world switch). I'm just not 100% sure how to
> easily sync the C and asm code on the first entry though. Doing
> local_irq_disable in C and undoing it in asm could become ugly with
> lazy interrupt disabling.

Lots of things get ugly with lazy interrupt disabling. :-(

There should be other examples of handling the lazy EE stuff in asm
code.  After you hard-disable, you should just need to poke the right
fields in the PACA with the state you plan to end up in after the rfi,
similar to exception return.

-Scott

Re: [PATCH 4/4] KVM: PPC: align vcpu_kick with x86

[Alexander Graf]

On 12.12.2011, at 18:32, Scott Wood wrote:

> On 12/09/2011 05:15 PM, Alexander Graf wrote:
>> 
>> On 09.12.2011, at 20:15, Scott Wood wrote:
>> 
>>> On 12/09/2011 01:10 PM, Alexander Graf wrote:
>>>> 
>>>> On 09.12.2011, at 19:19, Scott Wood <scottwood@freescale.com> wrote:
>>>> 
>>>>> On 12/09/2011 09:26 AM, Alexander Graf wrote:
>>>>>> Our vcpu kick implementation differs a bit from x86 which resulted in us not
>>>>>> disabling preemption during the kick. Get it a bit closer to what x86 does.
>>>>> 
>>>>> Disabling preemption only matters due to the other bit of functionality
>>>>> you brought over -- avoiding kicking the current CPU.
>>>> 
>>>> Nope, I had BUG_ON warnings in the kick code because preemption was on.
>>> 
>>> Coming from where?
>> 
>> From here:
>> 
>> BUG: using smp_processor_id() in preemptible [00000000] code: qemu-system-ppc/17448
>> caller is .smp_mpic_message_pass+0x88/0x10c
>> Call Trace:
>> [c000000078d83600] [c000000000013e70] .show_stack+0x6c/0x16c (unreliable)
>> [c000000078d836b0] [c00000000037b614] .debug_smp_processor_id+0xe4/0x11c
>> [c000000078d83740] [c000000000048988] .smp_mpic_message_pass+0x88/0x10c
>> [c000000078d837d0] [c00000000002f2b4] .smp_send_reschedule+0x4c/0x80
>> [c000000078d83850] [d000000005e68984] .kvm_vcpu_kick+0x5c/0x74 [kvm]
>> [c000000078d838d0] [d000000005e689d8] .kvm_vcpu_ioctl_interrupt+0x3c/0x54 [kvm]
>> [c000000078d83950] [d000000005e68a8c] .kvm_arch_vcpu_ioctl+0x9c/0x21c [kvm]
>> [c000000078d83b60] [d000000005e64520] .kvm_vcpu_ioctl+0x7c/0x6b8 [kvm]
>> [c000000078d83c20] [d000000005e64c2c] .kvm_vcpu_compat_ioctl+0xd0/0xfc [kvm]
>> [c000000078d83cd0] [c0000000001be750] .compat_sys_ioctl+0x160/0x1864
>> [c000000078d83e30] [c00000000000988c] syscall_exit+0x0/0x40
> 
> Hmm, unless smp_send_reschedule must be called with preemption disabled
> (doesn't seem like an obvious requirement, even if callers commonly have
> a reason to do so, and the only documentation of it I see is on the ia64
> implementation), that seems like a bug in the MPIC IPI implementation.
> 
> It wouldn't be an issue (and would make the MPIC driver slightly faster,
> too) if the MPIC driver were to use the magic CPU region instead of
> manually indexing the non-magic array of per-CPU regions.  Maybe it
> doesn't work on all hardware the driver supports?

I'd say that's a question I'll redirect to Ben :).

> Not that it matters much here -- we want this patch anyway, because we
> want to check that it's not the current CPU.

Yeah. Well - eventually we want the exact same vcpu_kick function across architectures. But this is a good step in the right direction.


Alex

Re: [PATCH 4/4] KVM: PPC: align vcpu_kick with x86

[Benjamin Herrenschmidt]

On Fri, 2011-12-23 at 13:56 +0100, Alexander Graf wrote:

> > Hmm, unless smp_send_reschedule must be called with preemption disabled
> > (doesn't seem like an obvious requirement, even if callers commonly have
> > a reason to do so, and the only documentation of it I see is on the ia64
> > implementation), that seems like a bug in the MPIC IPI implementation.
> > 
> > It wouldn't be an issue (and would make the MPIC driver slightly faster,
> > too) if the MPIC driver were to use the magic CPU region instead of
> > manually indexing the non-magic array of per-CPU regions.  Maybe it
> > doesn't work on all hardware the driver supports?

Right, the magic per-cpu region doesn't work on all HW.

> I'd say that's a question I'll redirect to Ben :).
> 
> > Not that it matters much here -- we want this patch anyway, because we
> > want to check that it's not the current CPU.
> 
> Yeah. Well - eventually we want the exact same vcpu_kick function across architectures.
> But this is a good step in the right direction.

Cheers,
Ben.