R2300 (not the hay baler)

R2300 (not the hay baler)

[Paul Burton]

Hello,

Does anyone still care about the R2300? I ask because I'm working on
the FP context switching code & noticed that I'm pretty sure the
fpu_save_single & fpu_restore_single macros used only from
r2300_switch.S are broken. They store each 32 bit value at the start
of the location of the 64 bit FP registers context in memory, which I
believe:

1) Won't work for odd indexed FP registers with the FPU emulator,
    ptrace or other code which assumes that 32 bit FP data is held in
    the even-indexed 64 bit FP register context.

2) On big endian systems the 32 bit values will get saved to the most
    significant bits of the 64 bit context which I imagine will cause
    yet more problems.

It seems like the only changes to r2300_switch.S for a *long* time have
been to keep it in sync with r4k_switch.S & the CPU is old enough that
all I get when I google for it is information about some hay baler.

In short: does anyone care if I just submit a patch removing the R2300
code instead of blindly attempting to fix it up?

Thanks,
     Paul

Re: R2300 (not the hay baler)

[Ralf Baechle]

On Tue, Nov 19, 2013 at 11:07:22AM +0000, Paul Burton wrote:

> Does anyone still care about the R2300? I ask because I'm working on
> the FP context switching code & noticed that I'm pretty sure the
> fpu_save_single & fpu_restore_single macros used only from
> r2300_switch.S are broken. They store each 32 bit value at the start
> of the location of the 64 bit FP registers context in memory, which I
> believe:
> 
> 1) Won't work for odd indexed FP registers with the FPU emulator,
>    ptrace or other code which assumes that 32 bit FP data is held in
>    the even-indexed 64 bit FP register context.

Note that much of that code has changed for 3.13 and the new code may or
may not have inherited this bug.

> 2) On big endian systems the 32 bit values will get saved to the most
>    significant bits of the 64 bit context which I imagine will cause
>    yet more problems.
> 
> It seems like the only changes to r2300_switch.S for a *long* time have
> been to keep it in sync with r4k_switch.S & the CPU is old enough that
> all I get when I google for it is information about some hay baler.
> 
> In short: does anyone care if I just submit a patch removing the R2300
> code instead of blindly attempting to fix it up?

Linux/MIPS is a product of the post-R3000 era - but Maciej (on cc) is doing
his best to keep it alive and going on DECstations, including R2000 and
R3000 based ones.  DECstations are little endian and all of them have a
R2010 rsp R3010, that is have hardware floating point.

I myself have an R3000-based workstation, a clone of a MIPS RS3230 (I think)
sitting on my floor and waiting to be reactivated.  It's still running
RISC/os.

  Ralf

Re: R2300 (not the hay baler)

[Paul Burton]

On 19/11/13 11:21, Ralf Baechle wrote:
> On Tue, Nov 19, 2013 at 11:07:22AM +0000, Paul Burton wrote:
>
>> Does anyone still care about the R2300? I ask because I'm working on
>> the FP context switching code & noticed that I'm pretty sure the
>> fpu_save_single & fpu_restore_single macros used only from
>> r2300_switch.S are broken. They store each 32 bit value at the start
>> of the location of the 64 bit FP registers context in memory, which I
>> believe:
>>
>> 1) Won't work for odd indexed FP registers with the FPU emulator,
>>     ptrace or other code which assumes that 32 bit FP data is held in
>>     the even-indexed 64 bit FP register context.
>
> Note that much of that code has changed for 3.13 and the new code may or
> may not have inherited this bug.
>

May I ask which code you mean? Is there some FP work not in mips-for-
linux-next or on the mailing list?

>> 2) On big endian systems the 32 bit values will get saved to the most
>>     significant bits of the 64 bit context which I imagine will cause
>>     yet more problems.
>>
>> It seems like the only changes to r2300_switch.S for a *long* time have
>> been to keep it in sync with r4k_switch.S & the CPU is old enough that
>> all I get when I google for it is information about some hay baler.
>>
>> In short: does anyone care if I just submit a patch removing the R2300
>> code instead of blindly attempting to fix it up?
>
> Linux/MIPS is a product of the post-R3000 era - but Maciej (on cc) is doing
> his best to keep it alive and going on DECstations, including R2000 and
> R3000 based ones.  DECstations are little endian and all of them have a
> R2010 rsp R3010, that is have hardware floating point.
>
> I myself have an R3000-based workstation, a clone of a MIPS RS3230 (I think)
> sitting on my floor and waiting to be reactivated.  It's still running
> RISC/os.
>
>    Ralf
>

