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.kis...@siemens.com>
>>> ---
>>>
>>> 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 ;-)
Cheers, Wolfgang
>>> + } else {
>>> /* Detect unhandled faults over non-root domains. */
>>> struct ipipe_domain *ipd = ipipe_current_domain;
>>>
>>> @@ -770,21 +778,29 @@ 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 (root_entry)
>>> + local_irq_restore_nosync(flags);
>>>
>>> return 0;
>>> }
>>>
>>> int __ipipe_divert_exception(struct pt_regs *regs, int vector)
>>> {
>>> - unsigned long flags;
>>> -
>>> - /* Same root state handling as in __ipipe_handle_exception. */
>>> - local_save_flags(flags);
>>> + bool root_entry = false;
>>> + unsigned long flags = 0;
>>>
>>> if (ipipe_root_domain_p) {
>>> - if (irqs_disabled_hw())
>>> + root_entry = true;
>>> +
>>> + local_save_flags(flags);
>>> +
>>> + if (irqs_disabled_hw()) {
>>> + /*
>>> + * Same root state handling as in
>>> + * __ipipe_handle_exception.
>>> + */
>>> local_irq_disable();
>>> + }
>>> }
>>> #ifdef CONFIG_KGDB
>>> /* catch int1 and int3 over non-root domains */
>>> @@ -804,16 +820,21 @@ int __ipipe_divert_exception(struct pt_regs *regs,
>>> int vector)
>>> #endif /* CONFIG_KGDB */
>>>
>>> if (unlikely(ipipe_trap_notify(vector, regs))) {
>>> - local_irq_restore_nosync(flags);
>>> + if (root_entry)
>>> + local_irq_restore_nosync(flags);
>>> return 1;
>>> }
>>>
>>> + if (likely(ipipe_root_domain_p)) {
>>> + /* see __ipipe_handle_exception */
>>> + __fixup_if(root_entry ? raw_irqs_disabled_flags(flags) :
>>> + raw_irqs_disabled(), regs);
>>> + }
>>> +
>>> /*
>>> - * 32-bit: Due to possible migration inside the event handler, we have
>>> - * to restore IF so that low-level return code sets the root domain
>>> - * state correctly.
>>> + * No need to restore root state in the 64-bit case, the Linux handler
>>> + * and the return code will take care of it.
>>> */
>>> - __fixup_if(raw_irqs_disabled_flags(flags), regs);
>>>
>>> return 0;
>>> }
>
> Jan
>
_______________________________________________
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core
