| Message ID | 1367597470-22214-1-git-send-email-mihai.caraman@freescale.com |
|---|---|
| State | New, archived |
| Headers | show |
On 03.05.2013, at 18:11, Mihai Caraman wrote: > A change in the generic code highlighted that we were running with IRQs > (soft) enabled on Book3E 64-bit when trying to restart interrupts from > handle_exit(). This is a lesson to configure lockdep often :) > > There is no reason to exit guest with soft_enabled == 1, a local_irq_enable() > call will do this for us so get rid of kvmppc_layz_ee() calls. With this fix > we eliminate irqs_disabled() warnings and some guest and host hangs revealed > under stress tests, but guests still exhibit some unresponsiveness. > > The unresponsiveness has to do with the fact that arch_local_irq_restore() > does not guarantees to hard enable interrupts. To do so replace exception > function calls like timer_interrupt() with irq_happened flags. The > local_irq_enable() call takes care of replaying them and lets the interrupts > hard enabled. > > Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com> Ben, could you please review? Alex > --- > arch/powerpc/kvm/booke.c | 9 +++------ > 1 files changed, 3 insertions(+), 6 deletions(-) > > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c > index 1020119..82f155e 100644 > --- a/arch/powerpc/kvm/booke.c > +++ b/arch/powerpc/kvm/booke.c > @@ -673,7 +673,6 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu) > ret = s; > goto out; > } > - kvmppc_lazy_ee_enable(); > > kvm_guest_enter(); > > @@ -789,16 +788,16 @@ static void kvmppc_restart_interrupt(struct kvm_vcpu *vcpu, > switch (exit_nr) { > case BOOKE_INTERRUPT_EXTERNAL: > kvmppc_fill_pt_regs(®s); > - do_IRQ(®s); > + local_paca->irq_happened |= PACA_IRQ_EE; > break; > case BOOKE_INTERRUPT_DECREMENTER: > kvmppc_fill_pt_regs(®s); > - timer_interrupt(®s); > + local_paca->irq_happened |= PACA_IRQ_DEC; > break; > #if defined(CONFIG_PPC_FSL_BOOK3E) || defined(CONFIG_PPC_BOOK3E_64) > case BOOKE_INTERRUPT_DOORBELL: > kvmppc_fill_pt_regs(®s); > - doorbell_exception(®s); > + local_paca->irq_happened |= PACA_IRQ_DBELL; > break; > #endif > case BOOKE_INTERRUPT_MACHINE_CHECK: > @@ -1148,8 +1147,6 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu, > if (s <= 0) { > local_irq_enable(); > r = (s << 2) | RESUME_HOST | (r & RESUME_FLAG_NV); > - } else { > - kvmppc_lazy_ee_enable(); > } > } > > -- > 1.7.4.1 > > > -- > To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Friday, May 03, 2013 9:05 PM > To: Caraman Mihai Claudiu-B02008 > Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc- > dev@lists.ozlabs.org; Caraman Mihai Claudiu-B02008 > Subject: Re: [PATCH] KVM: PPC: Book3E 64: Fix IRQs warnings and hangs > > > The unresponsiveness has to do with the fact that > > arch_local_irq_restore() > > does not guarantees to hard enable interrupts. > > Could you elaborate? If the saved IRQ state was "enabled", why > wouldn't arch_local_irq_restore() hard-enable IRQs? The last thing it > does is __hard_irq_enable(). if (!irq_happened) return; > > Where is the arch_local_irq_restore() instance you're talking about? ./arch/power/kernel/irq.c > > > To do so replace exception > > function calls like timer_interrupt() with irq_happened flags. The > > local_irq_enable() call takes care of replaying them and lets the > > interrupts > > hard enabled. > > Not sure what you mean by "lets the interrupts hard enabled"... Do you > mean the EE bit in regs->msr, as opposed to the EE bit in the current > MSR? If irq_happened "the last thing it does is __hard_irq_enable()". > > @@ -789,16 +788,16 @@ static void kvmppc_restart_interrupt(struct > > kvm_vcpu *vcpu, > > switch (exit_nr) { > > case BOOKE_INTERRUPT_EXTERNAL: > > kvmppc_fill_pt_regs(®s); > > - do_IRQ(®s); > > + local_paca->irq_happened |= PACA_IRQ_EE; > > break; > > Aren't you breaking 32-bit here? I had eyes only for 64-bit hangs :) -Mike -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/03/2013 03:01:26 PM, Caraman Mihai Claudiu-B02008 wrote: > > -----Original Message----- > > From: Wood Scott-B07421 > > Sent: Friday, May 03, 2013 9:05 PM > > To: Caraman Mihai Claudiu-B02008 > > Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc- > > dev@lists.ozlabs.org; Caraman Mihai Claudiu-B02008 > > Subject: Re: [PATCH] KVM: PPC: Book3E 64: Fix IRQs warnings and > hangs > > > > > The unresponsiveness has to do with the fact that > > > arch_local_irq_restore() > > > does not guarantees to hard enable interrupts. > > > > Could you elaborate? If the saved IRQ state was "enabled", why > > wouldn't arch_local_irq_restore() hard-enable IRQs? The last thing > it > > does is __hard_irq_enable(). > > if (!irq_happened) > return; OK, so the problem is that we're not setting PACA_IRQ_HARD_DIS when we hard-disable interrupts? > > Where is the arch_local_irq_restore() instance you're talking about? > > ./arch/power/kernel/irq.c I meant the caller. :-P -Scott -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Friday, May 03, 2013 11:15 PM > To: Caraman Mihai Claudiu-B02008 > Cc: Wood Scott-B07421; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; > linuxppc-dev@lists.ozlabs.org > Subject: Re: [PATCH] KVM: PPC: Book3E 64: Fix IRQs warnings and hangs > > > > > The unresponsiveness has to do with the fact that > > > > arch_local_irq_restore() > > > > does not guarantees to hard enable interrupts. > > > > > > Could you elaborate? If the saved IRQ state was "enabled", why > > > wouldn't arch_local_irq_restore() hard-enable IRQs? The last thing > > it > > > does is __hard_irq_enable(). > > > > if (!irq_happened) > > return; > > OK, so the problem is that we're not setting PACA_IRQ_HARD_DIS when we > hard-disable interrupts? We enter guest with local_irq_disable() which means soft disabled, when do we hard-disable interrupts? If we follow host exception handlers model they set PACA_IRQ_EE/DEC/DBELL but not PACA_IRQ_HARD_DIS. Can you give it a try to see how KVM behaves with PACA_IRQ_HARD_DIS? I can't do it right now. > > > > Where is the arch_local_irq_restore() instance you're talking about? > > > > ./arch/power/kernel/irq.c > > I meant the caller. :-P ./arch/powerpc/include/asm/hw_irq.h 55static inline unsigned long arch_local_irq_disable(void) 56{ 57 unsigned long flags, zero; 58 59 asm volatile( 60 "li %1,0; lbz %0,%2(13); stb %1,%2(13)" 61 : "=r" (flags), "=&r" (zero) 62 : "i" (offsetof(struct paca_struct, soft_enabled)) 63 : "memory"); 64 65 return flags; 66} 67 68extern void arch_local_irq_restore(unsigned long); 69 70static inline void arch_local_irq_enable(void) 71{ 72 arch_local_irq_restore(1); 73} -Mike -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2013-05-03 at 18:24 +0200, Alexander Graf wrote: > > There is no reason to exit guest with soft_enabled == 1, a local_irq_enable() > > call will do this for us so get rid of kvmppc_layz_ee() calls. With this fix > > we eliminate irqs_disabled() warnings and some guest and host hangs revealed > > under stress tests, but guests still exhibit some unresponsiveness. > > > > The unresponsiveness has to do with the fact that arch_local_irq_restore() > > does not guarantees to hard enable interrupts. To do so replace exception > > function calls like timer_interrupt() with irq_happened flags. The > > local_irq_enable() call takes care of replaying them and lets the interrupts > > hard enabled. > > > > Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com> > > Ben, could you please review? That does look like the right thing to do indeed. Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> Cheers, Ben. -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Saturday, May 04, 2013 1:07 AM > To: Caraman Mihai Claudiu-B02008 > Cc: Wood Scott-B07421; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; > linuxppc-dev@lists.ozlabs.org > Subject: Re: [PATCH] KVM: PPC: Book3E 64: Fix IRQs warnings and hangs > > I replaced the two calls to kvmppc_lazy_ee_enable() with calls to > hard_irq_disable(), and it seems to be working fine. Please take a look on 'KVM: PPC64: booke: Hard disable interrupts when entering guest' RFC thread and see if your solution addresses Ben's comments. > > > > > > Where is the arch_local_irq_restore() instance you're talking > > about? > > > > > > > > ./arch/power/kernel/irq.c > > > > > > I meant the caller. :-P > > > > ./arch/powerpc/include/asm/hw_irq.h > > > > 55static inline unsigned long arch_local_irq_disable(void) > > 56{ > > 57 unsigned long flags, zero; > > 58 > > 59 asm volatile( > > 60 "li %1,0; lbz %0,%2(13); stb %1,%2(13)" > > 61 : "=r" (flags), "=&r" (zero) > > 62 : "i" (offsetof(struct paca_struct, soft_enabled)) > > 63 : "memory"); > > 64 > > 65 return flags; > > 66} > > 67 > > 68extern void arch_local_irq_restore(unsigned long); > > 69 > > 70static inline void arch_local_irq_enable(void) > > 71{ > > 72 arch_local_irq_restore(1); > > 73} > > Sigh. I meant the real caller, who's calling local_irq_restore(). I'm not sure what you mean, arch_local_irq_restore() is called indirectly by local_irq_enable() in our case from handle_exit(). -Mike -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index 1020119..82f155e 100644 --- a/arch/powerpc/kvm/booke.c +++ b/arch/powerpc/kvm/booke.c @@ -673,7 +673,6 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu) ret = s; goto out; } - kvmppc_lazy_ee_enable(); kvm_guest_enter(); @@ -789,16 +788,16 @@ static void kvmppc_restart_interrupt(struct kvm_vcpu *vcpu, switch (exit_nr) { case BOOKE_INTERRUPT_EXTERNAL: kvmppc_fill_pt_regs(®s); - do_IRQ(®s); + local_paca->irq_happened |= PACA_IRQ_EE; break; case BOOKE_INTERRUPT_DECREMENTER: kvmppc_fill_pt_regs(®s); - timer_interrupt(®s); + local_paca->irq_happened |= PACA_IRQ_DEC; break; #if defined(CONFIG_PPC_FSL_BOOK3E) || defined(CONFIG_PPC_BOOK3E_64) case BOOKE_INTERRUPT_DOORBELL: kvmppc_fill_pt_regs(®s); - doorbell_exception(®s); + local_paca->irq_happened |= PACA_IRQ_DBELL; break; #endif case BOOKE_INTERRUPT_MACHINE_CHECK: @@ -1148,8 +1147,6 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu, if (s <= 0) { local_irq_enable(); r = (s << 2) | RESUME_HOST | (r & RESUME_FLAG_NV); - } else { - kvmppc_lazy_ee_enable(); } }
A change in the generic code highlighted that we were running with IRQs (soft) enabled on Book3E 64-bit when trying to restart interrupts from handle_exit(). This is a lesson to configure lockdep often :) There is no reason to exit guest with soft_enabled == 1, a local_irq_enable() call will do this for us so get rid of kvmppc_layz_ee() calls. With this fix we eliminate irqs_disabled() warnings and some guest and host hangs revealed under stress tests, but guests still exhibit some unresponsiveness. The unresponsiveness has to do with the fact that arch_local_irq_restore() does not guarantees to hard enable interrupts. To do so replace exception function calls like timer_interrupt() with irq_happened flags. The local_irq_enable() call takes care of replaying them and lets the interrupts hard enabled. Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com> --- arch/powerpc/kvm/booke.c | 9 +++------ 1 files changed, 3 insertions(+), 6 deletions(-)