* [PATCH 0/4] Fix book3s-pr KVM with preemption
@ 2011-12-09 15:26 Alexander Graf
2011-12-09 15:26 ` [PATCH 1/4] KVM: PPC: Book3s: PR: Disable preemption in vcpu_run Alexander Graf
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: Alexander Graf @ 2011-12-09 15:26 UTC (permalink / raw)
To: kvm-ppc; +Cc: Jörg Sommer, KVM list
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(-)
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH 1/4] KVM: PPC: Book3s: PR: Disable preemption in vcpu_run 2011-12-09 15:26 [PATCH 0/4] Fix book3s-pr KVM with preemption Alexander Graf @ 2011-12-09 15:26 ` Alexander Graf 2011-12-09 18:19 ` Scott Wood 2011-12-09 15:26 ` [PATCH 2/4] KVM: PPC: Book3s: PR: No irq_disable " Alexander Graf ` (2 subsequent siblings) 3 siblings, 1 reply; 15+ messages in thread From: Alexander Graf @ 2011-12-09 15:26 UTC (permalink / raw) To: kvm-ppc; +Cc: Jörg Sommer, KVM list 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] KVM: PPC: Book3s: PR: Disable preemption in vcpu_run 2011-12-09 15:26 ` [PATCH 1/4] KVM: PPC: Book3s: PR: Disable preemption in vcpu_run Alexander Graf @ 2011-12-09 18:19 ` Scott Wood 2011-12-09 23:18 ` Alexander Graf 0 siblings, 1 reply; 15+ messages in thread From: Scott Wood @ 2011-12-09 18:19 UTC (permalink / raw) To: Alexander Graf; +Cc: kvm-ppc, Jörg Sommer, KVM list 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] KVM: PPC: Book3s: PR: Disable preemption in vcpu_run 2011-12-09 18:19 ` Scott Wood @ 2011-12-09 23:18 ` Alexander Graf 2011-12-13 21:15 ` Scott Wood 0 siblings, 1 reply; 15+ messages in thread From: Alexander Graf @ 2011-12-09 23:18 UTC (permalink / raw) To: Scott Wood; +Cc: kvm-ppc, Jörg Sommer, KVM list 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] KVM: PPC: Book3s: PR: Disable preemption in vcpu_run 2011-12-09 23:18 ` Alexander Graf @ 2011-12-13 21:15 ` Scott Wood 0 siblings, 0 replies; 15+ messages in thread From: Scott Wood @ 2011-12-13 21:15 UTC (permalink / raw) To: Alexander Graf; +Cc: kvm-ppc, Jörg Sommer, KVM list 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/4] KVM: PPC: Book3s: PR: No irq_disable in vcpu_run 2011-12-09 15:26 [PATCH 0/4] Fix book3s-pr KVM with preemption Alexander Graf 2011-12-09 15:26 ` [PATCH 1/4] KVM: PPC: Book3s: PR: Disable preemption in vcpu_run Alexander Graf @ 2011-12-09 15:26 ` Alexander Graf 2011-12-09 15:26 ` [PATCH 3/4] KVM: PPC: Use get/set for to_svcpu to help preemption Alexander Graf 2011-12-09 15:26 ` [PATCH 4/4] KVM: PPC: align vcpu_kick with x86 Alexander Graf 3 siblings, 0 replies; 15+ messages in thread From: Alexander Graf @ 2011-12-09 15:26 UTC (permalink / raw) To: kvm-ppc; +Cc: Jörg Sommer, KVM list 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 3/4] KVM: PPC: Use get/set for to_svcpu to help preemption 2011-12-09 15:26 [PATCH 0/4] Fix book3s-pr KVM with preemption Alexander Graf 2011-12-09 15:26 ` [PATCH 1/4] KVM: PPC: Book3s: PR: Disable preemption in vcpu_run Alexander Graf 2011-12-09 15:26 ` [PATCH 2/4] KVM: PPC: Book3s: PR: No irq_disable " Alexander Graf @ 2011-12-09 15:26 ` Alexander Graf 2011-12-09 15:26 ` [PATCH 4/4] KVM: PPC: align vcpu_kick with x86 Alexander Graf 3 siblings, 0 replies; 15+ messages in thread From: Alexander Graf @ 2011-12-09 15:26 UTC (permalink / raw) To: kvm-ppc; +Cc: Jörg Sommer, KVM list 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 4/4] KVM: PPC: align vcpu_kick with x86 2011-12-09 15:26 [PATCH 0/4] Fix book3s-pr KVM with preemption Alexander Graf ` (2 preceding siblings ...) 2011-12-09 15:26 ` [PATCH 3/4] KVM: PPC: Use get/set for to_svcpu to help preemption Alexander Graf @ 2011-12-09 15:26 ` Alexander Graf 2011-12-09 18:19 ` Scott Wood 3 siblings, 1 reply; 15+ messages in thread From: Alexander Graf @ 2011-12-09 15:26 UTC (permalink / raw) To: kvm-ppc; +Cc: Jörg Sommer, KVM list 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 4/4] KVM: PPC: align vcpu_kick with x86 2011-12-09 15:26 ` [PATCH 4/4] KVM: PPC: align vcpu_kick with x86 Alexander Graf @ 2011-12-09 18:19 ` Scott Wood 2011-12-09 19:10 ` Alexander Graf 0 siblings, 1 reply; 15+ messages in thread From: Scott Wood @ 2011-12-09 18:19 UTC (permalink / raw) To: Alexander Graf; +Cc: kvm-ppc, Jörg Sommer, KVM list 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/4] KVM: PPC: align vcpu_kick with x86 2011-12-09 18:19 ` Scott Wood @ 2011-12-09 19:10 ` Alexander Graf 2011-12-09 19:15 ` Scott Wood 0 siblings, 1 reply; 15+ messages in thread From: Alexander Graf @ 2011-12-09 19:10 UTC (permalink / raw) To: Scott Wood; +Cc: <kvm-ppc@vger.kernel.org>, Jörg Sommer, KVM list 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/4] KVM: PPC: align vcpu_kick with x86 2011-12-09 19:10 ` Alexander Graf @ 2011-12-09 19:15 ` Scott Wood 2011-12-09 23:15 ` Alexander Graf 0 siblings, 1 reply; 15+ messages in thread From: Scott Wood @ 2011-12-09 19:15 UTC (permalink / raw) To: Alexander Graf Cc: <kvm-ppc@vger.kernel.org>, Jörg Sommer, KVM list 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/4] KVM: PPC: align vcpu_kick with x86 2011-12-09 19:15 ` Scott Wood @ 2011-12-09 23:15 ` Alexander Graf 2011-12-12 17:32 ` Scott Wood 0 siblings, 1 reply; 15+ messages in thread From: Alexander Graf @ 2011-12-09 23:15 UTC (permalink / raw) To: Scott Wood; +Cc: <kvm-ppc@vger.kernel.org>, Jörg Sommer, KVM list 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/4] KVM: PPC: align vcpu_kick with x86 2011-12-09 23:15 ` Alexander Graf @ 2011-12-12 17:32 ` Scott Wood 2011-12-23 12:56 ` Alexander Graf 0 siblings, 1 reply; 15+ messages in thread From: Scott Wood @ 2011-12-12 17:32 UTC (permalink / raw) To: Alexander Graf Cc: <kvm-ppc@vger.kernel.org>, Jörg Sommer, KVM list 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/4] KVM: PPC: align vcpu_kick with x86 2011-12-12 17:32 ` Scott Wood @ 2011-12-23 12:56 ` Alexander Graf 2011-12-23 21:50 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 15+ messages in thread From: Alexander Graf @ 2011-12-23 12:56 UTC (permalink / raw) To: Scott Wood; +Cc: kvm-ppc, Jörg Sommer, KVM list, Benjamin Herrenschmidt 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/4] KVM: PPC: align vcpu_kick with x86 2011-12-23 12:56 ` Alexander Graf @ 2011-12-23 21:50 ` Benjamin Herrenschmidt 0 siblings, 0 replies; 15+ messages in thread From: Benjamin Herrenschmidt @ 2011-12-23 21:50 UTC (permalink / raw) To: Alexander Graf; +Cc: Scott Wood, kvm-ppc, Jörg Sommer, KVM list 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2011-12-23 21:50 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-12-09 15:26 [PATCH 0/4] Fix book3s-pr KVM with preemption Alexander Graf 2011-12-09 15:26 ` [PATCH 1/4] KVM: PPC: Book3s: PR: Disable preemption in vcpu_run Alexander Graf 2011-12-09 18:19 ` Scott Wood 2011-12-09 23:18 ` Alexander Graf 2011-12-13 21:15 ` Scott Wood 2011-12-09 15:26 ` [PATCH 2/4] KVM: PPC: Book3s: PR: No irq_disable " Alexander Graf 2011-12-09 15:26 ` [PATCH 3/4] KVM: PPC: Use get/set for to_svcpu to help preemption Alexander Graf 2011-12-09 15:26 ` [PATCH 4/4] KVM: PPC: align vcpu_kick with x86 Alexander Graf 2011-12-09 18:19 ` Scott Wood 2011-12-09 19:10 ` Alexander Graf 2011-12-09 19:15 ` Scott Wood 2011-12-09 23:15 ` Alexander Graf 2011-12-12 17:32 ` Scott Wood 2011-12-23 12:56 ` Alexander Graf 2011-12-23 21:50 ` Benjamin Herrenschmidt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).