From: Zhangjin Wu <falcon@tinylab.org>
To: w@1wt.eu
Cc: arnd@arndb.de, falcon@tinylab.org, linux-kernel@vger.kernel.org,
linux-kselftest@vger.kernel.org, linux-riscv@lists.infradead.org,
thomas@t-8ch.de
Subject: Re: [PATCH v3 3/3] selftests/nolibc: riscv: customize makefile for rv32
Date: Wed, 7 Jun 2023 16:11:03 +0800 [thread overview]
Message-ID: <20230607081103.746962-1-falcon@tinylab.org> (raw)
In-Reply-To: <ZIAywHvr6UB1J4of@1wt.eu>
> 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
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2023-06-07 8:11 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-03 9:00 [PATCH v3 0/3] nolibc: add part2 of support for rv32 Zhangjin Wu
2023-06-03 9:01 ` [PATCH v3 1/3] tools/nolibc: fix up #error compile failures with -ENOSYS Zhangjin Wu
2023-06-06 7:35 ` Arnd Bergmann
2023-06-07 5:19 ` Zhangjin Wu
2023-06-07 8:45 ` Arnd Bergmann
2023-06-07 9:46 ` Zhangjin Wu
2023-06-07 10:02 ` Arnd Bergmann
2023-06-07 13:26 ` Zhangjin Wu
2023-06-03 9:04 ` [PATCH v3 2/3] tools/nolibc: fix up undeclared syscall macros with #ifdef and -ENOSYS Zhangjin Wu
2023-06-03 9:05 ` [PATCH v3 3/3] selftests/nolibc: riscv: customize makefile for rv32 Zhangjin Wu
2023-06-06 7:43 ` Arnd Bergmann
2023-06-06 11:12 ` Zhangjin Wu
2023-06-06 11:21 ` Arnd Bergmann
2023-06-06 12:07 ` Zhangjin Wu
2023-06-07 1:20 ` Zhangjin Wu
2023-06-07 4:17 ` Willy Tarreau
2023-06-07 6:33 ` Zhangjin Wu
2023-06-07 7:33 ` Willy Tarreau
2023-06-07 8:11 ` Zhangjin Wu [this message]
2023-06-07 10:44 ` Willy Tarreau
2023-06-06 4:25 ` [PATCH v3 0/3] nolibc: add part2 of support " Zhangjin Wu
2023-06-06 4:42 ` Willy Tarreau
2023-06-06 6:34 ` Zhangjin Wu
2023-06-06 6:45 ` Willy Tarreau
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230607081103.746962-1-falcon@tinylab.org \
--to=falcon@tinylab.org \
--cc=arnd@arndb.de \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=thomas@t-8ch.de \
--cc=w@1wt.eu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).