On Mon, Feb 23, 2015 at 09:49:44PM +0100, Philippe Gerum wrote:
> On 02/23/2015 09:38 PM, Gilles Chanteperdrix wrote:
> > On Mon, Feb 23, 2015 at 09:33:41PM +0100, Philippe Gerum wrote:
> >> On 02/23/2015 09:27 PM, Gilles Chanteperdrix wrote:
> >>> On Mon, Feb 23, 2015 at 09:25:39PM +0100, Philippe Gerum wrote:
> >>>> On 02/23/2015 06:12 PM, Gilles Chanteperdrix wrote:
> >>>>> On Mon, Feb 23, 2015 at 06:01:54PM +0100, Philippe Gerum wrote:
> >>>>>> On 02/23/2015 05:55 PM, Gilles Chanteperdrix wrote:
> >>>>>>> On Mon, Feb 23, 2015 at 05:50:13PM +0100, Jan Kiszka wrote:
> >>>>>>>> On 2015-02-23 17:37, Gilles Chanteperdrix wrote:
> >>>>>>>>> On Mon, Feb 23, 2015 at 05:32:56PM +0100, Philippe Gerum wrote:
> >>>>>>>>>> On 02/23/2015 05:06 PM, Jan Kiszka wrote:
> >>>>>>>>>>> On 2015-02-20 20:52, Philippe Gerum wrote:
> >>>>>>>>>>>> On 02/20/2015 08:47 PM, Philippe Gerum wrote:
> >>>>>>>>>>>>> On 02/20/2015 08:44 PM, Philippe Gerum wrote:
> >>>>>>>>>>>>>> On 02/20/2015 07:17 PM, Jan Kiszka wrote:
> >>>>>>>>>>>>>>> On 2015-02-20 19:03, Jan Kiszka wrote:
> >>>>>>>>>>>>>>>> Hi Gilles,
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> analyzing a lockdep warning on 3.16 with I-pipe enabled, I
> >>>>>>>>>>>>>>>> dug deeper
> >>>>>>>>>>>>>>>> into the hard and virtual interrupt state management during
> >>>>>>>>>>>>>>>> exception
> >>>>>>>>>>>>>>>> handling on ARM. I think there are several issues:
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> - ipipe_fault_entry should not fiddle with the root irq
> >>>>>>>>>>>>>>>> state if run
> >>>>>>>>>>>>>>>> over head, only when invoked over root.
> >>>>>>>>>>>>>>>> - ipipe_fault_exit must not change the root state unless we
> >>>>>>>>>>>>>>>> entered over
> >>>>>>>>>>>>>>>> head and are about to leave over root - see x86. The
> >>>>>>>>>>>>>>>> current code may
> >>>>>>>>>>>>>>>> keep root incorrectly stalled after an exception, though
> >>>>>>>>>>>>>>>> this will
> >>>>>>>>>>>>>>>> probably be fixed up again in practice quickly.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> And the adjustment of the root irq state after migration has
> >>>>>>>>>>>>>>> to happen
> >>>>>>>>>>>>>>> before Linux starts to handle the event. It would basically
> >>>>>>>>>>>>>>> be a late
> >>>>>>>>>>>>>>> ipipe_fault_entry.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> - do_sect_fault is only called by do_DataAbort and
> >>>>>>>>>>>>>>>> do_PrefetchAbort,
> >>>>>>>>>>>>>>>> in both cases already wrapped in ipipe_fault_entry/exit,
> >>>>>>>>>>>>>>>> thus it
> >>>>>>>>>>>>>>>> shouldn't invoke them once again.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Sorry, this was a misinterpretation - do_sect_fault is
> >>>>>>>>>>>>>>> invoked before
> >>>>>>>>>>>>>>> ipipe_fault_entry.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> What I need to add, though:
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> - do_DataAbort and do_PrefetchAbort call __ipipe_report_trap
> >>>>>>>>>>>>>>> after
> >>>>>>>>>>>>>>> ipipe_fault_entry, thus with hard IRQs on.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> This would break LPAE with the Xenomai nucleus as a module on
> >>>>>>>>>>>>>> 2.6.x, by
> >>>>>>>>>>>>>> treading over a non-linear kernel mapping before the page
> >>>>>>>>>>>>>> table could be
> >>>>>>>>>>>>>> fixed up. do_translation_fault() must run via the fsr handler
> >>>>>>>>>>>>>> indirection before any non-linear access.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Sorry, if you do that _after_ the fault entry notification,
> >>>>>>>>>>>>> then it's ok
> >>>>>>>>>>>>> in theory. However, I don't understand why we would need to
> >>>>>>>>>>>>> notify when
> >>>>>>>>>>>>> only a minor fixup is required, that does not entail a mode
> >>>>>>>>>>>>> migration.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>> To be clearer, do you intend to report the minor fault upon
> >>>>>>>>>>>> do_translation_fault() returning zero, or are you referring to a
> >>>>>>>>>>>> different context?
> >>>>>>>>>>>
> >>>>>>>>>>> No, I'm just talking about this potential change:
> >>>>>>>>>>>
> >>>>>>>>>>> diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
> >>>>>>>>>>> index 38834c6..b42632a 100644
> >>>>>>>>>>> --- a/arch/arm/mm/fault.c
> >>>>>>>>>>> +++ b/arch/arm/mm/fault.c
> >>>>>>>>>>> @@ -629,10 +629,10 @@ do_DataAbort(unsigned long addr, unsigned
> >>>>>>>>>>> int fsr, struct pt_regs *regs)
> >>>>>>>>>>> if (!inf->fn(addr, fsr & ~FSR_LNX_PF, regs))
> >>>>>>>>>>> return;
> >>>>>>>>>>>
> >>>>>>>>>>> - irqflags = ipipe_fault_entry();
> >>>>>>>>>>> -
> >>>>>>>>>>> if (__ipipe_report_trap(IPIPE_TRAP_UNKNOWN, regs))
> >>>>>>>>>>> - goto out;
> >>>>>>>>>>> + return;
> >>>>>>>>>>> +
> >>>>>>>>>>> + irqflags = ipipe_fault_entry();
> >>>>>>>>>>>
> >>>>>>>>>>> printk(KERN_ALERT "Unhandled fault: %s (0x%03x) at 0x%08lx\n",
> >>>>>>>>>>> inf->name, fsr, addr);
> >>>>>>>>>>> @@ -642,7 +642,7 @@ do_DataAbort(unsigned long addr, unsigned int
> >>>>>>>>>>> fsr, struct pt_regs *regs)
> >>>>>>>>>>> info.si_code = inf->code;
> >>>>>>>>>>> info.si_addr = (void __user *)addr;
> >>>>>>>>>>> arm_notify_die("", regs, &info, fsr, 0);
> >>>>>>>>>>> -out:
> >>>>>>>>>>> +
> >>>>>>>>>>> ipipe_fault_exit(irqflags);
> >>>>>>>>>>> }
> >>>>>>>>>>>
> >>>>>>>>>>> @@ -669,10 +669,10 @@ do_PrefetchAbort(unsigned long addr,
> >>>>>>>>>>> unsigned int ifsr, struct pt_regs *regs)
> >>>>>>>>>>> if (!inf->fn(addr, ifsr | FSR_LNX_PF, regs))
> >>>>>>>>>>> return;
> >>>>>>>>>>>
> >>>>>>>>>>> - irqflags = ipipe_fault_entry();
> >>>>>>>>>>> -
> >>>>>>>>>>> if (__ipipe_report_trap(IPIPE_TRAP_UNKNOWN, regs))
> >>>>>>>>>>> - goto out;
> >>>>>>>>>>> + return;
> >>>>>>>>>>> +
> >>>>>>>>>>> + irqflags = ipipe_fault_entry();
> >>>>>>>>>>>
> >>>>>>>>>>> printk(KERN_ALERT "Unhandled prefetch abort: %s (0x%03x) at
> >>>>>>>>>>> 0x%08lx\n",
> >>>>>>>>>>> inf->name, ifsr, addr);
> >>>>>>>>>>> @@ -682,7 +682,7 @@ do_PrefetchAbort(unsigned long addr, unsigned
> >>>>>>>>>>> int ifsr, struct pt_regs *regs)
> >>>>>>>>>>> info.si_code = inf->code;
> >>>>>>>>>>> info.si_addr = (void __user *)addr;
> >>>>>>>>>>> arm_notify_die("", regs, &info, ifsr, 0);
> >>>>>>>>>>> -out:
> >>>>>>>>>>> +
> >>>>>>>>>>> ipipe_fault_exit(irqflags);
> >>>>>>>>>>> }
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> This seems more consistent - if not more correct - as it now does
> >>>>>>>>>>> the
> >>>>>>>>>>> reporting with hard irqs off, like in the other cases.
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> Ack, definitely. The pattern is to cause any migration first if
> >>>>>>>>>> need be,
> >>>>>>>>>> _then_ flip the virtual IRQ state, so that ipipe_fault_restore()
> >>>>>>>>>> always
> >>>>>>>>>> reinstates the interrupt state in effect after the caller has
> >>>>>>>>>> migrated
> >>>>>>>>>> to the root domain.
> >>>>>>>>>
> >>>>>>>>> Is it even useful ? After a relax, the state of the root thread
> >>>>>>>>> stall bit and irq flags are well known...
> >>>>>>>>
> >>>>>>>> We still need to disable IRQs for root. HW IRQs are likely already
> >>>>>>>> on,
> >>>>>>>> right?
> >>>>>>>>
> >>>>>>>> And, again, we should refrain from restoring any root irq state on
> >>>>>>>> return - it belongs to Linux (once we migrated and synchronized the
> >>>>>>>> state).
> >>>>>>>
> >>>>>>> The ipipe_fault_exit in my tree is:
> >>>>>>>
> >>>>>>> static inline void ipipe_fault_exit(unsigned long x)
> >>>>>>> {
> >>>>>>> if (!arch_demangle_irq_bits(&x))
> >>>>>>> local_irq_enable();
> >>>>>>> else
> >>>>>>> hard_local_irq_restore(x);
> >>>>>>> }
> >>>>>>>
> >>>>>>> And I must say I am not sure I understand how it works. To me it
> >>>>>>> seems:
> >>>>>>
> >>>>>> It mangles both the real and virtual states in one word.
> >>>>>>
> >>>>>>> hard_local_irq_disable() should always be called in case entry.S
> >>>>>>> expects us to return as we entered: with hw irqs off
> >>>>>>
> >>>>>> Which is what ipipe_fault_exit() does by testing the mangled state. If
> >>>>>> the fault entered with virtual IRQs on, then you must exit with both
> >>>>>> the
> >>>>>> stall bit and CPSR_I bit cleared.
> >>>>>
> >>>>> Absolutely not. Imagine a Linux task, with root unstalled
> >>>>> experiencing a fault. entry.S is entered root is still unstalled,
> >>>>> with hardware irqs off. On fault entry, we must reflect this
> >>>>> hardware irq state on the stall bit and enable hw irqs. Then when
> >>>>> the fault is handled, undo that, unstall the root stage, disable hw
> >>>>> irqs and return to entry.S, so that it may resume the execution of
> >>>>> the Linux task. If it returns quickly to user-space, a stalled root
> >>>>> at this point would be a disaster, because nothing, certainly not
> >>>>> entry.S will unstall the root stage.
> >>>>>
> >>>>
> >>>> How is a user-space task supposed to enter a fault with the stall bit
> >>>> set?
> >>>
> >>> unstalled == stall bit NOT set.
> >>>
> >>
> >> Asking again a bit differently, which virtual state makes sense when a
> >> fault happens according to you: the one that prevailed before taking the
> >> fault, or the one forced by the fault handler?
> >
> > I do not understand your question, so I will answer with what I
> > already proposed:
> >
> > static inline unsigned long ipipe_fault_entry(void)
> > {
> > unsigned long flags;
> >
> > local_irq_save(flags);
> > hard_local_irq_enable();
> >
> > return flags;
> > }
> >
> > static inline void ipipe_fault_exit(unsigned long flags)
> > {
> > if (irqs_disabled())
> > hard_local_irq_disable();
> > ipipe_restore_root_nosync(flags);
> > }
> >
> > Assuming that:
> > - ipipe_fault_entry is always called from secondary mode (so after a
> > relax if there is a relax)
> > - ipipe_fault_exit is not called when a fault is handled in primary
> > mode.
> >
>
> This code basically means: let's propagate the current virtual interrupt
> state to the hardware interrupt state. I must admit that it does not
> make any sense to me. The hardware interrupt state has no reason to be
> alogned on the virtual interrupt state when executing the handlers, the
> converse dependency does exist.
If the exception code does:
local_irq_enable()
It means that it wants to return to entry.S with irqs on. In that
case, entry.S has to be prepared to work with hardware irqs on, so
we can do it. But I agree that this is an optimization and that we
could always return with hardware irqs off. It is the safe bet.
Also, I expect ipipe_fault_exit to be executed right before
returning to entry.S, so it would not be used "when executing the
handlers".
--
Gilles.
_______________________________________________
Xenomai mailing list
[email protected]
http://www.xenomai.org/mailman/listinfo/xenomai
