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-...@vger.kernel.org; kvm@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" 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-...@vger.kernel.org; kvm@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" 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-...@vger.kernel.org; kvm@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" 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" 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-...@vger.kernel.org; kvm@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" 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-...@vger.kernel.org; kvm@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" 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-...@vger.kernel.org; kvm@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" 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" 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" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] KVM: PPC: Book3E 64: Fix IRQs warnings and hangs
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
---
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" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