Fair enough, I'll leave the r2300_switch.S & fpu_{save,restore}_single
code alone (apart from changes which will leave the bug as-is).

Thanks,
     Paul

Re: R2300 (not the hay baler)

[Maciej W. Rozycki]

On Tue, 19 Nov 2013, Paul Burton wrote:

> Does anyone still care about the R2300? I ask because I'm working on
> the FP context switching code & noticed that I'm pretty sure the
> fpu_save_single & fpu_restore_single macros used only from
> r2300_switch.S are broken. They store each 32 bit value at the start
> of the location of the 64 bit FP registers context in memory, which I
> believe:
> 
> 1) Won't work for odd indexed FP registers with the FPU emulator,
>    ptrace or other code which assumes that 32 bit FP data is held in
>    the even-indexed 64 bit FP register context.
> 
> 2) On big endian systems the 32 bit values will get saved to the most
>    significant bits of the 64 bit context which I imagine will cause
>    yet more problems.
> 
> It seems like the only changes to r2300_switch.S for a *long* time have
> been to keep it in sync with r4k_switch.S & the CPU is old enough that
> all I get when I google for it is information about some hay baler.
> 
> In short: does anyone care if I just submit a patch removing the R2300
> code instead of blindly attempting to fix it up?

 Well, it worked the last time I tried (a couple of weeks ago) with actual 
hardware (an R3400 integrated CPU/FPU), though maybe I missed something.  
There hasn't been a lot of R2000/R3000-class hardware development recently 
so no surprise our code didn't need any changes to match hardware updates.  
At this point I see no reason to retire this code, there's nothing wrong 
with it.  If there's an actual bug, then it should be fixed.  A test case 
should be easy to make, and then we can start from there.

 If you are concerned about register layout in ptrace packets, then please 
see mips_read_fp_register_single and mips_read_fp_register_double in GDB 
sources and the comment above them; notice the register buffer offset of 4 
applied in the big-endian case -- what r2300_switch.S does is exactly what 
the userland expects (of course it might be that r4k_switch.S is wrong in 
some cases; actually I remember a discussion with Ralf where we came to 
this very conclusion and rather than converting r4k_switch.S to use 
LWC1/SWC1 -- that would degrade performance a bit for FP context switches 
-- considered a helper to convert between the internal and the ptrace 
format).

  Maciej

Re: R2300 (not the hay baler)

[Paul Burton]

On 19/11/13 12:27, Maciej W. Rozycki wrote:
> On Tue, 19 Nov 2013, Paul Burton wrote:
>
>> Does anyone still care about the R2300? I ask because I'm working on
>> the FP context switching code & noticed that I'm pretty sure the
>> fpu_save_single & fpu_restore_single macros used only from
>> r2300_switch.S are broken. They store each 32 bit value at the start
>> of the location of the 64 bit FP registers context in memory, which I
>> believe:
>>
>> 1) Won't work for odd indexed FP registers with the FPU emulator,
>>     ptrace or other code which assumes that 32 bit FP data is held in
>>     the even-indexed 64 bit FP register context.
>>
>> 2) On big endian systems the 32 bit values will get saved to the most
>>     significant bits of the 64 bit context which I imagine will cause
>>     yet more problems.
>>
>> It seems like the only changes to r2300_switch.S for a *long* time have
>> been to keep it in sync with r4k_switch.S & the CPU is old enough that
>> all I get when I google for it is information about some hay baler.
>>
>> In short: does anyone care if I just submit a patch removing the R2300
>> code instead of blindly attempting to fix it up?
>
>   Well, it worked the last time I tried (a couple of weeks ago) with actual
> hardware (an R3400 integrated CPU/FPU), though maybe I missed something.
> There hasn't been a lot of R2000/R3000-class hardware development recently
> so no surprise our code didn't need any changes to match hardware updates.
> At this point I see no reason to retire this code, there's nothing wrong
> with it.  If there's an actual bug, then it should be fixed.  A test case
> should be easy to make, and then we can start from there.
>

Yup that's fine, I'm not trying to scrap working code I just didn't
realise this code was still in use (which is why I asked about it). I
saw that the r2300_switch.S code seems to differ from the r4k_switch.S
code in its storage of 32 bit FP context & assumed that the older code
was less used & thus likely the incorrect one. It seems that assumption
was incorrect given the ABI expected by userland which you point out
below.

