linux-riscv.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: Zhangjin Wu <falcon@tinylab.org>
To: thomas@t-8ch.de
Cc: falcon@tinylab.org, linux-kernel@vger.kernel.org,
	linux-kselftest@vger.kernel.org, linux-riscv@lists.infradead.org,
	palmer@dabbelt.com, paul.walmsley@sifive.com, w@1wt.eu
Subject: Re: [PATCH 05/13] selftests/nolibc: riscv: customize makefile for rv32
Date: Fri, 26 May 2023 17:20:29 +0800	[thread overview]
Message-ID: <20230526092029.149351-1-falcon@tinylab.org> (raw)
In-Reply-To: <4a3b1cdf-91d5-4668-925e-21f8f5c64a92@t-8ch.de>

Hi, Thomas

> On 2023-05-25 01:50:57+0800, Zhangjin Wu wrote:
> > both riscv64 and riscv32 have the same ARCH value, it is riscv, the
> > default defconfig enables 64bit, to support riscv32, let's allow pass
> > "ARCH=riscv32" or "ARCH=riscv CONFIG_32BIT=1" to customize riscv32
> > setting.
> 
> What's the advantage of doing CONFIG_32BIT? For i386/x86_64, arm/arm64
> it's not necessary either.
> (Let's ignore the "x86" case)
>

Very good question, thanks.

This requirement may happen on mips, loongarch and even powerpc too, both x86
and arm are just the 'exception'.

It is 'designed' as a temp flag/variable to specifiy that current arch is
riscv32, not riscv64, of course, we can use something like this or even
use a meaningless 't' variable:

    # Allow pass ARCH=riscv|riscv32|riscv64, riscv implies riscv64
    ifneq ($(findstring xriscv,x$(ARCH)),)
      riscv32 := $(if $(findstring riscv32x,$(ARCH)x),1)
      override ARCH := riscv
    endif

Using CONFIG_32BIT instead of riscv32 has some extra considerations:

* Using it in command line is a 'side effect', if it is a meaningless
  variable, we will not use it for we can not remember it, here use it as a
  choice, riscv32 is enough, we can simply remove this comment from the
  commit message.

* The architectures like riscv, mips, loongarch share the same source code tree
  between 32bit and 64bit and even 128bit in the future, x86 and arm just not
  do so.

  The ARCH specified here differs from the one to kernel make, we should
  provide a flag/variable or anther ARCH variant to show the
  difference, _ARCH or XARCH have been used in my local patch.

  CONFIG_32BIT is meaningful to reflect the difference, even for future,
  we can use the same thing CONFIG_64BIT, CONFIG_128BIT, so simply
  BITS=32, BITS=64, BITS=128, but that is hard to be used as a oneline
  condition statement (although we can use something like findstring).

      $(if $(findstring x32y,x$(BITS)y),something whatevever)

      v.s.

      $(if $(CONFIG_32BIT),something whatevever)

  If not use a tmp flag/variable, this works too, but duplicated :-)

      $(if $(findstring xriscv32y,x$(ARCH)y),something whatevever)

* We are able to auto detect this config from include/config/auto.conf,
  there will be something like CONFIG_32BIT=y there.

  we did use such auto detect logic in my local patch, but it has some
  issues if we want a riscv64 build after we did a riscv32 config if we
  only pass ARCH=riscv, so, I just removed that logic but reserved the
  pontential for future.

> If for riscv the "normal" version is riscv64 then adding a new "riscv32" that
> works the same as the other architectures seems nicer and easier.
> 

The complexity here is what just explained above: The ARCH specified
here differs from the one to kernel make.

It is ok to add riscv32 like the other architectures as following:

    ifeq ($(ARCH),riscv32)
      _ARCH := riscv
    else
      _ARCH := $(ARCH)
    endif

    IMAGE_riscv32 = arch/riscv/boot/Image
    DEFCONFIG_riscv32 = rv32_defconfig
    QEMU_ARCH_riscv32 = riscv32
    QEMU_ARGS_riscv32 = -M virt -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
    CFLAGS_riscv32 = -march=rv32im -mabi=ilp32

And all of the other 'ARCH' variables passed to kernel 'make' should be
changed to $(_ARCH), include most of the core targets, like:

    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
    
It is not that easier, it touched more source code and make things a
little complex, we must mixes the using of ARCH and _ARCH in the whole
Makefile and that is not comfortable and may introduce more complexity,
for example, we may be worry about if the directories should be named
with the new $(_ARCH) ;-)

And CONFIG_32BIT variable is better than riscv32, because, we can share this
meaningful variable among mips, loongarch in the future if their maintainers
want to add more variants support for such platforms, they will meet the same
requirement.

Thanks very much.

Best regards,
Zhangjin

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

  reply	other threads:[~2023-05-26  9:20 UTC|newest]

Thread overview: 60+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-05-24 17:33 [PATCH 00/13] tools/nolibc: riscv: Add full rv32 support Zhangjin Wu
2023-05-24 17:41 ` [PATCH 01/13] Revert "tools/nolibc: riscv: Support __NR_llseek for rv32" Zhangjin Wu
2023-05-24 17:44 ` [PATCH 02/13] Revert "selftests/nolibc: Fix up compile error " Zhangjin Wu
2023-05-24 17:48 ` [PATCH 04/13] selftests/nolibc: syscall_args: use __NR_statx for rv32 Zhangjin Wu
2023-05-24 19:49   ` Thomas Weißschuh
2023-05-25  7:20     ` Zhangjin Wu
2023-05-26  9:21   ` Arnd Bergmann
2023-05-26 10:06     ` Willy Tarreau
2023-05-27  0:58     ` Zhangjin Wu
2023-05-24 17:50 ` [PATCH 05/13] selftests/nolibc: riscv: customize makefile " Zhangjin Wu
2023-05-26  6:57   ` Thomas Weißschuh
2023-05-26  9:20     ` Zhangjin Wu [this message]
2023-05-24 17:52 ` [PATCH 06/13] selftests/nolibc: allow specify a bios for qemu Zhangjin Wu
2023-05-26  7:00   ` Thomas Weißschuh
2023-05-26 10:25     ` Zhangjin Wu
2023-05-26 10:36       ` Conor Dooley
2023-05-26 13:38         ` Zhangjin Wu
2023-05-26 15:08           ` Conor Dooley
2023-05-28  7:52     ` Willy Tarreau
2023-05-24 17:54 ` [PATCH 07/13] selftests/nolibc: remove the duplicated gettimeofday_bad2 Zhangjin Wu
2023-05-24 17:55 ` [PATCH 08/13] tools/nolibc: sys_lseek: riscv: use __NR_llseek for rv32 Zhangjin Wu
2023-05-24 17:57 ` [PATCH 09/13] tools/nolibc: sys_poll: riscv: use __NR_ppoll_time64 " Zhangjin Wu
2023-05-26  7:15   ` Thomas Weißschuh
2023-05-26  9:34     ` Arnd Bergmann
2023-05-28  8:25       ` Zhangjin Wu
2023-05-28  8:48         ` Arnd Bergmann
2023-05-28 10:29         ` Willy Tarreau
2023-05-28 10:55           ` Arnd Bergmann
2023-05-28 11:03             ` Willy Tarreau
2023-05-24 17:58 ` [PATCH 10/13] tools/nolibc: ppoll/ppoll_time64: add a missing argument Zhangjin Wu
2023-05-24 17:59 ` [PATCH 11/13] tools/nolibc: sys_select: riscv: use __NR_pselect6_time64 for rv32 Zhangjin Wu
2023-05-24 20:22   ` Thomas Weißschuh
2023-05-25  7:10     ` Zhangjin Wu
2023-05-25  7:22       ` Thomas Weißschuh
2023-05-26  1:50         ` Zhangjin Wu
2023-05-26  9:19   ` Arnd Bergmann
2023-05-26 11:00     ` [PATCH 00/13] tools/nolibc: riscv: Add full rv32 support Zhangjin Wu
2023-05-26 11:13       ` Arnd Bergmann
2023-05-24 18:02 ` [PATCH 12/13] tools/nolibc: sys_wait4: riscv: use __NR_waitid for rv32 Zhangjin Wu
2023-05-24 18:03 ` [PATCH 13/13] tools/nolibc: sys_gettimeofday: riscv: use __NR_clock_gettime64 " Zhangjin Wu
2023-05-26  7:38   ` Thomas Weißschuh
2023-05-27  1:26     ` Zhangjin Wu
2023-05-27  3:39       ` Zhangjin Wu
2023-05-27  5:12       ` Willy Tarreau
2023-05-24 18:24 ` [PATCH 00/13] tools/nolibc: riscv: Add full rv32 support Zhangjin Wu
2023-05-24 18:28 ` [PATCH 03/13] selftests/nolibc: print name instead of number for EOVERFLOW Zhangjin Wu
2023-05-24 17:46   ` Zhangjin Wu
2023-05-24 20:23   ` Thomas Weißschuh
2023-05-28  7:59 ` [PATCH 00/13] tools/nolibc: riscv: Add full rv32 support Willy Tarreau
2023-05-28  8:42   ` Thomas Weißschuh
2023-05-28  9:41     ` Thomas Weißschuh
2023-05-28 10:17       ` Willy Tarreau
2023-05-28 10:39   ` Zhangjin Wu
2023-05-28 11:33     ` Willy Tarreau
2023-05-28 12:52       ` Zhangjin Wu
2023-05-28 13:45     ` Thomas Weißschuh 
2023-05-28 18:39       ` Zhangjin Wu
2023-05-29  8:45         ` Thomas Weißschuh
2023-05-29 11:31           ` Willy Tarreau
2023-05-30 10:06             ` Zhangjin Wu

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=20230526092029.149351-1-falcon@tinylab.org \
    --to=falcon@tinylab.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=linux-riscv@lists.infradead.org \
    --cc=palmer@dabbelt.com \
    --cc=paul.walmsley@sifive.com \
    --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).