diff mbox

kvm/ppc/booke64: Hard disable interrupts when entering the guest

Message ID 1367624723-22456-1-git-send-email-scottwood@freescale.com (mailing list archive)
State New, archived
Headers show

Commit Message

Scott Wood May 3, 2013, 11:45 p.m. UTC
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).

Signed-off-by: Scott Wood <scottwood@freescale.com>
Cc: Mihai Caraman <mihai.caraman@freescale.com>
---
 arch/powerpc/kvm/booke.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

Comments

Scott Wood May 3, 2013, 11:53 p.m. UTC | #1
On 05/03/2013 06:45:23 PM, Scott Wood wrote:
> 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).

s/32-bit guest lazy/32-bit gets lazy/

-Scott
--
To unsubscribe from this list: send the line "unsubscribe kvm" 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 4, 2013, 7:11 a.m. UTC | #2
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Saturday, May 04, 2013 2:45 AM
> To: Alexander Graf
> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc-
> dev@lists.ozlabs.org; Wood Scott-B07421; Caraman Mihai Claudiu-B02008
> Subject: [PATCH] kvm/ppc/booke64: Hard disable interrupts when entering
> the guest
> 
> 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.

Just to mention one special case. may_hard_irq_enable() called from do_IRQ()
and timer_interrupt() clears PACA_IRQ_HARD_DIS but it either hard-enable or 
let PACA_IRQ_EE set which is enough for local_irq_enable() 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).
> 
> Signed-off-by: Scott Wood <scottwood@freescale.com>
> Cc: Mihai Caraman <mihai.caraman@freescale.com>

Please add my signed-off, it builds on the same principle of interrupts
soft-disabled to fix warnings and irq_happened flags to force interrupts
hard-enabled ... and parts of the code ;)

-Mike

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Benjamin Herrenschmidt May 5, 2013, 9:03 p.m. UTC | #3
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"...

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.

Ben.


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index ecbe908..b216821 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -666,14 +666,14 @@  int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
 		return -EINVAL;
 	}
 
-	local_irq_disable();
+	hard_irq_disable();
+	trace_hardirqs_off();
 	s = kvmppc_prepare_to_enter(vcpu);
 	if (s <= 0) {
 		local_irq_enable();
 		ret = s;
 		goto out;
 	}
-	kvmppc_lazy_ee_enable();
 
 	kvm_guest_enter();
 
@@ -1150,13 +1150,12 @@  int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
 	 * aren't already exiting to userspace for some other reason.
 	 */
 	if (!(r & RESUME_HOST)) {
-		local_irq_disable();
+		hard_irq_disable();
+		trace_hardirqs_off();
 		s = kvmppc_prepare_to_enter(vcpu);
 		if (s <= 0) {
 			local_irq_enable();
 			r = (s << 2) | RESUME_HOST | (r & RESUME_FLAG_NV);
-		} else {
-			kvmppc_lazy_ee_enable();
 		}
 	}