Re: [PATCH] perf arm64: Fix mksyscalltbl, don't lose syscalls due to sort -nu

[Arnd Bergmann]

On Mon, Dec 12, 2022, at 11:52, Leo Yan wrote:
> Hi Arnd,
>
> On Fri, Nov 25, 2022 at 02:56:31PM +0100, Arnd Bergmann wrote:
>> On Fri, Nov 25, 2022, at 13:54, Leo Yan wrote:
>> > On Fri, Nov 25, 2022 at 12:53:10PM +0100, Vincent Whitchurch wrote:
>> 
>> >> It looks like this patch was never applied?  AFAICS it is still needed
>> >> on current HEAD and it still applies cleanly.
>> >
>> > Thanks a lot for bringing up this.
>> >
>> > Before there have a discussion [1] for refactoring Arm64 system call
>> > table but it didn't really happen.
>> 
>> I actually worked on this last week and did a new series to convert
>> the old asm-generic/unistd.h header into the syscall.tbl format,
>> and change arm64 to use that.
>> 
>> You can find my work in the 'syscall-tbl' branch of my asm-generic
>> tree [1]. This has only seen light build testing so far, and is
>> probably still buggy, but most of the work is there. The missing
>> bits are the Makefiles for the other seven architectures using
>> asm-generic/unistd.h, and checking the output to ensure the
>> contents are still the same.
>
> Thanks a lot for sharing the patch set.
>
> I went through the whole patch set, below are several things I observed:

Thanks for the review!

> - I did a quick compilation but found building failure, this failure
>   is caused by the first patch "arm64: convert unistd_32.h to
>   syscall.tbl format", it removes the macro __NR_compat_syscalls.
>
>   In file included from ./include/vdso/const.h:5,
>                  from ./include/linux/const.h:4,
>                  from ./arch/arm64/include/asm/alternative-macros.h:5,
>                  from ./arch/arm64/include/asm/alternative.h:5,
>                  from ./arch/arm64/include/asm/lse.h:15,
>                  from ./arch/arm64/include/asm/cmpxchg.h:14,
>                  from ./arch/arm64/include/asm/atomic.h:16,
>                  from ./include/linux/atomic.h:7,
>                  from ./include/linux/refcount.h:95,
>                  from kernel/seccomp.c:18:
>   ./arch/arm64/include/asm/seccomp.h:27:33: error: 
> ‘__NR_compat_syscalls’ undeclared here (not in a function); did you 
> mean ‘in_compat_syscall’?
>      27 | # define SECCOMP_ARCH_COMPAT_NR __NR_compat_syscalls
>         |                                 ^~~~~~~~~~~~~~~~~~~~
>   ./include/uapi/linux/const.h:34:40: note: in definition of macro 
> ‘__KERNEL_DIV_ROUND_UP’
>      34 | #define __KERNEL_DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
>         |                                        ^
>   ./include/linux/types.h:11:28: note: in expansion of macro 
> ‘BITS_TO_LONGS’
>      11 |         unsigned long name[BITS_TO_LONGS(bits)]
>         |                            ^~~~~~~~~~~~~
>   kernel/seccomp.c:168:9: note: in expansion of macro ‘DECLARE_BITMAP’
>     168 |         DECLARE_BITMAP(allow_compat, SECCOMP_ARCH_COMPAT_NR);
>         |         ^~~~~~~~~~~~~~
>   kernel/seccomp.c:168:38: note: in expansion of macro 
> ‘SECCOMP_ARCH_COMPAT_NR’
>     168 |         DECLARE_BITMAP(allow_compat, SECCOMP_ARCH_COMPAT_NR);
>         |                                      ^~~~~~~~~~~~~~~~~~~~~~

Sorry about that, I thought I had fixed it already. I'll need to
get back to the series and make sure this works.

> - The patch set breaks git bisection, when I use "git bisect" I can
>   find more building failures caused by middle patches.

Ok, I'll check these out as well.

> - The patch "arm64: generate 64-bit syscall.tbl" removes macros:
>
>   __ARCH_WANT_RENAMEAT
>   __ARCH_WANT_NEW_STAT
>   __ARCH_WANT_SET_GET_RLIMIT
>   __ARCH_WANT_TIME32_SYSCALLS
>   __ARCH_WANT_SYS_CLONE3
>   __ARCH_WANT_MEMFD_SECRET
>
>   Seems to me we still need to enable these macros so can enable Arm64
>   specific system calls?

The system call selection is now controlled with the line

ABIS_unistd64.h := common,64,renameat,newstat,rlimit,mmu,clone3,memfd_secret

which picks the types of ABIs that are enabled when converting
the syscall.tbl file into the headers. Removing these was
intentional, but I now see that we still need to define __ARCH_WANT_NEW_STAT
and __ARCH_WANT_SYS_CLONE3 because those two are referenced not
just in unistd.h but also in fs/stat.c and kernel/fork.c.

I'll put this on the list of things to check for the other architectures,
in addition to comparing the resulting syscall table object and the
generated asm/unistd.h.

> - We also need to update the header files:
>   tools/include/uapi/asm-generic/unistd.h
>
> You are welcome to CC me when you send out formal patches to mailing
> list and I can test it (and look if can refine perf code for this).

Ok, thanks again.

    Arnd