[Xenomai] [RFC] Consolidated exception prologue/epiloge for x86 and ARM

[Xenomai] [RFC] Consolidated exception prologue/epiloge for x86 and ARM

[Jan Kiszka]

Hi,

just pushed a first implementation of the general model that I proposed
for exception handling. You can find it at

http://git.xenomai.org/ipipe-jki.git/log/?h=queues/trap-rework

I can also relay the patches to the list if desired.

Besides the issue that Gilles remarked, I found no further problems
while implementing and testing this approach on both x86-64 and ARMv7.

I also added support for hardware breakpoint handling on ARM at this
chance but I didn't test that particular path yet.

Jan

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

Re: [Xenomai] [RFC] Consolidated exception prologue/epiloge for x86 and ARM

[Gilles Chanteperdrix]

On Fri, Feb 27, 2015 at 09:12:14PM +0100, Jan Kiszka wrote:
> Hi,
> 
> just pushed a first implementation of the general model that I proposed
> for exception handling. You can find it at
> 
> http://git.xenomai.org/ipipe-jki.git/log/?h=queues/trap-rework

NACK. The ARM implementation is broken, re-enabling hardware irqs
before stalling root is wrong.

Besides, I find that


if (foo) {
   bar;
} else {
  qux;
}

When there is nothing after adds useless indentation.

if (foo) {
   bar;
   return;
}

qux;

is much clearer.

Both thins were handled correctly in the implementation I proposed.
Also, reading your implementation as patches makes things uselessly
hard to read.

Simply posting the prologue and epilogue code would have made things
simpler.


> 
> I can also relay the patches to the list if desired.
> 
> Besides the issue that Gilles remarked, I found no further problems
> while implementing and testing this approach on both x86-64 and ARMv7.
> 
> I also added support for hardware breakpoint handling on ARM at this
> chance but I didn't test that particular path yet.

As you had been warned, your tests were useless since I disagree
with your implementation.

-- 
					    Gilles.

Re: [Xenomai] [RFC] Consolidated exception prologue/epiloge for x86 and ARM

[Jan Kiszka]

On 2015-02-27 21:24, Gilles Chanteperdrix wrote:
> On Fri, Feb 27, 2015 at 09:12:14PM +0100, Jan Kiszka wrote:
>> Hi,
>>
>> just pushed a first implementation of the general model that I proposed
>> for exception handling. You can find it at
>>
>> http://git.xenomai.org/ipipe-jki.git/log/?h=queues/trap-rework
> 
> NACK. The ARM implementation is broken, re-enabling hardware irqs
> before stalling root is wrong.

Ok, will adjust.

> 
> Besides, I find that
> 
> 
> if (foo) {
>    bar;
> } else {
>   qux;
> }
> 
> When there is nothing after adds useless indentation.
> 
> if (foo) {
>    bar;
>    return;
> }
> 
> qux;

Which function?

Jan

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

Re: [Xenomai] [RFC] Consolidated exception prologue/epiloge for x86 and ARM

[Gilles Chanteperdrix]

On Fri, Feb 27, 2015 at 09:37:24PM +0100, Jan Kiszka wrote:
> On 2015-02-27 21:24, Gilles Chanteperdrix wrote:
> > On Fri, Feb 27, 2015 at 09:12:14PM +0100, Jan Kiszka wrote:
> >> Hi,
> >>
> >> just pushed a first implementation of the general model that I proposed
> >> for exception handling. You can find it at
> >>
> >> http://git.xenomai.org/ipipe-jki.git/log/?h=queues/trap-rework
> > 
> > NACK. The ARM implementation is broken, re-enabling hardware irqs
> > before stalling root is wrong.
> 
> Ok, will adjust.
> 
> > 
> > Besides, I find that
> > 
> > 
> > if (foo) {
> >    bar;
> > } else {
> >   qux;
> > }
> > 
> > When there is nothing after adds useless indentation.
> > 
> > if (foo) {
> >    bar;
> >    return;
> > }
> > 
> > qux;
> 
> Which function?

Well, there are only two functions.. The one that has an if.

-- 
					    Gilles.

Re: [Xenomai] [RFC] Consolidated exception prologue/epiloge for x86 and ARM

[Jan Kiszka]

On 2015-02-27 21:37, Jan Kiszka wrote:
> On 2015-02-27 21:24, Gilles Chanteperdrix wrote:
>> On Fri, Feb 27, 2015 at 09:12:14PM +0100, Jan Kiszka wrote:
>>> Hi,
>>>
>>> just pushed a first implementation of the general model that I proposed
>>> for exception handling. You can find it at
>>>
>>> http://git.xenomai.org/ipipe-jki.git/log/?h=queues/trap-rework
>>
>> NACK. The ARM implementation is broken, re-enabling hardware irqs
>> before stalling root is wrong.
> 
> Ok, will adjust.

But I need to think about this again first. It seems we are doing this
for ages on x86, and I need to understand why it could be a problem on
ARM. Can you provide an example that would breaks?

Jan

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

Re: [Xenomai] [RFC] Consolidated exception prologue/epiloge for x86 and ARM

[Gilles Chanteperdrix]

On Fri, Feb 27, 2015 at 09:47:37PM +0100, Jan Kiszka wrote:
> On 2015-02-27 21:37, Jan Kiszka wrote:
> > On 2015-02-27 21:24, Gilles Chanteperdrix wrote:
> >> On Fri, Feb 27, 2015 at 09:12:14PM +0100, Jan Kiszka wrote:
> >>> Hi,
> >>>
> >>> just pushed a first implementation of the general model that I proposed
> >>> for exception handling. You can find it at
> >>>
> >>> http://git.xenomai.org/ipipe-jki.git/log/?h=queues/trap-rework
> >>
> >> NACK. The ARM implementation is broken, re-enabling hardware irqs
> >> before stalling root is wrong.
> > 
> > Ok, will adjust.
> 
> But I need to think about this again first. It seems we are doing this
> for ages on x86, and I need to understand why it could be a problem on
> ARM. Can you provide an example that would breaks?

You are creating a point where a Linux irq can happen which does not
exist on the mainline kernel.

If you do:
local_irq_save(flags);
hard_local_irq_enable();

There is no way a Linux interrupt can squeeze in.

(I mean in the case of exception over root domamin, in the case of
an exception over head, irqs are enabled anyway).

-- 
					    Gilles.

Re: [Xenomai] [RFC] Consolidated exception prologue/epiloge for x86 and ARM

[Jan Kiszka]

On 2015-02-27 21:50, Gilles Chanteperdrix wrote:
> On Fri, Feb 27, 2015 at 09:47:37PM +0100, Jan Kiszka wrote:
>> On 2015-02-27 21:37, Jan Kiszka wrote:
>>> On 2015-02-27 21:24, Gilles Chanteperdrix wrote:
>>>> On Fri, Feb 27, 2015 at 09:12:14PM +0100, Jan Kiszka wrote:
>>>>> Hi,
>>>>>
>>>>> just pushed a first implementation of the general model that I proposed
>>>>> for exception handling. You can find it at
>>>>>
>>>>> http://git.xenomai.org/ipipe-jki.git/log/?h=queues/trap-rework
>>>>
>>>> NACK. The ARM implementation is broken, re-enabling hardware irqs
>>>> before stalling root is wrong.
>>>
>>> Ok, will adjust.
>>
>> But I need to think about this again first. It seems we are doing this
>> for ages on x86, and I need to understand why it could be a problem on
>> ARM. Can you provide an example that would breaks?
> 
> You are creating a point where a Linux irq can happen which does not
> exist on the mainline kernel.
> 
> If you do:
> local_irq_save(flags);
> hard_local_irq_enable();
> 
> There is no way a Linux interrupt can squeeze in.
> 
> (I mean in the case of exception over root domamin, in the case of
> an exception over head, irqs are enabled anyway).

That is clear. I'm looking for examples where that is a real problem in
the given code path.

There are three relevant cases, I guess:
 - head entry -> we will enable IRQs anyway while migrating to root
 - root entry, root stalled -> uncritical
 - root entry, root unstalled -> what is the difference to the
   head-entry scenario if root is unstalled there as well?

