[Xenomai] ipipe: issues with ARM exception handling

[Xenomai] ipipe: issues with ARM exception handling

[Jan Kiszka]

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.
- 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.

Room for optimization:
- ipipe_fault_entry is always called with hard IRQs off from
  do_page_fault and do_translation_fault. I suspect this applies to the
  remaining callers (do_DataAbort and do_PrefetchAbort ) as well. Thus
  the hard IRQ state is actually known at compile time, right?

I can hack up patches, but I'd like to confirm first that I'm not
missing anything subtle or ARM-specific here.

Jan

Re: [Xenomai] ipipe: issues with ARM exception handling

[Gilles Chanteperdrix]

On Fri, Feb 20, 2015 at 07:03:14PM +0100, Jan Kiszka wrote:
> Hi Gilles,

Philippe is the author of this code. My code did something dumb
along the lines of what you are suggested and he found that it in
fact did not work.

> 
> 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.
> - 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.
> 
> Room for optimization:
> - ipipe_fault_entry is always called with hard IRQs off from
>   do_page_fault and do_translation_fault. I suspect this applies to the
>   remaining callers (do_DataAbort and do_PrefetchAbort ) as well. Thus
>   the hard IRQ state is actually known at compile time, right?
> 
> I can hack up patches, but I'd like to confirm first that I'm not
> missing anything subtle or ARM-specific here.
> 
> Jan

-- 
					    Gilles.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Jan Kiszka]

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. That's contradicting the
  pattern used otherwise. If we can avoid this, ipipe_fault_entry/exit
  would probably only run over the root domain, and things become
  simpler to sort out.

Jan

> 
> Room for optimization:
> - ipipe_fault_entry is always called with hard IRQs off from
>   do_page_fault and do_translation_fault. I suspect this applies to the
>   remaining callers (do_DataAbort and do_PrefetchAbort ) as well. Thus
>   the hard IRQ state is actually known at compile time, right?
> 
> I can hack up patches, but I'd like to confirm first that I'm not
> missing anything subtle or ARM-specific here.
> 
> Jan
> 

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] ipipe: issues with ARM exception handling

[Jan Kiszka]

Mails crossed - Philippe, can you comment on this topic?

On 2015-02-20 19:17, 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. That's contradicting the
>   pattern used otherwise. If we can avoid this, ipipe_fault_entry/exit
>   would probably only run over the root domain, and things become
>   simpler to sort out.
> 
> Jan
> 
>>
>> Room for optimization:
>> - ipipe_fault_entry is always called with hard IRQs off from
>>   do_page_fault and do_translation_fault. I suspect this applies to the
>>   remaining callers (do_DataAbort and do_PrefetchAbort ) as well. Thus
>>   the hard IRQ state is actually known at compile time, right?
>>
>> I can hack up patches, but I'd like to confirm first that I'm not
>> missing anything subtle or ARM-specific here.
>>
>> Jan
>>
> 

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] ipipe: issues with ARM exception handling

[Philippe Gerum]

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.

-- 
Philippe.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Philippe Gerum]

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.

-- 
Philippe.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Philippe Gerum]

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?

-- 
Philippe.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Jan Kiszka]

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.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] ipipe: issues with ARM exception handling

[Philippe Gerum]

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.

-- 
Philippe.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Gilles Chanteperdrix]

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...

-- 
					    Gilles.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Jan Kiszka]

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).

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] ipipe: issues with ARM exception handling

[Gilles Chanteperdrix]

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?

Everything is on, hw and sw irqs.

> 
> And, again, we should refrain from restoring any root irq state on
> return - it belongs to Linux (once we migrated and synchronized the state).

I think restoring the root irq state makes sense when the fault
handler was entered over root domain.

-- 
					    Gilles.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Jan Kiszka]

On 2015-02-23 17:52, 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?
> 
> Everything is on, hw and sw irqs.

SW IRQs still have to be disabled then, because this is what happens on
real hw. And we need to ensure the that caller's interrupt state visible
via the registers to Linux is either updated to reflect the previous
virtual state or otherwise carried forward so that Linux gets the right
results.

> 
>>
>> And, again, we should refrain from restoring any root irq state on
>> return - it belongs to Linux (once we migrated and synchronized the state).
> 
> I think restoring the root irq state makes sense when the fault
> handler was entered over root domain.

No, it's wrong, but maybe harmless in most cases (see the other part of
the thread). We should respect Linux' decision about its IRQ mask, not
overwrite it.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] ipipe: issues with ARM exception handling

[Gilles Chanteperdrix]

On Mon, Feb 23, 2015 at 06:02:08PM +0100, Jan Kiszka wrote:
> On 2015-02-23 17:52, 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?
> > 
> > Everything is on, hw and sw irqs.
> 
> SW IRQs still have to be disabled then, because this is what happens on
> real hw. And we need to ensure the that caller's interrupt state visible
> via the registers to Linux is either updated to reflect the previous
> virtual state or otherwise carried forward so that Linux gets the right
> results.
> 
> > 
> >>
> >> And, again, we should refrain from restoring any root irq state on
> >> return - it belongs to Linux (once we migrated and synchronized the state).
> > 
> > I think restoring the root irq state makes sense when the fault
> > handler was entered over root domain.
> 
> No, it's wrong, but maybe harmless in most cases (see the other part of
> the thread). We should respect Linux' decision about its IRQ mask, not
> overwrite it.

entry.S does not handle virtual irq state. So, if it returns to
user-space with the root stage stalled, nothing will unstall it. And
you probably get a lockup.

-- 
					    Gilles.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Jan Kiszka]

On 2015-02-23 18:14, Gilles Chanteperdrix wrote:
> On Mon, Feb 23, 2015 at 06:02:08PM +0100, Jan Kiszka wrote:
>> On 2015-02-23 17:52, 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?
>>>
>>> Everything is on, hw and sw irqs.
>>
>> SW IRQs still have to be disabled then, because this is what happens on
>> real hw. And we need to ensure the that caller's interrupt state visible
>> via the registers to Linux is either updated to reflect the previous
>> virtual state or otherwise carried forward so that Linux gets the right
>> results.
>>
>>>
>>>>
>>>> And, again, we should refrain from restoring any root irq state on
>>>> return - it belongs to Linux (once we migrated and synchronized the state).
>>>
>>> I think restoring the root irq state makes sense when the fault
>>> handler was entered over root domain.
>>
>> No, it's wrong, but maybe harmless in most cases (see the other part of
>> the thread). We should respect Linux' decision about its IRQ mask, not
>> overwrite it.
> 
> entry.S does not handle virtual irq state. So, if it returns to
> user-space with the root stage stalled, nothing will unstall it. And
> you probably get a lockup.

Right, that's a point. I'm seeing this pattern in x86 now as well.
Comparing further.

What's missing on ARM are flags adjustments in the register set before
letting Linux inspect that (__fixup_if equivalent). And we should
probably use ipipe_restore_root_nosync on return.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] ipipe: issues with ARM exception handling

[Jan Kiszka]

On 2015-02-23 18:38, Jan Kiszka wrote:
> On 2015-02-23 18:14, Gilles Chanteperdrix wrote:
>> entry.S does not handle virtual irq state. So, if it returns to
>> user-space with the root stage stalled, nothing will unstall it. And
>> you probably get a lockup.
> 
> Right, that's a point. I'm seeing this pattern in x86 now as well.
> Comparing further.
> 
> What's missing on ARM are flags adjustments in the register set before
> letting Linux inspect that (__fixup_if equivalent). And we should
> probably use ipipe_restore_root_nosync on return.

There is another difference that should be considered carefullly. x86 says

 * If we fault over the root domain, we need to replicate the
 * hw interrupt state into the virtual mask before calling the
 * I-pipe event handler.

and then updates the root state before it calls __ipipe_notify_trap -
why? x86-specific?

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] ipipe: issues with ARM exception handling

[Jan Kiszka]

On 2015-02-23 18:49, Jan Kiszka wrote:
> On 2015-02-23 18:38, Jan Kiszka wrote:
>> On 2015-02-23 18:14, Gilles Chanteperdrix wrote:
>>> entry.S does not handle virtual irq state. So, if it returns to
>>> user-space with the root stage stalled, nothing will unstall it. And
>>> you probably get a lockup.
>>
>> Right, that's a point. I'm seeing this pattern in x86 now as well.
>> Comparing further.
>>
>> What's missing on ARM are flags adjustments in the register set before
>> letting Linux inspect that (__fixup_if equivalent). And we should
>> probably use ipipe_restore_root_nosync on return.
> 
> There is another difference that should be considered carefullly. x86 says
> 
>  * If we fault over the root domain, we need to replicate the
>  * hw interrupt state into the virtual mask before calling the
>  * I-pipe event handler.
> 
> and then updates the root state before it calls __ipipe_notify_trap -
> why? x86-specific?

The why is referring to the "before calling the I-pipe event handler" -
that we have to do this afterwards when we ended up or already were in
root is clear.

Jan


-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] ipipe: issues with ARM exception handling

[Jan Kiszka]

On 2015-02-23 18:52, Jan Kiszka wrote:
> On 2015-02-23 18:49, Jan Kiszka wrote:
>> On 2015-02-23 18:38, Jan Kiszka wrote:
>>> On 2015-02-23 18:14, Gilles Chanteperdrix wrote:
>>>> entry.S does not handle virtual irq state. So, if it returns to
>>>> user-space with the root stage stalled, nothing will unstall it. And
>>>> you probably get a lockup.
>>>
>>> Right, that's a point. I'm seeing this pattern in x86 now as well.
>>> Comparing further.
>>>
>>> What's missing on ARM are flags adjustments in the register set before
>>> letting Linux inspect that (__fixup_if equivalent). And we should
>>> probably use ipipe_restore_root_nosync on return.
>>
>> There is another difference that should be considered carefullly. x86 says
>>
>>  * If we fault over the root domain, we need to replicate the
>>  * hw interrupt state into the virtual mask before calling the
>>  * I-pipe event handler.
>>
>> and then updates the root state before it calls __ipipe_notify_trap -
>> why? x86-specific?
> 
> The why is referring to the "before calling the I-pipe event handler" -
> that we have to do this afterwards when we ended up or already were in
> root is clear.

