[Xenomai] I-pipe exception handling - a general model

[Xenomai] I-pipe exception handling - a general model

[Jan Kiszka]

Hi,

let's try to express the steps in handling hardware exception under
I-pipe in a generic way:

ipipe_exception_handler(regs, ...)
{
	// ARM has this e.g.
	[if (early_handling(...))
		return;]

	// some architectures may only enter with irqs off
	[hard_irqs_off = hard_irqs_disabled();]

	if (__ipipe_notify_trap(...))
		return;

	local_save_flags(root_flags);
	// this adjusts irq mask in regs (like __fixup_if on x86)
	mask_irqs(regs, raw_irqs_disabled_flags(root_flags));

	if (ipipe_root_p) {
		[if (hard_irqs_off)]
			local_irq_disable();
	} else {
		hard_local_irq_disable()

		__ipipe_set_current_domain(ipipe_root_domain);

		[if (hard_irqs_off)]
			local_irq_disable();

		report_unhandled_fault(...);
	}

	linux_fault_handler(...);

	ipipe_restore_root_nosync(root_flags);

	// never return with irqs masked to root - may already be done
	// by the return code in entry.S on some architectures?
	[mask_irqs(regs, false);]

	// some architectures may require this?
	[hard_local_irq_disable()]
}

This is almost like x86 after my patch. Well, almost...

One difference is that I reverted the change that Philippe did on the
3.16 merge regarding when to restore the root state: before 3.16 we
called ipipe_restore_root_nosync() unconditionally, since 3.16 only if
entered over root. This change was not documented, and it's also not in
line with the discussion about how to handle this on ARM. So I dropped
it here and will drop it from x86.

What I changed furthermore is to unconditionally fix up the interrupt
mask as stored in the register set of the parent context before calling
Linux. And I added that explicit second fixup of regs. That is missing
on x86 right now, but it would only affect a BUG() scenario, I think.

Now let's run this through all scenarios that Gilles listed:

hardware irqs in parent context:
 - are reported unchanged to __ipipe_report_trap
 - will get overwritten with the current root state if
   __ipipe_report_trap doesn't consume the exception
 - will always get re-enabled if returning over root

entry over root:
 - save the current root state
 - transfer the current root state into regs so the Linux can pick up
   the parent root state from there
 - stall root if hw irqs are off, restore previous state after Linux
   handler ran

entry over non-root:
 - if __ipipe_report_trap consumes the exception, no state is changed in
   our handler
 - if the exception is forwarded and migration to root took place, we
   simply follow the path "entry over root"
 - "hard" migration just additionally ensures that hardware IRQs stay
   off during the handling and that reporting is done as needed

Please comment on this model, if it matches your ideas or if you think
there are corner cases still incorrectly handled.

Would be good to document it somewhere centrally and refer to it in the
implementations, suggestions welcome. Maybe we can even factor out some
code that can be shared between architectures. BTW, how does this map on
the other archs we support?

Jan

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

Re: [Xenomai] I-pipe exception handling - a general model

[Gilles Chanteperdrix]

