MIME-Version: 1.0
References: <20211221163532.2636028-1-guoren@kernel.org> <20211221163532.2636028-6-guoren@kernel.org>
 <CAK8P3a2XOVYB1Fm5TBdjtKx9DXoG93Zrw7TiquYL_Zy916dLwQ@mail.gmail.com> <CAJF2gTR2fAU=+0fvW_VCqaZfDkSTAxQ=cKE9iAYOoGORb3m+4g@mail.gmail.com>
In-Reply-To: <CAJF2gTR2fAU=+0fvW_VCqaZfDkSTAxQ=cKE9iAYOoGORb3m+4g@mail.gmail.com>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Wed, 22 Dec 2021 14:21:30 +0100
Message-ID: <CAK8P3a17A3MU_atWNEretDcr5sLRc7540tM4vfc=H4M8qVCDEg@mail.gmail.com>
Subject: Re: [PATCH 05/13] riscv: compat: syscall: Add compat_sys_call_table implementation
To:     Guo Ren <guoren@kernel.org>
Cc:     Arnd Bergmann <arnd@arndb.de>, Palmer Dabbelt <palmer@dabbelt.com>,
        Anup Patel <anup.patel@wdc.com>,
        gregkh <gregkh@linuxfoundation.org>,
        liush <liush@allwinnertech.com>, Wei Fu <wefu@redhat.com>,
        Drew Fustini <drew@beagleboard.org>,
        Wang Junqiang <wangjunqiang@iscas.ac.cn>,
        =?UTF-8?B?V2VpIFd1ICjlkLTkvJ8p?= <lazyparser@gmail.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-riscv <linux-riscv@lists.infradead.org>,
        linux-csky@vger.kernel.org, Guo Ren <guoren@linux.alibaba.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Dec 22, 2021 at 1:43 PM Guo Ren <guoren@kernel.org> wrote:
>
> On Wed, Dec 22, 2021 at 2:15 AM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Tue, Dec 21, 2021 at 5:35 PM <guoren@kernel.org> wrote:
> > >
> > > From: Guo Ren <guoren@linux.alibaba.com>
> > >
> > > Implement compat_syscall_table.c with compat_sys_call_table & fixup
> > > system call such as truncate64,pread64,fallocate which need two
> > > regs to indicate 64bit-arg (copied from arm64).
> > >
> > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> > > ---
> > >  arch/riscv/include/asm/syscall.h         |  3 +
> > >  arch/riscv/kernel/compat_syscall_table.c | 84 ++++++++++++++++++++++++
> >
> > Same here, I think most of these should go next to the actual syscalls, with the
> > duplicates removed from other platforms,
> Agree, I will try that next version.
>
> >
> > > +#define __SYSCALL_COMPAT
> > > +#undef __LP64__
> >
> > What is the #undef for?
>
> See arch/riscv/include/uapi/asm/unistd.h:
>
> #ifdef __LP64__
> #define __ARCH_WANT_NEW_STAT
> #define __ARCH_WANT_SET_GET_RLIMIT
> #endif /* __LP64__ */

Ok, in this case I would recommend changing that #ifdef to
check for __SYSCALL_COMPAT instead, as removing the
__LP64__ define may cause other unintended changes.


> > > +SYSCALL_DEFINE6(mmap2, unsigned long, addr, unsigned long, len,
> > > +       unsigned long, prot, unsigned long, flags,
> > > +       unsigned long, fd, unsigned long, offset)
> > > +{
> > > +       if ((prot & PROT_WRITE) && (prot & PROT_EXEC))
> > > +               if (unlikely(!(prot & PROT_READ)))
> > > +                       return -EINVAL;
> > > +
> > > +       return ksys_mmap_pgoff(addr, len, prot, flags, fd, offset);
> > > +}
> >
> > This is one that we may have to deal with separately, introducing
> > sys_mmap_pgoff() was a mistake in my opinion, and we should just have
>
> #if __BITS_PER_LONG == 32 || defined(__SYSCALL_COMPAT)
> #define __SC_3264(_nr, _32, _64) __SYSCALL(_nr, _32)
> #else
> #define __SC_3264(_nr, _32, _64) __SYSCALL(_nr, _64)
> #endif
>
> #define __NR3264_mmap 222
> __SC_3264(__NR3264_mmap, sys_mmap2, sys_mmap)
>
> > added a sys_mmap2() for all architectures that don't explicitly override it.
> That should be another patch, right? Let's keep it here.

Right, I think the patch would be a nice cleanup, but it appears that
riscv is among the few architectures that have defined their own
nonstandard mmap2() syscall after all, despite using the standard
name for the entry point. Not sure how this slipped past my original
review, but it certainly can't be changed now.

It also means that you need to change your implementation of
compat_sys_mmap2() to match the version from
arch/riscv/kernel/sys_riscv.c, rather than the version that
everyone else has. Maybe leave it there and change the
#ifdef to build mmap2 for both native rv32 and compat
mode.

FWIW, this is what a conversion from mmap_pgoff() to mmap2()
would look like, I think this is an overall win, but it's entirely
unrelated to your series now: https://pastebin.com/QtF55dn4
(I'm sure I got some details wrong, at least it needs some
#ifdef checks).

       Arnd