It seems this is now - at beast - historic logic of x86. We (no longer?)
report anything when __ipipe_notify_trap is invoked over the root domain:

        /*
         * We send a notification about all traps raised over a
         * registered head domain only.
         */
        if (__ipipe_root_p)
                goto out;

Will clean up on x86 soon.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] ipipe: issues with ARM exception handling

[Jan Kiszka]

On 2015-02-23 19:30, Jan Kiszka wrote:
> On 2015-02-23 18:52, Jan Kiszka wrote:
>> On 2015-02-23 18:49, Jan Kiszka wrote:
>>> On 2015-02-23 18:38, Jan Kiszka wrote:
>>>> On 2015-02-23 18:14, Gilles Chanteperdrix wrote:
>>>>> entry.S does not handle virtual irq state. So, if it returns to
>>>>> user-space with the root stage stalled, nothing will unstall it. And
>>>>> you probably get a lockup.
>>>>
>>>> Right, that's a point. I'm seeing this pattern in x86 now as well.
>>>> Comparing further.
>>>>
>>>> What's missing on ARM are flags adjustments in the register set before
>>>> letting Linux inspect that (__fixup_if equivalent). And we should
>>>> probably use ipipe_restore_root_nosync on return.
>>>
>>> There is another difference that should be considered carefullly. x86 says
>>>
>>>  * If we fault over the root domain, we need to replicate the
>>>  * hw interrupt state into the virtual mask before calling the
>>>  * I-pipe event handler.
>>>
>>> and then updates the root state before it calls __ipipe_notify_trap -
>>> why? x86-specific?
>>
>> The why is referring to the "before calling the I-pipe event handler" -
>> that we have to do this afterwards when we ended up or already were in
>> root is clear.
> 
> It seems this is now - at beast - historic logic of x86. We (no longer?)
> report anything when __ipipe_notify_trap is invoked over the root domain:
> 
>         /*
>          * We send a notification about all traps raised over a
>          * registered head domain only.
>          */
>         if (__ipipe_root_p)
>                 goto out;
> 
> Will clean up on x86 soon.

I've just pushed such a patch:
http://git.xenomai.org/ipipe-jki.git/commit/?h=for-upstream/master&id=fd52facbd86e09cc7fe17e7cb989ec4d46e2a798

Tests passed here, including sigdebug which triggers a fault over head.
Linux lock debugging was fully enabled and stayed happy as well.

I think we can convert ARM to this simplified x86 model now. I'll give
it a try.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] ipipe: issues with ARM exception handling

[Gilles Chanteperdrix]

On Tue, Feb 24, 2015 at 05:23:05PM +0100, Jan Kiszka wrote:
> On 2015-02-23 19:30, Jan Kiszka wrote:
> > On 2015-02-23 18:52, Jan Kiszka wrote:
> >> On 2015-02-23 18:49, Jan Kiszka wrote:
> >>> On 2015-02-23 18:38, Jan Kiszka wrote:
> >>>> On 2015-02-23 18:14, Gilles Chanteperdrix wrote:
> >>>>> entry.S does not handle virtual irq state. So, if it returns to
> >>>>> user-space with the root stage stalled, nothing will unstall it. And
> >>>>> you probably get a lockup.
> >>>>
> >>>> Right, that's a point. I'm seeing this pattern in x86 now as well.
> >>>> Comparing further.
> >>>>
> >>>> What's missing on ARM are flags adjustments in the register set before
> >>>> letting Linux inspect that (__fixup_if equivalent). And we should
> >>>> probably use ipipe_restore_root_nosync on return.
> >>>
> >>> There is another difference that should be considered carefullly. x86 says
> >>>
> >>>  * If we fault over the root domain, we need to replicate the
> >>>  * hw interrupt state into the virtual mask before calling the
> >>>  * I-pipe event handler.
> >>>
> >>> and then updates the root state before it calls __ipipe_notify_trap -
> >>> why? x86-specific?
> >>
> >> The why is referring to the "before calling the I-pipe event handler" -
> >> that we have to do this afterwards when we ended up or already were in
> >> root is clear.
> > 
> > It seems this is now - at beast - historic logic of x86. We (no longer?)
> > report anything when __ipipe_notify_trap is invoked over the root domain:
> > 
> >         /*
> >          * We send a notification about all traps raised over a
> >          * registered head domain only.
> >          */
> >         if (__ipipe_root_p)
> >                 goto out;
> > 
> > Will clean up on x86 soon.
> 
> I've just pushed such a patch:
> http://git.xenomai.org/ipipe-jki.git/commit/?h=for-upstream/master&id=fd52facbd86e09cc7fe17e7cb989ec4d46e2a798
> 
> Tests passed here, including sigdebug which triggers a fault over head.
> Linux lock debugging was fully enabled and stayed happy as well.
> 
> I think we can convert ARM to this simplified x86 model now. I'll give
> it a try.

Ok, before putting patches in your repository, please post the
proposed implementation of ipipe_fault_entry and ipipe_fault_exit,
so that we can discuss it before it gets merged by accident.

-- 
					    Gilles.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Jan Kiszka]

On 2015-02-24 17:45, Gilles Chanteperdrix wrote:
> On Tue, Feb 24, 2015 at 05:23:05PM +0100, Jan Kiszka wrote:
>> On 2015-02-23 19:30, Jan Kiszka wrote:
>>> On 2015-02-23 18:52, Jan Kiszka wrote:
>>>> On 2015-02-23 18:49, Jan Kiszka wrote:
>>>>> On 2015-02-23 18:38, Jan Kiszka wrote:
>>>>>> On 2015-02-23 18:14, Gilles Chanteperdrix wrote:
>>>>>>> entry.S does not handle virtual irq state. So, if it returns to
>>>>>>> user-space with the root stage stalled, nothing will unstall it. And
>>>>>>> you probably get a lockup.
>>>>>>
>>>>>> Right, that's a point. I'm seeing this pattern in x86 now as well.
>>>>>> Comparing further.
>>>>>>
>>>>>> What's missing on ARM are flags adjustments in the register set before
>>>>>> letting Linux inspect that (__fixup_if equivalent). And we should
>>>>>> probably use ipipe_restore_root_nosync on return.
>>>>>
>>>>> There is another difference that should be considered carefullly. x86 says
>>>>>
>>>>>  * If we fault over the root domain, we need to replicate the
>>>>>  * hw interrupt state into the virtual mask before calling the
>>>>>  * I-pipe event handler.
>>>>>
>>>>> and then updates the root state before it calls __ipipe_notify_trap -
>>>>> why? x86-specific?
>>>>
>>>> The why is referring to the "before calling the I-pipe event handler" -
>>>> that we have to do this afterwards when we ended up or already were in
>>>> root is clear.
>>>
>>> It seems this is now - at beast - historic logic of x86. We (no longer?)
>>> report anything when __ipipe_notify_trap is invoked over the root domain:
>>>
>>>         /*
>>>          * We send a notification about all traps raised over a
>>>          * registered head domain only.
>>>          */
>>>         if (__ipipe_root_p)
>>>                 goto out;
>>>
>>> Will clean up on x86 soon.
>>
>> I've just pushed such a patch:
>> http://git.xenomai.org/ipipe-jki.git/commit/?h=for-upstream/master&id=fd52facbd86e09cc7fe17e7cb989ec4d46e2a798
>>
>> Tests passed here, including sigdebug which triggers a fault over head.
>> Linux lock debugging was fully enabled and stayed happy as well.
>>
>> I think we can convert ARM to this simplified x86 model now. I'll give
>> it a try.
> 
> Ok, before putting patches in your repository, please post the
> proposed implementation of ipipe_fault_entry and ipipe_fault_exit,
> so that we can discuss it before it gets merged by accident.

Yes, sure. I will even test them.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] ipipe: issues with ARM exception handling

[Gilles Chanteperdrix]

On Tue, Feb 24, 2015 at 05:46:46PM +0100, Jan Kiszka wrote:
> On 2015-02-24 17:45, Gilles Chanteperdrix wrote:
> > On Tue, Feb 24, 2015 at 05:23:05PM +0100, Jan Kiszka wrote:
> >> On 2015-02-23 19:30, Jan Kiszka wrote:
> >>> On 2015-02-23 18:52, Jan Kiszka wrote:
> >>>> On 2015-02-23 18:49, Jan Kiszka wrote:
> >>>>> On 2015-02-23 18:38, Jan Kiszka wrote:
> >>>>>> On 2015-02-23 18:14, Gilles Chanteperdrix wrote:
> >>>>>>> entry.S does not handle virtual irq state. So, if it returns to
> >>>>>>> user-space with the root stage stalled, nothing will unstall it. And
> >>>>>>> you probably get a lockup.
> >>>>>>
> >>>>>> Right, that's a point. I'm seeing this pattern in x86 now as well.
> >>>>>> Comparing further.
> >>>>>>
> >>>>>> What's missing on ARM are flags adjustments in the register set before
> >>>>>> letting Linux inspect that (__fixup_if equivalent). And we should
> >>>>>> probably use ipipe_restore_root_nosync on return.
> >>>>>
> >>>>> There is another difference that should be considered carefullly. x86 says
> >>>>>
> >>>>>  * If we fault over the root domain, we need to replicate the
> >>>>>  * hw interrupt state into the virtual mask before calling the
> >>>>>  * I-pipe event handler.
> >>>>>
> >>>>> and then updates the root state before it calls __ipipe_notify_trap -
> >>>>> why? x86-specific?
> >>>>
> >>>> The why is referring to the "before calling the I-pipe event handler" -
> >>>> that we have to do this afterwards when we ended up or already were in
> >>>> root is clear.
> >>>
> >>> It seems this is now - at beast - historic logic of x86. We (no longer?)
> >>> report anything when __ipipe_notify_trap is invoked over the root domain:
> >>>
> >>>         /*
> >>>          * We send a notification about all traps raised over a
> >>>          * registered head domain only.
> >>>          */
> >>>         if (__ipipe_root_p)
> >>>                 goto out;
> >>>
> >>> Will clean up on x86 soon.
> >>
> >> I've just pushed such a patch:
> >> http://git.xenomai.org/ipipe-jki.git/commit/?h=for-upstream/master&id=fd52facbd86e09cc7fe17e7cb989ec4d46e2a798
> >>
> >> Tests passed here, including sigdebug which triggers a fault over head.
> >> Linux lock debugging was fully enabled and stayed happy as well.
> >>
> >> I think we can convert ARM to this simplified x86 model now. I'll give
> >> it a try.
> > 
> > Ok, before putting patches in your repository, please post the
> > proposed implementation of ipipe_fault_entry and ipipe_fault_exit,
> > so that we can discuss it before it gets merged by accident.
> 
> Yes, sure. I will even test them.