That said, reordering is not a big deal and could be done nevertheless.

Jan

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

Re: [Xenomai] [RFC] Consolidated exception prologue/epiloge for x86 and ARM

[Gilles Chanteperdrix]

On Mon, Mar 02, 2015 at 06:40:22PM +0100, Jan Kiszka wrote:
> On 2015-02-27 21:50, Gilles Chanteperdrix wrote:
> > On Fri, Feb 27, 2015 at 09:47:37PM +0100, Jan Kiszka wrote:
> >> On 2015-02-27 21:37, Jan Kiszka wrote:
> >>> On 2015-02-27 21:24, Gilles Chanteperdrix wrote:
> >>>> On Fri, Feb 27, 2015 at 09:12:14PM +0100, Jan Kiszka wrote:
> >>>>> Hi,
> >>>>>
> >>>>> just pushed a first implementation of the general model that I proposed
> >>>>> for exception handling. You can find it at
> >>>>>
> >>>>> http://git.xenomai.org/ipipe-jki.git/log/?h=queues/trap-rework
> >>>>
> >>>> NACK. The ARM implementation is broken, re-enabling hardware irqs
> >>>> before stalling root is wrong.
> >>>
> >>> Ok, will adjust.
> >>
> >> But I need to think about this again first. It seems we are doing this
> >> for ages on x86, and I need to understand why it could be a problem on
> >> ARM. Can you provide an example that would breaks?
> > 
> > You are creating a point where a Linux irq can happen which does not
> > exist on the mainline kernel.
> > 
> > If you do:
> > local_irq_save(flags);
> > hard_local_irq_enable();
> > 
> > There is no way a Linux interrupt can squeeze in.
> > 
> > (I mean in the case of exception over root domamin, in the case of
> > an exception over head, irqs are enabled anyway).
> 
> That is clear. I'm looking for examples where that is a real problem in
> the given code path.

It is really hard to be sure. Better safe than sorry.

-- 
					    Gilles.

Re: [Xenomai] [RFC] Consolidated exception prologue/epiloge for x86 and ARM

[Gilles Chanteperdrix]

On Fri, Feb 27, 2015 at 09:12:14PM +0100, Jan Kiszka wrote:
> Hi,
> 
> just pushed a first implementation of the general model that I proposed
> for exception handling. You can find it at
> 
> http://git.xenomai.org/ipipe-jki.git/log/?h=queues/trap-rework

Also, the clean way to pass virtual + physical flags is to use
arch_mangle_bits. Using two longs (potentially 128 bits then) is
completely useless since one of the longs simply has one significant
bit.

-- 
					    Gilles.

Re: [Xenomai] [RFC] Consolidated exception prologue/epiloge for x86 and ARM

[Jan Kiszka]

On 2015-02-27 21:27, Gilles Chanteperdrix wrote:
> On Fri, Feb 27, 2015 at 09:12:14PM +0100, Jan Kiszka wrote:
>> Hi,
>>
>> just pushed a first implementation of the general model that I proposed
>> for exception handling. You can find it at
>>
>> http://git.xenomai.org/ipipe-jki.git/log/?h=queues/trap-rework
> 
> Also, the clean way to pass virtual + physical flags is to use
> arch_mangle_bits. Using two longs (potentially 128 bits then) is
> completely useless since one of the longs simply has one significant
> bit.

The costs of mangling is higher than using two regs for passing that
data as-is, both binary and LOC-wise (tried it). Plus the code is more
readable.

Jan

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

Re: [Xenomai] [RFC] Consolidated exception prologue/epiloge for x86 and ARM

[Gilles Chanteperdrix]

On Fri, Feb 27, 2015 at 09:37:45PM +0100, Jan Kiszka wrote:
> On 2015-02-27 21:27, Gilles Chanteperdrix wrote:
> > On Fri, Feb 27, 2015 at 09:12:14PM +0100, Jan Kiszka wrote:
> >> Hi,
> >>
> >> just pushed a first implementation of the general model that I proposed
> >> for exception handling. You can find it at
> >>
> >> http://git.xenomai.org/ipipe-jki.git/log/?h=queues/trap-rework
> > 
> > Also, the clean way to pass virtual + physical flags is to use
> > arch_mangle_bits. Using two longs (potentially 128 bits then) is
> > completely useless since one of the longs simply has one significant
> > bit.
> 
> The costs of mangling is higher than using two regs for passing that
> data as-is, both binary and LOC-wise (tried it). Plus the code is more
> readable.

That is false on ARM. On ARM gcc does not pass structs by values in
registers. The values get passed on stack.

-- 
					    Gilles.

Re: [Xenomai] [RFC] Consolidated exception prologue/epiloge for x86 and ARM

[Gilles Chanteperdrix]

On Fri, Feb 27, 2015 at 10:21:30PM +0100, Gilles Chanteperdrix wrote:
> On Fri, Feb 27, 2015 at 09:37:45PM +0100, Jan Kiszka wrote:
> > On 2015-02-27 21:27, Gilles Chanteperdrix wrote:
> > > On Fri, Feb 27, 2015 at 09:12:14PM +0100, Jan Kiszka wrote:
> > >> Hi,
> > >>
> > >> just pushed a first implementation of the general model that I proposed
> > >> for exception handling. You can find it at
> > >>
> > >> http://git.xenomai.org/ipipe-jki.git/log/?h=queues/trap-rework
> > > 
> > > Also, the clean way to pass virtual + physical flags is to use
> > > arch_mangle_bits. Using two longs (potentially 128 bits then) is
> > > completely useless since one of the longs simply has one significant
> > > bit.
> > 
> > The costs of mangling is higher than using two regs for passing that
> > data as-is, both binary and LOC-wise (tried it). Plus the code is more
> > readable.
> 
> That is false on ARM. On ARM gcc does not pass structs by values in
> registers. The values get passed on stack.

Sorry, misread the assembler. They are passed by registers, however
the registers get uselessly saved on stack, then restored to other
registers. 

struct foo {
	int x;
	int y;
};

int f(struct foo f)
{
	return f.x + f.y;
}

Gives, with -Os:
00000000 <f>:
   0:   b082            sub     sp, #8
   2:   ab02            add     r3, sp, #8
   4:   e903 0003       stmdb   r3, {r0, r1}
   8:   e89d 0009       ldmia.w sp, {r0, r3}
   c:   4418            add     r0, r3
   e:   b002            add     sp, #8
  10:   4770            bx      lr

-- 
					    Gilles.

Re: [Xenomai] [RFC] Consolidated exception prologue/epiloge for x86 and ARM

[Jan Kiszka]

On 2015-02-27 22:24, Gilles Chanteperdrix wrote:
> On Fri, Feb 27, 2015 at 10:21:30PM +0100, Gilles Chanteperdrix wrote:
>> On Fri, Feb 27, 2015 at 09:37:45PM +0100, Jan Kiszka wrote:
>>> On 2015-02-27 21:27, Gilles Chanteperdrix wrote:
>>>> On Fri, Feb 27, 2015 at 09:12:14PM +0100, Jan Kiszka wrote:
>>>>> Hi,
>>>>>
>>>>> just pushed a first implementation of the general model that I proposed
>>>>> for exception handling. You can find it at
>>>>>
>>>>> http://git.xenomai.org/ipipe-jki.git/log/?h=queues/trap-rework
>>>>
>>>> Also, the clean way to pass virtual + physical flags is to use
>>>> arch_mangle_bits. Using two longs (potentially 128 bits then) is
>>>> completely useless since one of the longs simply has one significant
>>>> bit.
>>>
>>> The costs of mangling is higher than using two regs for passing that
>>> data as-is, both binary and LOC-wise (tried it). Plus the code is more
>>> readable.
>>
>> That is false on ARM. On ARM gcc does not pass structs by values in
>> registers. The values get passed on stack.
> 
> Sorry, misread the assembler. They are passed by registers, however
> the registers get uselessly saved on stack, then restored to other
> registers. 
> 
> struct foo {
> 	int x;
> 	int y;
> };
> 
> int f(struct foo f)
> {
> 	return f.x + f.y;
> }
> 
> Gives, with -Os:
> 00000000 <f>:
>    0:   b082            sub     sp, #8
>    2:   ab02            add     r3, sp, #8
>    4:   e903 0003       stmdb   r3, {r0, r1}
>    8:   e89d 0009       ldmia.w sp, {r0, r3}
>    c:   4418            add     r0, r3
>    e:   b002            add     sp, #8
>   10:   4770            bx      lr
> 

