Re: [PATCH v3 3/3] selftests/nolibc: riscv: customize makefile for rv32
From: Zhangjin Wu
Date: Tue Jun 06 2023 - 21:20:54 EST
Arnd, Thomas, Willy
> > > +# Additional ARCH settings for riscv
> > > +ifeq ($(ARCH),riscv32)
> > > + SRCARCH := riscv
> > > +endif
> > > +ifeq ($(ARCH),riscv64)
> > > + SRCARCH := riscv
> > > +endif
> > > +
> > > export cross_compiling :=
> > > ifneq ($(SRCARCH),$(SUBARCH))
> > > cross_compiling := 1
> >
> > I've never been a big fan of the top-level $(ARCH) setting
> > in the kernel, is there a reason this has to be the same
> > as the variable in tools/include/nolibc? If not, I'd just
> > leave the Linux Makefile unchanged.
> >
> > For userspace we have a lot more target names than
> > arch/*/ directories in the kernel, and I don't think
> > I'd want to enumerate all the possibilities in the
> > build system globally.
>
Good news, I did find a better solution without touching the top-level
Makefile, that is overriding the ARCH to 'riscv' just before the targets
and after we got the necessary settings with the original ARCH=riscv32
or ARCH=riscv64, but it requires to convert the '=' assignments to ':=',
which is not that hard to do and it is more acceptable, just verified it
and it worked well.
...
LDFLAGS := -s
+# top-level kernel Makefile only accept ARCH=riscv, override ARCH to make kernel happy
+ifneq ($(findstring riscv,$(ARCH)),)
+override ARCH := riscv
+endif
+
help:
@echo "Supported targets under selftests/nolibc:"
@echo " all call the \"run\" target below"
This change is not that big, and the left changes can keep consistent with the
other platforms. but I still need to add a standalone patch to convert the '='
to ':=' to avoid the before setting using our new overridded ARCH.
++ b/tools/testing/selftests/nolibc/Makefile
@@ -26,7 +26,7 @@ IMAGE_riscv64 = arch/riscv/boot/Image
IMAGE_riscv = arch/riscv/boot/Image
IMAGE_s390 = arch/s390/boot/bzImage
IMAGE_loongarch = arch/loongarch/boot/vmlinuz.efi
-IMAGE = $(IMAGE_$(ARCH))
+IMAGE := $(IMAGE_$(ARCH))
IMAGE_NAME = $(notdir $(IMAGE))
# default kernel configurations that appear to be usable
@@ -41,7 +41,7 @@ DEFCONFIG_riscv64 = defconfig
DEFCONFIG_riscv = defconfig
DEFCONFIG_s390 = defconfig
DEFCONFIG_loongarch = defconfig
-DEFCONFIG = $(DEFCONFIG_$(ARCH))
+DEFCONFIG := $(DEFCONFIG_$(ARCH))
# optional tests to run (default = all)
TEST =
@@ -58,7 +58,7 @@ QEMU_ARCH_riscv64 = riscv64
QEMU_ARCH_riscv = riscv64
QEMU_ARCH_s390 = s390x
QEMU_ARCH_loongarch = loongarch64
-QEMU_ARCH = $(QEMU_ARCH_$(ARCH))
+QEMU_ARCH := $(QEMU_ARCH_$(ARCH))
# QEMU_ARGS : some arch-specific args to pass to qemu
QEMU_ARGS_i386 = -M pc -append "console=ttyS0,9600 i8042.noaux panic=-1 $(TEST:%=NOLIBC_TEST=%)"
@@ -72,7 +72,7 @@ QEMU_ARGS_riscv64 = -M virt -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_T
QEMU_ARGS_riscv = -M virt -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
QEMU_ARGS_s390 = -M s390-ccw-virtio -m 1G -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
QEMU_ARGS_loongarch = -M virt -append "console=ttyS0,115200 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
-QEMU_ARGS = $(QEMU_ARGS_$(ARCH)) $(QEMU_ARGS_EXTRA)
+QEMU_ARGS := $(QEMU_ARGS_$(ARCH)) $(QEMU_ARGS_EXTRA)
# OUTPUT is only set when run from the main makefile, otherwise
# it defaults to this nolibc directory.
@@ -87,11 +87,18 @@ endif
CFLAGS_riscv32 = -march=rv32im -mabi=ilp32
CFLAGS_s390 = -m64
CFLAGS_STACKPROTECTOR ?= $(call cc-option,-mstack-protector-guard=global $(call cc-option,-fstack-protector-all))
-CFLAGS ?= -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \
+CFLAGS_default := -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \
$(call cc-option,-fno-stack-protector) \
$(CFLAGS_$(ARCH)) $(CFLAGS_STACKPROTECTOR)
+
+CFLAGS ?= $(CFLAGS_default)
Thanks a lot, will send v4 later.
Best regards,
Zhangjin
> Ok, agree very much, it is the root cause why we used the old method
> before, because I don't want to touch the top-level Makefile, here
> explains the details again just as did for Thomas and Willy [1] ;-)
>
> Without the top-level makefile change, we must add something in
> selftests/nolibc/Makefile like this, because the kernel makefile doesn't
> accept something like ARCH=riscv32 and ARCH=riscv64 currently, it only
> accepts ARCH=riscv (will paste the code later).
>
> ifneq ($(findstring riscv,$(ARCH)),)
> _ARCH = riscv
> else
> _ARCH = $(ARCH)
> endif
>
> ...
>
> sysroot/$(ARCH)/include:
> $(Q)rm -rf sysroot/$(ARCH) sysroot/sysroot
> $(QUIET_MKDIR)mkdir -p sysroot
> $(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(_ARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone
> $(Q)mv sysroot/sysroot sysroot/$(ARCH)
>
> defconfig:
> $(Q)$(MAKE) -C $(srctree) ARCH=$(_ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare
>
> kernel: initramfs
> $(Q)$(MAKE) -C $(srctree) ARCH=$(_ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs
>
> The above change really works, but it looks not that good, this is the
> mixing use of _ARCH and ARCH I mentioned in last reply.
>
> Otherwise, we will get such error:
>
> $ make run ARCH=riscv64 CROSS_COMPILE=riscv64-linux-gnu-
> MKDIR sysroot/riscv64/include
> make[1]: Entering directory '/labs/linux-lab/src/linux-stable/tools/include/nolibc'
> make[2]: Entering directory '/labs/linux-lab/src/linux-stable'
> Makefile:763: arch/riscv64/Makefile: No such file or directory
> make[2]: *** No rule to make target 'arch/riscv64/Makefile'. Stop.
> make[2]: Leaving directory '/labs/linux-lab/src/linux-stable'
> make[1]: *** [Makefile:87: headers_standalone] Error 2
> make[1]: Leaving directory '/labs/linux-lab/src/linux-stable/tools/include/nolibc'
> make: *** [Makefile:129: sysroot/riscv64/include] Error 2
> $ make run ARCH=riscv32 CROSS_COMPILE=riscv64-linux-gnu-
> MKDIR sysroot/riscv32/include
> make[1]: Entering directory '/labs/linux-lab/src/linux-stable/tools/include/nolibc'
> make[2]: Entering directory '/labs/linux-lab/src/linux-stable'
> Makefile:763: arch/riscv32/Makefile: No such file or directory
> make[2]: *** No rule to make target 'arch/riscv32/Makefile'. Stop.
> make[2]: Leaving directory '/labs/linux-lab/src/linux-stable'
> make[1]: *** [Makefile:87: headers_standalone] Error 2
> make[1]: Leaving directory '/labs/linux-lab/src/linux-stable/tools/include/nolibc'
> make: *** [Makefile:129: sysroot/riscv32/include] Error 2
>
> That's because in top-level Makefile, it doesn't accept ARCH=riscv32 and
> ARCH=riscv64, but x86 and sparc and even parisc support such variants,
> this allows the ARCH variants share the same arch/<SRCARCH>/ source code
> tree, otherwise, they will directly find the arch/<ARCH>/ source code,
> then fails.
>
> top-level Makefile:
>
> ...
> ARCH ?= $(SUBARCH)
>
> # Architecture as present in compile.h
> UTS_MACHINE := $(ARCH)
> SRCARCH := $(ARCH) ---> SRCARCH is assigned as ARCH by default
>
> # Additional ARCH settings for x86
> ifeq ($(ARCH),i386)
> SRCARCH := x86
> endif
> ifeq ($(ARCH),x86_64)
> SRCARCH := x86
> endif
>
> # Additional ARCH settings for sparc
> ifeq ($(ARCH),sparc32)
> SRCARCH := sparc
> endif
> ifeq ($(ARCH),sparc64)
> SRCARCH := sparc
> endif
>
> # Additional ARCH settings for parisc
> ifeq ($(ARCH),parisc64)
> SRCARCH := parisc
> endif
>
> So, to really align with x86, we should let the top-level makefile be
> able to get the right SRCARCH for riscv32 and riscv64 too ;-)
>
> I even tried to pass SRCARCH=riscv to the top-level Makefile, but it
> failed:
>
> diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
> index 1b2247a6365d..04067776b569 100644
> --- a/tools/testing/selftests/nolibc/Makefile
> +++ b/tools/testing/selftests/nolibc/Makefile
> @@ -14,6 +14,10 @@ include $(srctree)/scripts/subarch.include
> ARCH = $(SUBARCH)
> endif
>
> +ifneq ($(findstring riscv,$(ARCH)),)
> +SRCARCH := SRCARCH=riscv
> +endif
> +
> # kernel image names by architecture
> IMAGE_i386 = arch/x86/boot/bzImage
> IMAGE_x86_64 = arch/x86/boot/bzImage
> @@ -126,7 +130,7 @@ sysroot: sysroot/$(ARCH)/include
> sysroot/$(ARCH)/include:
> $(Q)rm -rf sysroot/$(ARCH) sysroot/sysroot
> $(QUIET_MKDIR)mkdir -p sysroot
> - $(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(ARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone
> + $(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(ARCH) $(SRCARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone
> $(Q)mv sysroot/sysroot sysroot/$(ARCH)
>
> nolibc-test: nolibc-test.c sysroot/$(ARCH)/include
> @@ -150,10 +154,10 @@ initramfs: nolibc-test
> $(Q)cp nolibc-test initramfs/init
>
> defconfig:
> - $(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare
> + $(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) $(SRCARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare
>
> kernel: initramfs
> - $(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs
> + $(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) $(SRCARCH )CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs
>
> # run the tests after building the kernel
> run: kernel
>
> $ make run ARCH=riscv32 CROSS_COMPILE=riscv64-linux-gnu- QEMU_ARGS_EXTRA="-bios /labs/linux-lab/opensbi-riscv32-generic-fw_dynamic.bin"
> MKDIR sysroot/riscv32/include
> make[1]: Entering directory '/labs/linux-lab/src/linux-stable/tools/include/nolibc'
> make[2]: Entering directory '/labs/linux-lab/src/linux-stable'
> Makefile:397: srcarch: riscv
> make[2]: Leaving directory '/labs/linux-lab/src/linux-stable'
> make[2]: Entering directory '/labs/linux-lab/src/linux-stable'
> Makefile:397: srcarch: riscv
> INSTALL /labs/linux-lab/src/linux-stable/tools/testing/selftests/nolibc/sysroot/sysroot/include
> make[2]: Leaving directory '/labs/linux-lab/src/linux-stable'
> make[1]: Leaving directory '/labs/linux-lab/src/linux-stable/tools/include/nolibc'
> CC nolibc-test
> MKDIR initramfs
> INSTALL initramfs/init
> make[1]: Entering directory '/labs/linux-lab/src/linux-stable'
> Makefile:397: srcarch: riscv32
> SYNC include/config/auto.conf.cmd
> Makefile:397: srcarch: riscv32
> Makefile:687: arch/riscv32/Makefile: No such file or directory
> make[2]: *** No rule to make target 'arch/riscv32/Makefile'. Stop.
> make[1]: *** [Makefile:795: include/config/auto.conf.cmd] Error 2
> make[1]: Leaving directory '/labs/linux-lab/src/linux-stable'
>
> So, to keep consistent eventually, perhaps we do need to touch the
> top-level Makefile.
>
> Best regards,
> Zhangjin
>
> [1]: https://lore.kernel.org/linux-riscv/20230526092029.149351-1-falcon@xxxxxxxxxxx/
>
> > Arnd