There is no point in testing it if we do not agree.

Besides, all the paths of the exception code are rarely exercised,
so it is better to make the exercise of enumerate the possibilities
(entry over head, entry over root, root stalled, root not stalled,
hw irqs off in parent context, hw irqs on) and verifying that the
code does the right thing for earh one than to believe than testing
is sufficient.

-- 
					    Gilles.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Gilles Chanteperdrix]

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:
hard_local_irq_disable() should always be called in case entry.S
expects us to return as we entered: with hw irqs off
And the root state stall bit should only be restored if the fault
handler was entered over root domain (otherwise, by relaxing, we
synchronized with the root domain, and it does not make sense to
stall it upon returning from exception, even if it was stalled at
the exception begin).

-- 
					    Gilles.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Philippe Gerum]

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.

> And the root state stall bit should only be restored if the fault
> handler was entered over root domain (otherwise, by relaxing, we
> synchronized with the root domain, and it does not make sense to
> stall it upon returning from exception, even if it was stalled at
> the exception begin).
> 

You may fault from a kernel context, in which case you do want the
virtual state to be preserved.

-- 
Philippe.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Gilles Chanteperdrix]

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.

Then there is the case where Linux re-enables irqs in the course of
the handler, in that case, we should not return to entry.S with hw
irqs off.

-- 
					    Gilles.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Gilles Chanteperdrix]

On Mon, Feb 23, 2015 at 06:12:50PM +0100, 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.
> 
> Then there is the case where Linux re-enables irqs in the course of
> the handler, in that case, we should not return to entry.S with hw
> irqs off.

To summarize:

when the fault is entered over head domain. The root stage can be
left unstalled on fault exit. The hw irqs state should reflect the
root stage stall bit state at exit.

when the fault is entered over root domain. The root stage stall bit
must be restorer as it was on entry, and the hw irqs state should
reflect the root stage stall bit state at exit.

-- 
					    Gilles.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Jan Kiszka]

On 2015-02-23 18:21, Gilles Chanteperdrix wrote:
> On Mon, Feb 23, 2015 at 06:12:50PM +0100, 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.
>>
>> Then there is the case where Linux re-enables irqs in the course of
>> the handler, in that case, we should not return to entry.S with hw
>> irqs off.
> 
> To summarize:
> 
> when the fault is entered over head domain. The root stage can be
> left unstalled on fault exit. The hw irqs state should reflect the
> root stage stall bit state at exit.

On x86, we pick up the desired state of hw and root after migration to
root completed.

> 
> when the fault is entered over root domain. The root stage stall bit
> must be restorer as it was on entry, and the hw irqs state should
> reflect the root stage stall bit state at exit.

Right. Actually, it should be restored to the state in the register set
on return from the Linux handler. But as Linux does not seem to
manipulate that state, we are fine with restoring to the entry state.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] ipipe: issues with ARM exception handling

[Gilles Chanteperdrix]

On Mon, Feb 23, 2015 at 06:43:23PM +0100, Jan Kiszka wrote:
> On 2015-02-23 18:21, Gilles Chanteperdrix wrote:
> > On Mon, Feb 23, 2015 at 06:12:50PM +0100, 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.
> >>
> >> Then there is the case where Linux re-enables irqs in the course of
> >> the handler, in that case, we should not return to entry.S with hw
> >> irqs off.
> > 
> > To summarize:
> > 
> > when the fault is entered over head domain. The root stage can be
> > left unstalled on fault exit. The hw irqs state should reflect the
> > root stage stall bit state at exit.
> 
> On x86, we pick up the desired state of hw and root after migration to
> root completed.
> 
> > 
> > when the fault is entered over root domain. The root stage stall bit
> > must be restorer as it was on entry, and the hw irqs state should
> > reflect the root stage stall bit state at exit.
> 
> Right. Actually, it should be restored to the state in the register set
> on return from the Linux handler.

No, the fault return path will take care of that by restoring the
saved cpsr. What is important is that if an irq return path assumes
that hw irqs are disabled to start pulling registers and playing
with sp, we must have disabled the hw irqs before returning to it,
to avoid register corruption if an irq happens at the bad time. On
the other hand, if Linux called local_irq_enable(), then entry.S
will not assume that hw irqs are off, and we can avoid the
hard_local_irq_disable() before returning from the fault, so as to
avoid creating a useless masking section (which is probably the
reason why Linux calls local_irq_enable()).

> But as Linux does not seem to
> manipulate that state, we are fine with restoring to the entry state.

Well, Linux seems to call local_irq_enable() in one of the fault
handlers. 

-- 
					    Gilles.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Jan Kiszka]

On 2015-02-23 18:51, Gilles Chanteperdrix wrote:
> On Mon, Feb 23, 2015 at 06:43:23PM +0100, Jan Kiszka wrote:
>> On 2015-02-23 18:21, Gilles Chanteperdrix wrote:
>>> On Mon, Feb 23, 2015 at 06:12:50PM +0100, 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.
>>>>
>>>> Then there is the case where Linux re-enables irqs in the course of
>>>> the handler, in that case, we should not return to entry.S with hw
>>>> irqs off.
>>>
>>> To summarize:
>>>
>>> when the fault is entered over head domain. The root stage can be
>>> left unstalled on fault exit. The hw irqs state should reflect the
>>> root stage stall bit state at exit.
>>
>> On x86, we pick up the desired state of hw and root after migration to
>> root completed.
>>
>>>
>>> when the fault is entered over root domain. The root stage stall bit
>>> must be restorer as it was on entry, and the hw irqs state should
>>> reflect the root stage stall bit state at exit.
>>
>> Right. Actually, it should be restored to the state in the register set
>> on return from the Linux handler.
> 
> No, the fault return path will take care of that by restoring the
> saved cpsr. What is important is that if an irq return path assumes
> that hw irqs are disabled to start pulling registers and playing
> with sp, we must have disabled the hw irqs before returning to it,
> to avoid register corruption if an irq happens at the bad time. On
> the other hand, if Linux called local_irq_enable(), then entry.S
> will not assume that hw irqs are off, and we can avoid the
> hard_local_irq_disable() before returning from the fault, so as to
> avoid creating a useless masking section (which is probably the
> reason why Linux calls local_irq_enable()).
> 
>> But as Linux does not seem to
>> manipulate that state, we are fine with restoring to the entry state.
> 
> Well, Linux seems to call local_irq_enable() in one of the fault
> handlers. 

I was referring to manipulating regs->ARM_cpsr, i.e. the state of the
context were we will return to. We don't consider this on x86 as well.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] ipipe: issues with ARM exception handling

[Gilles Chanteperdrix]

On Mon, Feb 23, 2015 at 06:54:36PM +0100, Jan Kiszka wrote:
> On 2015-02-23 18:51, Gilles Chanteperdrix wrote:
> > On Mon, Feb 23, 2015 at 06:43:23PM +0100, Jan Kiszka wrote:
> >> On 2015-02-23 18:21, Gilles Chanteperdrix wrote:
> >>> On Mon, Feb 23, 2015 at 06:12:50PM +0100, 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.
> >>>>
> >>>> Then there is the case where Linux re-enables irqs in the course of
> >>>> the handler, in that case, we should not return to entry.S with hw
> >>>> irqs off.
> >>>
> >>> To summarize:
> >>>
> >>> when the fault is entered over head domain. The root stage can be
> >>> left unstalled on fault exit. The hw irqs state should reflect the
> >>> root stage stall bit state at exit.
> >>
> >> On x86, we pick up the desired state of hw and root after migration to
> >> root completed.
> >>
> >>>
> >>> when the fault is entered over root domain. The root stage stall bit
> >>> must be restorer as it was on entry, and the hw irqs state should
> >>> reflect the root stage stall bit state at exit.
> >>
> >> Right. Actually, it should be restored to the state in the register set
> >> on return from the Linux handler.
> > 
> > No, the fault return path will take care of that by restoring the
> > saved cpsr. What is important is that if an irq return path assumes
> > that hw irqs are disabled to start pulling registers and playing
> > with sp, we must have disabled the hw irqs before returning to it,
> > to avoid register corruption if an irq happens at the bad time. On
> > the other hand, if Linux called local_irq_enable(), then entry.S
> > will not assume that hw irqs are off, and we can avoid the
> > hard_local_irq_disable() before returning from the fault, so as to
> > avoid creating a useless masking section (which is probably the
> > reason why Linux calls local_irq_enable()).
> > 
> >> But as Linux does not seem to
> >> manipulate that state, we are fine with restoring to the entry state.
> > 
> > Well, Linux seems to call local_irq_enable() in one of the fault
> > handlers. 
> 
> I was referring to manipulating regs->ARM_cpsr, i.e. the state of the
> context were we will return to. We don't consider this on x86 as well.

So, finally, I propose:

static inline unsigned long ipipe_fault_entry(void)
{
	unsigned long flags;

	local_irq_save(flags);      /* Re-enables hw irqs */

	return flags;
}