Ouch. I missed that this sneaked in.

The complications with the existing mangle functions are that they do
not play well with what I need for the existing
ipipe_restore_root_nosync. I can open-code the latter (size increases),
extend the former to alternatively return architectural flags (instead
of boolean), or provide another wrapper to convert the virt bit into flags.

Hmm, or - and that's probably cleanest - I simply align
ipipe_restore_root_nosync to ipipe_restore_root argument-wise. The
latter takes "x" (stall) as boolean, the former as architectural flags.
That's highly confusing anyway. And it seems there are no users to break
in Xenomai, despite that it is exported to modules.

Jan

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

Re: [Xenomai] [RFC] Consolidated exception prologue/epiloge for x86 and ARM

[Gilles Chanteperdrix]

On Mon, Mar 02, 2015 at 06:39:22PM +0100, Jan Kiszka wrote:
> On 2015-02-27 22:24, Gilles Chanteperdrix wrote:
> > On Fri, Feb 27, 2015 at 10:21:30PM +0100, Gilles Chanteperdrix wrote:
> >> On Fri, Feb 27, 2015 at 09:37:45PM +0100, Jan Kiszka wrote:
> >>> On 2015-02-27 21:27, Gilles Chanteperdrix wrote:
> >>>> On Fri, Feb 27, 2015 at 09:12:14PM +0100, Jan Kiszka wrote:
> >>>>> Hi,
> >>>>>
> >>>>> just pushed a first implementation of the general model that I proposed
> >>>>> for exception handling. You can find it at
> >>>>>
> >>>>> http://git.xenomai.org/ipipe-jki.git/log/?h=queues/trap-rework
> >>>>
> >>>> Also, the clean way to pass virtual + physical flags is to use
> >>>> arch_mangle_bits. Using two longs (potentially 128 bits then) is
> >>>> completely useless since one of the longs simply has one significant
> >>>> bit.
> >>>
> >>> The costs of mangling is higher than using two regs for passing that
> >>> data as-is, both binary and LOC-wise (tried it). Plus the code is more
> >>> readable.
> >>
> >> That is false on ARM. On ARM gcc does not pass structs by values in
> >> registers. The values get passed on stack.
> > 
> > Sorry, misread the assembler. They are passed by registers, however
> > the registers get uselessly saved on stack, then restored to other
> > registers. 
> > 
> > struct foo {
> > 	int x;
> > 	int y;
> > };
> > 
> > int f(struct foo f)
> > {
> > 	return f.x + f.y;
> > }
> > 
> > Gives, with -Os:
> > 00000000 <f>:
> >    0:   b082            sub     sp, #8
> >    2:   ab02            add     r3, sp, #8
> >    4:   e903 0003       stmdb   r3, {r0, r1}
> >    8:   e89d 0009       ldmia.w sp, {r0, r3}
> >    c:   4418            add     r0, r3
> >    e:   b002            add     sp, #8
> >   10:   4770            bx      lr
> > 
> 
> Ouch. I missed that this sneaked in.
> 
> The complications with the existing mangle functions are that they do
> not play well with what I need for the existing
> ipipe_restore_root_nosync. I can open-code the latter (size increases),
> extend the former to alternatively return architectural flags (instead
> of boolean), or provide another wrapper to convert the virt bit into flags.
> 
> Hmm, or - and that's probably cleanest - I simply align
> ipipe_restore_root_nosync to ipipe_restore_root argument-wise. The
> latter takes "x" (stall) as boolean, the former as architectural flags.
> That's highly confusing anyway. And it seems there are no users to break
> in Xenomai, despite that it is exported to modules.

Or simply use arch_mangle_bits ?

-- 
					    Gilles.

Re: [Xenomai] [RFC] Consolidated exception prologue/epiloge for x86 and ARM

[Jan Kiszka]

On 2015-03-02 18:41, Gilles Chanteperdrix wrote:
> On Mon, Mar 02, 2015 at 06:39:22PM +0100, Jan Kiszka wrote:
>> On 2015-02-27 22:24, Gilles Chanteperdrix wrote:
>>> On Fri, Feb 27, 2015 at 10:21:30PM +0100, Gilles Chanteperdrix wrote:
>>>> On Fri, Feb 27, 2015 at 09:37:45PM +0100, Jan Kiszka wrote:
>>>>> On 2015-02-27 21:27, Gilles Chanteperdrix wrote:
>>>>>> On Fri, Feb 27, 2015 at 09:12:14PM +0100, Jan Kiszka wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> just pushed a first implementation of the general model that I proposed
>>>>>>> for exception handling. You can find it at
>>>>>>>
>>>>>>> http://git.xenomai.org/ipipe-jki.git/log/?h=queues/trap-rework
>>>>>>
>>>>>> Also, the clean way to pass virtual + physical flags is to use
>>>>>> arch_mangle_bits. Using two longs (potentially 128 bits then) is
>>>>>> completely useless since one of the longs simply has one significant
>>>>>> bit.
>>>>>
>>>>> The costs of mangling is higher than using two regs for passing that
>>>>> data as-is, both binary and LOC-wise (tried it). Plus the code is more
>>>>> readable.
>>>>
>>>> That is false on ARM. On ARM gcc does not pass structs by values in
>>>> registers. The values get passed on stack.
>>>
>>> Sorry, misread the assembler. They are passed by registers, however
>>> the registers get uselessly saved on stack, then restored to other
>>> registers. 
>>>
>>> struct foo {
>>> 	int x;
>>> 	int y;
>>> };
>>>
>>> int f(struct foo f)
>>> {
>>> 	return f.x + f.y;
>>> }
>>>
>>> Gives, with -Os:
>>> 00000000 <f>:
>>>    0:   b082            sub     sp, #8
>>>    2:   ab02            add     r3, sp, #8
>>>    4:   e903 0003       stmdb   r3, {r0, r1}
>>>    8:   e89d 0009       ldmia.w sp, {r0, r3}
>>>    c:   4418            add     r0, r3
>>>    e:   b002            add     sp, #8
>>>   10:   4770            bx      lr
>>>
>>
>> Ouch. I missed that this sneaked in.
>>
>> The complications with the existing mangle functions are that they do
>> not play well with what I need for the existing
>> ipipe_restore_root_nosync. I can open-code the latter (size increases),
>> extend the former to alternatively return architectural flags (instead
>> of boolean), or provide another wrapper to convert the virt bit into flags.
>>
>> Hmm, or - and that's probably cleanest - I simply align
>> ipipe_restore_root_nosync to ipipe_restore_root argument-wise. The
>> latter takes "x" (stall) as boolean, the former as architectural flags.
>> That's highly confusing anyway. And it seems there are no users to break
>> in Xenomai, despite that it is exported to modules.
> 
> Or simply use arch_mangle_bits ?

This function does not exist. Or what do you mean?

Jan

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

Re: [Xenomai] [RFC] Consolidated exception prologue/epiloge for x86 and ARM

[Gilles Chanteperdrix]

