Skip to contentSkip to conversation

KVM: PPC: Book3E HV: call RECONCILE_IRQ_STATE to sync the software state

PATCH Applied Tiejun Chen 3 participants 8 messages Oct 23, 2013 – Dec 18, 2013

Subsystems (3)
KVM PowerPC KVM PowerPC
lore
Share & tools
lore

Tiejun Chen

author
#0 lore raw

PATCH v5 1/1 KVM: PPC: Book3E HV: call RECONCILE_IRQ_STATE to sync the software state

We enter with interrupts disabled in hardware, but we need to call RECONCILE_IRQ_STATE anyway to ensure that the software state is kept in sync instead of calling hard_irq_disable() directly.

Signed-off-by Tiejun Chen <[email protected]>

---
v5:

Fix one typo in the comment.

v4:

Fix one typo in the patch description.

v3:

Base on the latest tree, now we can use RECONCILE_IRQ_STATE instead of SOFT_DISABLE_INTS.

v2:

Move SOFT_DISABLE_INTS[1] earlier to avoid clobbering the arguments we want to pass to kvmppc_handle_exit.

 arch/powerpc/kvm/booke.c              |   11 -----------
 arch/powerpc/kvm/bookehv_interrupts.S |   11 +++++++++++
 2 files changed, 11 insertions(+), 11 deletions(-)
arch/powerpc/kvm/booke.c +0 -11
index 15d0149..0d211ff 100644
@@ -899,17 +899,6 @@ 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
-
-	/*
-	 * We enter with interrupts disabled in hardware, but
-	 * we need to call hard_irq_disable anyway to ensure that
-	 * the software state is kept in sync.
-	 */
-	hard_irq_disable();
-
 	/* update before a new last_exit_type is rewritten */
 	kvmppc_update_timing_stats(vcpu);
arch/powerpc/kvm/bookehv_interrupts.S +11 -0
index e8ed7d6..191c32b 100644
@@ -33,6 +33,8 @@
 
 #ifdef CONFIG_64BIT
 #include <asm/exception-64e.h>
+#include <asm/hw_irq.h>
+#include <asm/irqflags.h>
 #else
 #include "../kernel/head_booke.h" /* for THREAD_NORMSAVE() */
 #endif

@@ -465,6 +467,15 @@ _GLOBAL(kvmppc_resume_host)
 	mtspr	SPRN_EPCR, r3
 	isync
 
+#ifdef CONFIG_64BIT
+	/*
+	 * We enter with interrupts disabled in hardware, but
+	 * we need to call RECONCILE_IRQ_STATE anyway to ensure
+	 * that the software state is kept in sync.
+	 */
+	RECONCILE_IRQ_STATE(r3,r5)
+#endif
+
 	/* Switch to kernel stack and jump to handler. */
 	PPC_LL	r3, HOST_RUN(r1)
 	mr	r5, r14 /* intno */
Signature · 1 line
1.7.9.5
author
#1 lore raw
Quoted 9 lines · Tiejun Chen

We enter with interrupts disabled in hardware, but we need to call RECONCILE_IRQ_STATE anyway to ensure that the software state is kept in sync instead of calling hard_irq_disable() directly.

Signed-off-by Tiejun Chen <[email protected]>

---
v5:

Fix one typo in the comment.

Alex,

I already addressed Scott's comment, any further feedback?

Tiejun

Quoted 69 lines

v4:

Fix one typo in the patch description.

v3:

Base on the latest tree, now we can use RECONCILE_IRQ_STATE instead of SOFT_DISABLE_INTS.

v2:

Move SOFT_DISABLE_INTS[1] earlier to avoid clobbering the arguments we want to pass to kvmppc_handle_exit.

  arch/powerpc/kvm/booke.c              |   11 -----------
  arch/powerpc/kvm/bookehv_interrupts.S |   11 +++++++++++
  2 files changed, 11 insertions(+), 11 deletions(-)
arch/powerpc/kvm/booke.c +0 -11
index 15d0149..0d211ff 100644
@@ -899,17 +899,6 @@ 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
-
-	/*
-	 * We enter with interrupts disabled in hardware, but
-	 * we need to call hard_irq_disable anyway to ensure that
-	 * the software state is kept in sync.
-	 */
-	hard_irq_disable();
-
  	/* update before a new last_exit_type is rewritten */
  	kvmppc_update_timing_stats(vcpu);
arch/powerpc/kvm/bookehv_interrupts.S +11 -0
index e8ed7d6..191c32b 100644
@@ -33,6 +33,8 @@
 
  #ifdef CONFIG_64BIT
  #include <asm/exception-64e.h>
+#include <asm/hw_irq.h>
+#include <asm/irqflags.h>
  #else
  #include "../kernel/head_booke.h" /* for THREAD_NORMSAVE() */
  #endif

@@ -465,6 +467,15 @@ _GLOBAL(kvmppc_resume_host)
  	mtspr	SPRN_EPCR, r3
  	isync
 
