| Message ID | 20220824020548.62625-3-rmclure@linux.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | powerpc: Syscall wrapper and register clearing | expand |
On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote: > The powerpc fallocate compat syscall handler is identical to the > generic implementation provided by commit 59c10c52f573f ("riscv: > compat: syscall: Add compat_sys_call_table implementation"), and as > such can be removed in favour of the generic implementation. > > A future patch series will replace more architecture-defined syscall > handlers with generic implementations, dependent on introducing generic > implementations that are compatible with powerpc and arm's parameter > reorderings. > > Reported-by: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Rohan McLure <rmclure@linux.ibm.com> > --- > V1 -> V2: Remove arch-specific fallocate handler. > V2 -> V3: Remove generic fallocate prototype. Move to beginning of > series. > --- > arch/powerpc/include/asm/compat.h | 5 +++++ > arch/powerpc/include/asm/syscalls.h | 2 -- > arch/powerpc/include/asm/unistd.h | 1 + > 3 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/include/asm/compat.h b/arch/powerpc/include/asm/compat.h > index dda4091fd012..f20caae3f019 100644 > --- a/arch/powerpc/include/asm/compat.h > +++ b/arch/powerpc/include/asm/compat.h > @@ -16,6 +16,11 @@ typedef u16 compat_ipc_pid_t; > #include <asm-generic/compat.h> > > #ifdef __BIG_ENDIAN__ > +#define compat_arg_u64(name) u32 name##_hi, u32 name##_lo > +#define compat_arg_u64_dual(name) u32, name##_hi, u32, name##_lo > +#define compat_arg_u64_glue(name) (((u64)name##_lo & 0xffffffffUL) | \ > + ((u64)name##_hi << 32)) Is there a reason not to put this in asm-generic/compat.h? Possibly you want to put this with the other compat definitions and above the asm-generic include. The generic header expects the arch to include it after defining what it wants to override. Not sure why x_lo gets cast from u32 to u64 and masked before the | there, but generic code does the same so this isn't the place to change it. Thanks, Nick > + > #define COMPAT_UTS_MACHINE "ppc\0\0" > #else > #define COMPAT_UTS_MACHINE "ppcle\0\0" > diff --git a/arch/powerpc/include/asm/syscalls.h b/arch/powerpc/include/asm/syscalls.h > index 21c2faaa2957..675a8f5ec3ca 100644 > --- a/arch/powerpc/include/asm/syscalls.h > +++ b/arch/powerpc/include/asm/syscalls.h > @@ -39,8 +39,6 @@ compat_ssize_t compat_sys_readahead(int fd, u32 r4, u32 offset1, u32 offset2, u3 > int compat_sys_truncate64(const char __user *path, u32 reg4, > unsigned long len1, unsigned long len2); > > -long compat_sys_fallocate(int fd, int mode, u32 offset1, u32 offset2, u32 len1, u32 len2); > - > int compat_sys_ftruncate64(unsigned int fd, u32 reg4, unsigned long len1, > unsigned long len2); > > diff --git a/arch/powerpc/include/asm/unistd.h b/arch/powerpc/include/asm/unistd.h > index b1129b4ef57d..659a996c75aa 100644 > --- a/arch/powerpc/include/asm/unistd.h > +++ b/arch/powerpc/include/asm/unistd.h > @@ -45,6 +45,7 @@ > #define __ARCH_WANT_SYS_UTIME > #define __ARCH_WANT_SYS_NEWFSTATAT > #define __ARCH_WANT_COMPAT_STAT > +#define __ARCH_WANT_COMPAT_FALLOCATE > #define __ARCH_WANT_COMPAT_SYS_SENDFILE > #endif > #define __ARCH_WANT_SYS_FORK > -- > 2.34.1
On Mon, Sep 12, 2022, at 10:38 AM, Nicholas Piggin wrote: > On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote: >> The powerpc fallocate compat syscall handler is identical to the >> generic implementation provided by commit 59c10c52f573f ("riscv: >> compat: syscall: Add compat_sys_call_table implementation"), and as >> such can be removed in favour of the generic implementation. >> >> A future patch series will replace more architecture-defined syscall >> handlers with generic implementations, dependent on introducing generic >> implementations that are compatible with powerpc and arm's parameter >> reorderings. >> >> Reported-by: Arnd Bergmann <arnd@arndb.de> >> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com> >> --- >> V1 -> V2: Remove arch-specific fallocate handler. >> V2 -> V3: Remove generic fallocate prototype. Move to beginning of >> series. >> --- >> @@ -16,6 +16,11 @@ typedef u16 compat_ipc_pid_t; >> #include <asm-generic/compat.h> >> >> #ifdef __BIG_ENDIAN__ >> +#define compat_arg_u64(name) u32 name##_hi, u32 name##_lo >> +#define compat_arg_u64_dual(name) u32, name##_hi, u32, name##_lo >> +#define compat_arg_u64_glue(name) (((u64)name##_lo & 0xffffffffUL) | \ >> + ((u64)name##_hi << 32)) > > Is there a reason not to put this in asm-generic/compat.h? > > Possibly you want to put this with the other compat definitions and > above the asm-generic include. The generic header expects the arch to > include it after defining what it wants to override. Yes, makes sense. I think the riscv people added this to asm-generic, they tried to do only the minimal parts. In theory, any architecture could have its own calling conventions for each syscall and have them in the opposite order for one endianess. I checked the seven non-generic implementations of the sys_fallocate() syscall and all except powerpc have the same ABI as the generic one. The powerpc difference is that in little-endian mode, only the 'len' argument is swapped but the 'offset' argument is still high/low: long compat_sys_fallocate(int fd, int mode, u32 offset1, u32 offset2, u32 len1, u32 len2) { return ksys_fallocate(fd, mode, ((loff_t)offset1 << 32) | offset2, merge_64(len1, len2)); } It's probably best to first fix this by using merge_64(offset1, offset2) and allow that patch to be backported to stable kernels, before changing it over to the generic code in a separate patch within that series. A related issue seems to exist in ppc_fadvise64_64(), which uses the wrong argument order on ppc32le compat tasks, in addition to having at least three different calling conventions across architectures. Arnd
Le 12/09/2022 à 11:57, Arnd Bergmann a écrit : > On Mon, Sep 12, 2022, at 10:38 AM, Nicholas Piggin wrote: >> On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote: >>> The powerpc fallocate compat syscall handler is identical to the >>> generic implementation provided by commit 59c10c52f573f ("riscv: >>> compat: syscall: Add compat_sys_call_table implementation"), and as >>> such can be removed in favour of the generic implementation. >>> >>> A future patch series will replace more architecture-defined syscall >>> handlers with generic implementations, dependent on introducing generic >>> implementations that are compatible with powerpc and arm's parameter >>> reorderings. >>> >>> Reported-by: Arnd Bergmann <arnd@arndb.de> >>> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com> >>> --- >>> V1 -> V2: Remove arch-specific fallocate handler. >>> V2 -> V3: Remove generic fallocate prototype. Move to beginning of >>> series. >>> --- > >>> @@ -16,6 +16,11 @@ typedef u16 compat_ipc_pid_t; >>> #include <asm-generic/compat.h> >>> >>> #ifdef __BIG_ENDIAN__ >>> +#define compat_arg_u64(name) u32 name##_hi, u32 name##_lo >>> +#define compat_arg_u64_dual(name) u32, name##_hi, u32, name##_lo >>> +#define compat_arg_u64_glue(name) (((u64)name##_lo & 0xffffffffUL) | \ >>> + ((u64)name##_hi << 32)) >> >> Is there a reason not to put this in asm-generic/compat.h? >> >> Possibly you want to put this with the other compat definitions and >> above the asm-generic include. The generic header expects the arch to >> include it after defining what it wants to override. > > Yes, makes sense. I think the riscv people added this to asm-generic, > they tried to do only the minimal parts. > > In theory, any architecture could have its own calling conventions > for each syscall and have them in the opposite order for one > endianess. I checked the seven non-generic implementations of the > sys_fallocate() syscall and all except powerpc have the same > ABI as the generic one. > > The powerpc difference is that in little-endian mode, only > the 'len' argument is swapped but the 'offset' argument is > still high/low: > > long compat_sys_fallocate(int fd, int mode, u32 offset1, u32 offset2, > u32 len1, u32 len2) > { > return ksys_fallocate(fd, mode, ((loff_t)offset1 << 32) | offset2, > merge_64(len1, len2)); > } > > It's probably best to first fix this by using merge_64(offset1, > offset2) and allow that patch to be backported to stable kernels, > before changing it over to the generic code in a separate patch > within that series. > > A related issue seems to exist in ppc_fadvise64_64(), which > uses the wrong argument order on ppc32le compat tasks, in addition > to having at least three different calling conventions across > architectures. Do ppc32le exist at all ? Native ppc32 is be only, and I'm not aware that ppc64 is able to run ppc32le compat tasks. Christophe
On Mon, Sep 12, 2022, at 1:00 PM, Christophe Leroy wrote: > Le 12/09/2022 à 11:57, Arnd Bergmann a écrit : >> On Mon, Sep 12, 2022, at 10:38 AM, Nicholas Piggin wrote: >> >> The powerpc difference is that in little-endian mode, only >> the 'len' argument is swapped but the 'offset' argument is >> still high/low: >> >> long compat_sys_fallocate(int fd, int mode, u32 offset1, u32 offset2, >> u32 len1, u32 len2) >> { >> return ksys_fallocate(fd, mode, ((loff_t)offset1 << 32) | offset2, >> merge_64(len1, len2)); >> } >> >> It's probably best to first fix this by using merge_64(offset1, >> offset2) and allow that patch to be backported to stable kernels, >> before changing it over to the generic code in a separate patch >> within that series. >> >> A related issue seems to exist in ppc_fadvise64_64(), which >> uses the wrong argument order on ppc32le compat tasks, in addition >> to having at least three different calling conventions across >> architectures. > > Do ppc32le exist at all ? > > Native ppc32 is be only, and I'm not aware that ppc64 is able to run > ppc32le compat tasks. I'm not aware of anyone using it, but commit 6e944aed8859 ("powerpc/64: Make COMPAT user-selectable disabled on littleendian by default.") added support to the kernel, and commit 57f48b4b74e7 ("powerpc/compat_sys: swap hi/lo parts of 64-bit syscall args in LE mode") changed the compat syscall helpers to pass 64-bit arguments correctly but apparently got fallocate() and fadvise64_64() wrong. With Rohan's series, we use generic implementation, which is the sensible ABI but the change is technically an ABI change for ppc32le, and it makes sense to split the change that fixes the behavior from the cleanup. Arnd
diff --git a/arch/powerpc/include/asm/compat.h b/arch/powerpc/include/asm/compat.h index dda4091fd012..f20caae3f019 100644 --- a/arch/powerpc/include/asm/compat.h +++ b/arch/powerpc/include/asm/compat.h @@ -16,6 +16,11 @@ typedef u16 compat_ipc_pid_t; #include <asm-generic/compat.h> #ifdef __BIG_ENDIAN__ +#define compat_arg_u64(name) u32 name##_hi, u32 name##_lo +#define compat_arg_u64_dual(name) u32, name##_hi, u32, name##_lo +#define compat_arg_u64_glue(name) (((u64)name##_lo & 0xffffffffUL) | \ + ((u64)name##_hi << 32)) + #define COMPAT_UTS_MACHINE "ppc\0\0" #else #define COMPAT_UTS_MACHINE "ppcle\0\0" diff --git a/arch/powerpc/include/asm/syscalls.h b/arch/powerpc/include/asm/syscalls.h index 21c2faaa2957..675a8f5ec3ca 100644 --- a/arch/powerpc/include/asm/syscalls.h +++ b/arch/powerpc/include/asm/syscalls.h @@ -39,8 +39,6 @@ compat_ssize_t compat_sys_readahead(int fd, u32 r4, u32 offset1, u32 offset2, u3 int compat_sys_truncate64(const char __user *path, u32 reg4, unsigned long len1, unsigned long len2); -long compat_sys_fallocate(int fd, int mode, u32 offset1, u32 offset2, u32 len1, u32 len2); - int compat_sys_ftruncate64(unsigned int fd, u32 reg4, unsigned long len1, unsigned long len2); diff --git a/arch/powerpc/include/asm/unistd.h b/arch/powerpc/include/asm/unistd.h index b1129b4ef57d..659a996c75aa 100644 --- a/arch/powerpc/include/asm/unistd.h +++ b/arch/powerpc/include/asm/unistd.h @@ -45,6 +45,7 @@ #define __ARCH_WANT_SYS_UTIME #define __ARCH_WANT_SYS_NEWFSTATAT #define __ARCH_WANT_COMPAT_STAT +#define __ARCH_WANT_COMPAT_FALLOCATE #define __ARCH_WANT_COMPAT_SYS_SENDFILE #endif #define __ARCH_WANT_SYS_FORK
The powerpc fallocate compat syscall handler is identical to the generic implementation provided by commit 59c10c52f573f ("riscv: compat: syscall: Add compat_sys_call_table implementation"), and as such can be removed in favour of the generic implementation. A future patch series will replace more architecture-defined syscall handlers with generic implementations, dependent on introducing generic implementations that are compatible with powerpc and arm's parameter reorderings. Reported-by: Arnd Bergmann <arnd@arndb.de> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com> --- V1 -> V2: Remove arch-specific fallocate handler. V2 -> V3: Remove generic fallocate prototype. Move to beginning of series. --- arch/powerpc/include/asm/compat.h | 5 +++++ arch/powerpc/include/asm/syscalls.h | 2 -- arch/powerpc/include/asm/unistd.h | 1 + 3 files changed, 6 insertions(+), 2 deletions(-)