On Mon, Mar 02, 2015 at 06:45:20PM +0100, Jan Kiszka wrote:
> On 2015-03-02 18:41, Gilles Chanteperdrix wrote:
> > On Mon, Mar 02, 2015 at 06:39:22PM +0100, Jan Kiszka wrote:
> >> On 2015-02-27 22:24, Gilles Chanteperdrix wrote:
> >>> On Fri, Feb 27, 2015 at 10:21:30PM +0100, Gilles Chanteperdrix wrote:
> >>>> On Fri, Feb 27, 2015 at 09:37:45PM +0100, Jan Kiszka wrote:
> >>>>> On 2015-02-27 21:27, Gilles Chanteperdrix wrote:
> >>>>>> On Fri, Feb 27, 2015 at 09:12:14PM +0100, Jan Kiszka wrote:
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> just pushed a first implementation of the general model that I proposed
> >>>>>>> for exception handling. You can find it at
> >>>>>>>
> >>>>>>> http://git.xenomai.org/ipipe-jki.git/log/?h=queues/trap-rework
> >>>>>>
> >>>>>> Also, the clean way to pass virtual + physical flags is to use
> >>>>>> arch_mangle_bits. Using two longs (potentially 128 bits then) is
> >>>>>> completely useless since one of the longs simply has one significant
> >>>>>> bit.
> >>>>>
> >>>>> The costs of mangling is higher than using two regs for passing that
> >>>>> data as-is, both binary and LOC-wise (tried it). Plus the code is more
> >>>>> readable.
> >>>>
> >>>> That is false on ARM. On ARM gcc does not pass structs by values in
> >>>> registers. The values get passed on stack.
> >>>
> >>> Sorry, misread the assembler. They are passed by registers, however
> >>> the registers get uselessly saved on stack, then restored to other
> >>> registers. 
> >>>
> >>> struct foo {
> >>> 	int x;
> >>> 	int y;
> >>> };
> >>>
> >>> int f(struct foo f)
> >>> {
> >>> 	return f.x + f.y;
> >>> }
> >>>
> >>> Gives, with -Os:
> >>> 00000000 <f>:
> >>>    0:   b082            sub     sp, #8
> >>>    2:   ab02            add     r3, sp, #8
> >>>    4:   e903 0003       stmdb   r3, {r0, r1}
> >>>    8:   e89d 0009       ldmia.w sp, {r0, r3}
> >>>    c:   4418            add     r0, r3
> >>>    e:   b002            add     sp, #8
> >>>   10:   4770            bx      lr
> >>>
> >>
> >> Ouch. I missed that this sneaked in.
> >>
> >> The complications with the existing mangle functions are that they do
> >> not play well with what I need for the existing
> >> ipipe_restore_root_nosync. I can open-code the latter (size increases),
> >> extend the former to alternatively return architectural flags (instead
> >> of boolean), or provide another wrapper to convert the virt bit into flags.
> >>
> >> Hmm, or - and that's probably cleanest - I simply align
> >> ipipe_restore_root_nosync to ipipe_restore_root argument-wise. The
> >> latter takes "x" (stall) as boolean, the former as architectural flags.
> >> That's highly confusing anyway. And it seems there are no users to break
> >> in Xenomai, despite that it is exported to modules.
> > 
> > Or simply use arch_mangle_bits ?
> 
> This function does not exist. Or what do you mean?

I mean the function Philippe uses for the purpose of mixing hardware
flags with virtual flags, whose name I apparently forgot.

-- 
					    Gilles.

Re: [Xenomai] [RFC] Consolidated exception prologue/epiloge for x86 and ARM

[Jan Kiszka]

On 2015-03-02 18:47, Gilles Chanteperdrix wrote:
> On Mon, Mar 02, 2015 at 06:45:20PM +0100, Jan Kiszka wrote:
>> On 2015-03-02 18:41, Gilles Chanteperdrix wrote:
>>> On Mon, Mar 02, 2015 at 06:39:22PM +0100, Jan Kiszka wrote:
>>>> On 2015-02-27 22:24, Gilles Chanteperdrix wrote:
>>>>> On Fri, Feb 27, 2015 at 10:21:30PM +0100, Gilles Chanteperdrix wrote:
>>>>>> On Fri, Feb 27, 2015 at 09:37:45PM +0100, Jan Kiszka wrote:
>>>>>>> On 2015-02-27 21:27, Gilles Chanteperdrix wrote:
>>>>>>>> On Fri, Feb 27, 2015 at 09:12:14PM +0100, Jan Kiszka wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> just pushed a first implementation of the general model that I proposed
>>>>>>>>> for exception handling. You can find it at
>>>>>>>>>
>>>>>>>>> http://git.xenomai.org/ipipe-jki.git/log/?h=queues/trap-rework
>>>>>>>>
>>>>>>>> Also, the clean way to pass virtual + physical flags is to use
>>>>>>>> arch_mangle_bits. Using two longs (potentially 128 bits then) is
>>>>>>>> completely useless since one of the longs simply has one significant
>>>>>>>> bit.
>>>>>>>
>>>>>>> The costs of mangling is higher than using two regs for passing that
>>>>>>> data as-is, both binary and LOC-wise (tried it). Plus the code is more
>>>>>>> readable.
>>>>>>
>>>>>> That is false on ARM. On ARM gcc does not pass structs by values in
>>>>>> registers. The values get passed on stack.
>>>>>
>>>>> Sorry, misread the assembler. They are passed by registers, however
>>>>> the registers get uselessly saved on stack, then restored to other
>>>>> registers. 
>>>>>
>>>>> struct foo {
>>>>> 	int x;
>>>>> 	int y;
>>>>> };
>>>>>
>>>>> int f(struct foo f)
>>>>> {
>>>>> 	return f.x + f.y;
>>>>> }
>>>>>
>>>>> Gives, with -Os:
>>>>> 00000000 <f>:
>>>>>    0:   b082            sub     sp, #8
>>>>>    2:   ab02            add     r3, sp, #8
>>>>>    4:   e903 0003       stmdb   r3, {r0, r1}
>>>>>    8:   e89d 0009       ldmia.w sp, {r0, r3}
>>>>>    c:   4418            add     r0, r3
>>>>>    e:   b002            add     sp, #8
>>>>>   10:   4770            bx      lr
>>>>>
>>>>
>>>> Ouch. I missed that this sneaked in.
>>>>
>>>> The complications with the existing mangle functions are that they do
>>>> not play well with what I need for the existing
>>>> ipipe_restore_root_nosync. I can open-code the latter (size increases),
>>>> extend the former to alternatively return architectural flags (instead
>>>> of boolean), or provide another wrapper to convert the virt bit into flags.
>>>>
>>>> Hmm, or - and that's probably cleanest - I simply align
>>>> ipipe_restore_root_nosync to ipipe_restore_root argument-wise. The
>>>> latter takes "x" (stall) as boolean, the former as architectural flags.
>>>> That's highly confusing anyway. And it seems there are no users to break
>>>> in Xenomai, despite that it is exported to modules.
>>>
>>> Or simply use arch_mangle_bits ?
>>
>> This function does not exist. Or what do you mean?
> 
> I mean the function Philippe uses for the purpose of mixing hardware
> flags with virtual flags, whose name I apparently forgot.

arch_mangle_irq_bits and arch_demangle_irq_bits - yes, those are the
ones that are not compatible with current ipipe_restore_root_nosync.
I'll fix the latter, then it should work better.

Jan

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

Re: [Xenomai] [RFC] Consolidated exception prologue/epiloge for x86 and ARM

[Gilles Chanteperdrix]