This does differ from the context layout the FPU emulator expects, but
I suppose that's not an issue.

>   If you are concerned about register layout in ptrace packets, then please
> see mips_read_fp_register_single and mips_read_fp_register_double in GDB
> sources and the comment above them; notice the register buffer offset of 4
> applied in the big-endian case -- what r2300_switch.S does is exactly what
> the userland expects (of course it might be that r4k_switch.S is wrong in
> some cases; actually I remember a discussion with Ralf where we came to
> this very conclusion and rather than converting r4k_switch.S to use
> LWC1/SWC1 -- that would degrade performance a bit for FP context switches
> -- considered a helper to convert between the internal and the ptrace
> format).
>
>    Maciej
>

Do you know what happened to that or have a link to that discussion? I
don't see that conversion being done at the moment, which makes me
suspect that the kernel might handle ptrace incorrectly (arguably
more nicely, but still incorrectly) for mips32 tasks with FR=0 on an
R4K class CPU. I'll have a look.

Thanks,
     Paul

Re: R2300 (not the hay baler)

[Maciej W. Rozycki]

On Tue, 19 Nov 2013, Paul Burton wrote:

> >   Well, it worked the last time I tried (a couple of weeks ago) with actual
> > hardware (an R3400 integrated CPU/FPU), though maybe I missed something.
> > There hasn't been a lot of R2000/R3000-class hardware development recently
> > so no surprise our code didn't need any changes to match hardware updates.
> > At this point I see no reason to retire this code, there's nothing wrong
> > with it.  If there's an actual bug, then it should be fixed.  A test case
> > should be easy to make, and then we can start from there.
> > 
> 
> Yup that's fine, I'm not trying to scrap working code I just didn't
> realise this code was still in use (which is why I asked about it). I
> saw that the r2300_switch.S code seems to differ from the r4k_switch.S
> code in its storage of 32 bit FP context & assumed that the older code
> was less used & thus likely the incorrect one. It seems that assumption
> was incorrect given the ABI expected by userland which you point out
> below.
> 
> This does differ from the context layout the FPU emulator expects, but
> I suppose that's not an issue.

 I reckon the emulator had similar appropriate conditionals (in helper 
macros or suchlike) to handle both cases, I hope they haven't gone by 
accident (or I haven't got MIPS FP emulators mixed up in my memory, there 
are so many of them...).

> >   If you are concerned about register layout in ptrace packets, then please
> > see mips_read_fp_register_single and mips_read_fp_register_double in GDB
> > sources and the comment above them; notice the register buffer offset of 4
> > applied in the big-endian case -- what r2300_switch.S does is exactly what
> > the userland expects (of course it might be that r4k_switch.S is wrong in
> > some cases; actually I remember a discussion with Ralf where we came to
> > this very conclusion and rather than converting r4k_switch.S to use
> > LWC1/SWC1 -- that would degrade performance a bit for FP context switches
> > -- considered a helper to convert between the internal and the ptrace
> > format).
> 
> Do you know what happened to that or have a link to that discussion? I
> don't see that conversion being done at the moment, which makes me
> suspect that the kernel might handle ptrace incorrectly (arguably
> more nicely, but still incorrectly) for mips32 tasks with FR=0 on an
> R4K class CPU. I'll have a look.

 I think the discussion was off-list (Ralf, would you mind if I digged up 
any clues from there?).  The format has been set long ago, and is also odd 
enough to have 32 64-bit slots in the PTRACE_GETFPREGS/PTRACE_SETFPREGS 
structure even for o32 processes (that now should be unexpectedly helpful 
for FP64 o32 processes though), so there's little sense discussing its 
prettiness or ugliness at this point in the game.

 Also I'm not sure what the core file format is for the FP context, it may 
be worth double-checking too.

 Please feel free to poke me directly if you have any further issues about 
MIPS I ISA compatibility.

  Maciej

Re: R2300 (not the hay baler)

[Ralf Baechle]

On Thu, Nov 21, 2013 at 07:52:04PM +0000, Maciej W. Rozycki wrote:

>  I think the discussion was off-list (Ralf, would you mind if I digged up 
> any clues from there?).

Please go ahead.

