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.

> Jan
> 


-- 
Philippe.



_______________________________________________
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Reply via email to