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

[Scott Wood <scottwood@freescale.com>]

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=