Re: [PATCH v2 01/16] perf tools: Create generic syscall table support

[Charlie Jenkins <charlie@rivosinc.com>]

On Fri, Dec 13, 2024 at 03:22:53PM -0800, Namhyung Kim wrote:
> Hello,
> 
> On Thu, Dec 12, 2024 at 04:32:51PM -0800, Charlie Jenkins wrote:
> > Currently each architecture in perf independently generates syscall
> > headers. Adapt the work that has gone into unifying syscall header
> > implementations in the kernel to work with perf tools. Introduce this
> > framework with riscv at first. riscv previously relied on libaudit, but
> > with this change, perf tools for riscv no longer needs this external
> > dependency.
> 
> Nice work!
> 
> > 
> > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> > ---
> >  tools/perf/Makefile.config                         |  11 +-
> >  tools/perf/Makefile.perf                           |   4 +
> >  tools/perf/arch/riscv/Makefile                     |  22 --
> >  tools/perf/arch/riscv/entry/syscalls/Kbuild        |   2 +
> >  .../arch/riscv/entry/syscalls/Makefile.syscalls    |   4 +
> >  tools/perf/arch/riscv/entry/syscalls/mksyscalltbl  |  47 ---
> >  tools/perf/arch/riscv/include/syscall_table.h      |   8 +
> >  tools/perf/check-headers.sh                        |   1 +
> >  tools/perf/scripts/Makefile.syscalls               |  60 +++
> >  tools/perf/scripts/syscalltbl.sh                   |  86 +++++
> >  tools/perf/util/syscalltbl.c                       |   8 +-
> >  tools/scripts/syscall.tbl                          | 409 +++++++++++++++++++++
> >  12 files changed, 585 insertions(+), 77 deletions(-)
> > 
> > diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
> > index 2916d59c88cd08b299202a48ddb42fa32aac04a6..a72f25162714f0117a88d94474da336814d4f030 100644
> > --- a/tools/perf/Makefile.config
> > +++ b/tools/perf/Makefile.config
> > @@ -35,6 +35,13 @@ ifneq ($(NO_SYSCALL_TABLE),1)
> >      NO_SYSCALL_TABLE := 0
> >    endif
> >  
> > +  # architectures that use the generic syscall table scripts
> > +  ifeq ($(SRCARCH),riscv)
> > +    NO_SYSCALL_TABLE := 0
> > +    CFLAGS += -DGENERIC_SYSCALL_TABLE
> > +    CFLAGS += -I$(OUTPUT)/tools/perf/arch/$(SRCARCH)/include/generated
> > +  endif
> > +
> >    ifneq ($(NO_SYSCALL_TABLE),1)
> >      CFLAGS += -DHAVE_SYSCALL_TABLE_SUPPORT
> >    endif
> > @@ -83,10 +90,6 @@ ifeq ($(ARCH),mips)
> >    LIBUNWIND_LIBS = -lunwind -lunwind-mips
> >  endif
> >  
> > -ifeq ($(ARCH),riscv)
> > -  CFLAGS += -I$(OUTPUT)arch/riscv/include/generated
> > -endif
> > -
> >  # So far there's only x86 and arm libdw unwind support merged in perf.
> >  # Disable it on all other architectures in case libdw unwind
> >  # support is detected in system. Add supported architectures
> > diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
> > index d74241a151313bd09101aabb5d765a5a0a6efc84..f5278ed9f778f928436693a14e016c5c3c5171c1 100644
> > --- a/tools/perf/Makefile.perf
> > +++ b/tools/perf/Makefile.perf
> > @@ -310,6 +310,10 @@ ifeq ($(filter feature-dump,$(MAKECMDGOALS)),feature-dump)
> >  FEATURE_TESTS := all
> >  endif
> >  endif
> > +# architectures that use the generic syscall table
> > +ifeq ($(SRCARCH),riscv)
> > +include $(srctree)/tools/perf/scripts/Makefile.syscalls
> > +endif
> >  include Makefile.config
> >  endif
> >  
> > diff --git a/tools/perf/arch/riscv/Makefile b/tools/perf/arch/riscv/Makefile
> > index 18ad078000e2bba595f92efc5d97a63fdb83ef45..087e099fb453a9236db34878077a51f711881ce0 100644
> > --- a/tools/perf/arch/riscv/Makefile
> > +++ b/tools/perf/arch/riscv/Makefile
> > @@ -1,25 +1,3 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  PERF_HAVE_JITDUMP := 1
> >  HAVE_KVM_STAT_SUPPORT := 1
> > -
> > -#
> > -# Syscall table generation for perf
> > -#
> > -
> > -out    := $(OUTPUT)arch/riscv/include/generated/asm
> > -header := $(out)/syscalls.c
> > -incpath := $(srctree)/tools
> > -sysdef := $(srctree)/tools/arch/riscv/include/uapi/asm/unistd.h
> > -sysprf := $(srctree)/tools/perf/arch/riscv/entry/syscalls/
> > -systbl := $(sysprf)/mksyscalltbl
> > -
> > -# Create output directory if not already present
> > -$(shell [ -d '$(out)' ] || mkdir -p '$(out)')
> > -
> > -$(header): $(sysdef) $(systbl)
> > -	$(Q)$(SHELL) '$(systbl)' '$(CC)' '$(HOSTCC)' $(incpath) $(sysdef) > $@
> > -
> > -clean::
> > -	$(call QUIET_CLEAN, riscv) $(RM) $(header)
> > -
> > -archheaders: $(header)
> > diff --git a/tools/perf/arch/riscv/entry/syscalls/Kbuild b/tools/perf/arch/riscv/entry/syscalls/Kbuild
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..9a41e3572c3afd4f202321fd9e492714540e8fd3
> > --- /dev/null
> > +++ b/tools/perf/arch/riscv/entry/syscalls/Kbuild
> > @@ -0,0 +1,2 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +syscall-y += syscalls_64.h
> > diff --git a/tools/perf/arch/riscv/entry/syscalls/Makefile.syscalls b/tools/perf/arch/riscv/entry/syscalls/Makefile.syscalls
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..9668fd1faf60e828ed2786c2ee84739ac1f153fc
> > --- /dev/null
> > +++ b/tools/perf/arch/riscv/entry/syscalls/Makefile.syscalls
> > @@ -0,0 +1,4 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +syscall_abis_32 += riscv memfd_secret
> > +syscall_abis_64 += riscv rlimit memfd_secret
> > diff --git a/tools/perf/arch/riscv/entry/syscalls/mksyscalltbl b/tools/perf/arch/riscv/entry/syscalls/mksyscalltbl
> > deleted file mode 100755
> > index c59f5e852b97712a9a879b89e6ef6999ed4b6cd7..0000000000000000000000000000000000000000
> > --- a/tools/perf/arch/riscv/entry/syscalls/mksyscalltbl
> > +++ /dev/null
> > @@ -1,47 +0,0 @@
> > -#!/bin/sh
> > -# SPDX-License-Identifier: GPL-2.0
> > -#
> > -# Generate system call table for perf. Derived from
> > -# powerpc script.
> > -#
> > -# Copyright IBM Corp. 2017
> > -# Author(s):  Hendrik Brueckner <brueckner@linux.vnet.ibm.com>
> > -# Changed by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
> > -# Changed by: Kim Phillips <kim.phillips@arm.com>
> > -# Changed by: Björn Töpel <bjorn@rivosinc.com>
> > -
> > -gcc=$1
> > -hostcc=$2
> > -incpath=$3
> > -input=$4
> > -
> > -if ! test -r $input; then
> > -	echo "Could not read input file" >&2
> > -	exit 1
> > -fi
> > -
> > -create_sc_table()
> > -{
> > -	local sc nr max_nr
> > -
> > -	while read sc nr; do
> > -		printf "%s\n" "	[$nr] = \"$sc\","
> > -		max_nr=$nr
> > -	done
> > -
> > -	echo "#define SYSCALLTBL_RISCV_MAX_ID $max_nr"
> > -}
> > -
> > -create_table()
> > -{
> > -	echo "#include \"$input\""
> > -	echo "static const char *const syscalltbl_riscv[] = {"
> > -	create_sc_table
> > -	echo "};"
> > -}
> > -
> > -$gcc -E -dM -x c -I $incpath/include/uapi $input \
> > -	|awk '$2 ~ "__NR" && $3 !~ "__NR3264_" {
> > -		sub("^#define __NR(3264)?_", "");
> > -		print | "sort -k2 -n"}' \
> > -	|create_table
> > diff --git a/tools/perf/arch/riscv/include/syscall_table.h b/tools/perf/arch/riscv/include/syscall_table.h
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..7ff51b783000d727ec48be960730b81ecdb05575
> > --- /dev/null
> > +++ b/tools/perf/arch/riscv/include/syscall_table.h
> > @@ -0,0 +1,8 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#include <asm/bitsperlong.h>
> > +
> > +#if __BITS_PER_LONG == 64
> > +#include <asm/syscalls_64.h>
> > +#else
> > +#include <asm/syscalls_32.h>
> > +#endif
> > diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
> > index a05c1c105c51bf1bd18a59195220894598eb7461..692f48db810ccbef229e240db29261f0c60db632 100755
> > --- a/tools/perf/check-headers.sh
> > +++ b/tools/perf/check-headers.sh
> > @@ -71,6 +71,7 @@ FILES=(
> >    "include/uapi/asm-generic/ioctls.h"
> >    "include/uapi/asm-generic/mman-common.h"
> >    "include/uapi/asm-generic/unistd.h"
> > +  "scripts/syscall.tbl"
> >  )
> >  
> >  declare -a SYNC_CHECK_FILES
> > diff --git a/tools/perf/scripts/Makefile.syscalls b/tools/perf/scripts/Makefile.syscalls
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..e2b0c4b513d5c42c8e9ac51a9ea774c34a1311b7
> > --- /dev/null
> > +++ b/tools/perf/scripts/Makefile.syscalls
> > @@ -0,0 +1,60 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +# This Makefile generates tools/perf/arch/$(SRCARCH)/include/generated/asm from
> > +# the generic syscall table if that is supported by the architecture, otherwise
> > +# from the architecture's defined syscall table.
> > +
> > +PHONY := all
> > +all:
> > +
> > +obj := $(OUTPUT)/tools/perf/arch/$(SRCARCH)/include/generated/asm
> > +
> > +syscall_abis_32  += common,32
> > +syscall_abis_64  += common,64
> 
> Couldn't it be := instead of += ?