On Mon, Mar 02, 2015 at 06:49:57PM +0100, Jan Kiszka wrote:
> On 2015-03-02 18:47, Gilles Chanteperdrix wrote:
> > On Mon, Mar 02, 2015 at 06:45:20PM +0100, Jan Kiszka wrote:
> >> On 2015-03-02 18:41, Gilles Chanteperdrix wrote:
> >>> On Mon, Mar 02, 2015 at 06:39:22PM +0100, Jan Kiszka wrote:
> >>>> On 2015-02-27 22:24, Gilles Chanteperdrix wrote:
> >>>>> On Fri, Feb 27, 2015 at 10:21:30PM +0100, Gilles Chanteperdrix wrote:
> >>>>>> On Fri, Feb 27, 2015 at 09:37:45PM +0100, Jan Kiszka wrote:
> >>>>>>> On 2015-02-27 21:27, Gilles Chanteperdrix wrote:
> >>>>>>>> On Fri, Feb 27, 2015 at 09:12:14PM +0100, Jan Kiszka wrote:
> >>>>>>>>> Hi,
> >>>>>>>>>
> >>>>>>>>> just pushed a first implementation of the general model that I proposed
> >>>>>>>>> for exception handling. You can find it at
> >>>>>>>>>
> >>>>>>>>> http://git.xenomai.org/ipipe-jki.git/log/?h=queues/trap-rework
> >>>>>>>>
> >>>>>>>> Also, the clean way to pass virtual + physical flags is to use
> >>>>>>>> arch_mangle_bits. Using two longs (potentially 128 bits then) is
> >>>>>>>> completely useless since one of the longs simply has one significant
> >>>>>>>> bit.
> >>>>>>>
> >>>>>>> The costs of mangling is higher than using two regs for passing that
> >>>>>>> data as-is, both binary and LOC-wise (tried it). Plus the code is more
> >>>>>>> readable.
> >>>>>>
> >>>>>> That is false on ARM. On ARM gcc does not pass structs by values in
> >>>>>> registers. The values get passed on stack.
> >>>>>
> >>>>> Sorry, misread the assembler. They are passed by registers, however
> >>>>> the registers get uselessly saved on stack, then restored to other
> >>>>> registers. 
> >>>>>
> >>>>> struct foo {
> >>>>> 	int x;
> >>>>> 	int y;
> >>>>> };
> >>>>>
> >>>>> int f(struct foo f)
> >>>>> {
> >>>>> 	return f.x + f.y;
> >>>>> }
> >>>>>
> >>>>> Gives, with -Os:
> >>>>> 00000000 <f>:
> >>>>>    0:   b082            sub     sp, #8
> >>>>>    2:   ab02            add     r3, sp, #8
> >>>>>    4:   e903 0003       stmdb   r3, {r0, r1}
> >>>>>    8:   e89d 0009       ldmia.w sp, {r0, r3}
> >>>>>    c:   4418            add     r0, r3
> >>>>>    e:   b002            add     sp, #8
> >>>>>   10:   4770            bx      lr
> >>>>>
> >>>>
> >>>> Ouch. I missed that this sneaked in.
> >>>>
> >>>> The complications with the existing mangle functions are that they do
> >>>> not play well with what I need for the existing
> >>>> ipipe_restore_root_nosync. I can open-code the latter (size increases),
> >>>> extend the former to alternatively return architectural flags (instead
> >>>> of boolean), or provide another wrapper to convert the virt bit into flags.
> >>>>
> >>>> Hmm, or - and that's probably cleanest - I simply align
> >>>> ipipe_restore_root_nosync to ipipe_restore_root argument-wise. The
> >>>> latter takes "x" (stall) as boolean, the former as architectural flags.
> >>>> That's highly confusing anyway. And it seems there are no users to break
> >>>> in Xenomai, despite that it is exported to modules.
> >>>
> >>> Or simply use arch_mangle_bits ?
> >>
> >> This function does not exist. Or what do you mean?
> > 
> > I mean the function Philippe uses for the purpose of mixing hardware
> > flags with virtual flags, whose name I apparently forgot.
> 
> arch_mangle_irq_bits and arch_demangle_irq_bits - yes, those are the
> ones that are not compatible with current ipipe_restore_root_nosync.
> I'll fix the latter, then it should work better.

You do not want to use ipipe_restore_root_nosycn, as Philippe said.
You want to run the full local_irq_restore.

-- 
					    Gilles.

Re: [Xenomai] [RFC] Consolidated exception prologue/epiloge for x86 and ARM

[Philippe Gerum]

On 03/02/2015 06:39 PM, Jan Kiszka wrote:
> On 2015-02-27 22:24, Gilles Chanteperdrix wrote:
>> On Fri, Feb 27, 2015 at 10:21:30PM +0100, Gilles Chanteperdrix wrote:
>>> On Fri, Feb 27, 2015 at 09:37:45PM +0100, Jan Kiszka wrote:
>>>> On 2015-02-27 21:27, Gilles Chanteperdrix wrote:
>>>>> On Fri, Feb 27, 2015 at 09:12:14PM +0100, Jan Kiszka wrote:
>>>>>> Hi,
>>>>>>
>>>>>> just pushed a first implementation of the general model that I proposed
>>>>>> for exception handling. You can find it at
>>>>>>
>>>>>> http://git.xenomai.org/ipipe-jki.git/log/?h=queues/trap-rework
>>>>>
>>>>> Also, the clean way to pass virtual + physical flags is to use
>>>>> arch_mangle_bits. Using two longs (potentially 128 bits then) is
>>>>> completely useless since one of the longs simply has one significant
>>>>> bit.
>>>>
>>>> The costs of mangling is higher than using two regs for passing that
>>>> data as-is, both binary and LOC-wise (tried it). Plus the code is more
>>>> readable.
>>>
>>> That is false on ARM. On ARM gcc does not pass structs by values in
>>> registers. The values get passed on stack.
>>
>> Sorry, misread the assembler. They are passed by registers, however
>> the registers get uselessly saved on stack, then restored to other
>> registers. 
>>
>> struct foo {
>> 	int x;
>> 	int y;
>> };
>>
>> int f(struct foo f)
>> {
>> 	return f.x + f.y;
>> }
>>
>> Gives, with -Os:
>> 00000000 <f>:
>>    0:   b082            sub     sp, #8
>>    2:   ab02            add     r3, sp, #8
>>    4:   e903 0003       stmdb   r3, {r0, r1}
>>    8:   e89d 0009       ldmia.w sp, {r0, r3}
>>    c:   4418            add     r0, r3
>>    e:   b002            add     sp, #8
>>   10:   4770            bx      lr
>>
> 
> Ouch. I missed that this sneaked in.
> 
> The complications with the existing mangle functions are that they do
> not play well with what I need for the existing
> ipipe_restore_root_nosync. I can open-code the latter (size increases),

Unless this is part of a heavily used static inline, this increase
should be negligible.

> extend the former to alternatively return architectural flags (instead
> of boolean), or provide another wrapper to convert the virt bit into flags.
> 
> Hmm, or - and that's probably cleanest - I simply align
> ipipe_restore_root_nosync to ipipe_restore_root argument-wise. The
> latter takes "x" (stall) as boolean, the former as architectural flags.
> That's highly confusing anyway. And it seems there are no users to break
> in Xenomai, despite that it is exported to modules.
> 

It is exported because it was called from some static inline helper
which was part of an obsolete interface, not because client code should
use it.

The cleanest approach is not to use ipipe_restore_root_nosync() at all.
There are only a very few occasions when no syncing the interrupt log
ever makes sense, and all are now open-coded to make it clear that we
are doing something very unusual.

ipipe_restore_root_nosync() is confusing enough that people tend to use
it the wrong way, introducing nasty bugs, so I definitely plan to get
rid of it.

-- 
Philippe.

Re: [Xenomai] [RFC] Consolidated exception prologue/epiloge for x86 and ARM

[Jan Kiszka]

On 2015-03-02 19:53, Philippe Gerum wrote:
> On 03/02/2015 06:39 PM, Jan Kiszka wrote:
>> On 2015-02-27 22:24, Gilles Chanteperdrix wrote:
>>> On Fri, Feb 27, 2015 at 10:21:30PM +0100, Gilles Chanteperdrix wrote:
>>>> On Fri, Feb 27, 2015 at 09:37:45PM +0100, Jan Kiszka wrote:
>>>>> On 2015-02-27 21:27, Gilles Chanteperdrix wrote:
>>>>>> On Fri, Feb 27, 2015 at 09:12:14PM +0100, Jan Kiszka wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> just pushed a first implementation of the general model that I proposed
>>>>>>> for exception handling. You can find it at
>>>>>>>
>>>>>>> http://git.xenomai.org/ipipe-jki.git/log/?h=queues/trap-rework
>>>>>>
>>>>>> Also, the clean way to pass virtual + physical flags is to use
>>>>>> arch_mangle_bits. Using two longs (potentially 128 bits then) is
>>>>>> completely useless since one of the longs simply has one significant
>>>>>> bit.
>>>>>
>>>>> The costs of mangling is higher than using two regs for passing that
>>>>> data as-is, both binary and LOC-wise (tried it). Plus the code is more
>>>>> readable.
>>>>
>>>> That is false on ARM. On ARM gcc does not pass structs by values in
>>>> registers. The values get passed on stack.
>>>
>>> Sorry, misread the assembler. They are passed by registers, however
>>> the registers get uselessly saved on stack, then restored to other
>>> registers. 
>>>
>>> struct foo {
>>> 	int x;
>>> 	int y;
>>> };
>>>
>>> int f(struct foo f)
>>> {
>>> 	return f.x + f.y;
>>> }
>>>
>>> Gives, with -Os:
>>> 00000000 <f>:
>>>    0:   b082            sub     sp, #8
>>>    2:   ab02            add     r3, sp, #8
>>>    4:   e903 0003       stmdb   r3, {r0, r1}
>>>    8:   e89d 0009       ldmia.w sp, {r0, r3}
>>>    c:   4418            add     r0, r3
>>>    e:   b002            add     sp, #8
>>>   10:   4770            bx      lr
>>>
>>
>> Ouch. I missed that this sneaked in.
>>
>> The complications with the existing mangle functions are that they do
>> not play well with what I need for the existing
>> ipipe_restore_root_nosync. I can open-code the latter (size increases),
> 
> Unless this is part of a heavily used static inline, this increase
> should be negligible.
> 
>> extend the former to alternatively return architectural flags (instead
>> of boolean), or provide another wrapper to convert the virt bit into flags.
>>
>> Hmm, or - and that's probably cleanest - I simply align
>> ipipe_restore_root_nosync to ipipe_restore_root argument-wise. The
>> latter takes "x" (stall) as boolean, the former as architectural flags.
>> That's highly confusing anyway. And it seems there are no users to break
>> in Xenomai, despite that it is exported to modules.
>>
> 
> It is exported because it was called from some static inline helper
> which was part of an obsolete interface, not because client code should
> use it.
> 
> The cleanest approach is not to use ipipe_restore_root_nosync() at all.
> There are only a very few occasions when no syncing the interrupt log
> ever makes sense, and all are now open-coded to make it clear that we
> are doing something very unusual.

