Re: [PATCH] kvm/ppc: interrupt disabling fixes
On Wed, 2013-05-08 at 19:35 -0500, Scott Wood wrote:
Sigh, and then there's this:
#ifdef CONFIG_PPC64
/* lazy EE magic */
hard_irq_disable();
if (lazy_irq_pending()) {
/* Got an interrupt in between, try again */
local_irq_enable();
hard_irq_disable();
kvm_guest_exit();
continue;
}
trace_hardirqs_on();
#endif
Alex, could you be a bit more descriptive than magic please? Can
this chunk of code be removed if we do the other changes being
discussed? Or should we leave this in and drop the pre-enter
hard_irq_disable portion of the proposed changes?
Why are you calling trace_hardirqs_on() here and not in
kvmppc_lazy_ee_enable()? Why are you calling kvm_guest_exit() before
you've called kvm_guest_enter()?
I think I originated that magic... it more/less mimmics prep_for_idle,
the goal was to hard disable (because we had soft disabled earlier) and
check if anything happened in between... if it did, abort, and try
again, but it's a bit fishy really.
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: interrupt disabling fixes
On Wed, 2013-05-08 at 19:35 -0500, Scott Wood wrote:
Sigh, and then there's this:
#ifdef CONFIG_PPC64
/* lazy EE magic */
hard_irq_disable();
if (lazy_irq_pending()) {
/* Got an interrupt in between, try again */
local_irq_enable();
hard_irq_disable();
kvm_guest_exit();
continue;
}
trace_hardirqs_on();
#endif
Alex, could you be a bit more descriptive than magic please? Can
this chunk of code be removed if we do the other changes being
discussed? Or should we leave this in and drop the pre-enter
hard_irq_disable portion of the proposed changes?
Why are you calling trace_hardirqs_on() here and not in
kvmppc_lazy_ee_enable()? Why are you calling kvm_guest_exit() before
you've called kvm_guest_enter()?
I think I originated that magic... it more/less mimmics prep_for_idle,
the goal was to hard disable (because we had soft disabled earlier) and
check if anything happened in between... if it did, abort, and try
again, but it's a bit fishy really.
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: interrupt disabling fixes
On 05/07/2013 11:32 AM, Scott Wood wrote:
booke64 was not maintaing consistent lazy ee state when exiting the
One typo ;-)
s/maintaing/maintaining
guest, leading to warnings and worse.
booke32 was less affected due to the absence of lazy ee, but it was
still feeding bad information into trace_hardirqs_off/on -- we don't
want guest execution to be seen as an IRQs off interval. book3s_pr
also has this problem.
book3s_pr and booke both used kvmppc_lazy_ee_enable() without
hard-disabling EE first, which could lead to races when irq_happened is
cleared, or if an interrupt happens after kvmppc_lazy_ee_enable(), and
possibly other issues.
Now, on book3s_pr and booke, always hard-disable interrupts before
kvmppc_prepare_to_enter(), but leave them soft-enabled. On book3s,
this should results in the right lazy EE state when the asm code
hard-enables on an exit. On booke, we call hard_irq_disable() rather
than hard-enable immediately.
Looks we always need to call hard_irq_disable() before
kvmppc_prepare_to_enter(), so I think we can add hard_irq_disable() directly
into kvmppc_prepare_to_enater() since this can avoid forgetting to call
hard_irq_disable() when call kvmppc_prepare_to_enater() somewhere in the future.
Here I assume Ben's fix to hard_irq_disable() is before this :) So what about
this?
diff --git a/arch/powerpc/include/asm/kvm_ppc.h
b/arch/powerpc/include/asm/kvm_ppc.h
index d7339df..e4e2120 100644
--- a/arch/powerpc/include/asm/kvm_ppc.h
+++ b/arch/powerpc/include/asm/kvm_ppc.h
@@ -397,6 +397,13 @@ static inline void kvmppc_mmu_flush_icache(pfn_t pfn)
static inline void kvmppc_lazy_ee_enable(void)
{
#ifdef CONFIG_PPC64
+ /*
+* To avoid races, the caller must have gone directly from having
+* interrupts fully-enabled to hard-disabled.
+*/
+ WARN_ON(local_paca-irq_happened != PACA_IRQ_HARD_DIS);
+ trace_hardirqs_on();
+
/* Only need to enable IRQs by hard enabling them after this */
local_paca-irq_happened = 0;
local_paca-soft_enabled = 1;
diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
index d09baf1..1ea65cd 100644
--- a/arch/powerpc/kvm/book3s_pr.c
+++ b/arch/powerpc/kvm/book3s_pr.c
@@ -884,7 +884,6 @@ program_interrupt:
* and if we really did time things so badly, then we just exit
* again due to a host external interrupt.
*/
- local_irq_disable();
s = kvmppc_prepare_to_enter(vcpu);
if (s = 0) {
local_irq_enable();
@@ -1121,7 +1120,6 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct
kvm_vcpu *vcpu)
* really did time things so badly, then we just exit again due to
* a host external interrupt.
*/
- local_irq_disable();
ret = kvmppc_prepare_to_enter(vcpu);
if (ret = 0) {
local_irq_enable();
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index dc1f590..d412749 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -666,7 +666,6 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu
*vcpu)
return -EINVAL;
}
- local_irq_disable();
s = kvmppc_prepare_to_enter(vcpu);
if (s = 0) {
local_irq_enable();
@@ -832,6 +831,10 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu
*vcpu,
{
int r = RESUME_HOST;
int s;
+#ifdef CONFIG_PPC64
+ WARN_ON(local_paca-irq_happened != 0);
+#endif
+ hard_irq_disable();
/* update before a new last_exit_type is rewritten */
kvmppc_update_timing_stats(vcpu);
@@ -1143,7 +1146,6 @@ 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();
s = kvmppc_prepare_to_enter(vcpu);
if (s = 0) {
local_irq_enable();
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 31084c6..147ac0e 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -64,7 +64,8 @@ int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
{
int r = 1;
- WARN_ON_ONCE(!irqs_disabled());
+ hard_irq_disable();
+
while (true) {
if (need_resched()) {
local_irq_enable();
--
1.7.9.5
Tiejun
Signed-off-by: Scott Wood scottw...@freescale.com
Cc: Mihai Caraman mihai.cara...@freescale.com
Cc: Benjamin Herrenschmidt b...@kernel.crashing.org
Cc: Tiejun Chen tiejun.c...@windriver.com
---
Only tested on booke (32 and 64 bit). Testers of book3s_pr would be
appreciated (particularly with lockdep enabled).
---
arch/powerpc/include/asm/kvm_ppc.h |7 +++
arch/powerpc/kvm/book3s_pr.c |6 --
arch/powerpc/kvm/booke.c | 12
Re: [PATCH] kvm/ppc: interrupt disabling fixes
On Tue, 2013-05-07 at 13:58 -0500, Scott Wood wrote: This will have to wait until book3s_hv disables interrupts as well. If this does eventually happen, then the local_irq_enable()/kvmppc_lazy_ee_enable() should also probably go into kvmppc_prepare_to_enter() -- though that could cause problems in book3s_pr, if it's depending on the kvmppc_lazy_ee_enable() happening as late as it does. Is book3s calling prepare_to_enter at all ? It has its own things with no interrupt disabling which afaik is racy vs. checking for signals resched... (CC'ing Paul). BTW. Linus just pulled my tree which contains my changed to hard_irq_disable() to do the trace_hardirqs_off() when neeed. 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: interrupt disabling fixes
On Tue, 2013-05-07 at 13:58 -0500, Scott Wood wrote: This will have to wait until book3s_hv disables interrupts as well. If this does eventually happen, then the local_irq_enable()/kvmppc_lazy_ee_enable() should also probably go into kvmppc_prepare_to_enter() -- though that could cause problems in book3s_pr, if it's depending on the kvmppc_lazy_ee_enable() happening as late as it does. Is book3s calling prepare_to_enter at all ? It has its own things with no interrupt disabling which afaik is racy vs. checking for signals resched... (CC'ing Paul). BTW. Linus just pulled my tree which contains my changed to hard_irq_disable() to do the trace_hardirqs_off() when neeed. 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
[PATCH] kvm/ppc: interrupt disabling fixes
booke64 was not maintaing consistent lazy ee state when exiting the
guest, leading to warnings and worse.
booke32 was less affected due to the absence of lazy ee, but it was
still feeding bad information into trace_hardirqs_off/on -- we don't
want guest execution to be seen as an IRQs off interval. book3s_pr
also has this problem.
book3s_pr and booke both used kvmppc_lazy_ee_enable() without
hard-disabling EE first, which could lead to races when irq_happened is
cleared, or if an interrupt happens after kvmppc_lazy_ee_enable(), and
possibly other issues.
Now, on book3s_pr and booke, always hard-disable interrupts before
kvmppc_prepare_to_enter(), but leave them soft-enabled. On book3s,
this should results in the right lazy EE state when the asm code
hard-enables on an exit. On booke, we call hard_irq_disable() rather
than hard-enable immediately.
Signed-off-by: Scott Wood scottw...@freescale.com
Cc: Mihai Caraman mihai.cara...@freescale.com
Cc: Benjamin Herrenschmidt b...@kernel.crashing.org
Cc: Tiejun Chen tiejun.c...@windriver.com
---
Only tested on booke (32 and 64 bit). Testers of book3s_pr would be
appreciated (particularly with lockdep enabled).
---
arch/powerpc/include/asm/kvm_ppc.h |7 +++
arch/powerpc/kvm/book3s_pr.c |6 --
arch/powerpc/kvm/booke.c | 12 ++--
3 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/include/asm/kvm_ppc.h
b/arch/powerpc/include/asm/kvm_ppc.h
index a5287fe..e55d7e5 100644
--- a/arch/powerpc/include/asm/kvm_ppc.h
+++ b/arch/powerpc/include/asm/kvm_ppc.h
@@ -399,6 +399,13 @@ static inline void kvmppc_mmu_flush_icache(pfn_t pfn)
static inline void kvmppc_lazy_ee_enable(void)
{
#ifdef CONFIG_PPC64
+ /*
+* To avoid races, the caller must have gone directly from having
+* interrupts fully-enabled to hard-disabled.
+*/
+ WARN_ON(local_paca-irq_happened != PACA_IRQ_HARD_DIS);
+ trace_hardirqs_on();
+
/* Only need to enable IRQs by hard enabling them after this */
local_paca-irq_happened = 0;
local_paca-soft_enabled = 1;
diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
index d09baf1..a1e70113 100644
--- a/arch/powerpc/kvm/book3s_pr.c
+++ b/arch/powerpc/kvm/book3s_pr.c
@@ -884,7 +884,8 @@ program_interrupt:
* and if we really did time things so badly, then we just exit
* again due to a host external interrupt.
*/
- local_irq_disable();
+ hard_irq_disable();
+ trace_hardirqs_off();
s = kvmppc_prepare_to_enter(vcpu);
if (s = 0) {
local_irq_enable();
@@ -1121,7 +1122,8 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct
kvm_vcpu *vcpu)
* really did time things so badly, then we just exit again due to
* a host external interrupt.
*/
- local_irq_disable();
+ hard_irq_disable();
+ trace_hardirqs_off();
ret = kvmppc_prepare_to_enter(vcpu);
if (ret = 0) {
local_irq_enable();
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index ecbe908..5dc1f53 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -666,7 +666,8 @@ 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();
@@ -834,6 +835,12 @@ int kvmppc_handle_exit(struct kvm_run *run, struct
kvm_vcpu *vcpu,
int s;
int idx;
+#ifdef CONFIG_PPC64
+ WARN_ON(local_paca-irq_happened != 0);
+#endif
+ hard_irq_disable();
+ trace_hardirqs_off();
+
/* update before a new last_exit_type is rewritten */
kvmppc_update_timing_stats(vcpu);
@@ -1150,7 +1157,8 @@ 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();
--
1.7.10.4
--
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: interrupt disabling fixes
On Mon, 2013-05-06 at 22:32 -0500, Scott Wood wrote: + hard_irq_disable(); + trace_hardirqs_off(); I still think hard_irq_disable() should be fixed to do the right thing here :-) I'll do that standalone patch here and give it a spin. 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
[PATCH] kvm/ppc: interrupt disabling fixes
booke64 was not maintaing consistent lazy ee state when exiting the
guest, leading to warnings and worse.
booke32 was less affected due to the absence of lazy ee, but it was
still feeding bad information into trace_hardirqs_off/on -- we don't
want guest execution to be seen as an IRQs off interval. book3s_pr
also has this problem.
book3s_pr and booke both used kvmppc_lazy_ee_enable() without
hard-disabling EE first, which could lead to races when irq_happened is
cleared, or if an interrupt happens after kvmppc_lazy_ee_enable(), and
possibly other issues.
Now, on book3s_pr and booke, always hard-disable interrupts before
kvmppc_prepare_to_enter(), but leave them soft-enabled. On book3s,
this should results in the right lazy EE state when the asm code
hard-enables on an exit. On booke, we call hard_irq_disable() rather
than hard-enable immediately.
Signed-off-by: Scott Wood scottw...@freescale.com
Cc: Mihai Caraman mihai.cara...@freescale.com
Cc: Benjamin Herrenschmidt b...@kernel.crashing.org
Cc: Tiejun Chen tiejun.c...@windriver.com
---
Only tested on booke (32 and 64 bit). Testers of book3s_pr would be
appreciated (particularly with lockdep enabled).
---
arch/powerpc/include/asm/kvm_ppc.h |7 +++
arch/powerpc/kvm/book3s_pr.c |6 --
arch/powerpc/kvm/booke.c | 12 ++--
3 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/include/asm/kvm_ppc.h
b/arch/powerpc/include/asm/kvm_ppc.h
index a5287fe..e55d7e5 100644
--- a/arch/powerpc/include/asm/kvm_ppc.h
+++ b/arch/powerpc/include/asm/kvm_ppc.h
@@ -399,6 +399,13 @@ static inline void kvmppc_mmu_flush_icache(pfn_t pfn)
static inline void kvmppc_lazy_ee_enable(void)
{
#ifdef CONFIG_PPC64
+ /*
+* To avoid races, the caller must have gone directly from having
+* interrupts fully-enabled to hard-disabled.
+*/
+ WARN_ON(local_paca-irq_happened != PACA_IRQ_HARD_DIS);
+ trace_hardirqs_on();
+
/* Only need to enable IRQs by hard enabling them after this */
local_paca-irq_happened = 0;
local_paca-soft_enabled = 1;
diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
index d09baf1..a1e70113 100644
--- a/arch/powerpc/kvm/book3s_pr.c
+++ b/arch/powerpc/kvm/book3s_pr.c
@@ -884,7 +884,8 @@ program_interrupt:
* and if we really did time things so badly, then we just exit
* again due to a host external interrupt.
*/
- local_irq_disable();
+ hard_irq_disable();
+ trace_hardirqs_off();
s = kvmppc_prepare_to_enter(vcpu);
if (s = 0) {
local_irq_enable();
@@ -1121,7 +1122,8 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct
kvm_vcpu *vcpu)
* really did time things so badly, then we just exit again due to
* a host external interrupt.
*/
- local_irq_disable();
+ hard_irq_disable();
+ trace_hardirqs_off();
ret = kvmppc_prepare_to_enter(vcpu);
if (ret = 0) {
local_irq_enable();
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index ecbe908..5dc1f53 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -666,7 +666,8 @@ 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();
@@ -834,6 +835,12 @@ int kvmppc_handle_exit(struct kvm_run *run, struct
kvm_vcpu *vcpu,
int s;
int idx;
+#ifdef CONFIG_PPC64
+ WARN_ON(local_paca-irq_happened != 0);
+#endif
+ hard_irq_disable();
+ trace_hardirqs_off();
+
/* update before a new last_exit_type is rewritten */
kvmppc_update_timing_stats(vcpu);
@@ -1150,7 +1157,8 @@ 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();
--
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: interrupt disabling fixes
On Mon, 2013-05-06 at 22:32 -0500, Scott Wood wrote: + hard_irq_disable(); + trace_hardirqs_off(); I still think hard_irq_disable() should be fixed to do the right thing here :-) I'll do that standalone patch here and give it a spin. 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