Yes good point.

> 
> 
> > +syscalltbl := $(srctree)/tools/scripts/syscall.tbl
> > +
> > +# let architectures override $(syscall_abis_%) and $(syscalltbl)
> > +-include $(srctree)/tools/perf/arch/$(SRCARCH)/entry/syscalls/Makefile.syscalls
> > +include $(srctree)/scripts/Kbuild.include
> > +-include $(srctree)/tools/perf/arch/$(SRCARCH)/entry/syscalls/Kbuild
> > +
> > +systbl := $(srctree)/tools/perf/scripts/syscalltbl.sh
> > +
> > +syscall-y   := $(addprefix $(obj)/, $(syscall-y))
> > +
> > +# Remove stale wrappers when the corresponding files are removed from generic-y
> > +old-headers := $(wildcard $(obj)/*.c)
> > +unwanted    := $(filter-out $(syscall-y),$(old-headers))
> 
> Can you elaborate?  What's the stable wrappers?
> 
> I think syscall-y and old-headers have .h files but the wildcard above
> is called for .c files?

Oh yes! I meant to change that to .h, thank you for mentioning that, I
had .c at some point during development but changed it to .h files.

- Charlie

> 
> Thanks,
> Namhyung
> 
> > +
> > +quiet_cmd_remove = REMOVE  $(unwanted)
> > +      cmd_remove = rm -f $(unwanted)
> > +
> > +quiet_cmd_systbl = SYSTBL  $@
> > +      cmd_systbl = $(CONFIG_SHELL) $(systbl) \
> > +		   $(if $(systbl-args-$*),$(systbl-args-$*),$(systbl-args)) \
> > +		   --abis $(subst $(space),$(comma),$(strip $(syscall_abis_$*))) \
> > +		   $< $@
> > +
> > +all: $(syscall-y)
> > +	$(if $(unwanted),$(call cmd,remove))
> > +	@:
> > +
> > +$(obj)/syscalls_%.h: $(syscalltbl) $(systbl) FORCE
> > +	$(call if_changed,systbl)
> > +
> > +targets := $(syscall-y)
> > +
> > +# Create output directory. Skip it if at least one old header exists
> > +# since we know the output directory already exists.
> > +ifeq ($(old-headers),)
> > +$(shell mkdir -p $(obj))
> > +endif
> > +
> > +PHONY += FORCE
> > +
> > +FORCE:
> > +
> > +existing-targets := $(wildcard $(sort $(targets)))
> > +
> > +-include $(foreach f,$(existing-targets),$(dir $(f)).$(notdir $(f)).cmd)
> > +
> > +.PHONY: $(PHONY)