static inline void ipipe_fault_exit(unsigned long x)
{
	if (irqs_disabled()) {
		__ipipe_restore_root_nosync(x);
		hard_local_irq_disable();
	}
}

-- 
					    Gilles.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Gilles Chanteperdrix]

On Mon, Feb 23, 2015 at 07:04:25PM +0100, Gilles Chanteperdrix wrote:
> On Mon, Feb 23, 2015 at 06:54:36PM +0100, Jan Kiszka wrote:
> > On 2015-02-23 18:51, Gilles Chanteperdrix wrote:
> > > On Mon, Feb 23, 2015 at 06:43:23PM +0100, Jan Kiszka wrote:
> > >> On 2015-02-23 18:21, Gilles Chanteperdrix wrote:
> > >>> On Mon, Feb 23, 2015 at 06:12:50PM +0100, 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.
> > >>>>
> > >>>> Then there is the case where Linux re-enables irqs in the course of
> > >>>> the handler, in that case, we should not return to entry.S with hw
> > >>>> irqs off.
> > >>>
> > >>> To summarize:
> > >>>
> > >>> when the fault is entered over head domain. The root stage can be
> > >>> left unstalled on fault exit. The hw irqs state should reflect the
> > >>> root stage stall bit state at exit.
> > >>
> > >> On x86, we pick up the desired state of hw and root after migration to
> > >> root completed.
> > >>
> > >>>
> > >>> when the fault is entered over root domain. The root stage stall bit
> > >>> must be restorer as it was on entry, and the hw irqs state should
> > >>> reflect the root stage stall bit state at exit.
> > >>
> > >> Right. Actually, it should be restored to the state in the register set
> > >> on return from the Linux handler.
> > > 
> > > No, the fault return path will take care of that by restoring the
> > > saved cpsr. What is important is that if an irq return path assumes
> > > that hw irqs are disabled to start pulling registers and playing
> > > with sp, we must have disabled the hw irqs before returning to it,
> > > to avoid register corruption if an irq happens at the bad time. On
> > > the other hand, if Linux called local_irq_enable(), then entry.S
> > > will not assume that hw irqs are off, and we can avoid the
> > > hard_local_irq_disable() before returning from the fault, so as to
> > > avoid creating a useless masking section (which is probably the
> > > reason why Linux calls local_irq_enable()).
> > > 
> > >> But as Linux does not seem to
> > >> manipulate that state, we are fine with restoring to the entry state.
> > > 
> > > Well, Linux seems to call local_irq_enable() in one of the fault
> > > handlers. 
> > 
> > I was referring to manipulating regs->ARM_cpsr, i.e. the state of the
> > context were we will return to. We don't consider this on x86 as well.
> 
> So, finally, I propose:

Mmmm. try again.

> 
> static inline unsigned long ipipe_fault_entry(void)
> {
> 	unsigned long flags;
> 
> 	local_irq_save(flags);      /* Re-enables hw irqs */

We need the explicit hard_local_irq_enable() here.

> 
> 	return flags;
> }
> 
> static inline void ipipe_fault_exit(unsigned long x)
> {
> 	if (irqs_disabled()) {
> 		__ipipe_restore_root_nosync(x);
> 		hard_local_irq_disable();
> 	}
> }

We need hard_local_irq_disable() before __ipipe_restore_root_nosync,
to avoid the preemption of a Linux interrupt where this should not
be possible.

-- 
					    Gilles.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Jan Kiszka]

On 2015-02-23 19:11, Gilles Chanteperdrix wrote:
> On Mon, Feb 23, 2015 at 07:04:25PM +0100, Gilles Chanteperdrix wrote:
>> So, finally, I propose:
> 
> Mmmm. try again.
> 
>>
>> static inline unsigned long ipipe_fault_entry(void)
>> {
>> 	unsigned long flags;
>>
>> 	local_irq_save(flags);      /* Re-enables hw irqs */
> 
> We need the explicit hard_local_irq_enable() here.
> 
>>
>> 	return flags;
>> }
>>
>> static inline void ipipe_fault_exit(unsigned long x)
>> {
>> 	if (irqs_disabled()) {
>> 		__ipipe_restore_root_nosync(x);
>> 		hard_local_irq_disable();
>> 	}
>> }
> 
> We need hard_local_irq_disable() before __ipipe_restore_root_nosync,
> to avoid the preemption of a Linux interrupt where this should not
> be possible.

ipipe_restore_root_nosync?

But we first need to understand the differences to x86 to be sure that
we don't miss something. The remaining differences either have to have
an architecture-specific reason - or one side is still wrong (or
needlessly complex).

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] ipipe: issues with ARM exception handling

[Jan Kiszka]

On 2015-02-23 19:16, Jan Kiszka wrote:
> On 2015-02-23 19:11, Gilles Chanteperdrix wrote:
>> On Mon, Feb 23, 2015 at 07:04:25PM +0100, Gilles Chanteperdrix wrote:
>>> So, finally, I propose:
>>
>> Mmmm. try again.
>>
>>>
>>> static inline unsigned long ipipe_fault_entry(void)
>>> {
>>> 	unsigned long flags;
>>>
>>> 	local_irq_save(flags);      /* Re-enables hw irqs */
>>
>> We need the explicit hard_local_irq_enable() here.

And we need to fix up regs->ARM_cpsr according to flags.

>>
>>>
>>> 	return flags;
>>> }
>>>
>>> static inline void ipipe_fault_exit(unsigned long x)
>>> {
>>> 	if (irqs_disabled()) {
>>> 		__ipipe_restore_root_nosync(x);
>>> 		hard_local_irq_disable();
>>> 	}
>>> }
>>
>> We need hard_local_irq_disable() before __ipipe_restore_root_nosync,
>> to avoid the preemption of a Linux interrupt where this should not
>> be possible.
> 
> ipipe_restore_root_nosync?
> 
> But we first need to understand the differences to x86 to be sure that
> we don't miss something. The remaining differences either have to have
> an architecture-specific reason - or one side is still wrong (or
> needlessly complex).

Seems like we are converging now with the last mystery (special x86
handling on root entry) solved.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] ipipe: issues with ARM exception handling

[Gilles Chanteperdrix]

On Mon, Feb 23, 2015 at 07:32:21PM +0100, Jan Kiszka wrote:
> On 2015-02-23 19:16, Jan Kiszka wrote:
> > On 2015-02-23 19:11, Gilles Chanteperdrix wrote:
> >> On Mon, Feb 23, 2015 at 07:04:25PM +0100, Gilles Chanteperdrix wrote:
> >>> So, finally, I propose:
> >>
> >> Mmmm. try again.
> >>
> >>>
> >>> static inline unsigned long ipipe_fault_entry(void)
> >>> {
> >>> 	unsigned long flags;
> >>>
> >>> 	local_irq_save(flags);      /* Re-enables hw irqs */
> >>
> >> We need the explicit hard_local_irq_enable() here.
> 
> And we need to fix up regs->ARM_cpsr according to flags.

No, we have no reason to fix up regs->ARM_cpsr.

It has been saved when entering the exception, and contains the
state of the calling context. It needs to be restored exactly as it
is.

-- 
					    Gilles.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Jan Kiszka]

On 2015-02-23 19:34, Gilles Chanteperdrix wrote:
> On Mon, Feb 23, 2015 at 07:32:21PM +0100, Jan Kiszka wrote:
>> On 2015-02-23 19:16, Jan Kiszka wrote:
>>> On 2015-02-23 19:11, Gilles Chanteperdrix wrote:
>>>> On Mon, Feb 23, 2015 at 07:04:25PM +0100, Gilles Chanteperdrix wrote:
>>>>> So, finally, I propose:
>>>>
>>>> Mmmm. try again.
>>>>
>>>>>
>>>>> static inline unsigned long ipipe_fault_entry(void)
>>>>> {
>>>>> 	unsigned long flags;
>>>>>
>>>>> 	local_irq_save(flags);      /* Re-enables hw irqs */
>>>>
>>>> We need the explicit hard_local_irq_enable() here.
>>
>> And we need to fix up regs->ARM_cpsr according to flags.
> 
> No, we have no reason to fix up regs->ARM_cpsr.

do_page_fault:
	if (interrupts_enabled(regs))
		local_irq_enable();

It's very valuable to study x86 on this. It may overdo some things (we
will clean it up), but it is also very similar to ARM architecturally,
so we can draw conclusions from that.

> 
> It has been saved when entering the exception, and contains the
> state of the calling context. It needs to be restored exactly as it
> is.

See x86 for the reasons.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] ipipe: issues with ARM exception handling

[Gilles Chanteperdrix]

On Mon, Feb 23, 2015 at 08:14:22PM +0100, Jan Kiszka wrote:
> On 2015-02-23 19:34, Gilles Chanteperdrix wrote:
> > On Mon, Feb 23, 2015 at 07:32:21PM +0100, Jan Kiszka wrote:
> >> On 2015-02-23 19:16, Jan Kiszka wrote:
> >>> On 2015-02-23 19:11, Gilles Chanteperdrix wrote:
> >>>> On Mon, Feb 23, 2015 at 07:04:25PM +0100, Gilles Chanteperdrix wrote:
> >>>>> So, finally, I propose:
> >>>>
> >>>> Mmmm. try again.
> >>>>
> >>>>>
> >>>>> static inline unsigned long ipipe_fault_entry(void)
> >>>>> {
> >>>>> 	unsigned long flags;
> >>>>>
> >>>>> 	local_irq_save(flags);      /* Re-enables hw irqs */
> >>>>
> >>>> We need the explicit hard_local_irq_enable() here.
> >>
> >> And we need to fix up regs->ARM_cpsr according to flags.
> > 
> > No, we have no reason to fix up regs->ARM_cpsr.
> 
> do_page_fault:
> 	if (interrupts_enabled(regs))
> 		local_irq_enable();
> 
> It's very valuable to study x86 on this. It may overdo some things (we
> will clean it up), but it is also very similar to ARM architecturally,
> so we can draw conclusions from that.

