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

[Yury Norov <ynorov at caviumnetworks dot com>]

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:
> > Implementation details:
> >  - struct stat, stat64, statfs, statfs64 layouts are taken from
> >    sysdeps/unix/sysv/linux/aarch64/bits.
> 
> I was confused by the description for a moment. To clarify: 
> the layout is added in that directory, not taken from an
> existing file there.

It should be sysdeps/unix/sysv/linux/bits of course. Thanks for catch.

> >  - 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. 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.

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.

So for me it's more logical and correct to wire this syscalls to lp64.
By similar reasons I wire sys_getrlimit and sys_setrlimit to lp64.

> >  - 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.

> > 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?

Yury