Re: [Xenomai-core] Domain switch during page fault handling

[Jan Kiszka <jan.kiszka@domain.hid>]

[-- Attachment #1: Type: text/plain, Size: 7039 bytes --]

Philippe Gerum wrote:
> On Sat, 2010-01-23 at 11:33 +0100, Jan Kiszka wrote:
>> Philippe Gerum wrote:
>>> On Sat, 2010-01-23 at 11:09 +0100, Jan Kiszka wrote:
>>>> Philippe Gerum wrote:
>>>>> On Fri, 2010-01-22 at 19:08 +0100, Philippe Gerum wrote:
>>>>>> On Fri, 2010-01-22 at 19:03 +0100, Jan Kiszka wrote:
>>>>>>> Philippe Gerum wrote:
>>>>>>>> On Fri, 2010-01-22 at 18:41 +0100, Jan Kiszka wrote:
>>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>>> Philippe Gerum wrote:
>>>>>>>>>>> On Fri, 2010-01-22 at 17:58 +0100, Jan Kiszka wrote: 
>>>>>>>>>>>> Hi guys,
>>>>>>>>>>>>
>>>>>>>>>>>> we are currently trying to catch an ugly Linux pipeline state corruption
>>>>>>>>>>>> on x86-64.
>>>>>>>>>>>>
>>>>>>>>>>>> Conceptual question: If a Xenomai task causes a fault, we enter
>>>>>>>>>>>> ipipe_trap_notify over the primary domain and leave it over the root
>>>>>>>>>>>> domain, right? Now, if the root domain happened to be stalled when the
>>>>>>>>>>>> exception happened, where should it normally be unstalled again,
>>>>>>>>>>>> *for_that_task*? Our problem is that we generate a code path where this
>>>>>>>>>>>> does not happen.
>>>>>>>>>>> xnhadow_relax -> ipipe_reenter_root -> finish_task_switch ->
>>>>>>>>>>> finish_lock_switch -> unstall
>>>>>>>>>>>
>>>>>>>>>>> Since xnshadow_relax is called on behalf the event dispatcher, we should
>>>>>>>>>>> expect it to return with the root domain unstalled after a domain
>>>>>>>>>>> downgrade, from primary to root.
>>>>>>>>>> Ok, but what about local_irq_restore_nosync at the end of the function ?
>>>>>>>>>>
>>>>>>>>> That is, IMO, our problem: It replays the root state on fault entry, but
>>>>>>>>> that one is totally unrelated to the (Xenomai) task that caused the fault.
>>>>>>>> The code seems fishy. Try restoring only when the incoming domain was
>>>>>>>> the root one. Indeed.
>>>>>>>>
>>>>>>> Something like this?
>>>>>>>
>>>>>>> diff --git a/arch/x86/kernel/ipipe.c b/arch/x86/kernel/ipipe.c
>>>>>>> index 4442d96..0558ea3 100644
>>>>>>> --- a/arch/x86/kernel/ipipe.c
>>>>>>> +++ b/arch/x86/kernel/ipipe.c
>>>>>>> @@ -702,19 +702,21 @@ static int __ipipe_xlate_signo[] = {
>>>>>>>  
>>>>>>>  int __ipipe_handle_exception(struct pt_regs *regs, long error_code, int vector)
>>>>>>>  {
>>>>>>> +	bool restore_flags = false;
>>>>>>>  	unsigned long flags;
>>>>>>>  
>>>>>>> -	/* Pick up the root domain state of the interrupted context. */
>>>>>>> -	local_save_flags(flags);
>>>>>>> +	if (ipipe_root_domain_p && irqs_disabled_hw()) {
>>>>>>> +		/* Pick up the root domain state of the interrupted context. */
>>>>>>> +		local_save_flags(flags);
>>>>>>>  
>>>>>>> -	if (ipipe_root_domain_p) {
>>>>>>>  		/*
>>>>>>>  		 * Replicate hw interrupt state into the virtual mask before
>>>>>>>  		 * calling the I-pipe event handler over the root domain. Also
>>>>>>>  		 * required later when calling the Linux exception handler.
>>>>>>>  		 */
>>>>>>> -		if (irqs_disabled_hw())
>>>>>>> -			local_irq_disable();
>>>>>>> +		local_irq_disable();
>>>>>>> +
>>>>>>> +		restore_flags = true;
>>>>>>>  	}
>>>>>>>  #ifdef CONFIG_KGDB
>>>>>>>  	/* catch exception KGDB is interested in over non-root domains */
>>>>>>> @@ -725,7 +727,8 @@ int __ipipe_handle_exception(struct pt_regs *regs, long error_code, int vector)
>>>>>>>  #endif /* CONFIG_KGDB */
>>>>>>>  
>>>>>>>  	if (unlikely(ipipe_trap_notify(vector, regs))) {
>>>>>>> -		local_irq_restore_nosync(flags);
>>>>>>> +		if (restore_flags)
>>>>>>> +			local_irq_restore_nosync(flags);
>>>>>>>  		return 1;
>>>>>>>  	}
>>>>>>>  
>>>>>>> @@ -770,7 +773,8 @@ int __ipipe_handle_exception(struct pt_regs *regs, long error_code, int vector)
>>>>>>>  	 * Relevant for 64-bit: Restore root domain state as the low-level
>>>>>>>  	 * return code will not align it to regs.flags.
>>>>>>>  	 */
>>>>>>> -	local_irq_restore_nosync(flags);
>>>>>>> +	if (restore_flags)
>>>>>>> +		local_irq_restore_nosync(flags);
>>>>>>>  
>>>>>>>  	return 0;
>>>>>>>  }
>>>>>>>
>>>>>>>
>>>>>>> We are currently not able to test this on the system that triggers it,
>>>>>>> but we'll do so tomorrow (yeah...).
>>>>>>>
>>>>>> Should work. Famous last words.
>>>>>>
>>>>> Strike that. This won't work, because the fixup code will use the saved
>>>>> flags even when root is not the incoming domain and/or hw IRQs are on on
>>>>> entry. In short, local_save_flags() must be done unconditionally, as
>>>>> previously.
>>>> It will accidentally work for 64-bit where __fixup_if is empty. And for
>>>> 32-bit, I would say we need to make it depend on restore_flags as well.
>>>>
>>> AFAIK, Gilles is working on this. We just need to avoid stepping on
>>> 32bit toes to fix 64.
>>>
>> OK.
>>
>> Just realized that my suggestion would conflict with the comment above
>> __fixup_if. So I think we first need to clarify the various scenarios
>> again to avoid breaking one while fixing another.
>>
>> Entry over non-root, exit over non-root:
>>  - no need to fiddle with the root state
>>
> 
> Correct.
> 
>> Entry over root, exit over root, !irqs_disabled_hw():
>>  - no need to fiddle with the root state
>>  - 32 bit: regs fixup required?
>>
> 
> The iret emulation via iret_root needs proper fixup to have taken place,
> so doing the fixup is mandatory in any case.
> 
>> Entry over root, exit over root, irqs_disabled_hw():
>>  - save root state & disable root IRQs on entry
>>  - 32 bit: replicate saved state into regs.eflags before calling linux
>>    handler
>>  - restore saved state on exit
> 
> Correct.
> 
>> Entry over non-root, exit over root:
>>  - ?
>>
>> I tend to think that, for the 32-bit cases, we should pick up the flags
>> from the root state _after_ returning from ipipe_trap_notify and only if
>> we are truly running in the root domain then. That should be the value
>> the migration left behind, so the correct one, right?
>>
> 
> Yes.
> 
>> Any scenario missing?
>>
> 
> Should be ok. But the more I think of it, the more I'm convinced that we
> should make the 32 and 64 bit implementation converge to the x86_64 one.
> iret_root emulation is required because we virtualize the interrupt
> masking in the assembly-written portions for x86_32, which we don't for
> x86_64. 
> 
> Hyste^Horically, there was a strong requirement to do that with legacy
> 2.4 kernels the 32 bit implementation was designed for, because lengthy
> masked sections would raise the latency above the top. With current
> 2.6/x86 kernels, I don't think virtualizing interrupt masking in the
> assembly-written portions makes a lot of sense anymore, since
> significant work took place upstream over time, to allow for
> fine-grained preemption there.

I'm all for this, but I think it should happen gradually. In step 1, we
should fix the current situation. Step 2 would obsolete __fixup_if by
moving x86-32 towards the 64-bit scheme.

So a discussion of my last patch would be warmly welcomed.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]