Hmm, I still seeing it called by both x86 and ARM.

> 
> ipipe_restore_root_nosync() is confusing enough that people tend to use
> it the wrong way, introducing nasty bugs, so I definitely plan to get
> rid of it.

Indeed.

I'm currently re-analyzing x86 in this regard. It is called there after
every exception - but what if Linux didn't enable IRQs while handling
the exception and, thus, didn't flush the log. We could have some
interrupts pending, no?

The alternative - already lived in ARM's ipipe_fault_exit today - is
taking Linux interrupts at that point. If that is always OK is another
question...

Jan

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

Re: [Xenomai] [RFC] Consolidated exception prologue/epiloge for x86 and ARM

[Jan Kiszka]

On 2015-03-02 20:17, Jan Kiszka wrote:
> On 2015-03-02 19:53, Philippe Gerum wrote:
>> On 03/02/2015 06:39 PM, Jan Kiszka wrote:
>>> On 2015-02-27 22:24, Gilles Chanteperdrix wrote:
>>>> On Fri, Feb 27, 2015 at 10:21:30PM +0100, Gilles Chanteperdrix wrote:
>>>>> On Fri, Feb 27, 2015 at 09:37:45PM +0100, Jan Kiszka wrote:
>>>>>> On 2015-02-27 21:27, Gilles Chanteperdrix wrote:
>>>>>>> On Fri, Feb 27, 2015 at 09:12:14PM +0100, Jan Kiszka wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> just pushed a first implementation of the general model that I proposed
>>>>>>>> for exception handling. You can find it at
>>>>>>>>
>>>>>>>> http://git.xenomai.org/ipipe-jki.git/log/?h=queues/trap-rework
>>>>>>>
>>>>>>> Also, the clean way to pass virtual + physical flags is to use
>>>>>>> arch_mangle_bits. Using two longs (potentially 128 bits then) is
>>>>>>> completely useless since one of the longs simply has one significant
>>>>>>> bit.
>>>>>>
>>>>>> The costs of mangling is higher than using two regs for passing that
>>>>>> data as-is, both binary and LOC-wise (tried it). Plus the code is more
>>>>>> readable.
>>>>>
>>>>> That is false on ARM. On ARM gcc does not pass structs by values in
>>>>> registers. The values get passed on stack.
>>>>
>>>> Sorry, misread the assembler. They are passed by registers, however
>>>> the registers get uselessly saved on stack, then restored to other
>>>> registers. 
>>>>
>>>> struct foo {
>>>> 	int x;
>>>> 	int y;
>>>> };
>>>>
>>>> int f(struct foo f)
>>>> {
>>>> 	return f.x + f.y;
>>>> }
>>>>
>>>> Gives, with -Os:
>>>> 00000000 <f>:
>>>>    0:   b082            sub     sp, #8
>>>>    2:   ab02            add     r3, sp, #8
>>>>    4:   e903 0003       stmdb   r3, {r0, r1}
>>>>    8:   e89d 0009       ldmia.w sp, {r0, r3}
>>>>    c:   4418            add     r0, r3
>>>>    e:   b002            add     sp, #8
>>>>   10:   4770            bx      lr
>>>>
>>>
>>> Ouch. I missed that this sneaked in.
>>>
>>> The complications with the existing mangle functions are that they do
>>> not play well with what I need for the existing
>>> ipipe_restore_root_nosync. I can open-code the latter (size increases),
>>
>> Unless this is part of a heavily used static inline, this increase
>> should be negligible.
>>
>>> extend the former to alternatively return architectural flags (instead
>>> of boolean), or provide another wrapper to convert the virt bit into flags.
>>>
>>> Hmm, or - and that's probably cleanest - I simply align
>>> ipipe_restore_root_nosync to ipipe_restore_root argument-wise. The
>>> latter takes "x" (stall) as boolean, the former as architectural flags.
>>> That's highly confusing anyway. And it seems there are no users to break
>>> in Xenomai, despite that it is exported to modules.
>>>
>>
>> It is exported because it was called from some static inline helper
>> which was part of an obsolete interface, not because client code should
>> use it.
>>
>> The cleanest approach is not to use ipipe_restore_root_nosync() at all.
>> There are only a very few occasions when no syncing the interrupt log
>> ever makes sense, and all are now open-coded to make it clear that we
>> are doing something very unusual.
> 
> Hmm, I still seeing it called by both x86 and ARM.
> 
>>
>> ipipe_restore_root_nosync() is confusing enough that people tend to use
>> it the wrong way, introducing nasty bugs, so I definitely plan to get
>> rid of it.
> 
> Indeed.
> 
> I'm currently re-analyzing x86 in this regard. It is called there after
> every exception - but what if Linux didn't enable IRQs while handling
> the exception and, thus, didn't flush the log. We could have some
> interrupts pending, no?

Seems we have a couple of sleeping issues in x86:

 - page fault entry over root, root was unstalled
 - we stall root because hard irqs are found off in the exception
   context
 - we call the Linux handler
 - early in __do_page_fault, hard IRQs get reenabled
 - an interrupt for Linux arrives and gets logged
 - Linux doesn't reenable interrupts as it leaves early (vmalloc_fault
   or spurious_fault)
 - we restore root according to the state found on entry, but we do not
   flush the log
 - the exception return path on x86 no longer modifies or flushes as
   well
 => lost an event

With __ipipe_divert_exception on 3.14, there are also cases we we do not
seem to adjust the root state after forwarding an int3. Normally,
do_int3 re-enables interrupts, but not if it leaves early due to dynamic
ftrace or alternates related breakpoints.

BTW, we do not seem to enable hard IRQs while handling most exceptions
under Linux, probably only consistently if we migrated from head before
- or I'm missing that part right now. Hmm, need to check if some of them
are entered with hard irqs on anyway.

> 
> The alternative - already lived in ARM's ipipe_fault_exit today - is
> taking Linux interrupts at that point. If that is always OK is another
> question...

I'll check this on x86, and then it can become part of the common trap
functions (for archs that are capable of taking IRQs over exception
handlers).

Jan

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

Re: [Xenomai] [RFC] Consolidated exception prologue/epiloge for x86 and ARM

[Philippe Gerum]