Ok, but it means we have to undo on exit what we did on entry,
because it is wrong to return with root stalled on a context which had
hardware irqs off.

(also note that this is the only piece of code using the PSR I bit,
mangling the bits on each exception entry/exit for only that case,
is completely overkill, better replace interrupt_enabled(regs) with
a test for the stall bit saved by ipipe_fault_entry)

-- 
					    Gilles.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Gilles Chanteperdrix]

On Mon, Feb 23, 2015 at 07:16:08PM +0100, Jan Kiszka wrote:
> On 2015-02-23 19:11, Gilles Chanteperdrix wrote:
> > On Mon, Feb 23, 2015 at 07:04:25PM +0100, Gilles Chanteperdrix wrote:
> >> So, finally, I propose:
> > 
> > Mmmm. try again.
> > 
> >>
> >> static inline unsigned long ipipe_fault_entry(void)
> >> {
> >> 	unsigned long flags;
> >>
> >> 	local_irq_save(flags);      /* Re-enables hw irqs */
> > 
> > We need the explicit hard_local_irq_enable() here.
> > 
> >>
> >> 	return flags;
> >> }
> >>
> >> static inline void ipipe_fault_exit(unsigned long x)
> >> {
> >> 	if (irqs_disabled()) {
> >> 		__ipipe_restore_root_nosync(x);
> >> 		hard_local_irq_disable();
> >> 	}
> >> }
> > 
> > We need hard_local_irq_disable() before __ipipe_restore_root_nosync,
> > to avoid the preemption of a Linux interrupt where this should not
> > be possible.
> 
> ipipe_restore_root_nosync?

Since we called hard_local_irq_disable(), we know irqs are off, and
can call the underscored version.

> 
> But we first need to understand the differences to x86 to be sure that
> we don't miss something. The remaining differences either have to have
> an architecture-specific reason - or one side is still wrong (or
> needlessly complex).

What I have written here is completely specific to the situation we
described for ARM, I doubt very much it can be compared with what
happens on x86.

-- 
					    Gilles.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Gilles Chanteperdrix]

On Mon, Feb 23, 2015 at 07:11:24PM +0100, Gilles Chanteperdrix wrote:
> On Mon, Feb 23, 2015 at 07:04:25PM +0100, Gilles Chanteperdrix wrote:
> > On Mon, Feb 23, 2015 at 06:54:36PM +0100, Jan Kiszka wrote:
> > > On 2015-02-23 18:51, Gilles Chanteperdrix wrote:
> > > > On Mon, Feb 23, 2015 at 06:43:23PM +0100, Jan Kiszka wrote:
> > > >> On 2015-02-23 18:21, Gilles Chanteperdrix wrote:
> > > >>> On Mon, Feb 23, 2015 at 06:12:50PM +0100, 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.
> > > >>>>
> > > >>>> Then there is the case where Linux re-enables irqs in the course of
> > > >>>> the handler, in that case, we should not return to entry.S with hw
> > > >>>> irqs off.
> > > >>>
> > > >>> To summarize:
> > > >>>
> > > >>> when the fault is entered over head domain. The root stage can be
> > > >>> left unstalled on fault exit. The hw irqs state should reflect the
> > > >>> root stage stall bit state at exit.
> > > >>
> > > >> On x86, we pick up the desired state of hw and root after migration to
> > > >> root completed.
> > > >>
> > > >>>
> > > >>> when the fault is entered over root domain. The root stage stall bit
> > > >>> must be restorer as it was on entry, and the hw irqs state should
> > > >>> reflect the root stage stall bit state at exit.
> > > >>
> > > >> Right. Actually, it should be restored to the state in the register set
> > > >> on return from the Linux handler.
> > > > 
> > > > No, the fault return path will take care of that by restoring the
> > > > saved cpsr. What is important is that if an irq return path assumes
> > > > that hw irqs are disabled to start pulling registers and playing
> > > > with sp, we must have disabled the hw irqs before returning to it,
> > > > to avoid register corruption if an irq happens at the bad time. On
> > > > the other hand, if Linux called local_irq_enable(), then entry.S
> > > > will not assume that hw irqs are off, and we can avoid the
> > > > hard_local_irq_disable() before returning from the fault, so as to
> > > > avoid creating a useless masking section (which is probably the
> > > > reason why Linux calls local_irq_enable()).
> > > > 
> > > >> But as Linux does not seem to
> > > >> manipulate that state, we are fine with restoring to the entry state.
> > > > 
> > > > Well, Linux seems to call local_irq_enable() in one of the fault
> > > > handlers. 
> > > 
> > > I was referring to manipulating regs->ARM_cpsr, i.e. the state of the
> > > context were we will return to. We don't consider this on x86 as well.
> > 
> > So, finally, I propose:
> 
> Mmmm. try again.
> 
> > 
> > static inline unsigned long ipipe_fault_entry(void)
> > {
> > 	unsigned long flags;
> > 
> > 	local_irq_save(flags);      /* Re-enables hw irqs */
> 
> We need the explicit hard_local_irq_enable() here.
> 
> > 
> > 	return flags;
> > }
> > 
> > static inline void ipipe_fault_exit(unsigned long x)
> > {
> > 	if (irqs_disabled()) {
> > 		__ipipe_restore_root_nosync(x);
> > 		hard_local_irq_disable();
> > 	}
> > }
> 
> We need hard_local_irq_disable() before __ipipe_restore_root_nosync,
> to avoid the preemption of a Linux interrupt where this should not
> be possible.

This is still not correct. If the calling context took a fault while
the root stage was stalled, there is no reason to unstall the
calling context as a result of the handler calling local_irq_enable.

Hence:

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 x)
{
	if (irqs_disabled())
		hard_local_irq_disable();
	ipipe_restore_root_nosync(x);
}

-- 
					    Gilles.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Philippe Gerum]

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?

-- 
Philippe.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Gilles Chanteperdrix]

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.

-- 
					    Gilles.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Philippe Gerum]

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?

-- 
Philippe.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Gilles Chanteperdrix]

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.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Philippe Gerum]

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.

-- 
Philippe.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Gilles Chanteperdrix]

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.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Philippe Gerum]

On 02/23/2015 06:12 PM, Gilles Chanteperdrix wrote:
> On Mon, Feb 23, 2015 at 06:01:54PM +0100, Philippe Gerum wrote:

>>> 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.
> 
> Then there is the case where Linux re-enables irqs in the course of
> the handler, in that case, we should not return to entry.S with hw
> irqs off.
> 

Since Linux re-enables hw irqs in the course of the handler with a
regular kernel, and never restores any masked state in this case before
returning to entry.S, don't you think that the fault entry code in
entry.S is aware of such possibility? If so, why would restoring a
masked hw state be absolutely required when the pipeline is enabled?

-- 
Philippe.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Gilles Chanteperdrix]

On Mon, Feb 23, 2015 at 09:43:22PM +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:
> 
> >>> 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.
> > 
> > Then there is the case where Linux re-enables irqs in the course of
> > the handler, in that case, we should not return to entry.S with hw
> > irqs off.
> > 
> 
> Since Linux re-enables hw irqs in the course of the handler

in the course of some handlers, as far as I understood, not all of
them. Which is why I propose to only redisable irqs before returning
to entry.S if the root domain is stalled on exception exit (because
this indicates that Linux wants to return to entry.S with irqs off).

-- 
					    Gilles.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Gilles Chanteperdrix]

On Mon, Feb 23, 2015 at 05:55:49PM +0100, 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:
> hard_local_irq_disable() should always be called in case entry.S
> expects us to return as we entered: with hw irqs off

Well, unless linux called local_irq_enable(). So, in fact the hw
irqs state should be modeled after the current state of the stall
bit, it should not depend on the flags upon entry.

-- 
					    Gilles.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Gilles Chanteperdrix]

On Fri, Feb 20, 2015 at 07:03:14PM +0100, 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.
> - 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.
> 
> Room for optimization:
> - ipipe_fault_entry is always called with hard IRQs off from
>   do_page_fault and do_translation_fault. I suspect this applies to the
>   remaining callers (do_DataAbort and do_PrefetchAbort ) as well. Thus
>   the hard IRQ state is actually known at compile time, right?
> 
> I can hack up patches, but I'd like to confirm first that I'm not
> missing anything subtle or ARM-specific here.

Just to explain the original hack.

Some time ago, the faults handlers were executed irqs on ARM. The
irqs were enabled in entry.S before executing the handlers.

At some point, this was removed in entry.S and fault handlers
started to be executed irqs off. On ARM, all faults relax to be
handled in secondary mode, actually there is an exception, the FPU,
but it goes through a completely different path which had always
been executed irqs off until recently where the irqs are reenabled
when accessing user-space to be able to handle faults without
lockups. 

My concern was that the code thus executed could have assertion
about the root domain being stalled which would be fail, so I added
code which stalled root and enabled hardware irqs on fault entry and
unstalled root and disabled hardware irqs on fault exit (which
always happen on root domain). This should have worked even if a fault
had happened to be handled in head domain, because then the
operation would have been a nop (simply stall/then unstall). 

But Philippe found this dumb approach to fail when working on LPAE,
IIRC. IIRC, namely, if the root domain happens to be stalled when
entering a fault over head domain, it would end up unstalled after
the operation. So, I believe the code he added saves the stall state
on fault entry and restores it on fault exit. I have checked
Philippe's code details at the time and did not find anything wrong.

-- 
					    Gilles.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Jan Kiszka]

On 2015-02-20 19:38, Gilles Chanteperdrix wrote:
> On Fri, Feb 20, 2015 at 07:03:14PM +0100, 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.
>> - 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.
>>
>> Room for optimization:
>> - ipipe_fault_entry is always called with hard IRQs off from
>>   do_page_fault and do_translation_fault. I suspect this applies to the
>>   remaining callers (do_DataAbort and do_PrefetchAbort ) as well. Thus
>>   the hard IRQ state is actually known at compile time, right?

