Re: [PATCH] kvm/ppc/booke64: Hard disable interrupts when entering the guest
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 -- 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
Re: [PATCH] kvm/ppc/booke64: Hard disable interrupts when entering the guest
On Mon, 2013-05-06 at 18:53 -0500, Scott Wood wrote:
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?
32-bit doesn't do lazy disable, so the situation is a lot easier there.
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).
Except that you have to mark them as soft enabled before you enter the
guest with interrupts on...
But yes, I see your point. If interrupts are fully enabled and you call
hard_irq_disable(), there should be no chance for anything to mess
around with irq_happened.
However if you set soft-enabled later on before the rfid that returns to
the guest and sets EE, you *must* also clear PACA_IRQ_HARD_DIS in
irq_happened. If you get that out of sync bad things will happen later
on...
To be sure all is well, you might want to
WARN_ON(get_paca()-irq_happened == PACA_IRQ_HARD_DIS); (with a comment
explaining why so).
Another problem is that hard_irq_disable() doesn't call
trace_hardirqs_off()... We might want to fix that:
static inline void hard_irq_disable(void)
{
__hard_irq_disable();
if (get_paca()-soft_enabled)
trace_hardirqs_off();
get_paca()-soft_enabled = 0;
get_paca()-irq_happened |= PACA_IRQ_HARD_DIS;
}
What if we just take this patch, and add trace_hardirqs_on() just
before entering the guest?
You still want to set soft_enabled I'd say ... though I can see how you
may get away without it as long as you call trace_hardirqs_off() right
on the way back from the guest, but beware some lockdep bits will choke
if they ever spot the discrepancy between the traced irq state and
soft_enabled. I'd recommend you just keep it in sync.
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.
Then set it again. Don't leave the kernel in a state where soft_enabled
is 1 and irq_happened is non-zero. It might work in the specific KVM
case we are looking at now because we know we are coming back via KVM
exit and putting things right again but it's fragile, somebody will come
back and break it, etc...
If necessary, create (or improve existing) helpers that do the right
state adjustement. The cost of a couple of byte stores is negligible,
I'd rather you make sure everything remains in sync at all times.
Cheers,
Ben.
--
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
Re: [PATCH] kvm/ppc/booke64: Hard disable interrupts when entering the guest
On 05/06/2013 07:03:14 PM, Benjamin Herrenschmidt wrote:
On Mon, 2013-05-06 at 18:53 -0500, Scott Wood wrote:
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?
32-bit doesn't do lazy disable, so the situation is a lot easier
there.
Right, but it still currently enters the guest with interrupts marked
as disabled, so we'd have the same latency tracker issue.
Another problem is that hard_irq_disable() doesn't call
trace_hardirqs_off()... We might want to fix that:
static inline void hard_irq_disable(void)
{
__hard_irq_disable();
if (get_paca()-soft_enabled)
trace_hardirqs_off();
get_paca()-soft_enabled = 0;
get_paca()-irq_happened |= PACA_IRQ_HARD_DIS;
}
Is it possible there are places that assume the current behavior?
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.
Then set it again. Don't leave the kernel in a state where
soft_enabled
is 1 and irq_happened is non-zero. It might work in the specific KVM
case we are looking at now because we know we are coming back via KVM
exit and putting things right again but it's fragile, somebody will
come
back and break it, etc...
KVM is a pretty special case -- at least on booke, it's required that
all exits from guest state go through the KVM exception code. I think
it's less likely that that changes, than something breaks in the code
to fix up lazy ee state (especially since we've already seen the latter
happen).
I'll give it a shot, though.
If necessary, create (or improve existing) helpers that do the right
state adjustement. The cost of a couple of byte stores is negligible,
I'd rather you make sure everything remains in sync at all times.
My concern was mainly about complexity -- it seemed simpler to just say
that the during guest execution, CPU is in a special state that is not
visible to anything that cares about lazy EE. The fact that EE can
actually be *off* and we still take the interrupt supports its
specialness. :-)
-Scott
--
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
Re: [PATCH] kvm/ppc/booke64: Hard disable interrupts when entering the guest
On Mon, 2013-05-06 at 22:05 -0500, Scott Wood wrote:
On 05/06/2013 07:03:14 PM, Benjamin Herrenschmidt wrote:
On Mon, 2013-05-06 at 18:53 -0500, Scott Wood wrote:
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?
32-bit doesn't do lazy disable, so the situation is a lot easier
there.
Right, but it still currently enters the guest with interrupts marked
as disabled, so we'd have the same latency tracker issue.
Another problem is that hard_irq_disable() doesn't call
trace_hardirqs_off()... We might want to fix that:
static inline void hard_irq_disable(void)
{
__hard_irq_disable();
if (get_paca()-soft_enabled)
trace_hardirqs_off();
get_paca()-soft_enabled = 0;
get_paca()-irq_happened |= PACA_IRQ_HARD_DIS;
}
Is it possible there are places that assume the current behavior?
There aren't many callers, I think this should be safe. Most
callers call it with interrupts already soft disabled, so that
should be a nop in these cases (idle for example).
But I can give it a quick spin today on a machine or two.
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.
Then set it again. Don't leave the kernel in a state where
soft_enabled
is 1 and irq_happened is non-zero. It might work in the specific KVM
case we are looking at now because we know we are coming back via KVM
exit and putting things right again but it's fragile, somebody will
come
back and break it, etc...
KVM is a pretty special case -- at least on booke, it's required that
all exits from guest state go through the KVM exception code. I think
it's less likely that that changes, than something breaks in the code
to fix up lazy ee state (especially since we've already seen the latter
happen).
I'll give it a shot, though.
If necessary, create (or improve existing) helpers that do the right
state adjustement. The cost of a couple of byte stores is negligible,
I'd rather you make sure everything remains in sync at all times.
My concern was mainly about complexity -- it seemed simpler to just say
that the during guest execution, CPU is in a special state that is not
visible to anything that cares about lazy EE. The fact that EE can
actually be *off* and we still take the interrupt supports its
specialness. :-)
Yeah ... sort of :-)
Cheers,
Ben.
--
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
Re: [PATCH] kvm/ppc/booke64: Hard disable interrupts when entering the guest
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-ppc 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/booke64: Hard disable interrupts when entering the guest
-Original Message- From: Wood Scott-B07421 Sent: Saturday, May 04, 2013 2:45 AM To: Alexander Graf Cc: kvm-ppc@vger.kernel.org; k...@vger.kernel.org; linuxppc- d...@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 scottw...@freescale.com Cc: Mihai Caraman mihai.cara...@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-ppc 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/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.
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 scottw...@freescale.com
Cc: Mihai Caraman mihai.cara...@freescale.com
---
arch/powerpc/kvm/booke.c |9 -
1 file changed, 4 insertions(+), 5 deletions(-)
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();
}
}
--
1.7.10.4
--
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
Re: [PATCH] kvm/ppc/booke64: Hard disable interrupts when entering the guest
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-ppc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
