| Message ID | ca162c288c3eeda309c33049711b5272eb80f8e2.1685780412.git.falcon@tinylab.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | nolibc: add part2 of support for rv32 | expand |
On Sat, Jun 3, 2023, at 11:05, Zhangjin Wu wrote: > Both riscv64 and riscv32 have: > > * the same ARCH value, it is riscv > * the same arch/riscv source code tree > > The only differences are: > > * riscv64 uses defconfig, riscv32 uses rv32_defconfig > * riscv64 uses qemu-system-riscv64, riscv32 uses qemu-system-riscv32 > * riscv32 has different compiler options (-march= and -mabi=) > > So, riscv32 can share most of the settings with riscv64, there is no > need to add it as a whole new architecture but just need a flag to > record and reflect the difference. > > The 32bit mips and loongarch may be able to use the same method, so, > let's use a meaningful flag: CONFIG_32BIT. If required in the future, > this flag can also be automatically loaded from > include/config/auto.conf. If we use a CONFIG_* symbol, I think it should be the other way round, for consistency with the kernel, which uses CONFIG_64BIT on all architectures, but only uses CONFIG_32BIT on mips, loongarch powerpc and riscv. > # kernel image names by architecture > IMAGE_i386 = arch/x86/boot/bzImage > IMAGE_x86_64 = arch/x86/boot/bzImage > @@ -34,7 +40,7 @@ DEFCONFIG_x86 = defconfig > DEFCONFIG_arm64 = defconfig > DEFCONFIG_arm = multi_v7_defconfig > DEFCONFIG_mips = malta_defconfig > -DEFCONFIG_riscv = defconfig > +DEFCONFIG_riscv = $(if $(CONFIG_32BIT),rv32_defconfig,defconfig) > DEFCONFIG_s390 = defconfig > DEFCONFIG_loongarch = defconfig > DEFCONFIG = $(DEFCONFIG_$(ARCH)) This feels slightly odd, as we otherwise have a fixed defconfig per target, so doing DEFCONFIG_riscv = defconfig DEFCONFIG_riscv64 = defconfig DEFCONFIG_riscv32 = rv32_defconfig would seem more consistent with how x86 is handled, and would probably be more easily extensible if we want to also make this work with other sub-targets like mipseb, armv5 or ppc32 in the future. Arnd
Arnd, Thomas, Willy > On Sat, Jun 3, 2023, at 11:05, Zhangjin Wu wrote: > > Both riscv64 and riscv32 have: > > > > * the same ARCH value, it is riscv > > * the same arch/riscv source code tree > > > > The only differences are: > > > > * riscv64 uses defconfig, riscv32 uses rv32_defconfig > > * riscv64 uses qemu-system-riscv64, riscv32 uses qemu-system-riscv32 > > * riscv32 has different compiler options (-march= and -mabi=) > > > > So, riscv32 can share most of the settings with riscv64, there is no > > need to add it as a whole new architecture but just need a flag to > > record and reflect the difference. > > > > The 32bit mips and loongarch may be able to use the same method, so, > > let's use a meaningful flag: CONFIG_32BIT. If required in the future, > > this flag can also be automatically loaded from > > include/config/auto.conf. > > If we use a CONFIG_* symbol, I think it should be the other way > round, for consistency with the kernel, which uses CONFIG_64BIT > on all architectures, but only uses CONFIG_32BIT on mips, loongarch > powerpc and riscv. > > > > # kernel image names by architecture > > IMAGE_i386 = arch/x86/boot/bzImage > > IMAGE_x86_64 = arch/x86/boot/bzImage > > @@ -34,7 +40,7 @@ DEFCONFIG_x86 = defconfig > > DEFCONFIG_arm64 = defconfig > > DEFCONFIG_arm = multi_v7_defconfig > > DEFCONFIG_mips = malta_defconfig > > -DEFCONFIG_riscv = defconfig > > +DEFCONFIG_riscv = $(if $(CONFIG_32BIT),rv32_defconfig,defconfig) > > DEFCONFIG_s390 = defconfig > > DEFCONFIG_loongarch = defconfig > > DEFCONFIG = $(DEFCONFIG_$(ARCH)) > > This feels slightly odd, as we otherwise have a fixed defconfig > per target, so doing > > DEFCONFIG_riscv = defconfig > DEFCONFIG_riscv64 = defconfig > DEFCONFIG_riscv32 = rv32_defconfig > > would seem more consistent with how x86 is handled, and would > probably be more easily extensible if we want to also make > this work with other sub-targets like mipseb, armv5 or ppc32 > in the future. As Arnd and Thomas suggested to align with x86, I just tried to find a solution to avoid mixing the use of _ARCH and ARCH in this Makefile. Since both riscv32 and riscv64 share the same SRCARCH=riscv (arch/riscv), and the kernel side doesn't accept riscv32 or riscv64 currently, we need to manually convert them to _ARCH=riscv and pass them to the kernel makefile like this: ARCH=$(_ARCH), it mixes the use of _ARCH and ARCH, this is why I used the '$(if' method currently. The solution is adding something like x86 in the kernel Makefile: diff --git a/Makefile b/Makefile index 9d765ebcccf1..a442c893d795 100644 --- a/Makefile +++ b/Makefile @@ -415,6 +415,14 @@ ifeq ($(ARCH),parisc64) SRCARCH := parisc endif +# 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 And also, we need to let both of them use the arch-riscv.h in tools/include/nolibc/Makefile: diff --git a/tools/include/nolibc/Makefile b/tools/include/nolibc/Makefile index 64d67b080744..459eaddb230f 100644 --- a/tools/include/nolibc/Makefile +++ b/tools/include/nolibc/Makefile @@ -24,6 +24,7 @@ Q=@ endif nolibc_arch := $(patsubst arm64,aarch64,$(ARCH)) +nolibc_arch := $(patsubst riscv%,riscv,$(ARCH)) arch_file := arch-$(nolibc_arch).h all_files := \ compiler.h \ So, the left parts can be added like x86 too: diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index 4a3a105e1fdf..1b2247a6365d 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -21,6 +21,8 @@ IMAGE_x86 = arch/x86/boot/bzImage IMAGE_arm64 = arch/arm64/boot/Image IMAGE_arm = arch/arm/boot/zImage IMAGE_mips = vmlinuz +IMAGE_riscv32 = arch/riscv/boot/Image +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 @@ -34,6 +36,8 @@ DEFCONFIG_x86 = defconfig DEFCONFIG_arm64 = defconfig DEFCONFIG_arm = multi_v7_defconfig DEFCONFIG_mips = malta_defconfig +DEFCONFIG_riscv32 = rv32_defconfig +DEFCONFIG_riscv64 = defconfig DEFCONFIG_riscv = defconfig DEFCONFIG_s390 = defconfig DEFCONFIG_loongarch = defconfig @@ -49,6 +53,8 @@ QEMU_ARCH_x86 = x86_64 QEMU_ARCH_arm64 = aarch64 QEMU_ARCH_arm = arm QEMU_ARCH_mips = mipsel # works with malta_defconfig +QEMU_ARCH_riscv32 = riscv32 +QEMU_ARCH_riscv64 = riscv64 QEMU_ARCH_riscv = riscv64 QEMU_ARCH_s390 = s390x QEMU_ARCH_loongarch = loongarch64 @@ -61,6 +67,8 @@ QEMU_ARGS_x86 = -M pc -append "console=ttyS0,9600 i8042.noaux panic=-1 $( QEMU_ARGS_arm64 = -M virt -cpu cortex-a53 -append "panic=-1 $(TEST:%=NOLIBC_TEST=%)" QEMU_ARGS_arm = -M virt -append "panic=-1 $(TEST:%=NOLIBC_TEST=%)" QEMU_ARGS_mips = -M malta -append "panic=-1 $(TEST:%=NOLIBC_TEST=%)" +QEMU_ARGS_riscv32 = -M virt -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_TEST=%)" +QEMU_ARGS_riscv64 = -M virt -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_TEST=%)" 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=%)" @@ -76,6 +84,7 @@ else Q=@ 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 \ And just found the 'm' extension (-march=rv32im) is necessary to avoid linking libgcc, especially, my local compiler has no rv32 libgcc (target emulation `elf64-littleriscv' does not match `elf32-littleriscv'), so, added the 'm' extension back, this is supported by the generic riscv chips. The atomic and float extensions (include single and double) are not added, keep it as minimal currently. Just tested rv32 and rv64 on 20230606-nolibc-rv32+stkp7a with a trivial fixup of rcu (the problematic code is not in v6.4-rc4, so, this can be ignored, see below, what about rebase the new branch on a newer rc?), it works as expected. diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h index ce02bb09651b..72bd8fe0cad6 100644 --- a/kernel/rcu/tasks.h +++ b/kernel/rcu/tasks.h @@ -1934,11 +1934,13 @@ void show_rcu_tasks_gp_kthreads(void) } #endif /* #ifndef CONFIG_TINY_RCU */ +#ifdef CONFIG_TASKS_RCU struct task_struct *get_rcu_tasks_gp_kthread(void) { return rcu_tasks.kthread_ptr; } EXPORT_SYMBOL_GPL(get_rcu_tasks_gp_kthread); +#endif #ifdef CONFIG_PROVE_RCU struct rcu_tasks_test_desc { The steps I tested: $ history | grep make // riscv32 test 1416 make defconfig ARCH=riscv32 CROSS_COMPILE=riscv64-linux-gnu- 1430 make run ARCH=riscv32 CROSS_COMPILE=riscv64-linux-gnu- QEMU_ARGS_EXTRA="-bios /labs/linux-lab/opensbi-riscv32-generic-fw_dynamic.bin" 1438 make run-user ARCH=riscv32 CROSS_COMPILE=riscv64-linux-gnu- 113 test(s) passed, 3 skipped, 22 failed. See all results in /labs/linux-lab/src/linux-stable/tools/testing/selftests/nolibc/run.out // riscv64 test (old options) 1432 make defconfig ARCH=riscv CROSS_COMPILE=riscv64-linux-gnu- 1433 make run ARCH=riscv CROSS_COMPILE=riscv64-linux-gnu- 1443 make run-user ARCH=riscv CROSS_COMPILE=riscv64-linux-gnu- 135 test(s) passed, 3 skipped, 0 failed. See all results in /labs/linux-lab/src/linux-stable/tools/testing/selftests/nolibc/run.out // riscv64 test (new options) 1441 make run ARCH=riscv64 CROSS_COMPILE=riscv64-linux-gnu- 1439 make run-user ARCH=riscv64 CROSS_COMPILE=riscv64-linux-gnu- Thanks Arnd and Thomas for your persistence, If you agree, will send this as a new revision. @Willy, I plan to send v4 immediately, since the first two has not been merged yet, so, I will send them together as v4. Best regards, Zhangjin > > Arnd
On Tue, Jun 6, 2023, at 13:12, Zhangjin Wu wrote: >> On Sat, Jun 3, 2023, at 11:05, Zhangjin Wu wrote: >> would seem more consistent with how x86 is handled, and would >> probably be more easily extensible if we want to also make >> this work with other sub-targets like mipseb, armv5 or ppc32 >> in the future. > > As Arnd and Thomas suggested to align with x86, I just tried to find a > solution to avoid mixing the use of _ARCH and ARCH in this Makefile. > > Since both riscv32 and riscv64 share the same SRCARCH=riscv (arch/riscv), > and the kernel side doesn't accept riscv32 or riscv64 currently, we need to > manually convert them to _ARCH=riscv and pass them to the kernel makefile > like this: ARCH=$(_ARCH), it mixes the use of _ARCH and ARCH, this is why I > used the '$(if' method currently. > > The solution is adding something like x86 in the kernel Makefile: > > diff --git a/Makefile b/Makefile > index 9d765ebcccf1..a442c893d795 100644 > --- a/Makefile > +++ b/Makefile > @@ -415,6 +415,14 @@ ifeq ($(ARCH),parisc64) > SRCARCH := parisc > endif > > +# 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. > b/tools/testing/selftests/nolibc/Makefile > index 4a3a105e1fdf..1b2247a6365d 100644 > --- a/tools/testing/selftests/nolibc/Makefile > +++ b/tools/testing/selftests/nolibc/Makefile > @@ -21,6 +21,8 @@ IMAGE_x86 = arch/x86/boot/bzImage > IMAGE_arm64 = arch/arm64/boot/Image > IMAGE_arm = arch/arm/boot/zImage > IMAGE_mips = vmlinuz > +IMAGE_riscv32 = arch/riscv/boot/Image > +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 > @@ -34,6 +36,8 @@ DEFCONFIG_x86 = defconfig > DEFCONFIG_arm64 = defconfig > DEFCONFIG_arm = multi_v7_defconfig > DEFCONFIG_mips = malta_defconfig > +DEFCONFIG_riscv32 = rv32_defconfig > +DEFCONFIG_riscv64 = defconfig ... Right, that part looks good to me. Arnd
> On Tue, Jun 6, 2023, at 13:12, Zhangjin Wu wrote: > >> On Sat, Jun 3, 2023, at 11:05, Zhangjin Wu wrote: > >> would seem more consistent with how x86 is handled, and would > >> probably be more easily extensible if we want to also make > >> this work with other sub-targets like mipseb, armv5 or ppc32 > >> in the future. > > > > As Arnd and Thomas suggested to align with x86, I just tried to find a > > solution to avoid mixing the use of _ARCH and ARCH in this Makefile. > > > > Since both riscv32 and riscv64 share the same SRCARCH=riscv (arch/riscv), > > and the kernel side doesn't accept riscv32 or riscv64 currently, we need to > > manually convert them to _ARCH=riscv and pass them to the kernel makefile > > like this: ARCH=$(_ARCH), it mixes the use of _ARCH and ARCH, this is why I > > used the '$(if' method currently. > > > > The solution is adding something like x86 in the kernel Makefile: > > > > diff --git a/Makefile b/Makefile > > index 9d765ebcccf1..a442c893d795 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -415,6 +415,14 @@ ifeq ($(ARCH),parisc64) > > SRCARCH := parisc > > endif > > > > +# 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. 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@tinylab.org/ > Arnd
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@tinylab.org/ > > > Arnd
Hi, On Wed, Jun 07, 2023 at 09:20:32AM +0800, Zhangjin Wu wrote: > 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. Actually it's not exactly used by nolibc, except to pass to the kernel for the install part to extract kernel headers (make headers_install). It's one of the parts that has required to stick to most of the kernel's variables very closely (the other one being for nolibc-test which needs to build a kernel). > 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 > + That can be one approach indeed. Another one if we continue to face difficulties for this one would be to use a distinct KARCH variable to assign to ARCH in all kernel-specific operations. > 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. I don't even see why the other ones below are needed, given that as long as they remain assigned as macros, they will be replaced in-place where they are used, so they will reference the last known assignment to 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) Why did you need to split this one like this instead of proceeding like for the other ones ? Because of the "?=" maybe ? Please double-check that you really need to turn this from a macro to a variable, if as I suspect it it's not needed, it would be even simpler. Thanks, Willy
> On Wed, Jun 07, 2023 at 09:20:32AM +0800, Zhangjin Wu wrote: > > Arnd, Thomas, Willy > > ... > > > > LDFLAGS := -s > > > > +# top-level kernel Makefile only accept ARCH=riscv, override ARCH to make kernel happy > > +ifneq ($(findstring riscv,$(ARCH)),) > > +override ARCH := riscv > > +endif > > + > > That can be one approach indeed. Another one if we continue to face > difficulties for this one would be to use a distinct KARCH variable > to assign to ARCH in all kernel-specific operations. > Yeah, I have replied that method to Arnd and Thomas too, it looks like this: 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 Using KARCH seems better than _ARCH: ifneq ($(findstring riscv,$(ARCH)),) KARCH = riscv else KARCH = $(ARCH) endif ... sysroot/$(ARCH)/include: $(Q)rm -rf sysroot/$(ARCH) sysroot/sysroot $(QUIET_MKDIR)mkdir -p sysroot $(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(KARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone $(Q)mv sysroot/sysroot sysroot/$(ARCH) defconfig: $(Q)$(MAKE) -C $(srctree) ARCH=$(KARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare kernel: initramfs $(Q)$(MAKE) -C $(srctree) ARCH=$(KARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs but the new method mentioned here differs, it split the whole Makefile to two 'parts', the before part accept something like ARCH=riscv32, ARCH=riscv64, ARCH=riscv, the after part use the ARCH=riscv, this avoid touch the targets context: ... 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=%)" @@ -61,10 +67,12 @@ QEMU_ARGS_x86 = -M pc -append "console=ttyS0,9600 i8042.noaux panic=-1 $( QEMU_ARGS_arm64 = -M virt -cpu cortex-a53 -append "panic=-1 $(TEST:%=NOLIBC_TEST=%)" QEMU_ARGS_arm = -M virt -append "panic=-1 $(TEST:%=NOLIBC_TEST=%)" QEMU_ARGS_mips = -M malta -append "panic=-1 $(TEST:%=NOLIBC_TEST=%)" +QEMU_ARGS_riscv32 = -M virt -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_TEST=%)" +QEMU_ARGS_riscv64 = -M virt -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_TEST=%)" 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. @@ -76,13 +84,24 @@ else Q=@ 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) LDFLAGS := -s ... variable assignments before this line ... +# Some architectures share the same arch/<ARCH>/ source code tree among the <ARCH>xyz variants +# Top-level kernel Makefile only accepts ARCH=<ARCH>, override <ARCH>xyz variants to make kernel happy +ARCHS := riscv +_ARCH := $(strip $(foreach arch, $(ARCHS), $(if $(findstring x$(arch),x$(ARCH)),$(arch)))) +ifneq ($(_ARCH),) +override ARCH := $(_ARCH) +endif + ... targets after this line ... [1]: https://lore.kernel.org/lkml/20230606120755.548017-1-falcon@tinylab.org/#R > > 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. > > I don't even see why the other ones below are needed, given that as > long as they remain assigned as macros, they will be replaced in-place > where they are used, so they will reference the last known assignment > to ARCH. The reason is really: "they will reference the last known assignment to ARCH" If we use something like 'KARCH' or '_ARCH' and not override the ARCH in the middle, then, no need to touch the ':' and '?='. otherwise, the variable will accept something like QEMU_ARGS_riscv for riscv32, it breaks our requirement. > > ... > > 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) > > Why did you need to split this one like this instead of proceeding > like for the other ones ? Because of the "?=" maybe ? Please > double-check that you really need to turn this from a macro to a > variable, if as I suspect it it's not needed, it would be even > simpler. It depends on the method we plan to use, just as explained above. For a standalone KARCH, no need to touch the assignment, otherwise, we should let the assignment take effect immediately to avoid they use the one we plan to override. For the KARCH method, I will tune it to be more scalable like this: ARCHS = riscv _ARCH = $(strip $(foreach arch, $(ARCHS), $(if $(findstring x$(arch),x$(ARCH)),$(arch)))) KARCH = $(or $(_ARCH),$(ARCH)) Willy, Which one do you prefer? Thanks, Zhangjin > > Thanks, Willy
On Wed, Jun 07, 2023 at 02:33:14PM +0800, Zhangjin Wu wrote: > > That can be one approach indeed. Another one if we continue to face > > difficulties for this one would be to use a distinct KARCH variable > > to assign to ARCH in all kernel-specific operations. > > > > Yeah, I have replied that method to Arnd and Thomas too, it looks like this: > > 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 > > Using KARCH seems better than _ARCH: > > ifneq ($(findstring riscv,$(ARCH)),) > KARCH = riscv > else > KARCH = $(ARCH) > endif (...) At least it suggests what it's going to be used for instead of just being marked as "special" (something the underscore does). > but the new method mentioned here differs, it split the whole Makefile > to two 'parts', the before part accept something like ARCH=riscv32, > ARCH=riscv64, ARCH=riscv, the after part use the ARCH=riscv, this avoid > touch the targets context: We don't care about touching *code*. What is important is that it scales and is understandable, thus maintainable. Code that has many exceptions or requires a lot of head scratching to figure what's being done is a pain to maintain and nobody wants to take the risk to touch it. That was exactly the purpose of the enumeration of per-target args, flags etc in the makefile: nobody needs to be expert in multiple areas to touch their own area. If we face a showstopper, we need to address it, and not work around it for the sake of touching less context. > ... variable assignments before this line ... > > +# Some architectures share the same arch/<ARCH>/ source code tree among the <ARCH>xyz variants > +# Top-level kernel Makefile only accepts ARCH=<ARCH>, override <ARCH>xyz variants to make kernel happy > +ARCHS := riscv > +_ARCH := $(strip $(foreach arch, $(ARCHS), $(if $(findstring x$(arch),x$(ARCH)),$(arch)))) > +ifneq ($(_ARCH),) > +override ARCH := $(_ARCH) > +endif > + So actually this is a perfect example of head scratching for me. I suspect it would replace x86_64 with x86 if x86 would be placed there for example, though it would not change anything for i386. Maybe for s390/s390x, arm/arm64 or ppc/ppc64 etc it would act similarly while these are different archs. Thus this seems to be trying to generalize a rule around a single particular case. Probably that instead this particular case should be addressed explicitly until we find a generalization (if ever) to other archs: ifeq($(ARCH),riscv32) override ARCH := riscv else ifeq($(ARCH),riscv64) override ARCH := riscv endif endif Or maybe even better you can decide to remap arch names explicitly: # use KARCH_from=to to rename ARCH from $from to $to past this point. KARCH_riscv32 := riscv KARCH_riscv64 := riscv ... ifneq($(KARCH_$(ARCH)),) override ARCH := $(KARCH_$(ARCH)) endif And this does deserve an explicit note in the makefile that anything using $(ARCH) using a macro will see the renamed arch while anything using it as a variable before that line will see the original one. If you want to avoid the '=' vs ':=' mess you can even keep a copy of the original ARCH at the beginning of the makefile: # keep a copy of the arch name requested by the user, for use later # when the original form is preferred over the kernel's arch name. USER_ARCH = $(ARCH) > Willy, Which one do you prefer? The most explicit ones like above. Generally speaking when you try to add support for your own arch here, you look there for similar ones, where commands are called, and read in reverse mode till the beginning, hoping to understand the transformations. I think the current ones and the proposed ones above are self-explanatory. Anything doing too much magic renaming or doing too much hard-coded automatic stuff can quickly obfuscate the principle and make things more complicated. I already despise "override" because it messes up with macros, but I agree it can sometimes have some value. If you dup it into ORIG_ARCH or USER_ARCH, and modify the few lines overriding arch in an explicit manner, I think it would preserve its maintainability. What do you think ? thanks, Willy
> On Wed, Jun 07, 2023 at 02:33:14PM +0800, Zhangjin Wu wrote: > > > > ifneq ($(findstring riscv,$(ARCH)),) > > KARCH = riscv > > else > > KARCH = $(ARCH) > > endif > (...) > > At least it suggests what it's going to be used for instead of just > being marked as "special" (something the underscore does). > Yeah. > > but the new method mentioned here differs, it split the whole Makefile > > to two 'parts', the before part accept something like ARCH=riscv32, > > ARCH=riscv64, ARCH=riscv, the after part use the ARCH=riscv, this avoid > > touch the targets context: > > We don't care about touching *code*. What is important is that it scales > and is understandable, thus maintainable. Code that has many exceptions > or requires a lot of head scratching to figure what's being done is a > pain to maintain and nobody wants to take the risk to touch it. That was > exactly the purpose of the enumeration of per-target args, flags etc in > the makefile: nobody needs to be expert in multiple areas to touch their > own area. If we face a showstopper, we need to address it, and not work > around it for the sake of touching less context. > Get it clearly. > > ... variable assignments before this line ... > > > > +# Some architectures share the same arch/<ARCH>/ source code tree among the <ARCH>xyz variants > > +# Top-level kernel Makefile only accepts ARCH=<ARCH>, override <ARCH>xyz variants to make kernel happy > > +ARCHS := riscv > > +_ARCH := $(strip $(foreach arch, $(ARCHS), $(if $(findstring x$(arch),x$(ARCH)),$(arch)))) > > +ifneq ($(_ARCH),) > > +override ARCH := $(_ARCH) > > +endif > > + > > So actually this is a perfect example of head scratching for me. I suspect > it would replace x86_64 with x86 if x86 would be placed there for example, > though it would not change anything for i386. Maybe for s390/s390x, > arm/arm64 or ppc/ppc64 etc it would act similarly while these are different > archs. Thus this seems to be trying to generalize a rule around a single > particular case. > It is true, we did worry about when users wrongly add new ARCH in the list, a generic way is very hard before we really use them. > Probably that instead this particular case should be addressed explicitly > until we find a generalization (if ever) to other archs: > > ifeq($(ARCH),riscv32) > override ARCH := riscv > else ifeq($(ARCH),riscv64) > override ARCH := riscv > endif > endif > > Or maybe even better you can decide to remap arch names explicitly: > > # use KARCH_from=to to rename ARCH from $from to $to past this point. > KARCH_riscv32 := riscv > KARCH_riscv64 := riscv > ... > ifneq($(KARCH_$(ARCH)),) > override ARCH := $(KARCH_$(ARCH)) > endif > This did inspire me a lot, so, what about simply go back to the KARCH method without any overriding: diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index 4a3a105e1fdf..bde635b083f4 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -14,6 +14,12 @@ include $(srctree)/scripts/subarch.include ARCH = $(SUBARCH) endif +# kernel supported ARCH names by architecture +KARCH_riscv32 = riscv +KARCH_riscv64 = riscv +KARCH_riscv = riscv +KARCH = $(or $(KARCH_$(ARCH)),$(ARCH)) + # kernel image names by architecture IMAGE_i386 = arch/x86/boot/bzImage IMAGE_x86_64 = arch/x86/boot/bzImage @@ -21,6 +27,8 @@ IMAGE_x86 = arch/x86/boot/bzImage IMAGE_arm64 = arch/arm64/boot/Image IMAGE_arm = arch/arm/boot/zImage IMAGE_mips = vmlinuz And this: @@ -117,7 +132,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=$(KARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone $(Q)mv sysroot/sysroot sysroot/$(ARCH) nolibc-test: nolibc-test.c sysroot/$(ARCH)/include @@ -141,10 +156,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=$(KARCH) 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=$(KARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs It is almost consistent with the original Makefile now. I do like this method more than the override method now, the override method may break the maintainability a lot especially that the developers may be hard to know which ARCH value it is when he touch a line of the Makefile. > And this does deserve an explicit note in the makefile that anything > using $(ARCH) using a macro will see the renamed arch while anything > using it as a variable before that line will see the original one. > > If you want to avoid the '=' vs ':=' mess you can even keep a copy of > the original ARCH at the beginning of the makefile: > > # keep a copy of the arch name requested by the user, for use later > # when the original form is preferred over the kernel's arch name. > USER_ARCH = $(ARCH) > Yeah, a copy is good for the override case. > > Willy, Which one do you prefer? > > The most explicit ones like above. Generally speaking when you try to > add support for your own arch here, you look there for similar ones, > where commands are called, and read in reverse mode till the beginning, > hoping to understand the transformations. I think the current ones and > the proposed ones above are self-explanatory. Anything doing too much > magic renaming or doing too much hard-coded automatic stuff can quickly > obfuscate the principle and make things more complicated. I already > despise "override" because it messes up with macros, but I agree it can > sometimes have some value. If you dup it into ORIG_ARCH or USER_ARCH, > and modify the few lines overriding arch in an explicit manner, I think > it would preserve its maintainability. > Agree, let's give up the 'override' stuff. > What do you think ? So, let's go with the KARCH method if you agree too. Best regards, Zhangjin > > thanks, > Willy
On Wed, Jun 07, 2023 at 04:11:03PM +0800, Zhangjin Wu wrote: > This did inspire me a lot, so, what about simply go back to the KARCH > method without any overriding: > > diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile > index 4a3a105e1fdf..bde635b083f4 100644 > --- a/tools/testing/selftests/nolibc/Makefile > +++ b/tools/testing/selftests/nolibc/Makefile > @@ -14,6 +14,12 @@ include $(srctree)/scripts/subarch.include > ARCH = $(SUBARCH) > endif > > +# kernel supported ARCH names by architecture > +KARCH_riscv32 = riscv > +KARCH_riscv64 = riscv > +KARCH_riscv = riscv > +KARCH = $(or $(KARCH_$(ARCH)),$(ARCH)) > + > # kernel image names by architecture > IMAGE_i386 = arch/x86/boot/bzImage > IMAGE_x86_64 = arch/x86/boot/bzImage > @@ -21,6 +27,8 @@ IMAGE_x86 = arch/x86/boot/bzImage > IMAGE_arm64 = arch/arm64/boot/Image > IMAGE_arm = arch/arm/boot/zImage > IMAGE_mips = vmlinuz > > And this: > > @@ -117,7 +132,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=$(KARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone > $(Q)mv sysroot/sysroot sysroot/$(ARCH) > > nolibc-test: nolibc-test.c sysroot/$(ARCH)/include > @@ -141,10 +156,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=$(KARCH) 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=$(KARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs > > It is almost consistent with the original Makefile now. If it works, I like it! > I do like this method more than the override method now, the override > method may break the maintainability a lot especially that the > developers may be hard to know which ARCH value it is when he touch a > line of the Makefile. Yes definitely, add to this the risk that a patch applies at the wrong line and only breaks one or two archs, etc. > > Generally speaking when you try to > > add support for your own arch here, you look there for similar ones, > > where commands are called, and read in reverse mode till the beginning, > > hoping to understand the transformations. I think the current ones and > > the proposed ones above are self-explanatory. Anything doing too much > > magic renaming or doing too much hard-coded automatic stuff can quickly > > obfuscate the principle and make things more complicated. I already > > despise "override" because it messes up with macros, but I agree it can > > sometimes have some value. If you dup it into ORIG_ARCH or USER_ARCH, > > and modify the few lines overriding arch in an explicit manner, I think > > it would preserve its maintainability. > > > > Agree, let's give up the 'override' stuff. > > > What do you think ? > > So, let's go with the KARCH method if you agree too. I'm fine with it! Thanks, Willy
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index 44088535682e..ea434a0acdc1 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -14,6 +14,12 @@ include $(srctree)/scripts/subarch.include ARCH = $(SUBARCH) endif +# Allow pass ARCH=riscv|riscv32|riscv64, riscv implies riscv64 +ifneq ($(findstring xriscv,x$(ARCH)),) + CONFIG_32BIT := $(if $(findstring 32x,$(ARCH)x),1) + override ARCH := riscv +endif + # kernel image names by architecture IMAGE_i386 = arch/x86/boot/bzImage IMAGE_x86_64 = arch/x86/boot/bzImage @@ -34,7 +40,7 @@ DEFCONFIG_x86 = defconfig DEFCONFIG_arm64 = defconfig DEFCONFIG_arm = multi_v7_defconfig DEFCONFIG_mips = malta_defconfig -DEFCONFIG_riscv = defconfig +DEFCONFIG_riscv = $(if $(CONFIG_32BIT),rv32_defconfig,defconfig) DEFCONFIG_s390 = defconfig DEFCONFIG_loongarch = defconfig DEFCONFIG = $(DEFCONFIG_$(ARCH)) @@ -49,7 +55,7 @@ QEMU_ARCH_x86 = x86_64 QEMU_ARCH_arm64 = aarch64 QEMU_ARCH_arm = arm QEMU_ARCH_mips = mipsel # works with malta_defconfig -QEMU_ARCH_riscv = riscv64 +QEMU_ARCH_riscv = $(if $(CONFIG_32BIT),riscv32,riscv64) QEMU_ARCH_s390 = s390x QEMU_ARCH_loongarch = loongarch64 QEMU_ARCH = $(QEMU_ARCH_$(ARCH)) @@ -76,6 +82,7 @@ else Q=@ endif +CFLAGS_riscv = $(if $(CONFIG_32BIT),-march=rv32i -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 \
Both riscv64 and riscv32 have: * the same ARCH value, it is riscv * the same arch/riscv source code tree The only differences are: * riscv64 uses defconfig, riscv32 uses rv32_defconfig * riscv64 uses qemu-system-riscv64, riscv32 uses qemu-system-riscv32 * riscv32 has different compiler options (-march= and -mabi=) So, riscv32 can share most of the settings with riscv64, there is no need to add it as a whole new architecture but just need a flag to record and reflect the difference. The 32bit mips and loongarch may be able to use the same method, so, let's use a meaningful flag: CONFIG_32BIT. If required in the future, this flag can also be automatically loaded from include/config/auto.conf. With this patch, it is able to run nolibc test for rv32 like this: $ make run ARCH=riscv32 CROSS_COMPILE=riscv64-linux-gnu- ... Signed-off-by: Zhangjin Wu <falcon@tinylab.org> --- tools/testing/selftests/nolibc/Makefile | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)