To follow up on this: do_DataAbort and do_PrefetchAbort are always
invoked with hard IRQs disable when a regular exception takes us there.
Only the ghost syscall cmpxchg simulates do_DataAbort without adjusting
hardware interrupt. It's probably easier to adjust that than to account
for hw irqs being potentially on an fault entry.

>>
>> I can hack up patches, but I'd like to confirm first that I'm not
>> missing anything subtle or ARM-specific here.
> 
> Just to explain the original hack.
> 
> Some time ago, the faults handlers were executed irqs on ARM. The
> irqs were enabled in entry.S before executing the handlers.
> 
> At some point, this was removed in entry.S and fault handlers
> started to be executed irqs off. On ARM, all faults relax to be
> handled in secondary mode, actually there is an exception, the FPU,
> but it goes through a completely different path which had always
> been executed irqs off until recently where the irqs are reenabled
> when accessing user-space to be able to handle faults without
> lockups. 
> 
> My concern was that the code thus executed could have assertion
> about the root domain being stalled which would be fail, so I added
> code which stalled root and enabled hardware irqs on fault entry and
> unstalled root and disabled hardware irqs on fault exit (which
> always happen on root domain). This should have worked even if a fault
> had happened to be handled in head domain, because then the
> operation would have been a nop (simply stall/then unstall). 
> 
> But Philippe found this dumb approach to fail when working on LPAE,
> IIRC. IIRC, namely, if the root domain happens to be stalled when
> entering a fault over head domain, it would end up unstalled after
> the operation. So, I believe the code he added saves the stall state
> on fault entry and restores it on fault exit. I have checked
> Philippe's code details at the time and did not find anything wrong.

I suspect the LPAE scenario takes the do_page_fault path? Then it should
rather be solved by providing the right information to or preventing
the execution of

	/* Enable interrupts if they were enabled in the parent context. */
	if (interrupts_enabled(regs))
		local_irq_enable();

Now we unconditionally restore to the root state on entry, overwriting
what may happen to it during the handler execution - specifically via
the snippet above.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] ipipe: issues with ARM exception handling

[Gilles Chanteperdrix]

On Fri, Feb 20, 2015 at 07:51:19PM +0100, Jan Kiszka wrote:
> On 2015-02-20 19:38, Gilles Chanteperdrix wrote:
> > On Fri, Feb 20, 2015 at 07:03:14PM +0100, 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.
> >> - 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.
> >>
> >> Room for optimization:
> >> - ipipe_fault_entry is always called with hard IRQs off from
> >>   do_page_fault and do_translation_fault. I suspect this applies to the
> >>   remaining callers (do_DataAbort and do_PrefetchAbort ) as well. Thus
> >>   the hard IRQ state is actually known at compile time, right?
> 
> To follow up on this: do_DataAbort and do_PrefetchAbort are always
> invoked with hard IRQs disable when a regular exception takes us there.
> Only the ghost syscall cmpxchg simulates do_DataAbort without adjusting
> hardware interrupt. It's probably easier to adjust that than to account
> for hw irqs being potentially on an fault entry.
> 
> >>
> >> I can hack up patches, but I'd like to confirm first that I'm not
> >> missing anything subtle or ARM-specific here.
> > 
> > Just to explain the original hack.
> > 
> > Some time ago, the faults handlers were executed irqs on ARM. The
> > irqs were enabled in entry.S before executing the handlers.
> > 
> > At some point, this was removed in entry.S and fault handlers
> > started to be executed irqs off. On ARM, all faults relax to be
> > handled in secondary mode, actually there is an exception, the FPU,
> > but it goes through a completely different path which had always
> > been executed irqs off until recently where the irqs are reenabled
> > when accessing user-space to be able to handle faults without
> > lockups. 
> > 
> > My concern was that the code thus executed could have assertion
> > about the root domain being stalled which would be fail, so I added
> > code which stalled root and enabled hardware irqs on fault entry and
> > unstalled root and disabled hardware irqs on fault exit (which
> > always happen on root domain). This should have worked even if a fault
> > had happened to be handled in head domain, because then the
> > operation would have been a nop (simply stall/then unstall). 
> > 
> > But Philippe found this dumb approach to fail when working on LPAE,
> > IIRC. IIRC, namely, if the root domain happens to be stalled when
> > entering a fault over head domain, it would end up unstalled after
> > the operation. So, I believe the code he added saves the stall state
> > on fault entry and restores it on fault exit. I have checked
> > Philippe's code details at the time and did not find anything wrong.
> 
> I suspect the LPAE scenario takes the do_page_fault path? Then it should
> rather be solved by providing the right information to or preventing
> the execution of
> 
> 	/* Enable interrupts if they were enabled in the parent context. */
> 	if (interrupts_enabled(regs))
> 		local_irq_enable();
> 
> Now we unconditionally restore to the root state on entry, overwriting
> what may happen to it during the handler execution - specifically via
> the snippet above.

This code is part of the mainline kernel.

-- 
					    Gilles.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Jan Kiszka]

On 2015-02-20 19:53, Gilles Chanteperdrix wrote:
> On Fri, Feb 20, 2015 at 07:51:19PM +0100, Jan Kiszka wrote:
>> On 2015-02-20 19:38, Gilles Chanteperdrix wrote:
>>> On Fri, Feb 20, 2015 at 07:03:14PM +0100, 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.
>>>> - 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.
>>>>
>>>> Room for optimization:
>>>> - ipipe_fault_entry is always called with hard IRQs off from
>>>>   do_page_fault and do_translation_fault. I suspect this applies to the
>>>>   remaining callers (do_DataAbort and do_PrefetchAbort ) as well. Thus
>>>>   the hard IRQ state is actually known at compile time, right?
>>
>> To follow up on this: do_DataAbort and do_PrefetchAbort are always
>> invoked with hard IRQs disable when a regular exception takes us there.
>> Only the ghost syscall cmpxchg simulates do_DataAbort without adjusting
>> hardware interrupt. It's probably easier to adjust that than to account
>> for hw irqs being potentially on an fault entry.
>>
>>>>
>>>> I can hack up patches, but I'd like to confirm first that I'm not
>>>> missing anything subtle or ARM-specific here.
>>>
>>> Just to explain the original hack.
>>>
>>> Some time ago, the faults handlers were executed irqs on ARM. The
>>> irqs were enabled in entry.S before executing the handlers.
>>>
>>> At some point, this was removed in entry.S and fault handlers
>>> started to be executed irqs off. On ARM, all faults relax to be
>>> handled in secondary mode, actually there is an exception, the FPU,
>>> but it goes through a completely different path which had always
>>> been executed irqs off until recently where the irqs are reenabled
>>> when accessing user-space to be able to handle faults without
>>> lockups. 
>>>
>>> My concern was that the code thus executed could have assertion
>>> about the root domain being stalled which would be fail, so I added
>>> code which stalled root and enabled hardware irqs on fault entry and
>>> unstalled root and disabled hardware irqs on fault exit (which
>>> always happen on root domain). This should have worked even if a fault
>>> had happened to be handled in head domain, because then the
>>> operation would have been a nop (simply stall/then unstall). 
>>>
>>> But Philippe found this dumb approach to fail when working on LPAE,
>>> IIRC. IIRC, namely, if the root domain happens to be stalled when
>>> entering a fault over head domain, it would end up unstalled after
>>> the operation. So, I believe the code he added saves the stall state
>>> on fault entry and restores it on fault exit. I have checked
>>> Philippe's code details at the time and did not find anything wrong.
>>
>> I suspect the LPAE scenario takes the do_page_fault path? Then it should
>> rather be solved by providing the right information to or preventing
>> the execution of
>>
>> 	/* Enable interrupts if they were enabled in the parent context. */
>> 	if (interrupts_enabled(regs))
>> 		local_irq_enable();
>>
>> Now we unconditionally restore to the root state on entry, overwriting
>> what may happen to it during the handler execution - specifically via
>> the snippet above.
> 
> This code is part of the mainline kernel.

Correct. But we can adjust it to take interrupt virtualization and
domain migration into account.


I just also followed hook_ifault_code to find out if there is something
unaddressed /wrt I-pipe. There is: hardware break/watchpoints. If
triggered over head, they will go to hw_breakpoint_pending directly
without prior domain migration. We probably want another
__ipipe_report_trap here.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] ipipe: issues with ARM exception handling

[Gilles Chanteperdrix]

On Fri, Feb 20, 2015 at 07:57:51PM +0100, Jan Kiszka wrote:
> On 2015-02-20 19:53, Gilles Chanteperdrix wrote:
> > On Fri, Feb 20, 2015 at 07:51:19PM +0100, Jan Kiszka wrote:
> >> On 2015-02-20 19:38, Gilles Chanteperdrix wrote:
> >>> On Fri, Feb 20, 2015 at 07:03:14PM +0100, 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.
> >>>> - 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.
> >>>>
> >>>> Room for optimization:
> >>>> - ipipe_fault_entry is always called with hard IRQs off from
> >>>>   do_page_fault and do_translation_fault. I suspect this applies to the
> >>>>   remaining callers (do_DataAbort and do_PrefetchAbort ) as well. Thus
> >>>>   the hard IRQ state is actually known at compile time, right?
> >>
> >> To follow up on this: do_DataAbort and do_PrefetchAbort are always
> >> invoked with hard IRQs disable when a regular exception takes us there.
> >> Only the ghost syscall cmpxchg simulates do_DataAbort without adjusting
> >> hardware interrupt. It's probably easier to adjust that than to account
> >> for hw irqs being potentially on an fault entry.
> >>
> >>>>
> >>>> I can hack up patches, but I'd like to confirm first that I'm not
> >>>> missing anything subtle or ARM-specific here.
> >>>
> >>> Just to explain the original hack.
> >>>
> >>> Some time ago, the faults handlers were executed irqs on ARM. The
> >>> irqs were enabled in entry.S before executing the handlers.
> >>>
> >>> At some point, this was removed in entry.S and fault handlers
> >>> started to be executed irqs off. On ARM, all faults relax to be
> >>> handled in secondary mode, actually there is an exception, the FPU,
> >>> but it goes through a completely different path which had always
> >>> been executed irqs off until recently where the irqs are reenabled
> >>> when accessing user-space to be able to handle faults without
> >>> lockups. 
> >>>
> >>> My concern was that the code thus executed could have assertion
> >>> about the root domain being stalled which would be fail, so I added
> >>> code which stalled root and enabled hardware irqs on fault entry and
> >>> unstalled root and disabled hardware irqs on fault exit (which
> >>> always happen on root domain). This should have worked even if a fault
> >>> had happened to be handled in head domain, because then the
> >>> operation would have been a nop (simply stall/then unstall). 
> >>>
> >>> But Philippe found this dumb approach to fail when working on LPAE,
> >>> IIRC. IIRC, namely, if the root domain happens to be stalled when
> >>> entering a fault over head domain, it would end up unstalled after
> >>> the operation. So, I believe the code he added saves the stall state
> >>> on fault entry and restores it on fault exit. I have checked
> >>> Philippe's code details at the time and did not find anything wrong.
> >>
> >> I suspect the LPAE scenario takes the do_page_fault path? Then it should
> >> rather be solved by providing the right information to or preventing
> >> the execution of
> >>
> >> 	/* Enable interrupts if they were enabled in the parent context. */
> >> 	if (interrupts_enabled(regs))
> >> 		local_irq_enable();
> >>
> >> Now we unconditionally restore to the root state on entry, overwriting
> >> what may happen to it during the handler execution - specifically via
> >> the snippet above.
> > 
> > This code is part of the mainline kernel.
> 
> Correct. But we can adjust it to take interrupt virtualization and
> domain migration into account.

