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.
>
> --
> Philippe.
--
Gilles.
_______________________________________________
Xenomai mailing list
[email protected]
http://www.xenomai.org/mailman/listinfo/xenomai
