Re: [musl] ppc64le and 32-bit LE userland compatibility
On Tue, Jun 09, 2020 at 10:29:57AM +, Will Springer wrote:
> On Saturday, May 30, 2020 3:56:47 PM PDT you wrote:
> > On Friday, May 29, 2020 12:24:27 PM PDT Rich Felker wrote:
> > > The argument passing for pread/pwrite is historically a mess and
> > > differs between archs. musl has a dedicated macro that archs can
> > > define to override it. But it looks like it should match regardless of
> > > BE vs LE, and musl already defines it for powerpc with the default
> > > definition, adding a zero arg to start on an even arg-slot index,
> > > which is an odd register (since ppc32 args start with an odd one, r3).
> > >
> > > > [6]:
> > > > https://gist.github.com/Skirmisher/02891c1a8cafa0ff18b2460933ef4f3c
> > >
> > > I don't think this is correct, but I'm confused about where it's
> > > getting messed up because it looks like it should already be right.
> >
> > Hmm, interesting. Will have to go back to it I guess...
> >
> > > > This was enough to fix up the `file` bug. I'm no seasoned kernel
> > > > hacker, though, and there is still concern over the right way to
> > > > approach this, whether it should live in the kernel or libc, etc.
> > > > Frankly, I don't know the ABI structure enough to understand why the
> > > > register padding has to be different in this case, or what
> > > > lower-level component is responsible for it.. For comparison, I had
> > > > a
> > > > look at the mips tree, since it's bi-endian and has a similar 32/64
> > > > situation. There is a macro conditional upon endianness that is
> > > > responsible for munging long longs; it uses __MIPSEB__ and
> > > > __MIPSEL__
> > > > instead of an if/else on the generic __LITTLE_ENDIAN__. Not sure
> > > > what
> > > > to make of that. (It also simply swaps registers for LE, unlike what
> > > > I did for ppc.)
> > >
> > > Indeed the problem is probably that you need to swap registers for LE,
> > > not remove the padding slot. Did you check what happens if you pass a
> > > value larger than 32 bits?
> > >
> > > If so, the right way to fix this on the kernel side would be to
> > > construct the value as a union rather than by bitwise ops so it's
> > >
> > > endian-agnostic:
> > > (union { u32 parts[2]; u64 val; }){{ arg1, arg2 }}.val
> > >
> > > But the kernel folks might prefer endian ifdefs for some odd reason...
> >
> > You are right, this does seem odd considering what the other archs do.
> > It's quite possible I made a silly mistake, of course...
> >
> > I haven't tested with values outside the 32-bit range yet; again, this
> > is new territory for me, so I haven't exactly done exhaustive tests on
> > everything. I'll give it a closer look.
>
> I took some cues from the mips linux32 syscall setup, and drafted a new
> patch defining a macro to compose the hi/lo parts within the function,
> instead of swapping the args at the function definition. `file /bin/bash`
> and `truncate -s 5G test` both work correctly now. This appears to be the
> correct solution, so I'm not sure what silly mistake I made before, but
> apologies for the confusion. I've updated my gist with the new patch [1].
> [...]
>
> [1]: https://gist.github.com/Skirmisher/02891c1a8cafa0ff18b2460933ef4f3c
This patch looks correct. I prefer the union approach with no #ifdef
but I'm fine with either.
Rich
Re: [musl] ppc64le and 32-bit LE userland compatibility
On Saturday, May 30, 2020 3:56:47 PM PDT you wrote:
> On Friday, May 29, 2020 12:24:27 PM PDT Rich Felker wrote:
> > The argument passing for pread/pwrite is historically a mess and
> > differs between archs. musl has a dedicated macro that archs can
> > define to override it. But it looks like it should match regardless of
> > BE vs LE, and musl already defines it for powerpc with the default
> > definition, adding a zero arg to start on an even arg-slot index,
> > which is an odd register (since ppc32 args start with an odd one, r3).
> >
> > > [6]:
> > > https://gist.github.com/Skirmisher/02891c1a8cafa0ff18b2460933ef4f3c
> >
> > I don't think this is correct, but I'm confused about where it's
> > getting messed up because it looks like it should already be right.
>
> Hmm, interesting. Will have to go back to it I guess...
>
> > > This was enough to fix up the `file` bug. I'm no seasoned kernel
> > > hacker, though, and there is still concern over the right way to
> > > approach this, whether it should live in the kernel or libc, etc.
> > > Frankly, I don't know the ABI structure enough to understand why the
> > > register padding has to be different in this case, or what
> > > lower-level component is responsible for it.. For comparison, I had
> > > a
> > > look at the mips tree, since it's bi-endian and has a similar 32/64
> > > situation. There is a macro conditional upon endianness that is
> > > responsible for munging long longs; it uses __MIPSEB__ and
> > > __MIPSEL__
> > > instead of an if/else on the generic __LITTLE_ENDIAN__. Not sure
> > > what
> > > to make of that. (It also simply swaps registers for LE, unlike what
> > > I did for ppc.)
> >
> > Indeed the problem is probably that you need to swap registers for LE,
> > not remove the padding slot. Did you check what happens if you pass a
> > value larger than 32 bits?
> >
> > If so, the right way to fix this on the kernel side would be to
> > construct the value as a union rather than by bitwise ops so it's
> >
> > endian-agnostic:
> > (union { u32 parts[2]; u64 val; }){{ arg1, arg2 }}.val
> >
> > But the kernel folks might prefer endian ifdefs for some odd reason...
>
> You are right, this does seem odd considering what the other archs do.
> It's quite possible I made a silly mistake, of course...
>
> I haven't tested with values outside the 32-bit range yet; again, this
> is new territory for me, so I haven't exactly done exhaustive tests on
> everything. I'll give it a closer look.
I took some cues from the mips linux32 syscall setup, and drafted a new
patch defining a macro to compose the hi/lo parts within the function,
instead of swapping the args at the function definition. `file /bin/bash`
and `truncate -s 5G test` both work correctly now. This appears to be the
correct solution, so I'm not sure what silly mistake I made before, but
apologies for the confusion. I've updated my gist with the new patch [1].
> > > Also worth noting is the one other outstanding bug, where the
> > > time-related syscalls in the 32-bit vDSO seem to return garbage. It
> > > doesn't look like an endian bug to me, and it doesn't affect
> > > standard
> > > syscalls (which is why if you run `date` on musl it prints the
> > > correct time, unlike on glibc). The vDSO time functions are
> > > implemented in ppc asm (arch/powerpc/kernel/vdso32/ gettimeofday.S),
> > > and I've never touched the stuff, so if anyone has a clue I'm all
> > > ears.
> >
> > Not sure about this. Worst-case, just leave it disabled until someone
> > finds a fix.
>
> Apparently these asm implementations are being replaced by the generic C
> ones [1], so it may be this fixes itself on its own.
>
> Thanks,
> Will [she/her]
>
> [1]:
> https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=173231
I mentioned in Christophe's thread the other day, but his patchset does
solve the vdso32 issues, though it introduced problems in vdso64 in my
testing. With that solved and the syscall situation established, I think
the kernel state is stable enough to start looking at solidifying libc/
compiler stuff. I'll try to get a larger userland built in the near future
to try to catch any remaining problems (before rebuilding it all when
libc/ABI support becomes explicit).
Cheers,
Will [she/her]
[1]: https://gist.github.com/Skirmisher/02891c1a8cafa0ff18b2460933ef4f3c
Re: [musl] ppc64le and 32-bit LE userland compatibility
On Friday, May 29, 2020 12:24:27 PM PDT Rich Felker wrote:
> The argument passing for pread/pwrite is historically a mess and
> differs between archs. musl has a dedicated macro that archs can
> define to override it. But it looks like it should match regardless of
> BE vs LE, and musl already defines it for powerpc with the default
> definition, adding a zero arg to start on an even arg-slot index,
> which is an odd register (since ppc32 args start with an odd one, r3).
>
> > [6]:
> > https://gist.github.com/Skirmisher/02891c1a8cafa0ff18b2460933ef4f3c
> I don't think this is correct, but I'm confused about where it's
> getting messed up because it looks like it should already be right.
Hmm, interesting. Will have to go back to it I guess...
> > This was enough to fix up the `file` bug. I'm no seasoned kernel
> > hacker, though, and there is still concern over the right way to
> > approach this, whether it should live in the kernel or libc, etc.
> > Frankly, I don't know the ABI structure enough to understand why the
> > register padding has to be different in this case, or what
> > lower-level component is responsible for it.. For comparison, I had a
> > look at the mips tree, since it's bi-endian and has a similar 32/64
> > situation. There is a macro conditional upon endianness that is
> > responsible for munging long longs; it uses __MIPSEB__ and __MIPSEL__
> > instead of an if/else on the generic __LITTLE_ENDIAN__. Not sure what
> > to make of that. (It also simply swaps registers for LE, unlike what
> > I did for ppc.)
> Indeed the problem is probably that you need to swap registers for LE,
> not remove the padding slot. Did you check what happens if you pass a
> value larger than 32 bits?
>
> If so, the right way to fix this on the kernel side would be to
> construct the value as a union rather than by bitwise ops so it's
> endian-agnostic:
>
> (union { u32 parts[2]; u64 val; }){{ arg1, arg2 }}.val
>
> But the kernel folks might prefer endian ifdefs for some odd reason...
You are right, this does seem odd considering what the other archs do.
It's quite possible I made a silly mistake, of course...
I haven't tested with values outside the 32-bit range yet; again, this is
new territory for me, so I haven't exactly done exhaustive tests on
everything. I'll give it a closer look.
> > Also worth noting is the one other outstanding bug, where the
> > time-related syscalls in the 32-bit vDSO seem to return garbage. It
> > doesn't look like an endian bug to me, and it doesn't affect standard
> > syscalls (which is why if you run `date` on musl it prints the
> > correct time, unlike on glibc). The vDSO time functions are
> > implemented in ppc asm (arch/powerpc/kernel/vdso32/ gettimeofday.S),
> > and I've never touched the stuff, so if anyone has a clue I'm all
> > ears.
> Not sure about this. Worst-case, just leave it disabled until someone
> finds a fix.
Apparently these asm implementations are being replaced by the generic C
ones [1], so it may be this fixes itself on its own.
Thanks,
Will [she/her]
[1]: https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=173231
Re: [musl] ppc64le and 32-bit LE userland compatibility
On Fri, May 29, 2020 at 07:03:48PM +, Will Springer wrote:
> The next problem concerns the ABI more directly. The failure mode was `file`
> surfacing EINVAL from pread64 when invoked on an ELF; pread64 was passed a
> garbage value for `pos`, which didn't appear to be caused by anything in
> `file`. Initially it seemed as though the 32-bit components of the arg were
> getting swapped, and we made hacky fixes to glibc and musl to put them in the
> "right order"; however, we weren't sure if that was the correct approach, or
> if there were knock-on effects we didn't know about. So we found the relevant
> compat code path in the kernel, at arch/powerpc/kernel/sys_ppc32.c, where
> there exists this comment:
>
> > /*
> > * long long munging:
> > * The 32 bit ABI passes long longs in an odd even register pair.
> > */
>
> It seems that the opposite is true in LE mode, and something is expecting long
> longs to start on an even register. I realized this after I tried swapping hi/
> lo `u32`s here and didn't see an improvement. I whipped up a patch [6] that
> switches which syscalls use padding arguments depending on endianness, while
> hopefully remaining tidy enough to be unobtrusive. (I took some liberties with
> variable names/types so that the macro could be consistent.)
The argument passing for pread/pwrite is historically a mess and
differs between archs. musl has a dedicated macro that archs can
define to override it. But it looks like it should match regardless of
BE vs LE, and musl already defines it for powerpc with the default
definition, adding a zero arg to start on an even arg-slot index,
which is an odd register (since ppc32 args start with an odd one, r3).
> [6]: https://gist.github.com/Skirmisher/02891c1a8cafa0ff18b2460933ef4f3c
I don't think this is correct, but I'm confused about where it's
getting messed up because it looks like it should already be right.
> This was enough to fix up the `file` bug. I'm no seasoned kernel hacker,
> though, and there is still concern over the right way to approach this,
> whether it should live in the kernel or libc, etc. Frankly, I don't know the
> ABI structure enough to understand why the register padding has to be
> different in this case, or what lower-level component is responsible for it..
> For comparison, I had a look at the mips tree, since it's bi-endian and has a
> similar 32/64 situation. There is a macro conditional upon endianness that is
> responsible for munging long longs; it uses __MIPSEB__ and __MIPSEL__ instead
> of an if/else on the generic __LITTLE_ENDIAN__. Not sure what to make of
> that.
> (It also simply swaps registers for LE, unlike what I did for ppc.)
Indeed the problem is probably that you need to swap registers for LE,
not remove the padding slot. Did you check what happens if you pass a
value larger than 32 bits?
If so, the right way to fix this on the kernel side would be to
construct the value as a union rather than by bitwise ops so it's
endian-agnostic:
(union { u32 parts[2]; u64 val; }){{ arg1, arg2 }}.val
But the kernel folks might prefer endian ifdefs for some odd reason...
> Also worth noting is the one other outstanding bug, where the time-related
> syscalls in the 32-bit vDSO seem to return garbage. It doesn't look like an
> endian bug to me, and it doesn't affect standard syscalls (which is why if you
> run `date` on musl it prints the correct time, unlike on glibc). The vDSO time
> functions are implemented in ppc asm (arch/powerpc/kernel/vdso32/
> gettimeofday.S), and I've never touched the stuff, so if anyone has a clue
> I'm
> all ears.
Not sure about this. Worst-case, just leave it disabled until someone
finds a fix.
Rich
