KVM: PPC: disable preemption when using hard_irq_disable()

[1] SOFT_DISABLE_INTS seems an odd name for something that updates the software state to be consistent with interrupts being *hard* disabled. I can sort of see the logic in it, but it's confusing when first encountered. From the name it looks like all it would do is set soft_enabled to 1.

Quoted 6 lines · Scott Wood

[1] SOFT_DISABLE_INTS seems an odd name for something that updates the software state to be consistent with interrupts being *hard* disabled. I can sort of see the logic in it, but it's confusing when first encountered. From the name it looks like all it would do is set soft_enabled to 1.

It's indeed odd. Also worse when we use DISABLE_INTS which is just a macro on top of SOFT_DISABLE_INTS :-)

I've been wanting to change the macro name for a while now and never got to it. Patch welcome :-)

What about SOFT_IRQ_DISABLE? This is close to name hard_irq_disable() :) And
then remove all DISABLE_INTS as well?

Quoted 3 lines · tiejun.chen

What about SOFT_IRQ_DISABLE? This is close to name hard_irq_disable() :) And
then remove all DISABLE_INTS as well?

Or RECONCILE_IRQ_STATE...

Quoted 16 lines · tiejun.chen

Quoted 14 lines · Benjamin Herrenschmidt

Quoted 6 lines · Scott Wood

[1] SOFT_DISABLE_INTS seems an odd name for something that updates the software state to be consistent with interrupts being *hard* disabled. I can sort of see the logic in it, but it's confusing when first encountered. From the name it looks like all it would do is set soft_enabled to 1.

It's indeed odd. Also worse when we use DISABLE_INTS which is just a macro on top of SOFT_DISABLE_INTS :-)

I've been wanting to change the macro name for a while now and never got to it. Patch welcome :-)

Quoted 1 line

What about SOFT_IRQ_DISABLE?

What is semantically different about that from SOFT_DISABLE_INTS?

Quoted 1 line

This is close to name hard_irq_disable() :)

Except that one says "soft" and the other says "hard". :-)

Quoted 2 lines

And then remove all DISABLE_INTS as well?

You mean opencode WHATEVER_WE_CALL_IT(r3,r4) everwhere? Why?

Quoted 42 lines · tiejun.chen

If so, why not to remove directly hard_irq_disable() inside kvmppc_handle_exit() by reverting that commit, "kvm/ppc/booke64: Fix lazy ee handling in kvmppc_handle_exit()"?

Then we can use SOFT_DISABLE_INTS() explicitly before call kvmppc_handle_exit() like this:

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

We enter with interrupts disabled in hardware, but we need to call SOFT_DISABLE_INTS anyway to ensure that the software state is kept in sync.

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

arch/powerpc/kvm/bookehv_interrupts.S +10 -0

index e8ed7d6..b521d21 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

@@ -469,6 +471,14 @@ _GLOBAL(kvmppc_resume_host)
        PPC_LL  r3, HOST_RUN(r1)
        mr      r5, r14 /* intno */
        mr      r14, r4 /* Save vcpu pointer. */
+#ifdef CONFIG_64BIT
+       /*
+        * We enter with interrupts disabled in hardware, but
+        * we need to call SOFT_DISABLE_INTS anyway to ensure
+        * that the software state is kept in sync.
+        */
+       SOFT_DISABLE_INTS(r7,r8)
+#endif
 

arch/powerpc/kvm/bookehv_interrupts.S _GLOBAL(kvmppc_resume_host) · line 483

Quoted 1 line

arch/powerpc/kvm/bookehv_interrupts.S +0 -0

        bl      kvmppc_handle_exit

This will clobber the arguments we want to pass to kvmppc_handle_exit. That can be fixed by moving SOFT_DISABLE_INTS[1] earlier, and maybe it's more idiomatic

to use SOFT_DISABLE_INTS rather than what we currently do, but we still want to fix hard_irq_disable(). There are other places where we call hard_irq_disable() where interrupts (and I believe preemption) were previously enabled.