On Sun, 2010-01-24 at 11:45 +0100, Jan Kiszka wrote:
> 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.
I'm not saying anything else. But I want to stress the fact that we are
fixing the current mess for the last time; next time, let's kick out
this legacy.
>
> So a discussion of my last patch would be warmly welcomed.
>
> Jan
>
--
Philippe.
_______________________________________________
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core
