Re: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()
On 07/16/2013 01:27 AM, Scott Wood wrote: On 07/14/2013 09:20:00 PM, tiejun.chen wrote: 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? What is semantically different about that from SOFT_DISABLE_INTS? This is close to name hard_irq_disable() :) Except that one says "soft" and the other says "hard". :-) Yes, I want to leave as SOFT_IRQ_DISABLE and close to hard_irq_disable() just since I think the irq state is always needed to be reconciled when we disable soft irq. So maybe we shouldn't necessarily underline to sync the software state here as I understand. But looks you also agree with that name, RECONCILE_IRQ_STATE, Ben mentioned previously. So I'd like to turn back :) And then remove all DISABLE_INTS as well? You mean opencode WHATEVER_WE_CALL_IT(r3,r4) everwhere? Why? OOPS :-P Tiejun -- 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: [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" 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 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" 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" 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 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 +#include +#include #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" 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 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" 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 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 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. 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. 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: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()
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 :-) 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: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()
On 07/12/2013 11:57 AM, Benjamin Herrenschmidt wrote: On Fri, 2013-07-12 at 10:13 +0800, tiejun.chen wrote: #define hard_irq_disable()do {\ u8 _was_enabled = get_paca()->soft_enabled; \ Current problem I met is issued from the above line. __hard_irq_disable(); \ - get_paca()->soft_enabled = 0; \ Not here. If I'm misunderstanding what you guys means, please correct me since this is a long discussion thread. I have to reread that carefully. Then make it u8 _was_enabled; __hard_irq_disable(); was_enabled = local_paca-> Once you have hard disabled, using local_paca directly *should* be safe (minus that gcc problem I mentioned). 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 --- 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 Or what about that change to call SOFT_DISABLE_INTS only in KVM scenario? Which better? Then I can send to review? Thanks, Tiejun -- 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: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()
On Fri, 2013-07-12 at 10:13 +0800, tiejun.chen wrote: > > #define hard_irq_disable()do {\ > > u8 _was_enabled = get_paca()->soft_enabled; \ > > Current problem I met is issued from the above line. > > > __hard_irq_disable(); \ > > - get_paca()->soft_enabled = 0; \ > > Not here. > > If I'm misunderstanding what you guys means, please correct me since this is > a > long discussion thread. I have to reread that carefully. Then make it u8 _was_enabled; __hard_irq_disable(); was_enabled = local_paca-> Once you have hard disabled, using local_paca directly *should* be safe (minus that gcc problem I mentioned). 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: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()
On 07/12/2013 12:36 AM, Scott Wood wrote: On 07/11/2013 11:30:41 AM, Alexander Graf wrote: On 11.07.2013, at 18:18, Scott Wood wrote: > On 07/11/2013 08:07:30 AM, Alexander Graf wrote: >> get_paca() warns when we're preemptible. We're only not preemptible when either preempt is disabled or irqs are disabled. Irqs are disabled, but arch_irqs_disabled() doesn't know, because it only checks for soft disabled IRQs. >> So we can fix this either by setting IRQs as soft disabled as well > > If we set IRQs as soft-disabled prior to calling hard_irq_disable(), then hard_irq_disable() will fail to call trace_hardirqs_off(). Right... Plus we'd have the same problem trying to set soft_enabled to 0. >> Any preferences? > > Use arch_local_save_flags() in hard_irq_disable() instead of reading soft_enabled with C code. That only operates on the soft_enabled bit. We also need to access irq_happened. OK, so we'll need more inline asm. 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 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 +#include +#include #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 /* Restore vcpu pointer and the nonvolatiles we used. */ Tiejun -- 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: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()
On 07/12/2013 08:19 AM, Benjamin Herrenschmidt wrote: On Thu, 2013-07-11 at 15:07 +0200, Alexander Graf wrote: Ok, let me quickly explain the problem. We are leaving host context, switching slowly into guest context. During that transition we call get_paca() indirectly (apparently by another call to hard_disable() which sounds bogus, but that's another story). get_paca() warns when we're preemptible. We're only not preemptible when either preempt is disabled or irqs are disabled. Irqs are disabled, but arch_irqs_disabled() doesn't know, because it only checks for soft disabled IRQs. So we can fix this either by setting IRQs as soft disabled as well or by disabling preemption until we enter the guest for real. Any preferences? Well, if you hard disable first (ie, direct transition from full enabled to hard disabled), you know you have nothing lazy pending in irq_pending, then it's ok to mess around with local_paca->soft_enabled to make it "look disabled". IE. Call hard_irq_disable(), then only turn local_paca->soft_enabled back on late in the process, some time before the final rfi(d). That works as long as you had a transition from full enabled to full disabled and don't hard re-enable in the process. IE, You are certain that there is nothing pending in irq_happened. HOWEVER ! If you do that, you *ALSO* need to clear irq_happened. You must *NEVER* leave PACA_IRQ_HARD_DIS set in irq_happened if you are soft-enabled, and since the above means that you *will* be seen as soft enabled on the way out of the guest, you can kaboom. BTW. I'm fine with a patch that does: #define hard_irq_disable() do {\ u8 _was_enabled = get_paca()->soft_enabled; \ Current problem I met is issued from the above line. __hard_irq_disable(); \ - get_paca()->soft_enabled = 0;\ Not here. If I'm misunderstanding what you guys means, please correct me since this is a long discussion thread. I have to reread that carefully. Tiejun + local_paca->soft_enabled = 0;\ In fact we should probably do it anyway. 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: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()
On Thu, 2013-07-11 at 11:18 -0500, Scott Wood wrote: > > If we set IRQs as soft-disabled prior to calling hard_irq_disable(), > then hard_irq_disable() will fail to call trace_hardirqs_off(). Sure because setting them as soft-disabled will have done it. However by doing so, you also create the possibility of latching a new event in irq_happened. > > or by disabling preemption until we enter the guest for real. > > I don't follow this one. We're exiting, not entering. > > > Any preferences? > > Use arch_local_save_flags() in hard_irq_disable() instead of reading > soft_enabled with C code. That or just use local_paca... Though we'd had problems in the past where gcc would defeat us there and essentially copy r13 to another register and start indexing from there. That kills you if you preempt. 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: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()
On Thu, 2013-07-11 at 15:07 +0200, Alexander Graf wrote: > Ok, let me quickly explain the problem. > > We are leaving host context, switching slowly into guest context. > During that transition we call get_paca() indirectly (apparently by > another call to hard_disable() which sounds bogus, but that's another > story). > > get_paca() warns when we're preemptible. We're only not preemptible > when either preempt is disabled or irqs are disabled. Irqs are > disabled, but arch_irqs_disabled() doesn't know, because it only > checks for soft disabled IRQs. > > So we can fix this either by setting IRQs as soft disabled as well or > by disabling preemption until we enter the guest for real. Any > preferences? Well, if you hard disable first (ie, direct transition from full enabled to hard disabled), you know you have nothing lazy pending in irq_pending, then it's ok to mess around with local_paca->soft_enabled to make it "look disabled". IE. Call hard_irq_disable(), then only turn local_paca->soft_enabled back on late in the process, some time before the final rfi(d). That works as long as you had a transition from full enabled to full disabled and don't hard re-enable in the process. IE, You are certain that there is nothing pending in irq_happened. HOWEVER ! If you do that, you *ALSO* need to clear irq_happened. You must *NEVER* leave PACA_IRQ_HARD_DIS set in irq_happened if you are soft-enabled, and since the above means that you *will* be seen as soft enabled on the way out of the guest, you can kaboom. BTW. I'm fine with a patch that does: #define hard_irq_disable() do {\ u8 _was_enabled = get_paca()->soft_enabled; \ __hard_irq_disable(); \ - get_paca()->soft_enabled = 0; \ + local_paca->soft_enabled = 0; \ In fact we should probably do it anyway. 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: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()
On 11.07.2013, at 14:54, Benjamin Herrenschmidt wrote: > On Thu, 2013-07-11 at 14:47 +0200, Alexander Graf wrote: >>> Yes of course, that's what we call "soft disabled" :-) It's even the >>> whole point of doing lazy disable... >> >> Meh. Of course it's soft_enabled = 1; hard_enabled = 0. > > That doesn't happen in "normal" C code. It happens under very specific > circumstances, such as in the guts of entry_64.S, in areas where we just > want to temporarily mask HW interrupts without changing the SW state > (and thus without having to deal with replays etc...). > > We typically also do that right before going to idle on some processors > where we come back from idle with interrupts hard enabled, possibly > right in an interrupt vector. > > Typically that's a state that makes some sense on KVM entry. From a > Linux perspective, you enter KVM with interrupts enabled. But you > temporarily hard disable on the way down while doing the low level > context switch. > > This works well as long as you know you have no pending replay event. > That should be fine if you do a direct transition from soft+hard enabled > to hard disabled (without soft disabling). In that case there should be > nothing in irq_happened. > > It's equivalent to returning to userspace from the kernel. We are > soft-enabled, but the code in ret_from_except hard disables while > mucking around with TIF_FLAGS etc... until the final rfid Ok, let me quickly explain the problem. We are leaving host context, switching slowly into guest context. During that transition we call get_paca() indirectly (apparently by another call to hard_disable() which sounds bogus, but that's another story). get_paca() warns when we're preemptible. We're only not preemptible when either preempt is disabled or irqs are disabled. Irqs are disabled, but arch_irqs_disabled() doesn't know, because it only checks for soft disabled IRQs. So we can fix this either by setting IRQs as soft disabled as well or by disabling preemption until we enter the guest for real. Any preferences? Alex -- 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: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()
On Thu, 2013-07-11 at 14:47 +0200, Alexander Graf wrote: > > Yes of course, that's what we call "soft disabled" :-) It's even the > > whole point of doing lazy disable... > > Meh. Of course it's soft_enabled = 1; hard_enabled = 0. That doesn't happen in "normal" C code. It happens under very specific circumstances, such as in the guts of entry_64.S, in areas where we just want to temporarily mask HW interrupts without changing the SW state (and thus without having to deal with replays etc...). We typically also do that right before going to idle on some processors where we come back from idle with interrupts hard enabled, possibly right in an interrupt vector. Typically that's a state that makes some sense on KVM entry. From a Linux perspective, you enter KVM with interrupts enabled. But you temporarily hard disable on the way down while doing the low level context switch. This works well as long as you know you have no pending replay event. That should be fine if you do a direct transition from soft+hard enabled to hard disabled (without soft disabling). In that case there should be nothing in irq_happened. It's equivalent to returning to userspace from the kernel. We are soft-enabled, but the code in ret_from_except hard disables while mucking around with TIF_FLAGS etc... until the final rfid 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: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()
On 11.07.2013, at 14:28, Benjamin Herrenschmidt wrote: > On Thu, 2013-07-11 at 11:49 +0200, Alexander Graf wrote: >> Ben, is soft_enabled == 0; hard_enabled == 1 a valid combination that >> may ever occur? > > Yes of course, that's what we call "soft disabled" :-) It's even the > whole point of doing lazy disable... Meh. Of course it's soft_enabled = 1; hard_enabled = 0. Alex -- 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: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()
On Thu, 2013-07-11 at 11:49 +0200, Alexander Graf wrote: > Ben, is soft_enabled == 0; hard_enabled == 1 a valid combination that > may ever occur? Yes of course, that's what we call "soft disabled" :-) It's even the whole point of doing lazy disable... 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: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()
On 11.07.2013, at 04:48, tiejun.chen wrote: > On 07/10/2013 05:49 PM, Alexander Graf wrote: >> >> On 10.07.2013, at 08:02, Tiejun Chen wrote: >> >>> We should ensure the preemption cannot occur while calling get_paca() >>> insdide hard_irq_disable(), otherwise the paca_struct may be the >>> wrong one just after. And btw, we may update timing stats in this case. >>> >>> Signed-off-by: Tiejun Chen >>> --- >>> arch/powerpc/kvm/booke.c |2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c >>> index dcc94f0..9dae25d 100644 >>> --- a/arch/powerpc/kvm/booke.c >>> +++ b/arch/powerpc/kvm/booke.c >>> @@ -839,6 +839,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct >>> kvm_vcpu *vcpu, >>> WARN_ON(local_paca->irq_happened != 0); >>> #endif >>> >>> + preempt_disable(); >>> /* >>> * We enter with interrupts disabled in hardware, but >>> * we need to call hard_irq_disable anyway to ensure that >>> @@ -848,6 +849,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct >>> kvm_vcpu *vcpu, >>> >>> /* update before a new last_exit_type is rewritten */ >>> kvmppc_update_timing_stats(vcpu); >>> + preempt_enable(); >> >> All of the code here is already called with interrupts disabled. I don't see >> how we could preempt then? > > But the kernel still check this in preempt case, > > #define get_paca() ((void) debug_smp_processor_id(), local_paca) > > then trigger that known call trace as I observed :) > > BUG: using smp_processor_id() in preemptible [] code: > qemu-system-ppc/2065 # define preemptible() (preempt_count() == 0 && !irqs_disabled()) So we are only hitting this BUG() when either preempt_count is 0 (which your patch is trying to fix) and at the same time interrupts are enabled. But wait - interrupts are disabled, aren't they? Let's check. #define irqs_disabled() \ ({ \ unsigned long _flags; \ raw_local_save_flags(_flags); \ raw_irqs_disabled_flags(_flags);\ }) #define raw_irqs_disabled_flags(flags) \ ({ \ typecheck(unsigned long, flags);\ arch_irqs_disabled_flags(flags);\ }) static inline unsigned long arch_local_save_flags(void) { unsigned long flags; asm volatile( "lbz %0,%1(13)" : "=r" (flags) : "i" (offsetof(struct paca_struct, soft_enabled))); return flags; } static inline bool arch_irqs_disabled_flags(unsigned long flags) { return flags == 0; } So we're running with soft_enabled == 0 here which means that irqs_disabled() also returns 0. Ben, is soft_enabled == 0; hard_enabled == 1 a valid combination that may ever occur? Alex > caller is .kvmppc_handle_exit+0x48/0x810 > CPU: 0 PID: 2065 Comm: qemu-system-ppc Not tainted > 3.10.0-172036-ge2daa28-dirty #116 > Call Trace: > [c001fc637570] [c000835c] .show_stack+0x7c/0x1f0 (unreliable) > [c001fc637640] [c0673a0c] .dump_stack+0x28/0x3c > [c001fc6376b0] [c02f04d8] .debug_smp_processor_id+0x108/0x120 > [c001fc637740] [c00444e8] .kvmppc_handle_exit+0x48/0x810 > [c001fc6377f0] [c0047c80] .kvmppc_resume_host+0xa4/0xf8 > > 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 -- 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: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()
On 07/11/2013 03:15 AM, Scott Wood wrote: On 07/10/2013 01:02:19 AM, Tiejun Chen wrote: We should ensure the preemption cannot occur while calling get_paca() insdide hard_irq_disable(), otherwise the paca_struct may be the wrong one just after. And btw, we may update timing stats in this case. The soft-ee mechanism depends on accessing the PACA directly via r13 to avoid this. We probably should be using inline asm to read was_enabled rather than Yes. hoping the compiler doesn't do anything silly. Do you recommend I should directly replace get_paca() with local_paca inside hard_irq_disable()? #define hard_irq_disable() do {\ u8 _was_enabled = get_paca()->soft_enabled; \ -> u8 _was_enabled = local_paca->soft_enabled; But is this safe for all scenarios? Tiejun -- 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: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()
On 07/10/2013 05:49 PM, Alexander Graf wrote: On 10.07.2013, at 08:02, Tiejun Chen wrote: We should ensure the preemption cannot occur while calling get_paca() insdide hard_irq_disable(), otherwise the paca_struct may be the wrong one just after. And btw, we may update timing stats in this case. Signed-off-by: Tiejun Chen --- arch/powerpc/kvm/booke.c |2 ++ 1 file changed, 2 insertions(+) diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index dcc94f0..9dae25d 100644 --- a/arch/powerpc/kvm/booke.c +++ b/arch/powerpc/kvm/booke.c @@ -839,6 +839,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu, WARN_ON(local_paca->irq_happened != 0); #endif + preempt_disable(); /* * We enter with interrupts disabled in hardware, but * we need to call hard_irq_disable anyway to ensure that @@ -848,6 +849,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu, /* update before a new last_exit_type is rewritten */ kvmppc_update_timing_stats(vcpu); + preempt_enable(); All of the code here is already called with interrupts disabled. I don't see how we could preempt then? But the kernel still check this in preempt case, #define get_paca() ((void) debug_smp_processor_id(), local_paca) then trigger that known call trace as I observed :) BUG: using smp_processor_id() in preemptible [] code: qemu-system-ppc/2065 caller is .kvmppc_handle_exit+0x48/0x810 CPU: 0 PID: 2065 Comm: qemu-system-ppc Not tainted 3.10.0-172036-ge2daa28-dirty #116 Call Trace: [c001fc637570] [c000835c] .show_stack+0x7c/0x1f0 (unreliable) [c001fc637640] [c0673a0c] .dump_stack+0x28/0x3c [c001fc6376b0] [c02f04d8] .debug_smp_processor_id+0x108/0x120 [c001fc637740] [c00444e8] .kvmppc_handle_exit+0x48/0x810 [c001fc6377f0] [c0047c80] .kvmppc_resume_host+0xa4/0xf8 Tiejun -- 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: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()
On 07/10/2013 01:02:19 AM, Tiejun Chen wrote: We should ensure the preemption cannot occur while calling get_paca() insdide hard_irq_disable(), otherwise the paca_struct may be the wrong one just after. And btw, we may update timing stats in this case. The soft-ee mechanism depends on accessing the PACA directly via r13 to avoid this. We probably should be using inline asm to read was_enabled rather than hoping the compiler doesn't do anything silly. Plus what Alex said, regarding this patch specifically. -Scott -- 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: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()
On 10.07.2013, at 08:02, Tiejun Chen wrote: > We should ensure the preemption cannot occur while calling get_paca() > insdide hard_irq_disable(), otherwise the paca_struct may be the > wrong one just after. And btw, we may update timing stats in this case. > > Signed-off-by: Tiejun Chen > --- > arch/powerpc/kvm/booke.c |2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c > index dcc94f0..9dae25d 100644 > --- a/arch/powerpc/kvm/booke.c > +++ b/arch/powerpc/kvm/booke.c > @@ -839,6 +839,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct > kvm_vcpu *vcpu, > WARN_ON(local_paca->irq_happened != 0); > #endif > > + preempt_disable(); > /* >* We enter with interrupts disabled in hardware, but >* we need to call hard_irq_disable anyway to ensure that > @@ -848,6 +849,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct > kvm_vcpu *vcpu, > > /* update before a new last_exit_type is rewritten */ > kvmppc_update_timing_stats(vcpu); > + preempt_enable(); All of the code here is already called with interrupts disabled. I don't see how we could preempt then? Alex > > /* restart interrupts if they were meant for the host */ > kvmppc_restart_interrupt(vcpu, exit_nr); > -- > 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 -- 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
[v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()
We should ensure the preemption cannot occur while calling get_paca() insdide hard_irq_disable(), otherwise the paca_struct may be the wrong one just after. And btw, we may update timing stats in this case. Signed-off-by: Tiejun Chen --- arch/powerpc/kvm/booke.c |2 ++ 1 file changed, 2 insertions(+) diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index dcc94f0..9dae25d 100644 --- a/arch/powerpc/kvm/booke.c +++ b/arch/powerpc/kvm/booke.c @@ -839,6 +839,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu, WARN_ON(local_paca->irq_happened != 0); #endif + preempt_disable(); /* * We enter with interrupts disabled in hardware, but * we need to call hard_irq_disable anyway to ensure that @@ -848,6 +849,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu, /* update before a new last_exit_type is rewritten */ kvmppc_update_timing_stats(vcpu); + preempt_enable(); /* restart interrupts if they were meant for the host */ kvmppc_restart_interrupt(vcpu, exit_nr); -- 1.7.9.5 -- 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