I do not think we should. Everything should appear to the kernel as
if interrupts are NOT virtualized. So, local_irq_enable() should in
fact do an unstall root, and everything around should be made so
that it works.

-- 
					    Gilles.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Jan Kiszka]

On 2015-02-20 19:59, Gilles Chanteperdrix wrote:
> On Fri, Feb 20, 2015 at 07:57:51PM +0100, Jan Kiszka wrote:
>> On 2015-02-20 19:53, Gilles Chanteperdrix wrote:
>>> On Fri, Feb 20, 2015 at 07:51:19PM +0100, Jan Kiszka wrote:
>>>> On 2015-02-20 19:38, Gilles Chanteperdrix wrote:
>>>>> On Fri, Feb 20, 2015 at 07:03:14PM +0100, 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.
>>>>>> - 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.
>>>>>>
>>>>>> Room for optimization:
>>>>>> - ipipe_fault_entry is always called with hard IRQs off from
>>>>>>   do_page_fault and do_translation_fault. I suspect this applies to the
>>>>>>   remaining callers (do_DataAbort and do_PrefetchAbort ) as well. Thus
>>>>>>   the hard IRQ state is actually known at compile time, right?
>>>>
>>>> To follow up on this: do_DataAbort and do_PrefetchAbort are always
>>>> invoked with hard IRQs disable when a regular exception takes us there.
>>>> Only the ghost syscall cmpxchg simulates do_DataAbort without adjusting
>>>> hardware interrupt. It's probably easier to adjust that than to account
>>>> for hw irqs being potentially on an fault entry.
>>>>
>>>>>>
>>>>>> I can hack up patches, but I'd like to confirm first that I'm not
>>>>>> missing anything subtle or ARM-specific here.
>>>>>
>>>>> Just to explain the original hack.
>>>>>
>>>>> Some time ago, the faults handlers were executed irqs on ARM. The
>>>>> irqs were enabled in entry.S before executing the handlers.
>>>>>
>>>>> At some point, this was removed in entry.S and fault handlers
>>>>> started to be executed irqs off. On ARM, all faults relax to be
>>>>> handled in secondary mode, actually there is an exception, the FPU,
>>>>> but it goes through a completely different path which had always
>>>>> been executed irqs off until recently where the irqs are reenabled
>>>>> when accessing user-space to be able to handle faults without
>>>>> lockups. 
>>>>>
>>>>> My concern was that the code thus executed could have assertion
>>>>> about the root domain being stalled which would be fail, so I added
>>>>> code which stalled root and enabled hardware irqs on fault entry and
>>>>> unstalled root and disabled hardware irqs on fault exit (which
>>>>> always happen on root domain). This should have worked even if a fault
>>>>> had happened to be handled in head domain, because then the
>>>>> operation would have been a nop (simply stall/then unstall). 
>>>>>
>>>>> But Philippe found this dumb approach to fail when working on LPAE,
>>>>> IIRC. IIRC, namely, if the root domain happens to be stalled when
>>>>> entering a fault over head domain, it would end up unstalled after
>>>>> the operation. So, I believe the code he added saves the stall state
>>>>> on fault entry and restores it on fault exit. I have checked
>>>>> Philippe's code details at the time and did not find anything wrong.
>>>>
>>>> I suspect the LPAE scenario takes the do_page_fault path? Then it should
>>>> rather be solved by providing the right information to or preventing
>>>> the execution of
>>>>
>>>> 	/* Enable interrupts if they were enabled in the parent context. */
>>>> 	if (interrupts_enabled(regs))
>>>> 		local_irq_enable();
>>>>
>>>> Now we unconditionally restore to the root state on entry, overwriting
>>>> what may happen to it during the handler execution - specifically via
>>>> the snippet above.
>>>
>>> This code is part of the mainline kernel.
>>
>> Correct. But we can adjust it to take interrupt virtualization and
>> domain migration into account.
> 
> I do not think we should. Everything should appear to the kernel as
> if interrupts are NOT virtualized. So, local_irq_enable() should in
> fact do an unstall root, and everything around should be made so
> that it works.

We are on the same page. I guess I just didn't get what you wanted to
state with the previous statement. My point is: we have to ensure that
interrupts_enabled(regs) decides about enabling irqs for root based on
the correct information. And we must not overwrite this decision afterward.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] ipipe: issues with ARM exception handling

[Philippe Gerum]

On 02/20/2015 07:51 PM, Jan Kiszka wrote:
> On 2015-02-20 19:38, Gilles Chanteperdrix wrote:
>> On Fri, Feb 20, 2015 at 07:03:14PM +0100, 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.
>>> - 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.
>>>
>>> Room for optimization:
>>> - ipipe_fault_entry is always called with hard IRQs off from
>>>   do_page_fault and do_translation_fault. I suspect this applies to the
>>>   remaining callers (do_DataAbort and do_PrefetchAbort ) as well. Thus
>>>   the hard IRQ state is actually known at compile time, right?
> 
> To follow up on this: do_DataAbort and do_PrefetchAbort are always
> invoked with hard IRQs disable when a regular exception takes us there.
> Only the ghost syscall cmpxchg simulates do_DataAbort without adjusting
> hardware interrupt. It's probably easier to adjust that than to account
> for hw irqs being potentially on an fault entry.
> 
>>>
>>> I can hack up patches, but I'd like to confirm first that I'm not
>>> missing anything subtle or ARM-specific here.
>>
>> Just to explain the original hack.
>>
>> Some time ago, the faults handlers were executed irqs on ARM. The
>> irqs were enabled in entry.S before executing the handlers.
>>
>> At some point, this was removed in entry.S and fault handlers
>> started to be executed irqs off. On ARM, all faults relax to be
>> handled in secondary mode, actually there is an exception, the FPU,
>> but it goes through a completely different path which had always
>> been executed irqs off until recently where the irqs are reenabled
>> when accessing user-space to be able to handle faults without
>> lockups. 
>>
>> My concern was that the code thus executed could have assertion
>> about the root domain being stalled which would be fail, so I added
>> code which stalled root and enabled hardware irqs on fault entry and
>> unstalled root and disabled hardware irqs on fault exit (which
>> always happen on root domain). This should have worked even if a fault
>> had happened to be handled in head domain, because then the
>> operation would have been a nop (simply stall/then unstall). 
>>
>> But Philippe found this dumb approach to fail when working on LPAE,
>> IIRC. IIRC, namely, if the root domain happens to be stalled when
>> entering a fault over head domain, it would end up unstalled after
>> the operation. So, I believe the code he added saves the stall state
>> on fault entry and restores it on fault exit. I have checked
>> Philippe's code details at the time and did not find anything wrong.
> 
> I suspect the LPAE scenario takes the do_page_fault path? Then it should

It takes the do_translation_fault path, where the page table fixups will
happen.

-- 
Philippe.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Jan Kiszka]

On 2015-02-21 10:13, Philippe Gerum wrote:
> On 02/20/2015 07:51 PM, Jan Kiszka wrote:
>> On 2015-02-20 19:38, Gilles Chanteperdrix wrote:
>>> On Fri, Feb 20, 2015 at 07:03:14PM +0100, 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.
>>>> - 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.
>>>>
>>>> Room for optimization:
>>>> - ipipe_fault_entry is always called with hard IRQs off from
>>>>   do_page_fault and do_translation_fault. I suspect this applies to the
>>>>   remaining callers (do_DataAbort and do_PrefetchAbort ) as well. Thus
>>>>   the hard IRQ state is actually known at compile time, right?
>>
>> To follow up on this: do_DataAbort and do_PrefetchAbort are always
>> invoked with hard IRQs disable when a regular exception takes us there.
>> Only the ghost syscall cmpxchg simulates do_DataAbort without adjusting
>> hardware interrupt. It's probably easier to adjust that than to account
>> for hw irqs being potentially on an fault entry.
>>
>>>>
>>>> I can hack up patches, but I'd like to confirm first that I'm not
>>>> missing anything subtle or ARM-specific here.
>>>
>>> Just to explain the original hack.
>>>
>>> Some time ago, the faults handlers were executed irqs on ARM. The
>>> irqs were enabled in entry.S before executing the handlers.
>>>
>>> At some point, this was removed in entry.S and fault handlers
>>> started to be executed irqs off. On ARM, all faults relax to be
>>> handled in secondary mode, actually there is an exception, the FPU,
>>> but it goes through a completely different path which had always
>>> been executed irqs off until recently where the irqs are reenabled
>>> when accessing user-space to be able to handle faults without
>>> lockups. 
>>>
>>> My concern was that the code thus executed could have assertion
>>> about the root domain being stalled which would be fail, so I added
>>> code which stalled root and enabled hardware irqs on fault entry and
>>> unstalled root and disabled hardware irqs on fault exit (which
>>> always happen on root domain). This should have worked even if a fault
>>> had happened to be handled in head domain, because then the
>>> operation would have been a nop (simply stall/then unstall). 
>>>
>>> But Philippe found this dumb approach to fail when working on LPAE,
>>> IIRC. IIRC, namely, if the root domain happens to be stalled when
>>> entering a fault over head domain, it would end up unstalled after
>>> the operation. So, I believe the code he added saves the stall state
>>> on fault entry and restores it on fault exit. I have checked
>>> Philippe's code details at the time and did not find anything wrong.
>>
>> I suspect the LPAE scenario takes the do_page_fault path? Then it should
> 
> It takes the do_translation_fault path, where the page table fixups will
> happen.