+#ifdef CONFIG_64BIT
+	/*
+	 * We enter with interrupts disabled in hardware, but
+	 * we need to call RECONCILE_IRQ_STATE anyway to ensure
+	 * that the software state is kept in sync.
+	 */
+	RECONCILE_IRQ_STATE(r3,r5)
+#endif
+
  	/* Switch to kernel stack and jump to handler. */
  	PPC_LL	r3, HOST_RUN(r1)
  	mr	r5, r14 /* intno */
#2 lore raw
Quoted 4 lines · “tiejun.chen”
Quoted 3 lines · Tiejun Chen

We enter with interrupts disabled in hardware, but we need to call RECONCILE_IRQ_STATE anyway to ensure that the software state is kept in sync instead of calling hard_irq_disable() directly.

Why didn't this happen before? What is this patch fixing?

Quoted 10 lines
Quoted 6 lines
Signed-off-by Tiejun Chen <[email protected]>

---
v5:

Fix one typo in the comment.

Alex,

I already addressed Scott's comment, any further feedback?

Apart from the fact that we don't build PR for 64bit and only 64bit does do lazy EE, is there any reason this code moves from booke.c into HV only paths?

Oh, and sorry for the late reply :).

Alex

Quoted 73 lines

Tiejun

Quoted 69 lines

v4:

Fix one typo in the patch description.

v3:

Base on the latest tree, now we can use RECONCILE_IRQ_STATE instead of SOFT_DISABLE_INTS.

v2:

Move SOFT_DISABLE_INTS[1] earlier to avoid clobbering the arguments we want to pass to kvmppc_handle_exit.

 arch/powerpc/kvm/booke.c              |   11 -----------
 arch/powerpc/kvm/bookehv_interrupts.S |   11 +++++++++++
 2 files changed, 11 insertions(+), 11 deletions(-)
arch/powerpc/kvm/booke.c +0 -11
index 15d0149..0d211ff 100644
@@ -899,17 +899,6 @@ 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
-
-	/*
-	 * We enter with interrupts disabled in hardware, but
-	 * we need to call hard_irq_disable anyway to ensure that
-	 * the software state is kept in sync.
-	 */
-	hard_irq_disable();
-
 	/* update before a new last_exit_type is rewritten */
 	kvmppc_update_timing_stats(vcpu);
arch/powerpc/kvm/bookehv_interrupts.S +11 -0
index e8ed7d6..191c32b 100644
@@ -33,6 +33,8 @@
 
 #ifdef CONFIG_64BIT
 #include <asm/exception-64e.h>
+#include <asm/hw_irq.h>
+#include <asm/irqflags.h>
 #else
 #include "../kernel/head_booke.h" /* for THREAD_NORMSAVE() */
 #endif

@@ -465,6 +467,15 @@ _GLOBAL(kvmppc_resume_host)
 	mtspr	SPRN_EPCR, r3
 	isync
 
+#ifdef CONFIG_64BIT
+	/*
+	 * We enter with interrupts disabled in hardware, but
+	 * we need to call RECONCILE_IRQ_STATE anyway to ensure
+	 * that the software state is kept in sync.
+	 */
+	RECONCILE_IRQ_STATE(r3,r5)
+#endif
+
 	/* Switch to kernel stack and jump to handler. */
 	PPC_LL	r3, HOST_RUN(r1)
 	mr	r5, r14 /* intno */
#3 lore raw
Quoted 8 lines · Alexander Graf
Quoted 4 lines · “tiejun.chen”
Quoted 3 lines · Tiejun Chen

We enter with interrupts disabled in hardware, but we need to call RECONCILE_IRQ_STATE anyway to ensure that the software state is kept in sync instead of calling hard_irq_disable() directly.

Why didn't this happen before? What is this patch fixing?

It's cleanup, not a fix. It makes things more consistent with other 64-bit kernel entry code.

-Scott

#4 lore raw

Am 19.11.2013 um 00:49 schrieb Scott Wood <[email protected]>:

Quoted 12 lines · Alexander Graf
Quoted 8 lines · �gtiejun.chen�h
Quoted 5 lines · Tiejun Chen
Quoted 3 lines

We enter with interrupts disabled in hardware, but we need to call RECONCILE_IRQ_STATE anyway to ensure that the software state is kept in sync instead of calling hard_irq_disable() directly.

Why didn't this happen before? What is this patch fixing?

It's cleanup, not a fix. It makes things more consistent with other 64-bit kernel entry code.

Could we please note this in the comit message so that whoever stumbles over the patch later knows that this is effectively a no-op (and just prepones the lazy sync)?

Also, I'm still wary of lazy breakage in the pr code path, but I guess since it's completely untested today already it's ok to ignore.

Alex

#5 lore raw
Quoted 11 lines · Alexander Graf

Am 19.11.2013 um 00:49 schrieb Scott Wood <[email protected]>:

Quoted 8 lines · Alexander Graf
Quoted 7 lines · “tiejun.chen”
Quoted 5 lines · Tiejun Chen
Quoted 3 lines

We enter with interrupts disabled in hardware, but we need to call RECONCILE_IRQ_STATE anyway to ensure that the software state is kept in sync instead of calling hard_irq_disable() directly.

Quoted 8 lines
Quoted 4 lines
Quoted 1 line

Why didn't this happen before? What is this patch fixing?

It's cleanup, not a fix. It makes things more consistent with other 64-bit kernel entry code.

Could we please note this in the comit message so that whoever stumbles over the patch later knows that this is effectively a no-op (and just prepones the lazy sync)?

Also, I'm still wary of lazy breakage in the pr code path, but I guess since it's completely untested today already it's ok to ignore.

PR doesn't support 64-bit at all (e.g. it's all stw/lwz rather than some word-size abstraction). In the unlikely event that lazy EE is ever extended to 32-bit, then fixing up booke_interrupts.S would be just one part of the task of updating all the 32-bit asm (e.g. you'll find RECONCILE_IRQ_STATE in entry_64.S but not entry_32.S).

-Scott

author
#6 lore raw
In reply to Alexander Graf

Alex,

I suppose Scott already elaborate anything you want to know.

Tiejun

Quoted 103 lines · Alexander Graf
Quoted 4 lines · �tiejun.chen�
Quoted 3 lines · Tiejun Chen

We enter with interrupts disabled in hardware, but we need to call RECONCILE_IRQ_STATE anyway to ensure that the software state is kept in sync instead of calling hard_irq_disable() directly.

Why didn't this happen before? What is this patch fixing?

Quoted 10 lines
Quoted 6 lines
Signed-off-by Tiejun Chen <[email protected]>

---
v5:

Fix one typo in the comment.

Alex,

I already addressed Scott's comment, any further feedback?

Apart from the fact that we don't build PR for 64bit and only 64bit does do lazy EE, is there any reason this code moves from booke.c into HV only paths?

Oh, and sorry for the late reply :).

Alex

Quoted 73 lines

Tiejun

Quoted 69 lines

v4:

Fix one typo in the patch description.

v3:

Base on the latest tree, now we can use RECONCILE_IRQ_STATE instead of SOFT_DISABLE_INTS.

v2:

Move SOFT_DISABLE_INTS[1] earlier to avoid clobbering the arguments we want to pass to kvmppc_handle_exit.

  arch/powerpc/kvm/booke.c              |   11 -----------
  arch/powerpc/kvm/bookehv_interrupts.S |   11 +++++++++++
  2 files changed, 11 insertions(+), 11 deletions(-)
arch/powerpc/kvm/booke.c +0 -11
index 15d0149..0d211ff 100644
@@ -899,17 +899,6 @@ 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
-
-	/*
-	 * We enter with interrupts disabled in hardware, but
-	 * we need to call hard_irq_disable anyway to ensure that
-	 * the software state is kept in sync.
-	 */
-	hard_irq_disable();
-
  	/* update before a new last_exit_type is rewritten */
  	kvmppc_update_timing_stats(vcpu);
arch/powerpc/kvm/bookehv_interrupts.S +11 -0
index e8ed7d6..191c32b 100644
@@ -33,6 +33,8 @@
 
  #ifdef CONFIG_64BIT
  #include <asm/exception-64e.h>
+#include <asm/hw_irq.h>
+#include <asm/irqflags.h>
  #else
  #include "../kernel/head_booke.h" /* for THREAD_NORMSAVE() */
  #endif

@@ -465,6 +467,15 @@ _GLOBAL(kvmppc_resume_host)
  	mtspr	SPRN_EPCR, r3
  	isync
 
+#ifdef CONFIG_64BIT
+	/*
+	 * We enter with interrupts disabled in hardware, but
+	 * we need to call RECONCILE_IRQ_STATE anyway to ensure
+	 * that the software state is kept in sync.
+	 */
+	RECONCILE_IRQ_STATE(r3,r5)
+#endif
+
  	/* Switch to kernel stack and jump to handler. */
  	PPC_LL	r3, HOST_RUN(r1)
  	mr	r5, r14 /* intno */
Applied
#7 lore raw
Quoted 3 lines · “tiejun.chen”

Alex,

I suppose Scott already elaborate anything you want to know.

Thanks, applied with this minor patch on top:

arch/powerpc/kvm/bookehv_interrupts.S +1 -1
index 191c32b..be3de1d 100644
@@ -470,7 +470,7 @@ _GLOBAL(kvmppc_resume_host)
 #ifdef CONFIG_64BIT
        /*
         * We enter with interrupts disabled in hardware, but
-        * we need to call RECONCILE_IRQ_STATE anyway to ensure
+        * we need to call RECONCILE_IRQ_STATE to ensure
         * that the software state is kept in sync.
         */
        RECONCILE_IRQ_STATE(r3,r5)

I've also taken the liberty of making the patch description be more verbose:

KVM: PPC: Book3E HV: call RECONCILE_IRQ_STATE to sync the software state

Rather than calling hard_irq_disable() when we're back in C code we can just call RECONCILE_IRQ_STATE to soft disable IRQs while we're already in hard disabled state.

This should be functionally equivalent to the code before, but cleaner and faster.

Signed-off-by Tiejun Chen <[email protected]>
[agraf: fix comment, commit message]
Signed-off-by Alexander Graf <[email protected]>

Thanks, applied to kvm-ppc-queue.

Alex