Re: [PATCH V3 08/17] riscv: compat: syscall: Add compat_sys_call_table implementation

[Guo Ren]

On Thu, Jan 20, 2022 at 10:43 PM Arnd Bergmann <arnd@xxxxxxxx> wrote:
>
> On Thu, Jan 20, 2022 at 8:39 AM <guoren@xxxxxxxxxx> wrote:
> >
> >  /* The array of function pointers for syscalls. */
> >  extern void * const sys_call_table[];
> > +#ifdef CONFIG_COMPAT
> > +extern void * const compat_sys_call_table[];
> > +#endif
>
> No need for the #ifdef, the normal convention is to just define the
> extern declaration unconditionally for symbols that may or may not be defined.
Okay

>
> > +COMPAT_SYSCALL_DEFINE3(truncate64, const char __user *, pathname,
> > +                      arg_u32p(length))
> > +{
> > +       return ksys_truncate(pathname, arg_u64(length));
> > +}
>
> Are you sure these are the right calling conventions? According to [1],
> I think the 64-bit argument should be in an aligned pair of registers,
> which means you need an extra pad argument as in the arm64 version
> of these functions. Same for ftruncate64, pread64, pwrite64, and
> readahead.

[1] has abandoned.

See:
https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-cc.adoc

Ltp test results:

ftruncate01                                        PASS       0
ftruncate01_64                                     PASS       0
ftruncate03                                        PASS       0
ftruncate03_64                                     PASS       0
ftruncate04                                        CONF       32
ftruncate04_64                                     CONF       32

truncate02                                         PASS       0
truncate02_64                                      PASS       0
truncate03                                         PASS       0
truncate03_64                                      PASS       0

pread01                                            PASS       0
pread01_64                                         PASS       0
pread02                                            PASS       0
pread02_64                                         PASS       0
pread03                                            PASS       0
pread03_64                                         PASS       0

pwrite01_64                                        PASS       0
pwrite02_64                                        PASS       0
pwrite03_64                                        PASS       0
pwrite04_64                                        PASS       0

readahead01                                        PASS       0
readahead02                                        CONF       32


>
> > +COMPAT_SYSCALL_DEFINE3(ftruncate64, unsigned int, fd, arg_u32p(length))
> > +{
> > +       return ksys_ftruncate(fd, arg_u64(length));
> > +}
> > +
> > +COMPAT_SYSCALL_DEFINE6(fallocate, int, fd, int, mode,
> > +                      arg_u32p(offset), arg_u32p(len))
> > +{
> > +       return ksys_fallocate(fd, mode, arg_u64(offset), arg_u64(len));
> > +}
> > +
> > +COMPAT_SYSCALL_DEFINE5(pread64, unsigned int, fd, char __user *, buf,
> > +                      size_t, count, arg_u32p(pos))
> > +{
> > +       return ksys_pread64(fd, buf, count, arg_u64(pos));
> > +}
> > +
> > +COMPAT_SYSCALL_DEFINE5(pwrite64, unsigned int, fd,
> > +                      const char __user *, buf, size_t, count, arg_u32p(pos))
> > +{
> > +       return ksys_pwrite64(fd, buf, count, arg_u64(pos));
> > +}
> > +
> > +COMPAT_SYSCALL_DEFINE6(sync_file_range, int, fd, arg_u32p(offset),
> > +                      arg_u32p(nbytes), unsigned int, flags)
> > +{
> > +       return ksys_sync_file_range(fd, arg_u64(offset), arg_u64(nbytes),
> > +                                   flags);
> > +}
> > +
> > +COMPAT_SYSCALL_DEFINE4(readahead, int, fd, arg_u32p(offset),
> > +                      size_t, count)
> > +{
> > +       return ksys_readahead(fd, arg_u64(offset), count);
> > +}
> > +
> > +COMPAT_SYSCALL_DEFINE6(fadvise64_64, int, fd, int, advice, arg_u32p(offset),
> > +                      arg_u32p(len))
> > +{
> > +       return ksys_fadvise64_64(fd, arg_u64(offset), arg_u64(len), advice);
> > +}
>
> I still feel like these should be the common implementations next to the
> native handlers inside of an #ifdef CONFIG_COMPAT.
>
> The names clash with the custom versions defined for powerpc and sparc,
> but the duplicates look compatible if you can account for the padded
> argument and the lo/hi order of the pairs, so could just be removed here
> (all other architectures use custom function names instead).
I would try it later.

>
>         Arnd
>
> [1] https://riscv.org/wp-content/uploads/2015/01/riscv-calling.pdf



-- 
Best Regards
 Guo Ren

ML: https://lore.kernel.org/linux-csky/