Patchwork KVM: PPC: Book3E 64: Fix IRQs warnings and hangs

login
register
mail settings
Submitter Mihai Caraman
Date May 3, 2013, 4:11 p.m.
Message ID <1367597470-22214-1-git-send-email-mihai.caraman@freescale.com>
Download mbox | patch
Permalink /patch/241344/
State Not Applicable
Headers show

Comments

Mihai Caraman - May 3, 2013, 4:11 p.m.
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(-)
Alexander Graf - May 3, 2013, 4:24 p.m.
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(&regs);
> -		do_IRQ(&regs);
> +		local_paca->irq_happened |= PACA_IRQ_EE;
> 		break;
> 	case BOOKE_INTERRUPT_DECREMENTER:
> 		kvmppc_fill_pt_regs(&regs);
> -		timer_interrupt(&regs);
> +		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(&regs);
> -		doorbell_exception(&regs);
> +		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
Caraman Mihai Claudiu-B02008 - May 3, 2013, 8:01 p.m.
> -----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(&regs);
> > -		do_IRQ(&regs);
> > +		local_paca->irq_happened |= PACA_IRQ_EE;
> >  		break;
> 
> Aren't you breaking 32-bit here?

I had eyes only for 64-bit hangs :)

-Mike
Scott Wood - May 3, 2013, 8:15 p.m.
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
Caraman Mihai Claudiu-B02008 - May 3, 2013, 8:56 p.m.
> -----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
Benjamin Herrenschmidt - May 3, 2013, 10:03 p.m.
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.
Caraman Mihai Claudiu-B02008 - May 3, 2013, 10:59 p.m.
> -----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

Patch

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(&regs);
-		do_IRQ(&regs);
+		local_paca->irq_happened |= PACA_IRQ_EE;
 		break;
 	case BOOKE_INTERRUPT_DECREMENTER:
 		kvmppc_fill_pt_regs(&regs);
-		timer_interrupt(&regs);
+		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(&regs);
-		doorbell_exception(&regs);
+		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();
 		}
 	}