Re: [PATCH 2/2] [AARCH64] ILP32: stat syscalls

[Yury Norov <ynorov at caviumnetworks dot com>]

On Mon, Sep 12, 2016 at 11:43:08AM +0200, Arnd Bergmann wrote:
> On Monday, September 12, 2016 1:17:48 AM CEST Yury Norov wrote:
> > On Fri, Sep 09, 2016 at 09:47:16PM +0200, Arnd Bergmann wrote:
> > > On Friday, September 9, 2016 6:49:59 PM CEST Yury Norov wrote:
> > > >  - struct stat has broken st_ino field (legacy from arm32), so it's
> > > >    handled in corresponding header in a way to make this structure
> > > >    identical to struct stat64.
> > > 
> > > I think this is best, yes.
> > > 
> > > >  - statfs{,64} is identical to lp64, so corresponding syscalls are
> > > >    wired to lp64 handlers in kernel, and implemented in custom files in
> > > >    glibc.
> > > 
> > > During the discussion about stat64, we didn't really get to talk
> > > about statfs64. I didn't expect to see an override here but simply
> > > use the standard definition from
> > > sysdeps/unix/sysv/linux/generic/bits/statfs.h or
> > > sysdeps/unix/sysv/linux/bits/statfs.h together with the 32-bit
> > > syscall emulation in the kernel, as we do for almost all other
> > > syscalls now.
> > > 
> > > What's the motivation for using the aarch64 structure layout
> > > in this case?
> > 
> > I turned __FSWORD_T_TYPE to 64-bit type, as other stat fields.  I think
> > this is correct.
> 
> I don't see why: For the other syscalls, we decided to follow the
> generic/wordsize-32 mode, so why not for this one?
> 
> > With that, struct statfs{,64} is not identical to arm32
> > (taken from sysdeps/unix/sysv/linux/generic/bits/statfs.h) anyway. I take
> > struct statfs from sysdeps/unix/sysv/linux/bits/statfs.h, so it's
> > pretty standard.
> 
> I don't see the difference, can you be more specific? The only thing
> I see is that the "struct statfs" layout in the arm32 case is
> different, but we don't use that as the kernel only has the statfs64
> syscall interface, which is identical.
> 
> > Also notice that statfs syscalls are broken as wrong sizeof(struct
> > stat) is passed, and kernel has wrappers to handle it -
> > compat_sys_statfs64_wrapper and compat_sys_fstatfs64_wrapper.
> 
> I was sure we had this discussion before, and found this in the
> archives: https://lkml.org/lkml/2015/12/23/377
> 
> Did something change after that?
>  
> > By similar reasons I wire sys_getrlimit and sys_setrlimit to lp64.
> 
> I don't see that one making sense either.

It was suggested by Andreas:
https://lkml.org/lkml/2016/7/5/77

> > > >  - XSTAT_IS_XSTAT64 is defined to use the same implementations for
> > > >    syscalls.
> > > >  - STAT_IS_KERNEL_STAT is defined, and _STAT_VER is defined to
> > > >    _STAT_VER_KERNEL for ilp32 to bypass __xstat_conv().
> > > 
> > > Sounds good.
> > > 
> > > >  - __NR_stat, __NR_fstat, __NR_newfstatat and __NR_lstat are redefined
> > > >    to symbols that linux exports.
> > > 
> > > Why is this in the architecture specific part? Isn't this the same
> > > as all "generic" architectures now?
> >   
> > I don't understand it. Some of this defines are needed because arm64
> > takes syscalls from sysdeps/unix/sysv/linux, not from
> > sysdeps/unix/sysv/linux/generic/worsize-32. I don't think it's
> > generic. Though, if it's generic, could you explain it in details, and
> > I'll change it.
> 
> I didn't realize we took syscalls from sysdeps/unix/sysv/linux instead
> of sysdeps/unix/sysv/linux/generic/wordsize-32. Why is that? Changing
> this seems like the easiest fix.

Because syscalls under generic/wordsize-32 use stat_overflow(), and it
fails at compile time as struct stat has no pads:
../sysdeps/unix/sysv/linux/generic/wordsize-32/overflow.h:39:10: error: ‘struct stat’ has no member named      ‘__st_ino_pad’
    if (buf->__st_ino_pad == 0 && buf->__st_size_pad == 0 &&

GCC complains on stat_overflow() even when I try to build
sysdeps/unix/sysv/linux/generic/worsize-32/statfs.c

There's XSTAT_IS_XSTAT64 option to bypass stat_overflow() in stat
syscalls but there's no such option for statfs syscalls. The other
problem I see is that standard syscall files generate different
function bodies for 32- and 64-bit syscalls. And this is the
duplication I'd like to avoid.

So statfs syscalls under generic/wordsize-32:
 - are broken, and should be fixed in kernel. 
 - create duplicated function bodies in binaries.
 - make me do the deep rework for owerflow.h, probably introduce new
   option.
 - limit __FSWORD_T_TYPE, to 32 bit, which is probably makes troubles
   for big filesystems, which may be a case for servers.

That's why I took sysdeps/unix/sysv/linux version. It looks more
natural if I set __FSWORD_T_TYPE to 64-bit: I don't have to change
anything to make things work.

Yury.

> > > > diff --git a/sysdeps/unix/sysv/linux/aarch64/ilp32/fxstat.c b/sysdeps/unix/sysv/linux/aarch64/ilp32/fxstat.c
> > > > new file mode 100644
> > > > index 0000000..6fa13ad
> > > > --- /dev/null
> > > > +++ b/sysdeps/unix/sysv/linux/aarch64/ilp32/fxstat.c
> > > > @@ -0,0 +1 @@
> > > > +#include <sysdeps/unix/sysv/linux/fxstat.c>
> > > 
> > > Why not sysdeps/unix/sysv/linux/generic/xstat.c ?
> > > 
> > > Same question for the other xstat files.
> > 
> > sysdeps/unix/sysv/linux/generic has only lxstat.c and 
> > xstat.c files. Both are taken from
> > sysdeps/unix/sysv/linux/generic/wordsize-32. Other files are
> > taken from sysdeps/unix/sysv/linux. Is my understanding correct
> > that sysdeps/unix/sysv/linux is the default choice for new ports?
> 
> Maybe I'm the one who is misunderstanding it, but I thought the
> opposite was the case: From what I can tell, sysdeps/unix/sysv/linux
> is used on "old-style" architectures like arm32 or x86-32 that
> have the full set of syscalls, while the generic/wordsize-32
> directory is for modern architectures that have a reduced set
> of syscalls e.g. leaving out the 32-bit off_t interfaces.
> 
> 	Arnd