[v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()

[v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()

[Tiejun Chen]

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 <tiejun.chen@windriver.com>
---
 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

Re: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()

[Alexander Graf]

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 <tiejun.chen@windriver.com>
> ---
> 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 majordomo@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()

[tiejun.chen]

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 <tiejun.chen@windriver.com>
>> ---
>> 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 [00000000] 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:
[c0000001fc637570] [c00000000000835c] .show_stack+0x7c/0x1f0 (unreliable)
[c0000001fc637640] [c000000000673a0c] .dump_stack+0x28/0x3c
[c0000001fc6376b0] [c0000000002f04d8] .debug_smp_processor_id+0x108/0x120
[c0000001fc637740] [c0000000000444e8] .kvmppc_handle_exit+0x48/0x810
[c0000001fc6377f0] [c000000000047c80] .kvmppc_resume_host+0xa4/0xf8

Tiejun

Re: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()

[Alexander Graf]

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 <tiejun.chen@windriver.com>
>>> ---
>>> 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 [00000000] 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:
> [c0000001fc637570] [c00000000000835c] .show_stack+0x7c/0x1f0 (unreliable)
> [c0000001fc637640] [c000000000673a0c] .dump_stack+0x28/0x3c
> [c0000001fc6376b0] [c0000000002f04d8] .debug_smp_processor_id+0x108/0x120
> [c0000001fc637740] [c0000000000444e8] .kvmppc_handle_exit+0x48/0x810
> [c0000001fc6377f0] [c000000000047c80] .kvmppc_resume_host+0xa4/0xf8
> 
> Tiejun
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
> the body of a message to majordomo@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()

[Benjamin Herrenschmidt]

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.

Re: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()

[Alexander Graf]

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

Re: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()

[Benjamin Herrenschmidt]

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.

Re: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()

[Alexander Graf]

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

Re: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()

[Benjamin Herrenschmidt]

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.

Re: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()

[tiejun.chen]

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.
>
>
>
>

Re: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()

[Benjamin Herrenschmidt]

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.

Re: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()

[tiejun.chen]

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 <tiejun.chen@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


Or what about that change to call SOFT_DISABLE_INTS only in KVM scenario? Which 
better?

Then I can send to review?

Thanks,

Tiejun

Re: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()

[Benjamin Herrenschmidt]

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.chen@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.

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.

Re: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()

[tiejun.chen]

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.chen@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

Re: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()

[Scott Wood]

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

Re: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()

[tiejun.chen]

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

Re: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()

[Scott Wood]

On 07/10/2013 10:00:33 PM, tiejun.chen wrote:
> 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?

get_paca() is just a #define for local_paca.  It won't make a  
difference, other than to evade the debug check.

In practice, it's unlikely that GCC would do anything other than a load  
directly from r13, but to be sure we should use inline asm to do the  
load, just like arch_local_save_flags and arch_local_irq_disable do.

-Scott

Re: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()

[Benjamin Herrenschmidt]

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.

Re: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()

[tiejun.chen]

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 <tiejun.chen@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

         /* Restore vcpu pointer and the nonvolatiles we used. */


Tiejun

Re: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()

[Benjamin Herrenschmidt]

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.

Re: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()

[tiejun.chen]

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

Re: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()

[Benjamin Herrenschmidt]

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.

Re: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()

[tiejun.chen]

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

Re: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()

[tiejun.chen]

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

Re: [v1][PATCH 1/1] KVM: PPC: disable preemption when using hard_irq_disable()

[tiejun.chen]

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.chen@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