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
