From mboxrd@z Thu Jan 1 00:00:00 1970 Date: Mon, 23 Feb 2015 21:38:21 +0100 From: Gilles Chanteperdrix Message-ID: <20150223203821.GS22377@hermes.click-hack.org> References: <54EB5021.3030508@siemens.com> <54EB5638.3050805@xenomai.org> <20150223163743.GA22377@hermes.click-hack.org> <54EB5A45.9000002@siemens.com> <20150223165549.GC22377@hermes.click-hack.org> <54EB5D02.8080105@xenomai.org> <20150223171250.GE22377@hermes.click-hack.org> <54EB8CC3.5050005@xenomai.org> <20150223202759.GR22377@hermes.click-hack.org> <54EB8EA5.5020808@xenomai.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <54EB8EA5.5020808@xenomai.org> Subject: Re: [Xenomai] ipipe: issues with ARM exception handling List-Id: Discussions about the Xenomai project List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Philippe Gerum Cc: Jan Kiszka , Xenomai 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.