Re: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()
On 07/13/2013 07:05 AM, Benjamin Herrenschmidt wrote: On Fri, 2013-07-12 at 12:50 -0500, Scott Wood wrote: [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? Tiejun -- 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: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()
On 07/13/2013 01:50 AM, Scott Wood wrote: On 07/11/2013 10:22:28 PM, tiejun.chen wrote: 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 tiejun.c...@windriver.com diff --git a/arch/powerpc/kvm/bookehv_interrupts.S b/arch/powerpc/kvm/bookehv_interrupts.S index e8ed7d6..b521d21 100644 --- a/arch/powerpc/kvm/bookehv_interrupts.S +++ b/arch/powerpc/kvm/bookehv_interrupts.S @@ -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 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 Okay. Once we have a final name to replace SOFT_DISABLE_INTS, I can regenerate this as you comment. 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. Yes, I had a preliminary change ACKed by Ben, and I guess you also saw :) so I'll send that firstly. Thanks, Tiejun -- 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
[v1][PATCH 1/1] powerpc: to access local paca after hard irq disabled
We can access paca directly after hard interrupt disabled, and
this can avoid accessing wrong paca when using get_paca() in
preempt case.
Signed-off-by: Tiejun Chen tiejun.c...@windriver.com
---
arch/powerpc/include/asm/hw_irq.h |7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/include/asm/hw_irq.h
b/arch/powerpc/include/asm/hw_irq.h
index ba713f1..10be1dd 100644
--- a/arch/powerpc/include/asm/hw_irq.h
+++ b/arch/powerpc/include/asm/hw_irq.h
@@ -96,10 +96,11 @@ static inline bool arch_irqs_disabled(void)
#endif
#define hard_irq_disable() do {\
- u8 _was_enabled = get_paca()-soft_enabled; \
+ u8 _was_enabled;\
__hard_irq_disable(); \
- get_paca()-soft_enabled = 0; \
- get_paca()-irq_happened |= PACA_IRQ_HARD_DIS; \
+ _was_enabled = local_paca-soft_enabled;\
+ local_paca-soft_enabled = 0; \
+ local_paca-irq_happened |= PACA_IRQ_HARD_DIS; \
if (_was_enabled) \
trace_hardirqs_off(); \
} while(0)
--
1.7.9.5
--
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: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()
On Mon, 2013-07-15 at 10:20 +0800, tiejun.chen wrote: 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... 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: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()
On 07/14/2013 12:13 PM, Benjamin Herrenschmidt wrote: On Fri, 2013-07-12 at 12:54 +0800, tiejun.chen wrote: Is the following fine? powerpc: to access local paca after hard irq disabled We can access paca directly after hard interrupt disabled, and this can avoid accessing wrong paca when using get_paca() in preempt case. Signed-off-by: Tiejun Chen tiejun.c...@windriver.com Ack. We still have an unresolved problem where gcc decides to copy r13 to another register and then index from that, or even store and reload it, and this possibly accross preempt sections. It's unclear to me in what circumstances it will do it and whether there's a case of us getting completely screwed over, I need to investigate. This is the reason why we originally made the accesses to soft_enabled be inline asm. Understood. We might need to do a bulk conversion of all PACA accesses to either such inline asm or hide r13 behind asm (forcing essentially a copy to another register on each use) or a combination of both. IE. inline asm for direct access of things like soft_enabled, and a get_paca/put_paca style interface that copies r13 and includes a preempt_disable/enable for the rest. I'd like to check this possibility later. Tiejun -- 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: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()
On 07/15/2013 10:47 AM, Benjamin Herrenschmidt wrote: On Mon, 2013-07-15 at 10:20 +0800, tiejun.chen wrote: 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... But sounds this doesn't imply this key point that the soft-irq is always disabled here :) And as I understand, the irq state is always needed to be reconciled when we disable soft irq, right? Tiejun -- 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