On Tue, Feb 24, 2015 at 08:10:02PM +0100, Jan Kiszka wrote:
> Hi,
> 
> let's try to express the steps in handling hardware exception under
> I-pipe in a generic way:
> 
> ipipe_exception_handler(regs, ...)
> {
> 	// ARM has this e.g.
> 	[if (early_handling(...))
> 		return;]
> 
> 	// some architectures may only enter with irqs off
> 	[hard_irqs_off = hard_irqs_disabled();]

Not OK. Here what you will get is:
- hard_irqs_off is true if the exception is taken on root
- hard_irqs_off is false if the exception is taken on head (as a
result of the relax). So, what would make sense is to put it on the
exception entry, but why bother, it will always be true on arm and I
guess x86, since ARM stopped handling exceptions with irqs on to
imitate x86.

> 
> 	if (__ipipe_notify_trap(...))
> 		return;
> 



> 	local_save_flags(root_flags);
> 	// this adjusts irq mask in regs (like __fixup_if on x86)
> 	mask_irqs(regs, raw_irqs_disabled_flags(root_flags));

Please provide the code of this function.

> 
> 	if (ipipe_root_p) {
> 		[if (hard_irqs_off)]
> 			local_irq_disable();

Also this is uselessly complicated. Calling local_irq_save is much
simpler. This works because hard_irqs_off is always true on ARM
(provided that you put it in the right place, that is before the
early_handling).


> 	} else {


> 		hard_local_irq_disable()
> 




> 		__ipipe_set_current_domain(ipipe_root_domain);
> 
> 		[if (hard_irqs_off)]
> 			local_irq_disable();
> 
> 		report_unhandled_fault(...);
> 	}

Ok, why not. We currently do not have this, but adding it make
sense. I hope that we can make it more readable though.

> 
> 	linux_fault_handler(...);
> 
> 	ipipe_restore_root_nosync(root_flags);
> 
> 	// never return with irqs masked to root - may already be done
> 	// by the return code in entry.S on some architectures?
> 	[mask_irqs(regs, false);]

Wrong: as I said yesterday, the regs should be restored to the value
they had on entry. If the irqs were masked when the exception was
taken, then we can return to the caller with irq masked:
- it will unmask them sooner or later
- this is what the mainline kernel does when irqs are not
virtualized.

Possibly, if some handler fiddles with the PSR_I_BIT in regs->psr,
this should be reflected on the stall bit, but I am not sure this
case even exists.

> 
> 	// some architectures may require this?
> 	[hard_local_irq_disable()]

If you want to do this, you should put it before
ipipe_restore_root_nosync, otherwise you create a window where Linux
irqs can be taken which does not exist in the mainline kernel.

But after discussion with Philippe, I think we want to always return
with irqs on to entry.S 

Again, I propose:

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

	local_irq_save(flags);
	hard_local_irq_enable();

	return flags;
}

static inline void ipipe_fault_exit(unsigned long flags)
{
	ipipe_restore_root_nosync(flags);
}


Which is simple and readable.

-- 
					    Gilles.

Re: [Xenomai] I-pipe exception handling - a general model

[Jan Kiszka]

