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

["tiejun.chen" <tiejun.chen@windriver.com>]

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