>  The format has been set long ago, and is also odd 
> enough to have 32 64-bit slots in the PTRACE_GETFPREGS/PTRACE_SETFPREGS 
> structure even for o32 processes (that now should be unexpectedly helpful 
> for FP64 o32 processes though), so there's little sense discussing its 
> prettiness or ugliness at this point in the game.
> 
>  Also I'm not sure what the core file format is for the FP context, it may 
> be worth double-checking too.

Is there some test suite for that kind of stuff?

>  Please feel free to poke me directly if you have any further issues about 
> MIPS I ISA compatibility.

Makes me wonder if there's a MIPS emulator that emulates a MIPS I system
faithfully.

  Ralf

Re: R2300 (not the hay baler)

[Maciej W. Rozycki]

On Thu, 21 Nov 2013, Ralf Baechle wrote:

> >  Also I'm not sure what the core file format is for the FP context, it may 
> > be worth double-checking too.
> 
> Is there some test suite for that kind of stuff?

 GDB has some core file support coverage; I'm not sure if it's meticulous 
enough to catch such issues though, and also if it checks OS-produced 
cores or ones made with its own `generate-core-file' (aka `gcore') command 
only.  Besides if ptrace(2) and the core dumper are consistent with each 
other, then chances are that any bugs there cancel each other too.

> >  Please feel free to poke me directly if you have any further issues about 
> > MIPS I ISA compatibility.
> 
> Makes me wonder if there's a MIPS emulator that emulates a MIPS I system
> faithfully.

 The Algorithmics FPU emulator (the original version shipped with their 
SDE kit, not the kernel version we got contributed) could have been 
strapped for true MIPS I support I believe.  Also GNU sim might be able to 
emulate a whole MIPS I CPU, although I haven't checked.  Other than that I 
haven't seen any (QEMU does not).

  Maciej

Re: R2300 (not the hay baler)

[Paul Burton]

On 21/11/13 19:52, Maciej W. Rozycki wrote:
>>>   If you are concerned about register layout in ptrace packets, then please
>>> see mips_read_fp_register_single and mips_read_fp_register_double in GDB
>>> sources and the comment above them; notice the register buffer offset of 4
>>> applied in the big-endian case -- what r2300_switch.S does is exactly what
>>> the userland expects (of course it might be that r4k_switch.S is wrong in
>>> some cases; actually I remember a discussion with Ralf where we came to
>>> this very conclusion and rather than converting r4k_switch.S to use
>>> LWC1/SWC1 -- that would degrade performance a bit for FP context switches
>>> -- considered a helper to convert between the internal and the ptrace
>>> format).
>>
>> Do you know what happened to that or have a link to that discussion? I
>> don't see that conversion being done at the moment, which makes me
>> suspect that the kernel might handle ptrace incorrectly (arguably
>> more nicely, but still incorrectly) for mips32 tasks with FR=0 on an
>> R4K class CPU. I'll have a look.
> 
>  I think the discussion was off-list (Ralf, would you mind if I digged up 
> any clues from there?).  The format has been set long ago, and is also odd 
> enough to have 32 64-bit slots in the PTRACE_GETFPREGS/PTRACE_SETFPREGS 
> structure even for o32 processes (that now should be unexpectedly helpful 
> for FP64 o32 processes though), so there's little sense discussing its 
> prettiness or ugliness at this point in the game.
> 
>  Also I'm not sure what the core file format is for the FP context, it may 
> be worth double-checking too.
> 
>  Please feel free to poke me directly if you have any further issues about 
> MIPS I ISA compatibility.

Ok I finally had time to look at this. It seems that r2300_switch.S used
to match the current behaviour of r4k_switch.S. Ralf made it that way by
saving to the appropriate 32 bits of the even numbered 64 bit values of
the FP context, taking endianness into account, in the following commit:

http://git.linux-mips.org/?p=ralf/linux.git;a=commitdiff;h=42533948caacb82574ccf91cae84df851d4f0521#patch28

...and then you fixed up ptrace to always expect values stored in the
format now used by r4k_switch.S (& at the time used by r2300_switch.S too):

http://git.linux-mips.org/?p=ralf/linux.git;a=commitdiff;h=849fa7a50dff104cbf6654c421b666eefd6da0c1;hp=364e85467c9c08c803087c5b75ae2e70540e3bb5

Unfortunately later when Ralf replaced the FPU_SAVE_SINGLE macro with
the fpu_save_single macro in this commit:

