Re: [PATCH] KVM: PPC: Book3E 64: Fix IRQs warnings and hangs
On 05/03/2013 05:59:32 PM, Caraman Mihai Claudiu-B02008 wrote: > -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; k...@vger.kernel.org; > linuxppc-...@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. My original one didn't (there was a race if an interrupt comes in between soft-disabling and hard-disabling, it wouldn't be received until the guest exits for some other reason). Instead, I turned the local_irq_disable() into hard_irq_disable() plus trace_hardirqs_off(). This worked without warnings. -Scott -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] KVM: PPC: Book3E 64: Fix IRQs warnings and hangs
> -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; k...@vger.kernel.org; > linuxppc-...@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{ > > 57unsigned long flags, zero; > > 58 > > 59asm 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 > > 65return flags; > > 66} > > 67 > > 68extern void arch_local_irq_restore(unsigned long); > > 69 > > 70static inline void arch_local_irq_enable(void) > > 71{ > > 72arch_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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: PPC: Book3E 64: Fix IRQs warnings and hangs
On 05/03/2013 03:56:47 PM, Caraman Mihai Claudiu-B02008 wrote: > -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; k...@vger.kernel.org; > linuxppc-...@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, Hmm... I don't see any obvious breakage from that, but it makes me nervous. I'd be more comfortable if we just hard-disabled interrupts there. when do we hard-disable interrupts? Interrupts will be hard-disabled when we take an exception to exit guest state. 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. I replaced the two calls to kvmppc_lazy_ee_enable() with calls to hard_irq_disable(), and it seems to be working fine. > > > 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{ 57unsigned long flags, zero; 58 59asm 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 65return flags; 66} 67 68extern void arch_local_irq_restore(unsigned long); 69 70static inline void arch_local_irq_enable(void) 71{ 72arch_local_irq_restore(1); 73} Sigh. I meant the real caller, who's calling local_irq_restore(). -Scott -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: PPC: Book3E 64: Fix IRQs warnings and hangs
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 > > Ben, could you please review? That does look like the right thing to do indeed. Acked-by: Benjamin Herrenschmidt Cheers, Ben. -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] KVM: PPC: Book3E 64: Fix IRQs warnings and hangs
> -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; k...@vger.kernel.org; > linuxppc-...@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{ 57unsigned long flags, zero; 58 59asm 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 65return flags; 66} 67 68extern void arch_local_irq_restore(unsigned long); 69 70static inline void arch_local_irq_enable(void) 71{ 72arch_local_irq_restore(1); 73} -Mike -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: PPC: Book3E 64: Fix IRQs warnings and hangs
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; k...@vger.kernel.org; linuxppc- > d...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] KVM: PPC: Book3E 64: Fix IRQs warnings and hangs
> -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; k...@vger.kernel.org; linuxppc- > d...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: PPC: Book3E 64: Fix IRQs warnings and hangs
On 05/03/2013 11:11:10 AM, 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. 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(). Where is the arch_local_irq_restore() instance you're talking about? 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? Signed-off-by: Mihai Caraman --- 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 Aren't you breaking 32-bit here? -Scott -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: PPC: Book3E 64: Fix IRQs warnings and hangs
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 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 majord...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html