Re: [Xenomai-core] [PATCH v2] x86: Fix root domain state restoring on exception return

[Wolfgang Mauerer <wolfgang.mauerer@domain.hid>]

Jan Kiszka wrote:
> Wolfgang Mauerer wrote:
>> Jan Kiszka wrote:
>>> Wolfgang Mauerer wrote:
>>>> Jan Kiszka wrote:
>>>>> Wolfgang Mauerer wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> Wolfgang Mauerer wrote:
>>>>>>>> Jan Kiszka wrote:
>>>>>>>>> If we enter __ipipe_handle_exception over a non-root domain and leave it
>>>>>>>>> due to migration in the event handler over root, we must not restore the
>>>>>>>>> root domain state so far saved on entry. This caused subtle pipeline
>>>>>>>>> state corruptions. Instead, only save and restore them if we were
>>>>>>>>> entering over root.
>>>>>>>>>
>>>>>>>>> However, the x86-32 regs.flags fixup is required nevertheless to take
>>>>>>>>> care of mismatches between the root domain state and the hardware flags
>>>>>>>>> on entry. That may happen if we fault in the iret path. But also in this
>>>>>>>>> case we must not restore an invalid root domain state. So if we entered
>>>>>>>>> over non-root, pick up the input for __fixup_if from the root domain
>>>>>>>>> after running the ipipe handler.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> Next try. But this time I think I finally understood what scenario
>>>>>>>>> __fixup_if is actually fixing. Please correct me if I'm still missing
>>>>>>>>> one.
>>>>>>>> looks good - it works for my test cases and solves the problems with
>>>>>>>> the hw/pipeline state mismatch during early bootup. But do you happen
>>>>>>>> to have any scenario at hand with ipipe_domain_root_p && !root_entry?
>>>>>>>> Couldn't trigger this one yet so only the raw_irqs_disabled_flags
>>>>>>>> fixup is excercised, though I guess it can't do any harm to really
>>>>>>>> ensure that the explanation fits reality this time...
>>>>>>> You mean non-root entry -> migration -> __fixup_if? In that case we pick
>>>>>>> up the flags for fixup _after_ the migration (raw_irqs_disabled()). Or
>>>>>>> what do you mean?
>>>>>> (...)
>>>>>>>>> -	if (unlikely(!ipipe_root_domain_p)) {
>>>>>>>>> +	if (likely(ipipe_root_domain_p)) {
>>>>>>>>> +		/*
>>>>>>>>> +		 * 32-bit: In case we faulted in the iret path, regs.flags do
>>>>>>>>> +		 * not match the root domain state as the low-level return
>>>>>>>>> +		 * code will evaluate it. Fix this up, either by the root
>>>>>>>>> +		 * state sampled on entry or, if we migrated to root, with the
>>>>>>>>> +		 * current state.
>>>>>>>>> +		 */
>>>>>>>>> +		__fixup_if(root_entry ? raw_irqs_disabled_flags(flags) :
>>>>>>>>> +					raw_irqs_disabled(), regs);
>>>>>> I'm referring to the case that evaluates to
>>>>>> __fixup_if(raw_irqs_disabled(), regs); That is, something that
>>>>>> triggers
>>>>>>
>>>>>> 		if (!root_entry)
>>>>>> 			do_something();
>>>>>>
>>>>>> Could be that we're talking about to the same case, although I'm not
>>>>>> sure ;-)
>>>>> Right, that's the case I described above. What problem do you precisely
>>>>> see or what concerns to you have about the suggested behavior?
>>>> None. I'd just like to be able to trigger it to avoid that
>>>> there are any unforseen problems we're still missing.
>>>> Since this corner of Ipipe seems to have proven tricky before
>>>> AFAIK, I thought it might perhaps be worth while to really excercise
>>>> every possible code path.
>>> To trigger it, you need to recreate the scenario that our colleagues
>>> managed to generate on the MARS: Have Linux IRQs disabled (or enabled,
>>> to have both variations), schedule in a Xenomai task, let it raise a
>>> fault so that Xenomai migrates it back.
>> Yep, but allocating a large amount of memory in non-rt, switching to
>> rt and then accessing this and switching back to non-rt
>> in the hope of generating many faults so that the scenario would be
>> synthesised without expensive and hard to access hardware was
>> unsuccessful, so my little innocent question was really just if you
>> happened to have a testcase.
> 
> OK, then I totally misunderstood your question.

Seems we have nevertheless finally managed to reach an agreement on
what we mean ;-)
> 
> You do not need to generate a fixable fault, and unfixable one (*(int
> *)NULL = 0;) should suffice.

yeah, although that would maybe be a bit ugly wrt. handling the SEGV
and continuing to allow continuous faults without
having to restart the program all the time.

As we just discussed offline, this would anyway not yet trigger the
"fault in iret"-scenario, and I don't think it's worth while spending
the large extra effort for this considering that the code will sooner
or later die anyway. Or does maybe anyone have an idea how to trigger
such a fault with simple means?

Cheers, Wolfgang