* [PATCH] kvm/ppc: interrupt disabling fixes
@ 2013-05-07 3:32 Scott Wood
2013-05-07 3:55 ` Benjamin Herrenschmidt
2013-05-07 10:00 ` tiejun.chen
0 siblings, 2 replies; 5+ messages in thread
From: Scott Wood @ 2013-05-07 3:32 UTC (permalink / raw)
To: Alexander Graf
Cc: kvm-ppc, kvm, Scott Wood, Mihai Caraman, Benjamin Herrenschmidt,
Tiejun Chen
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 <scottwood@freescale.com>
Cc: Mihai Caraman <mihai.caraman@freescale.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Tiejun Chen <tiejun.chen@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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] kvm/ppc: interrupt disabling fixes
2013-05-07 3:32 [PATCH] kvm/ppc: interrupt disabling fixes Scott Wood
@ 2013-05-07 3:55 ` Benjamin Herrenschmidt
2013-05-07 10:00 ` tiejun.chen
1 sibling, 0 replies; 5+ messages in thread
From: Benjamin Herrenschmidt @ 2013-05-07 3:55 UTC (permalink / raw)
To: Scott Wood; +Cc: Alexander Graf, kvm-ppc, kvm, Mihai Caraman, Tiejun Chen
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] kvm/ppc: interrupt disabling fixes
2013-05-07 3:32 [PATCH] kvm/ppc: interrupt disabling fixes Scott Wood
2013-05-07 3:55 ` Benjamin Herrenschmidt
@ 2013-05-07 10:00 ` tiejun.chen
1 sibling, 0 replies; 5+ messages in thread
From: tiejun.chen @ 2013-05-07 10:00 UTC (permalink / raw)
To: Scott Wood
Cc: Alexander Graf, kvm-ppc, kvm, Mihai Caraman,
Benjamin Herrenschmidt
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 <scottwood@freescale.com>
> Cc: Mihai Caraman <mihai.caraman@freescale.com>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Tiejun Chen <tiejun.chen@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();
>
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] kvm/ppc: interrupt disabling fixes
[not found] <1367953139.3398.28@snotra>
@ 2013-05-07 20:23 ` Benjamin Herrenschmidt
0 siblings, 0 replies; 5+ messages in thread
From: Benjamin Herrenschmidt @ 2013-05-07 20:23 UTC (permalink / raw)
To: Scott Wood
Cc: tiejun.chen, Alexander Graf, kvm-ppc, kvm, Mihai Caraman,
Paul Mackerras
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] kvm/ppc: interrupt disabling fixes
[not found] <1368059739.3398.64@snotra>
@ 2013-05-09 2:51 ` Benjamin Herrenschmidt
0 siblings, 0 replies; 5+ messages in thread
From: Benjamin Herrenschmidt @ 2013-05-09 2:51 UTC (permalink / raw)
To: Scott Wood; +Cc: tiejun.chen, Alexander Graf, kvm-ppc, kvm, Mihai Caraman
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-05-09 2:51 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-05-07 3:32 [PATCH] kvm/ppc: interrupt disabling fixes Scott Wood
2013-05-07 3:55 ` Benjamin Herrenschmidt
2013-05-07 10:00 ` tiejun.chen
[not found] <1367953139.3398.28@snotra>
2013-05-07 20:23 ` Benjamin Herrenschmidt
[not found] <1368059739.3398.64@snotra>
2013-05-09 2:51 ` Benjamin Herrenschmidt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox