[PATCH] KVM: PPC: Book3E 64: Fix IRQs warnings and hangs

[PATCH] KVM: PPC: Book3E 64: Fix IRQs warnings and hangs

[Mihai Caraman]

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(-)

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

Re: [PATCH] KVM: PPC: Book3E 64: Fix IRQs warnings and hangs

[Alexander Graf]

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 :)
>=20
> There is no reason to exit guest with soft_enabled =3D=3D 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.
>=20
> 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.
>=20
> 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(-)
>=20
> 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 =3D s;
> 		goto out;
> 	}
> -	kvmppc_lazy_ee_enable();
>=20
> 	kvm_guest_enter();
>=20
> @@ -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 |=3D PACA_IRQ_EE;
> 		break;
> 	case BOOKE_INTERRUPT_DECREMENTER:
> 		kvmppc_fill_pt_regs(&regs);
> -		timer_interrupt(&regs);
> +		local_paca->irq_happened |=3D 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 |=3D 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 <=3D 0) {
> 			local_irq_enable();
> 			r =3D (s << 2) | RESUME_HOST | (r & =
RESUME_FLAG_NV);
> -		} else {
> -			kvmppc_lazy_ee_enable();
> 		}
> 	}
>=20
> --=20
> 1.7.4.1
>=20
>=20
> --
> 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

Re: [PATCH] KVM: PPC: Book3E 64: Fix IRQs warnings and hangs

[Benjamin Herrenschmidt]

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.

Re: [PATCH] KVM: PPC: Book3E 64: Fix IRQs warnings and hangs

[Scott Wood]

On 05/03/2013 11:11:10 AM, Mihai Caraman wrote:
> A change in the generic code highlighted that we were running with =20
> IRQs
> (soft) enabled on Book3E 64-bit when trying to restart interrupts from
> handle_exit(). This is a lesson to configure lockdep often :)
>=20
> There is no reason to exit guest with soft_enabled =3D=3D 1, a =20
> local_irq_enable()
> call will do this for us so get rid of kvmppc_layz_ee() calls. With =20
> this fix
> we eliminate irqs_disabled() warnings and some guest and host hangs =20
> revealed
> under stress tests, but guests still exhibit some unresponsiveness.
>=20
> The unresponsiveness has to do with the fact that =20
> arch_local_irq_restore()
> does not guarantees to hard enable interrupts.

Could you elaborate?  If the saved IRQ state was "enabled", why =20
wouldn't arch_local_irq_restore() hard-enable IRQs?  The last thing it =20
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 =20
> interrupts
> hard enabled.

Not sure what you mean by "lets the interrupts hard enabled"... Do you =20
mean the EE bit in regs->msr, as opposed to the EE bit in the current =20
MSR?

> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
> ---
>  arch/powerpc/kvm/booke.c |    9 +++------
>  1 files changed, 3 insertions(+), 6 deletions(-)
>=20
> 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, =20
> struct kvm_vcpu *vcpu)
>  		ret =3D s;
>  		goto out;
>  	}
> -	kvmppc_lazy_ee_enable();
>=20
>  	kvm_guest_enter();
>=20
> @@ -789,16 +788,16 @@ static void kvmppc_restart_interrupt(struct =20
> kvm_vcpu *vcpu,
>  	switch (exit_nr) {
>  	case BOOKE_INTERRUPT_EXTERNAL:
>  		kvmppc_fill_pt_regs(&regs);
> -		do_IRQ(&regs);
> +		local_paca->irq_happened |=3D PACA_IRQ_EE;
>  		break;
>  	case BOOKE_INTERRUPT_DECREMENTER:
>  		kvmppc_fill_pt_regs(&regs);
> -		timer_interrupt(&regs);
> +		local_paca->irq_happened |=3D 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 |=3D PACA_IRQ_DBELL;
>  		break;
>  #endif

Aren't you breaking 32-bit here?

-Scott=

RE: [PATCH] KVM: PPC: Book3E 64: Fix IRQs warnings and hangs

[Caraman Mihai Claudiu-B02008]

> -----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
>=20
> > The unresponsiveness has to do with the fact that
> > arch_local_irq_restore()
> > does not guarantees to hard enable interrupts.
>=20
> 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;

>=20
> Where is the arch_local_irq_restore() instance you're talking about?

./arch/power/kernel/irq.c

>=20
> > 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.
>=20
> 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 |=3D PACA_IRQ_EE;
> >  		break;
>=20
> Aren't you breaking 32-bit here?

I had eyes only for 64-bit hangs :)

-Mike

Re: [PATCH] KVM: PPC: Book3E 64: Fix IRQs warnings and hangs

[Scott Wood]

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 =20
> 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 =20
> it
> > does is __hard_irq_enable().
>=20
> 	if (!irq_happened)
> 		return;

OK, so the problem is that we're not setting PACA_IRQ_HARD_DIS when we =20
hard-disable interrupts?

> > Where is the arch_local_irq_restore() instance you're talking about?
>=20
> ./arch/power/kernel/irq.c

I meant the caller. :-P

-Scott=

RE: [PATCH] KVM: PPC: Book3E 64: Fix IRQs warnings and hangs

[Caraman Mihai Claudiu-B02008]

> -----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
>=20
> > > > 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;
>=20
> 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 no=
w.

>=20
> > > Where is the arch_local_irq_restore() instance you're talking about?
> >
> > ./arch/power/kernel/irq.c
>=20
> 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                : "=3Dr" (flags), "=3D&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

Re: [PATCH] KVM: PPC: Book3E 64: Fix IRQs warnings and hangs

[Scott Wood]

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; kvm@vger.kernel.org;
> > linuxppc-dev@lists.ozlabs.org
> > Subject: Re: [PATCH] KVM: PPC: Book3E 64: Fix IRQs warnings and =20
> 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 =20
> 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 =20
> we
> > hard-disable interrupts?
>=20
> 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 =20
nervous.  I'd be more comfortable if we just hard-disabled interrupts =20
there.

> when do we hard-disable interrupts?

Interrupts will be hard-disabled when we take an exception to exit =20
guest state.

> If we follow host exception handlers model
> they set PACA_IRQ_EE/DEC/DBELL but not PACA_IRQ_HARD_DIS. Can you =20
> give it
> a try to see how KVM behaves with PACA_IRQ_HARD_DIS? I can't do it =20
> right now.

I replaced the two calls to kvmppc_lazy_ee_enable() with calls to =20
hard_irq_disable(), and it seems to be working fine.

> > > > Where is the arch_local_irq_restore() instance you're talking =20
> about?
> > >
> > > ./arch/power/kernel/irq.c
> >
> > I meant the caller. :-P
>=20
> ./arch/powerpc/include/asm/hw_irq.h
>=20
>   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                : "=3Dr" (flags), "=3D&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().

-Scott=

RE: [PATCH] KVM: PPC: Book3E 64: Fix IRQs warnings and hangs

[Caraman Mihai Claudiu-B02008]

> -----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
>=20
> 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.

>=20
> > > > > 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                : "=3Dr" (flags), "=3D&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}
>=20
> 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

Re: [PATCH] KVM: PPC: Book3E 64: Fix IRQs warnings and hangs

[Scott Wood]

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; kvm@vger.kernel.org;
> > linuxppc-dev@lists.ozlabs.org
> > Subject: Re: [PATCH] KVM: PPC: Book3E 64: Fix IRQs warnings and =20
> hangs
> >
> > I replaced the two calls to kvmppc_lazy_ee_enable() with calls to
> > hard_irq_disable(), and it seems to be working fine.
>=20
> 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 =20
between soft-disabling and hard-disabling, it wouldn't be received =20
until the guest exits for some other reason).

Instead, I turned the local_irq_disable() into hard_irq_disable() plus =20
trace_hardirqs_off().  This worked without warnings.

-Scott=