Re: [PATCH] kvm/ppc/booke64: Hard disable interrupts when entering the guest

[Scott Wood <scottwood@freescale.com>]

On 05/05/2013 04:03:08 PM, Benjamin Herrenschmidt wrote:
> On Fri, 2013-05-03 at 18:45 -0500, Scott Wood wrote:
> > kvmppc_lazy_ee_enable() was causing interrupts to be soft-enabled
> > (albeit hard-disabled) in kvmppc_restart_interrupt().  This led to
> > warnings, and possibly breakage if the interrupt state was later  
> saved
> > and then restored (leading to interrupts being hard-and-soft enabled
> > when they should be at least soft-disabled).
> >
> > Simply removing kvmppc_lazy_ee_enable() leaves interrupts only
> > soft-disabled when we enter the guest, but they will be  
> hard-disabled
> > when we exit the guest -- without PACA_IRQ_HARD_DIS ever being set,  
> so
> > the local_irq_enable() fails to hard-enable.
> >
> > While we could just set PACA_IRQ_HARD_DIS after an exit to  
> compensate,
> > instead hard-disable interrupts before entering the guest.  This  
> way,
> > we won't have to worry about interactions if we take an interrupt
> > during the guest entry code.  While I don't see any obvious
> > interactions, it could change in the future (e.g. it would be bad if
> > the non-hv code were used on 64-bit or if 32-bit guest lazy  
> interrupt
> > disabling, since the non-hv code changes IVPR among other things).
> 
> Shouldn't the interrupts be marked soft-enabled (even if hard  
> disabled)
> when entering the guest ?
> 
> Ie. The last stage of entry will hard enable, so they should be
> soft-enabled too... if not, latency trackers will consider the whole
> guest periods as "interrupt disabled"...

OK... I guess we already have that problem on 32-bit as well?

> Now, kvmppc_lazy_ee_enable() seems to be clearly bogus to me. It will
> unconditionally set soft_enabled and clear irq_happened from a
> soft-disabled state, thus potentially losing a pending event.
> 
> Book3S "HV" seems to be keeping interrupts fully enabled all the way
> until the asm hard disables, which would be fine except that I'm  
> worried
> we are racy vs. need_resched & signals.
> 
> One thing you may be able to do is call prep_irq_for_idle(). This will
> tell you if something happened, giving you a chance to abort/re-enable
> before you go the guest.

As long as we go straight from IRQs fully enabled to hard-disabled,  
before we check for signals and such, I don't think we need that (and  
using it would raise the question of what to do on 32-bit).

What if we just take this patch, and add trace_hardirqs_on() just  
before entering the guest?  This would be similar to what the 32-bit  
non-KVM exception return code does (except it would be in C code).   
Perhaps we could set soft_enabled as well, but then we'd have to clear  
it again before calling kvmppc_restart_interrupt() -- since the KVM  
exception handlers don't actually care about soft_enabled (it would  
just be for consistency), I'd rather just leave soft_enabled off.

We also don't want PACA_IRQ_HARD_DIS to be cleared the way  
prep_irq_for_idle() does, because that's what lets the  
local_irq_enable() do the hard-enabling after we exit the guest.

-Scott