On 03/02/2015 09:31 PM, Jan Kiszka wrote:
> On 2015-03-02 20:17, Jan Kiszka wrote:
>> On 2015-03-02 19:53, Philippe Gerum wrote:
>>> On 03/02/2015 06:39 PM, Jan Kiszka wrote:
>>>> On 2015-02-27 22:24, Gilles Chanteperdrix wrote:
>>>>> On Fri, Feb 27, 2015 at 10:21:30PM +0100, Gilles Chanteperdrix wrote:
>>>>>> On Fri, Feb 27, 2015 at 09:37:45PM +0100, Jan Kiszka wrote:
>>>>>>> On 2015-02-27 21:27, Gilles Chanteperdrix wrote:
>>>>>>>> On Fri, Feb 27, 2015 at 09:12:14PM +0100, Jan Kiszka wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> just pushed a first implementation of the general model that I proposed
>>>>>>>>> for exception handling. You can find it at
>>>>>>>>>
>>>>>>>>> http://git.xenomai.org/ipipe-jki.git/log/?h=queues/trap-rework
>>>>>>>>
>>>>>>>> Also, the clean way to pass virtual + physical flags is to use
>>>>>>>> arch_mangle_bits. Using two longs (potentially 128 bits then) is
>>>>>>>> completely useless since one of the longs simply has one significant
>>>>>>>> bit.
>>>>>>>
>>>>>>> The costs of mangling is higher than using two regs for passing that
>>>>>>> data as-is, both binary and LOC-wise (tried it). Plus the code is more
>>>>>>> readable.
>>>>>>
>>>>>> That is false on ARM. On ARM gcc does not pass structs by values in
>>>>>> registers. The values get passed on stack.
>>>>>
>>>>> Sorry, misread the assembler. They are passed by registers, however
>>>>> the registers get uselessly saved on stack, then restored to other
>>>>> registers. 
>>>>>
>>>>> struct foo {
>>>>> 	int x;
>>>>> 	int y;
>>>>> };
>>>>>
>>>>> int f(struct foo f)
>>>>> {
>>>>> 	return f.x + f.y;
>>>>> }
>>>>>
>>>>> Gives, with -Os:
>>>>> 00000000 <f>:
>>>>>    0:   b082            sub     sp, #8
>>>>>    2:   ab02            add     r3, sp, #8
>>>>>    4:   e903 0003       stmdb   r3, {r0, r1}
>>>>>    8:   e89d 0009       ldmia.w sp, {r0, r3}
>>>>>    c:   4418            add     r0, r3
>>>>>    e:   b002            add     sp, #8
>>>>>   10:   4770            bx      lr
>>>>>
>>>>
>>>> Ouch. I missed that this sneaked in.
>>>>
>>>> The complications with the existing mangle functions are that they do
>>>> not play well with what I need for the existing
>>>> ipipe_restore_root_nosync. I can open-code the latter (size increases),
>>>
>>> Unless this is part of a heavily used static inline, this increase
>>> should be negligible.
>>>
>>>> extend the former to alternatively return architectural flags (instead
>>>> of boolean), or provide another wrapper to convert the virt bit into flags.
>>>>
>>>> Hmm, or - and that's probably cleanest - I simply align
>>>> ipipe_restore_root_nosync to ipipe_restore_root argument-wise. The
>>>> latter takes "x" (stall) as boolean, the former as architectural flags.
>>>> That's highly confusing anyway. And it seems there are no users to break
>>>> in Xenomai, despite that it is exported to modules.
>>>>
>>>
>>> It is exported because it was called from some static inline helper
>>> which was part of an obsolete interface, not because client code should
>>> use it.
>>>
>>> The cleanest approach is not to use ipipe_restore_root_nosync() at all.
>>> There are only a very few occasions when no syncing the interrupt log
>>> ever makes sense, and all are now open-coded to make it clear that we
>>> are doing something very unusual.
>>
>> Hmm, I still seeing it called by both x86 and ARM.
>>
>>>
>>> ipipe_restore_root_nosync() is confusing enough that people tend to use
>>> it the wrong way, introducing nasty bugs, so I definitely plan to get
>>> rid of it.
>>
>> Indeed.
>>
>> I'm currently re-analyzing x86 in this regard. It is called there after
>> every exception - but what if Linux didn't enable IRQs while handling
>> the exception and, thus, didn't flush the log. We could have some
>> interrupts pending, no?
> 
> Seems we have a couple of sleeping issues in x86:
> 
>  - page fault entry over root, root was unstalled
>  - we stall root because hard irqs are found off in the exception
>    context
>  - we call the Linux handler
>  - early in __do_page_fault, hard IRQs get reenabled
>  - an interrupt for Linux arrives and gets logged
>  - Linux doesn't reenable interrupts as it leaves early (vmalloc_fault
>    or spurious_fault)
>  - we restore root according to the state found on entry, but we do not
>    flush the log
>  - the exception return path on x86 no longer modifies or flushes as
>    well
>  => lost an event
> 

This is exactly the issue I mentioned about using the nosync form of the
root_unstall helper. I suspect this code is a left-over from the ancient
times when IRQs were virtualized in the assembly path as well, and the
interrupt log got flushed soon before iret then.

-- 
Philippe.

Re: [Xenomai] [RFC] Consolidated exception prologue/epiloge for x86 and ARM

[Philippe Gerum]

On 03/03/2015 03:26 PM, Philippe Gerum wrote:
> On 03/02/2015 09:31 PM, Jan Kiszka wrote:
>> On 2015-03-02 20:17, Jan Kiszka wrote:
>>> On 2015-03-02 19:53, Philippe Gerum wrote:
>>>> On 03/02/2015 06:39 PM, Jan Kiszka wrote:
>>>>> On 2015-02-27 22:24, Gilles Chanteperdrix wrote:
>>>>>> On Fri, Feb 27, 2015 at 10:21:30PM +0100, Gilles Chanteperdrix wrote:
>>>>>>> On Fri, Feb 27, 2015 at 09:37:45PM +0100, Jan Kiszka wrote:
>>>>>>>> On 2015-02-27 21:27, Gilles Chanteperdrix wrote:
>>>>>>>>> On Fri, Feb 27, 2015 at 09:12:14PM +0100, Jan Kiszka wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> just pushed a first implementation of the general model that I proposed
>>>>>>>>>> for exception handling. You can find it at
>>>>>>>>>>
>>>>>>>>>> http://git.xenomai.org/ipipe-jki.git/log/?h=queues/trap-rework
>>>>>>>>>
>>>>>>>>> Also, the clean way to pass virtual + physical flags is to use
>>>>>>>>> arch_mangle_bits. Using two longs (potentially 128 bits then) is
>>>>>>>>> completely useless since one of the longs simply has one significant
>>>>>>>>> bit.
>>>>>>>>
>>>>>>>> The costs of mangling is higher than using two regs for passing that
>>>>>>>> data as-is, both binary and LOC-wise (tried it). Plus the code is more
>>>>>>>> readable.
>>>>>>>
>>>>>>> That is false on ARM. On ARM gcc does not pass structs by values in
>>>>>>> registers. The values get passed on stack.
>>>>>>
>>>>>> Sorry, misread the assembler. They are passed by registers, however
>>>>>> the registers get uselessly saved on stack, then restored to other
>>>>>> registers. 
>>>>>>
>>>>>> struct foo {
>>>>>> 	int x;
>>>>>> 	int y;
>>>>>> };
>>>>>>
>>>>>> int f(struct foo f)
>>>>>> {
>>>>>> 	return f.x + f.y;
>>>>>> }
>>>>>>
>>>>>> Gives, with -Os:
>>>>>> 00000000 <f>:
>>>>>>    0:   b082            sub     sp, #8
>>>>>>    2:   ab02            add     r3, sp, #8
>>>>>>    4:   e903 0003       stmdb   r3, {r0, r1}
>>>>>>    8:   e89d 0009       ldmia.w sp, {r0, r3}
>>>>>>    c:   4418            add     r0, r3
>>>>>>    e:   b002            add     sp, #8
>>>>>>   10:   4770            bx      lr
>>>>>>
>>>>>
>>>>> Ouch. I missed that this sneaked in.
>>>>>
>>>>> The complications with the existing mangle functions are that they do
>>>>> not play well with what I need for the existing
>>>>> ipipe_restore_root_nosync. I can open-code the latter (size increases),
>>>>
>>>> Unless this is part of a heavily used static inline, this increase
>>>> should be negligible.
>>>>
>>>>> extend the former to alternatively return architectural flags (instead
>>>>> of boolean), or provide another wrapper to convert the virt bit into flags.
>>>>>
>>>>> Hmm, or - and that's probably cleanest - I simply align
>>>>> ipipe_restore_root_nosync to ipipe_restore_root argument-wise. The
>>>>> latter takes "x" (stall) as boolean, the former as architectural flags.
>>>>> That's highly confusing anyway. And it seems there are no users to break
>>>>> in Xenomai, despite that it is exported to modules.
>>>>>
>>>>
>>>> It is exported because it was called from some static inline helper
>>>> which was part of an obsolete interface, not because client code should
>>>> use it.
>>>>
>>>> The cleanest approach is not to use ipipe_restore_root_nosync() at all.
>>>> There are only a very few occasions when no syncing the interrupt log
>>>> ever makes sense, and all are now open-coded to make it clear that we
>>>> are doing something very unusual.
>>>
>>> Hmm, I still seeing it called by both x86 and ARM.
>>>
>>>>
>>>> ipipe_restore_root_nosync() is confusing enough that people tend to use
>>>> it the wrong way, introducing nasty bugs, so I definitely plan to get
>>>> rid of it.
>>>
>>> Indeed.
>>>
>>> I'm currently re-analyzing x86 in this regard. It is called there after
>>> every exception - but what if Linux didn't enable IRQs while handling
>>> the exception and, thus, didn't flush the log. We could have some
>>> interrupts pending, no?
>>
>> Seems we have a couple of sleeping issues in x86:
>>
>>  - page fault entry over root, root was unstalled
>>  - we stall root because hard irqs are found off in the exception
>>    context
>>  - we call the Linux handler
>>  - early in __do_page_fault, hard IRQs get reenabled
>>  - an interrupt for Linux arrives and gets logged
>>  - Linux doesn't reenable interrupts as it leaves early (vmalloc_fault
>>    or spurious_fault)
>>  - we restore root according to the state found on entry, but we do not
>>    flush the log
>>  - the exception return path on x86 no longer modifies or flushes as
>>    well
>>  => lost an event
>>
> 
> This is exactly the issue I mentioned about using the nosync form of the
> root_unstall helper. I suspect this code is a left-over from the ancient
> times when IRQs were virtualized in the assembly path as well, and the
> interrupt log got flushed soon before iret then.
> 