For kernel space addresses, the fixup happens directly, indeed. But here
we need no fiddling with the root IRQ state at all as no sensitive
kernel functions are called.

User space will end up in do_page_fault.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] ipipe: issues with ARM exception handling

[Philippe Gerum]

On 02/23/2015 04:59 PM, Jan Kiszka wrote:
> On 2015-02-21 10:13, Philippe Gerum wrote:
>> On 02/20/2015 07:51 PM, Jan Kiszka wrote:
>>> On 2015-02-20 19:38, Gilles Chanteperdrix wrote:
>>>> On Fri, Feb 20, 2015 at 07:03:14PM +0100, 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.
>>>>> - 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.
>>>>>
>>>>> Room for optimization:
>>>>> - ipipe_fault_entry is always called with hard IRQs off from
>>>>>   do_page_fault and do_translation_fault. I suspect this applies to the
>>>>>   remaining callers (do_DataAbort and do_PrefetchAbort ) as well. Thus
>>>>>   the hard IRQ state is actually known at compile time, right?
>>>
>>> To follow up on this: do_DataAbort and do_PrefetchAbort are always
>>> invoked with hard IRQs disable when a regular exception takes us there.
>>> Only the ghost syscall cmpxchg simulates do_DataAbort without adjusting
>>> hardware interrupt. It's probably easier to adjust that than to account
>>> for hw irqs being potentially on an fault entry.
>>>
>>>>>
>>>>> I can hack up patches, but I'd like to confirm first that I'm not
>>>>> missing anything subtle or ARM-specific here.
>>>>
>>>> Just to explain the original hack.
>>>>
>>>> Some time ago, the faults handlers were executed irqs on ARM. The
>>>> irqs were enabled in entry.S before executing the handlers.
>>>>
>>>> At some point, this was removed in entry.S and fault handlers
>>>> started to be executed irqs off. On ARM, all faults relax to be
>>>> handled in secondary mode, actually there is an exception, the FPU,
>>>> but it goes through a completely different path which had always
>>>> been executed irqs off until recently where the irqs are reenabled
>>>> when accessing user-space to be able to handle faults without
>>>> lockups. 
>>>>
>>>> My concern was that the code thus executed could have assertion
>>>> about the root domain being stalled which would be fail, so I added
>>>> code which stalled root and enabled hardware irqs on fault entry and
>>>> unstalled root and disabled hardware irqs on fault exit (which
>>>> always happen on root domain). This should have worked even if a fault
>>>> had happened to be handled in head domain, because then the
>>>> operation would have been a nop (simply stall/then unstall). 
>>>>
>>>> But Philippe found this dumb approach to fail when working on LPAE,
>>>> IIRC. IIRC, namely, if the root domain happens to be stalled when
>>>> entering a fault over head domain, it would end up unstalled after
>>>> the operation. So, I believe the code he added saves the stall state
>>>> on fault entry and restores it on fault exit. I have checked
>>>> Philippe's code details at the time and did not find anything wrong.
>>>
>>> I suspect the LPAE scenario takes the do_page_fault path? Then it should
>>
>> It takes the do_translation_fault path, where the page table fixups will
>> happen.
> 
> For kernel space addresses, the fixup happens directly, indeed. But here
> we need no fiddling with the root IRQ state at all as no sensitive
> kernel functions are called.
> 
> User space will end up in do_page_fault.
> 

If you refer to a first level translation fault, we currently don't
fiddle with the virtual IRQ state, and also refrain from running any
client hook until the page table is fixed up, so that we don't do any
access to non-linear memory before this happened either.

If you refer to do_page_fault instead, I see no issue in restoring the
root context across a migration from head to root, although not
required. But this is hardly an optimization, given the cost of a domain
migration in the first place. Do you have any scenario in mind which
would trigger a bug?

There is indeed the issue of the vanilla kernel code re-enabling
interrupts if the caller entered the trap handler with the CPSR_I bit
set, in which case a fault taken on behalf a kernel context in a
virtually masked section, but actually unmasked CPU state, would
eventually exit with the root domain stalled. The main problem I see
here, is that a scheduler transition to a blocked state (e.g.
down_read()) would be entered with virtual IRQs off, which may not be a
good idea.

-- 
Philippe.

Re: [Xenomai] ipipe: issues with ARM exception handling

[Jan Kiszka]

On 2015-02-23 17:29, Philippe Gerum wrote:
> On 02/23/2015 04:59 PM, Jan Kiszka wrote:
>> On 2015-02-21 10:13, Philippe Gerum wrote:
>>> On 02/20/2015 07:51 PM, Jan Kiszka wrote:
>>>> On 2015-02-20 19:38, Gilles Chanteperdrix wrote:
>>>>> On Fri, Feb 20, 2015 at 07:03:14PM +0100, 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.
>>>>>> - 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.
>>>>>>
>>>>>> Room for optimization:
>>>>>> - ipipe_fault_entry is always called with hard IRQs off from
>>>>>>   do_page_fault and do_translation_fault. I suspect this applies to the
>>>>>>   remaining callers (do_DataAbort and do_PrefetchAbort ) as well. Thus
>>>>>>   the hard IRQ state is actually known at compile time, right?
>>>>
>>>> To follow up on this: do_DataAbort and do_PrefetchAbort are always
>>>> invoked with hard IRQs disable when a regular exception takes us there.
>>>> Only the ghost syscall cmpxchg simulates do_DataAbort without adjusting
>>>> hardware interrupt. It's probably easier to adjust that than to account
>>>> for hw irqs being potentially on an fault entry.
>>>>
>>>>>>
>>>>>> I can hack up patches, but I'd like to confirm first that I'm not
>>>>>> missing anything subtle or ARM-specific here.
>>>>>
>>>>> Just to explain the original hack.
>>>>>
>>>>> Some time ago, the faults handlers were executed irqs on ARM. The
>>>>> irqs were enabled in entry.S before executing the handlers.
>>>>>
>>>>> At some point, this was removed in entry.S and fault handlers
>>>>> started to be executed irqs off. On ARM, all faults relax to be
>>>>> handled in secondary mode, actually there is an exception, the FPU,
>>>>> but it goes through a completely different path which had always
>>>>> been executed irqs off until recently where the irqs are reenabled
>>>>> when accessing user-space to be able to handle faults without
>>>>> lockups. 
>>>>>
>>>>> My concern was that the code thus executed could have assertion
>>>>> about the root domain being stalled which would be fail, so I added
>>>>> code which stalled root and enabled hardware irqs on fault entry and
>>>>> unstalled root and disabled hardware irqs on fault exit (which
>>>>> always happen on root domain). This should have worked even if a fault
>>>>> had happened to be handled in head domain, because then the
>>>>> operation would have been a nop (simply stall/then unstall). 
>>>>>
>>>>> But Philippe found this dumb approach to fail when working on LPAE,
>>>>> IIRC. IIRC, namely, if the root domain happens to be stalled when
>>>>> entering a fault over head domain, it would end up unstalled after
>>>>> the operation. So, I believe the code he added saves the stall state
>>>>> on fault entry and restores it on fault exit. I have checked
>>>>> Philippe's code details at the time and did not find anything wrong.
>>>>
>>>> I suspect the LPAE scenario takes the do_page_fault path? Then it should
>>>
>>> It takes the do_translation_fault path, where the page table fixups will
>>> happen.
>>
>> For kernel space addresses, the fixup happens directly, indeed. But here
>> we need no fiddling with the root IRQ state at all as no sensitive
>> kernel functions are called.
>>
>> User space will end up in do_page_fault.
>>
> 
> If you refer to a first level translation fault, we currently don't
> fiddle with the virtual IRQ state, and also refrain from running any
> client hook until the page table is fixed up, so that we don't do any
> access to non-linear memory before this happened either.

Right, indeed.

> 
> If you refer to do_page_fault instead, I see no issue in restoring the
> root context across a migration from head to root, although not
> required. But this is hardly an optimization, given the cost of a domain
> migration in the first place. Do you have any scenario in mind which
> would trigger a bug?

On x86, I've seen temporal confusion of Linux /wrt its interrupt mask on
return from exceptions that caused a migration. Possibly, there is no
real scenario that causes a bug (besides in bug-checking tracers) - yet.
I simply consider this overwriting of the Linux-owned state a design
issue that can bite back. Better provide the state as Linux would expect
it without I-pipe bits around.

> 
> There is indeed the issue of the vanilla kernel code re-enabling
> interrupts if the caller entered the trap handler with the CPSR_I bit
> set, in which case a fault taken on behalf a kernel context in a
> virtually masked section, but actually unmasked CPU state, would
> eventually exit with the root domain stalled. The main problem I see
> here, is that a scheduler transition to a blocked state (e.g.
> down_read()) would be entered with virtual IRQs off, which may not be a
> good idea.

Well, the scenarios are sufficiently complex. Better model them along
Linux expectations so that we can worry less about the "what if".

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux