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

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 =20
> case.

The soft-ee mechanism depends on accessing the PACA directly via r13 to =20
avoid this.  We probably should be using inline asm to read was_enabled =20
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 =20
>>> 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 =20
>>> case.
>>=20
>> The soft-ee mechanism depends on accessing the PACA directly via r13 =20
>> to avoid
>> this.  We probably should be using inline asm to read was_enabled =20
>> rather than
>=20
> Yes.
>=20
>> hoping the compiler doesn't do anything silly.
>=20
> Do you recommend I should directly replace get_paca() with local_paca =20
> inside hard_irq_disable()?
>=20
> #define hard_irq_disable()      do {                    \
>         u8 _was_enabled =3D get_paca()->soft_enabled;     \
>=20
> ->	u8 _was_enabled =3D local_paca->soft_enabled;
>=20
> But is this safe for all scenarios?

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

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

-Scott=