http://git.linux-mips.org/?p=ralf/linux.git;a=commitdiff;h=bf0b3bb876115b1e69b2266477128d8270d0b356;hp=39507fed032849b72552062883d143025be8be36

...he effectively reverted r2300_switch.S to its old behaviour, whilst
ptrace continues to expect the r4k_switch.S-like behaviour. So as far as
I can tell the original intended FP register layout was that currently
used by r4k_switch.S. That makes r2300_switch.S the incorrect one -
fixed 11 years ago & broken again 10 years ago.

What I'm less sure about right now is what gdb has come to expect in the
meantime - but from your description it sounds like it expects the
r2300_switch.S behaviour? In which case I suspect that although it seems
the original intended ptrace ABI was broken long ago & the easiest fix
may be for the kernel to just go with the unintended ABI on r4k-class
cores too? I'll have a read through more gdb code & try to confirm.

Thanks,
    Paul

Re: MASS MAILING: Re: R2300 (not the hay baler)

[Paul Burton]

On 27/11/13 17:11, Paul Burton wrote:
> On 21/11/13 19:52, Maciej W. Rozycki wrote:
>>  I think the discussion was off-list (Ralf, would you mind if I digged up 
>> any clues from there?).  The format has been set long ago, and is also odd 
>> enough to have 32 64-bit slots in the PTRACE_GETFPREGS/PTRACE_SETFPREGS 
>> structure even for o32 processes (that now should be unexpectedly helpful 
>> for FP64 o32 processes though), so there's little sense discussing its 
>> prettiness or ugliness at this point in the game.
>>
>>  Also I'm not sure what the core file format is for the FP context, it may 
>> be worth double-checking too.
>>
>>  Please feel free to poke me directly if you have any further issues about 
>> MIPS I ISA compatibility.
> 
> Ok I finally had time to look at this. It seems that r2300_switch.S used
> to match the current behaviour of r4k_switch.S. Ralf made it that way by
> saving to the appropriate 32 bits of the even numbered 64 bit values of
> the FP context, taking endianness into account, in the following commit:
> 
> http://git.linux-mips.org/?p=ralf/linux.git;a=commitdiff;h=42533948caacb82574ccf91cae84df851d4f0521#patch28
> 
> ...and then you fixed up ptrace to always expect values stored in the
> format now used by r4k_switch.S (& at the time used by r2300_switch.S too):
> 
> http://git.linux-mips.org/?p=ralf/linux.git;a=commitdiff;h=849fa7a50dff104cbf6654c421b666eefd6da0c1;hp=364e85467c9c08c803087c5b75ae2e70540e3bb5
> 
> Unfortunately later when Ralf replaced the FPU_SAVE_SINGLE macro with
> the fpu_save_single macro in this commit:
> 
> http://git.linux-mips.org/?p=ralf/linux.git;a=commitdiff;h=bf0b3bb876115b1e69b2266477128d8270d0b356;hp=39507fed032849b72552062883d143025be8be36
> 
> ...he effectively reverted r2300_switch.S to its old behaviour, whilst
> ptrace continues to expect the r4k_switch.S-like behaviour. So as far as
> I can tell the original intended FP register layout was that currently
> used by r4k_switch.S. That makes r2300_switch.S the incorrect one -
> fixed 11 years ago & broken again 10 years ago.
> 
> What I'm less sure about right now is what gdb has come to expect in the
> meantime - but from your description it sounds like it expects the
> r2300_switch.S behaviour? In which case I suspect that although it seems
> the original intended ptrace ABI was broken long ago & the easiest fix
> may be for the kernel to just go with the unintended ABI on r4k-class
> cores too? I'll have a read through more gdb code & try to confirm.

Maciej: are you sure this is working correctly with r2300_switch.S? gdb
seems to be working as I'd expect with r4k_switch.S and I have no
r2k/r3k hardware to test on.

Thanks,
    Paul

Re: R2300 (not the hay baler)

[Ralf Baechle]

On Tue, Nov 19, 2013 at 11:07:22AM +0000, Paul Burton wrote:
> Date:   Tue, 19 Nov 2013 11:07:22 +0000
> From: Paul Burton <paul.burton@imgtec.com>
> To: "linux-mips@linux-mips.org" <linux-mips@linux-mips.org>
> Subject: R2300 (not the hay baler)

:-) on the hay baler btw.

Of course this thread is only going to make google think that linux-mips.org
is into R2300 hay balers ;-)

  Ralf