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

Commit Message

Tiejun Chen July 12, 2013, 4:54 a.m. UTC

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 <[e-mail]>
---
  arch/powerpc/include/asm/hw_irq.h |    7 ++++---
  1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Benjamin Herrenschmidt July 14, 2013, 4:13 a.m. UTC | #1

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 <[e-mail]>

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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tiejun Chen July 15, 2013, 3:04 a.m. UTC | #2

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 <[e-mail]>
>
> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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)