i.e. __ipipe_unstall_iret_root, from the EMULATE_ROOT_IRET blocks in the
x86 assembly path. Still visible in v2.6/adeos-ipipe-2.6.24-x86-2.0-03
for instance.

-- 
Philippe.

Re: [Xenomai] [RFC] Consolidated exception prologue/epiloge for x86 and ARM

[Philippe Gerum]

On 03/02/2015 08:17 PM, Jan Kiszka wrote:
> On 2015-03-02 19:53, Philippe Gerum wrote:
>> On 03/02/2015 06:39 PM, Jan Kiszka wrote:
>>> On 2015-02-27 22:24, Gilles Chanteperdrix wrote:
>>>> On Fri, Feb 27, 2015 at 10:21:30PM +0100, Gilles Chanteperdrix wrote:
>>>>> On Fri, Feb 27, 2015 at 09:37:45PM +0100, Jan Kiszka wrote:
>>>>>> On 2015-02-27 21:27, Gilles Chanteperdrix wrote:
>>>>>>> On Fri, Feb 27, 2015 at 09:12:14PM +0100, Jan Kiszka wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> just pushed a first implementation of the general model that I proposed
>>>>>>>> for exception handling. You can find it at
>>>>>>>>
>>>>>>>> http://git.xenomai.org/ipipe-jki.git/log/?h=queues/trap-rework
>>>>>>>
>>>>>>> Also, the clean way to pass virtual + physical flags is to use
>>>>>>> arch_mangle_bits. Using two longs (potentially 128 bits then) is
>>>>>>> completely useless since one of the longs simply has one significant
>>>>>>> bit.
>>>>>>
>>>>>> The costs of mangling is higher than using two regs for passing that
>>>>>> data as-is, both binary and LOC-wise (tried it). Plus the code is more
>>>>>> readable.
>>>>>
>>>>> That is false on ARM. On ARM gcc does not pass structs by values in
>>>>> registers. The values get passed on stack.
>>>>
>>>> Sorry, misread the assembler. They are passed by registers, however
>>>> the registers get uselessly saved on stack, then restored to other
>>>> registers. 
>>>>
>>>> struct foo {
>>>> 	int x;
>>>> 	int y;
>>>> };
>>>>
>>>> int f(struct foo f)
>>>> {
>>>> 	return f.x + f.y;
>>>> }
>>>>
>>>> Gives, with -Os:
>>>> 00000000 <f>:
>>>>    0:   b082            sub     sp, #8
>>>>    2:   ab02            add     r3, sp, #8
>>>>    4:   e903 0003       stmdb   r3, {r0, r1}
>>>>    8:   e89d 0009       ldmia.w sp, {r0, r3}
>>>>    c:   4418            add     r0, r3
>>>>    e:   b002            add     sp, #8
>>>>   10:   4770            bx      lr
>>>>
>>>
>>> Ouch. I missed that this sneaked in.
>>>
>>> The complications with the existing mangle functions are that they do
>>> not play well with what I need for the existing
>>> ipipe_restore_root_nosync. I can open-code the latter (size increases),
>>
>> Unless this is part of a heavily used static inline, this increase
>> should be negligible.
>>
>>> extend the former to alternatively return architectural flags (instead
>>> of boolean), or provide another wrapper to convert the virt bit into flags.
>>>
>>> Hmm, or - and that's probably cleanest - I simply align
>>> ipipe_restore_root_nosync to ipipe_restore_root argument-wise. The
>>> latter takes "x" (stall) as boolean, the former as architectural flags.
>>> That's highly confusing anyway. And it seems there are no users to break
>>> in Xenomai, despite that it is exported to modules.
>>>
>>
>> It is exported because it was called from some static inline helper
>> which was part of an obsolete interface, not because client code should
>> use it.
>>
>> The cleanest approach is not to use ipipe_restore_root_nosync() at all.
>> There are only a very few occasions when no syncing the interrupt log
>> ever makes sense, and all are now open-coded to make it clear that we
>> are doing something very unusual.
> 
> Hmm, I still seeing it called by both x86 and ARM.
> 

Yes, but it is wrong. The issue with the interrupt log not being synced
for the root domain prior to returning from a trap.

>>
>> ipipe_restore_root_nosync() is confusing enough that people tend to use
>> it the wrong way, introducing nasty bugs, so I definitely plan to get
>> rid of it.
> 
> Indeed.
> 
> I'm currently re-analyzing x86 in this regard. It is called there after
> every exception - but what if Linux didn't enable IRQs while handling
> the exception and, thus, didn't flush the log. We could have some
> interrupts pending, no?

For the bug to bite, hardware interrupts must be re-enabled when the
root domain is stalled by the ipipe preparation code to fault handling.
In this case, using _nosync to restore the root state is broken, 100%.

On the other hand, if the root domain was stalled as a result of a
kernel code taking a trap while running inside a (virtually) masked
section, the unstall operation will/must happen when execution is back
to that code at some point anyway, flushing the pending IRQs. Userland
over the root domain is in essence not a problem, since this domain
won't be stalled in this case.

> 
> The alternative - already lived in ARM's ipipe_fault_exit today - is
> taking Linux interrupts at that point. If that is always OK is another
> question...
> 

ARM has to follow the same pattern than any other architecture with
respect to this. We should never enable hw interrupts when handling a
trap unless, alternatively:

- the root domain is currently unstalled
- the interrupt log gets flushed when unstalling the root domain we
stalled in the preparation code (i.e. no _nosync).
- the regular linux trap handling codes wants it and therefore unstalls
the root domain in the same move (e.g. local_irq_enable() <- do_page_fault).

Now, as we all mentioned a couple of times already, whether this is
legit to re-enable linux IRQs (virtually) from within the ipipe
preparation code to fault handling essentially depends on the assembly
trampoline code doing the early trap handling. This must be a
per-architecture decision, so a common fault handling pattern should not
assume anything regarding this aspect.

-- 
Philippe.