On 2015-02-24 20:27, Gilles Chanteperdrix wrote:
> On Tue, Feb 24, 2015 at 08:10:02PM +0100, Jan Kiszka wrote:
>> Hi,
>>
>> let's try to express the steps in handling hardware exception under
>> I-pipe in a generic way:
>>
>> ipipe_exception_handler(regs, ...)
>> {
>> 	// ARM has this e.g.
>> 	[if (early_handling(...))
>> 		return;]
>>
>> 	// some architectures may only enter with irqs off
>> 	[hard_irqs_off = hard_irqs_disabled();]
> 
> Not OK. Here what you will get is:
> - hard_irqs_off is true if the exception is taken on root
> - hard_irqs_off is false if the exception is taken on head (as a

No, this is invariant of the context. The hardware(-configuration)
decides if we enter the handler with hard irqs off or not. See x86, this
is known to work code.

> result of the relax). So, what would make sense is to put it on the
> exception entry, but why bother, it will always be true on arm and I
> guess x86, since ARM stopped handling exceptions with irqs on to
> imitate x86.
> 
>>
>> 	if (__ipipe_notify_trap(...))
>> 		return;
>>
> 
> 
> 
>> 	local_save_flags(root_flags);
>> 	// this adjusts irq mask in regs (like __fixup_if on x86)
>> 	mask_irqs(regs, raw_irqs_disabled_flags(root_flags));
> 
> Please provide the code of this function.

-> __fixup_if (same semantic, just clearer naming)

> 
>>
>> 	if (ipipe_root_p) {
>> 		[if (hard_irqs_off)]
>> 			local_irq_disable();
> 
> Also this is uselessly complicated. Calling local_irq_save is much
> simpler. This works because hard_irqs_off is always true on ARM
> (provided that you put it in the right place, that is before the
> early_handling).

No, this would be wrong if hard_irqs_off is false, thus the trap is
taken without disabling hardware interrupts. On ARM, hard_irqs_off would
be always true, correct, but this is a generic model.

> 
> 
>> 	} else {
> 
> 
>> 		hard_local_irq_disable()
>>
> 
> 
> 
> 
>> 		__ipipe_set_current_domain(ipipe_root_domain);
>>
>> 		[if (hard_irqs_off)]
>> 			local_irq_disable();
>>
>> 		report_unhandled_fault(...);
>> 	}
> 
> Ok, why not. We currently do not have this, but adding it make
> sense. I hope that we can make it more readable though.
> 
>>
>> 	linux_fault_handler(...);
>>
>> 	ipipe_restore_root_nosync(root_flags);
>>
>> 	// never return with irqs masked to root - may already be done
>> 	// by the return code in entry.S on some architectures?
>> 	[mask_irqs(regs, false);]
> 
> Wrong: as I said yesterday, the regs should be restored to the value
> they had on entry. If the irqs were masked when the exception was
> taken, then we can return to the caller with irq masked:
> - it will unmask them sooner or later
> - this is what the mainline kernel does when irqs are not
> virtualized.

Right, we should safe/restore the state in regs equally. I guess that
mostly matters in BUG() scenarios, though. But let's play safe.

> 
> Possibly, if some handler fiddles with the PSR_I_BIT in regs->psr,
> this should be reflected on the stall bit, but I am not sure this
> case even exists.

That would be important to know. We don't do this on x86 so far.

> 
>>
>> 	// some architectures may require this?
>> 	[hard_local_irq_disable()]
> 
> If you want to do this, you should put it before
> ipipe_restore_root_nosync, otherwise you create a window where Linux
> irqs can be taken which does not exist in the mainline kernel.
> 
> But after discussion with Philippe, I think we want to always return
> with irqs on to entry.S 

Then we will take Linux IRQs after the ipipe_restore_root_nosync - but
that's not different to existing x86 code at least.

Jan

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

Re: [Xenomai] I-pipe exception handling - a general model

[Philippe Gerum]

On 02/24/2015 08:10 PM, Jan Kiszka wrote:
> Hi,
> 
> let's try to express the steps in handling hardware exception under
> I-pipe in a generic way:
> 
> ipipe_exception_handler(regs, ...)
> {
> 	// ARM has this e.g.
> 	[if (early_handling(...))
> 		return;]
> 
> 	// some architectures may only enter with irqs off
> 	[hard_irqs_off = hard_irqs_disabled();]
> 
> 	if (__ipipe_notify_trap(...))
> 		return;
> 
> 	local_save_flags(root_flags);
> 	// this adjusts irq mask in regs (like __fixup_if on x86)
> 	mask_irqs(regs, raw_irqs_disabled_flags(root_flags));
> 
> 	if (ipipe_root_p) {
> 		[if (hard_irqs_off)]
> 			local_irq_disable();
> 	} else {
> 		hard_local_irq_disable()
> 
> 		__ipipe_set_current_domain(ipipe_root_domain);
> 
> 		[if (hard_irqs_off)]
> 			local_irq_disable();
> 
> 		report_unhandled_fault(...);
> 	}
> 
> 	linux_fault_handler(...);
> 
> 	ipipe_restore_root_nosync(root_flags);
> 
> 	// never return with irqs masked to root - may already be done
> 	// by the return code in entry.S on some architectures?
> 	[mask_irqs(regs, false);]
> 
> 	// some architectures may require this?
> 	[hard_local_irq_disable()]
> }
> 
> This is almost like x86 after my patch. Well, almost...
> 
> One difference is that I reverted the change that Philippe did on the
> 3.16 merge regarding when to restore the root state: before 3.16 we
> called ipipe_restore_root_nosync() unconditionally, since 3.16 only if
> entered over root. This change was not documented, and it's also not in
> line with the discussion about how to handle this on ARM. So I dropped
> it here and will drop it from x86.

FWIW, I don't agree with much of the discussion on the ARM stuff,
although I could agree on changes making fault entry/exit more cautious.
I'm waiting for the dust to settle.

Besides, I find the idea of generalizing the fault handling practically
very difficult, since the arch-dependent assembly-written trampolines
may make different assumptions when dispatching faults. Check how ppc64
virtualizes interrupts, or how blackfin cannot reschedule directly on
behalf an interrupt or trap, and have fun.

I'm ok with any meaningful change, but to sum it up :

- I would definitely not use x86 as a reference, because there may be
assumptions from the ancient times when interrupts were virtualized in
entry.S, which disappeared a few years ago. If things are done only for
the sake of preserving the TRACE_IRQFLAGS feature, then fine, but in
such a case this should not alter the fast path. I always wondered why
things got so complicated with x86 over time, maybe they should be
considered again. To me, your patch related to the interrupt state when
calling __ipipe_notify_trap() definitely goes in the right direction.
Nice to see the x86 implementation shrink for once.

- the code invoking ipipe_restore_root_nosync() should better make sure
that 1) interrupts were never virtually masked with hw irqs on before
ipipe_restore_root_nosync() is invoked while handling the fault, OR 2)
the interrupt pipeline is synced before entry.S returns from the
exception trap. I'm unsure that any of these conditions is granted for
all archs (read: I'm pretty sure of the opposite).

Other than that, I'm fine with any simplification and obviously, fixes.
Please make sure to channel all ARM changes through Gilles's tree, so
that my favorite ARM maintainer is always happy, and stops saturating my
IRC buffer with the contents of his emacs buffers, sending me more code
in an hour than I could ever managed to write over the last twenty years.

TIA,

-- 
Philippe.

Re: [Xenomai] I-pipe exception handling - a general model

[Gilles Chanteperdrix]

On Tue, Feb 24, 2015 at 09:29:43PM +0100, Jan Kiszka wrote:
> On 2015-02-24 20:27, Gilles Chanteperdrix wrote:
> > On Tue, Feb 24, 2015 at 08:10:02PM +0100, Jan Kiszka wrote:
> >> Hi,
> >>
> >> let's try to express the steps in handling hardware exception under
> >> I-pipe in a generic way:
> >>
> >> ipipe_exception_handler(regs, ...)
> >> {
> >> 	// ARM has this e.g.
> >> 	[if (early_handling(...))
> >> 		return;]
> >>
> >> 	// some architectures may only enter with irqs off
> >> 	[hard_irqs_off = hard_irqs_disabled();]
> > 
> > Not OK. Here what you will get is:
> > - hard_irqs_off is true if the exception is taken on root
> > - hard_irqs_off is false if the exception is taken on head (as a
> 
> No, this is invariant of the context. The hardware(-configuration)
> decides if we enter the handler with hard irqs off or not. See x86, this
> is known to work code.

No. Relaxing re-enables hw irqs. So, if you took the exception over
head, you relaxed, and hw irqs are on here. The information you get
this way is irrelevant.

> 
> > result of the relax). So, what would make sense is to put it on the
> > exception entry, but why bother, it will always be true on arm and I
> > guess x86, since ARM stopped handling exceptions with irqs on to
> > imitate x86.
> > 
> >>
> >> 	if (__ipipe_notify_trap(...))
> >> 		return;
> >>
> > 
> > 
> > 
> >> 	local_save_flags(root_flags);
> >> 	// this adjusts irq mask in regs (like __fixup_if on x86)
> >> 	mask_irqs(regs, raw_irqs_disabled_flags(root_flags));
> > 
> > Please provide the code of this function.
> 
> -> __fixup_if (same semantic, just clearer naming)
> 
> > 
> >>
> >> 	if (ipipe_root_p) {
> >> 		[if (hard_irqs_off)]
> >> 			local_irq_disable();
> > 
> > Also this is uselessly complicated. Calling local_irq_save is much
> > simpler. This works because hard_irqs_off is always true on ARM
> > (provided that you put it in the right place, that is before the
> > early_handling).
> 
> No, this would be wrong if hard_irqs_off is false, thus the trap is
> taken without disabling hardware interrupts. On ARM, hard_irqs_off would
> be always true, correct, but this is a generic model.

Ok, sorry, I am not interested in the discussion about the
generic model. The only thing I am interested in is the ARM
implementation, because this is what I maintain. I just want to
discuss what will land in the I-pipe tree before it happens.

So, following your suggestions, I propose the following
implementation: 

static inline unsigned long ipipe_fault_entry(struct pt_regs *regs)
{
	unsigned long flags;
	int s;

	hard_local_irq_disable();
	s = __test_and_set_bit(IPIPE_STALL_FLAG, &__ipipe_root_status);
	hard_local_irq_enable();

	flags = regs->ARM_cpsr;
	regs->ARM_cpsr = flags & ~PSR_I_BIT | (s ? PSR_I_BIT : 0);

	if (ipipe_root_p)
		return flags;

	local_irq_enable();
	BUG();
	return ~0UL;
}

static inline void ipipe_fault_exit(struct pt_regs *regs, unsigned long flags)
{
	if (interrupts_enabled(regs))
		local_irq_enable();
	reg->ARM_cpsr = flags;
}

By saving the stall bit in the saved regs CPSR, it will cope
automatically with the case where the exception handler fiddles with
the PSR_I_BIT. If this is found to be bad taste, I can use
arch_mangle_bits instead.

-- 
					    Gilles.

Re: [Xenomai] I-pipe exception handling - a general model

[Gilles Chanteperdrix]

On Tue, Feb 24, 2015 at 09:38:37PM +0100, Philippe Gerum wrote:
> and stops saturating my
> IRC buffer with the contents of his emacs buffers, sending me more code
> in an hour than I could ever managed to write over the last twenty years.

That is the problem with using emacs as an irc client, it is too
tempting to paste code.

I have just posted another implementation in answer to Jan. Will
not post it here or on IRC, then :-)

-- 
					    Gilles.

Re: [Xenomai] I-pipe exception handling - a general model

[Jan Kiszka]

On 2015-02-24 21:48, Gilles Chanteperdrix wrote:
> On Tue, Feb 24, 2015 at 09:29:43PM +0100, Jan Kiszka wrote:
>> On 2015-02-24 20:27, Gilles Chanteperdrix wrote:
>>> On Tue, Feb 24, 2015 at 08:10:02PM +0100, Jan Kiszka wrote:
>>>> Hi,
>>>>
>>>> let's try to express the steps in handling hardware exception under
>>>> I-pipe in a generic way:
>>>>
>>>> ipipe_exception_handler(regs, ...)
>>>> {
>>>> 	// ARM has this e.g.
>>>> 	[if (early_handling(...))
>>>> 		return;]
>>>>
>>>> 	// some architectures may only enter with irqs off
>>>> 	[hard_irqs_off = hard_irqs_disabled();]
>>>
>>> Not OK. Here what you will get is:
>>> - hard_irqs_off is true if the exception is taken on root
>>> - hard_irqs_off is false if the exception is taken on head (as a
>>
>> No, this is invariant of the context. The hardware(-configuration)
>> decides if we enter the handler with hard irqs off or not. See x86, this
>> is known to work code.
> 
> No. Relaxing re-enables hw irqs. So, if you took the exception over
> head, you relaxed, and hw irqs are on here. The information you get
> this way is irrelevant.

ipipe_exception_handler() is supposed to be the function that the
assembly exception trampoline calls directly - there can't be any
migration/relaxation before that. Thus, hard_irqs_disabled() reflects if
the exception handler was called by the hardware with hard interrupts on
or off.

Jan

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