Message-Id: <20230718170153.35765-1-falcon@tinylab.org>
Date:   Wed, 19 Jul 2023 01:01:53 +0800
From:   Zhangjin Wu <falcon@...ylab.org>
To:     thomas@...ch.de
Cc:     arnd@...db.de, falcon@...ylab.org, linux-kernel@...r.kernel.org,
        linux-kselftest@...r.kernel.org, w@....eu
Subject: Re: [PATCH v1 00/22] selftests/nolibc: add minimal kernel config support

Hi, Thomas

> On 2023-07-18 21:43:23+0800, Zhangjin Wu wrote:
> > Hi, Willy, Thomas
> > 
> > I have prepared the powerpc + powerpc64 support together with the
> > tinyconfig for them, still have some questions for more discussion.
> > 
> > > On Wed, Jul 12, 2023 at 01:18:26AM +0800, Zhangjin Wu wrote:
> > [...]
> > > 
> > > Reading the beginning of the sentence made me immediately think that it's
> > > what doc is for. You know, if you give a fish to a hungry man he eats one
> > > day, if you teach him to fish he eats every day. Knowing what to download
> > > from where is much more instructive than running "make download" or even
> > > worse, "make" and having the downloads secretly succeed (or fail). If you
> > > think the doc is hard to find I'm also fine with a patch for makefile
> > > and/or nolibc-test passing a pointer to its location as a reminding
> > > comment for example.
> > >
> > 
> > The whole tinyconfig support for every architecture is 'huge', I plan to
> > send them one by one, if required, will document them with the required
> > bios and/or toolchain.
> 
> Which part is huge?

Actually, we only need to add ~30 lines for all of them ;-)

But some architectures require extra operations, for example,

- x86 requires extra "tsc=unstable noapic" kernel cmdline to save the
  CONFIG_ACPI options, if we only reserve tinyconfig target, it is ok to
  add it directly to the original QEMU_ARGS_<ARCH>

- x86 and some other architectures may fail to poweroff due to save some
  time-cost options or due to missing kernel poweroff support, they
  require qemu-poweroff timeout support

- some architectures have no built-in firmware for qemu, we need to
  document them carefully

- some architectures have no available toolchain in any Linux
  distributions, we need to document them carefully

...

> This is surprising.
>

For one architecture, it is not that hard, but for all of the supported
architectures, weird bugs came out one by one ;-)

> > The first architectures plan to support are powerpc + powerpc64, powerpc does
> > require extra kernel config options even with defconfig, so, it is a very good
> > first example, and the extconfig target will be added together.
> 
> Are you planning to do powerpc and tinyconfig support in one series?
> Splitting it would be better in my opinion.
>

Ok, as Willy replied in another thread, let's add powerpc support with
defconfig as the first patchset.

Since, even with defconfig, we still need extra console options to make
the test print normally, so, the extra options support code will be
added anyway, adding tinyconfig support is a fastforward step.

> > The left question from me is that if is it ok to just use tinyconfig instead of
> > defconfig after we enable tinyconfig for a new architecture, I mean just add a
> > new DEFCONFIG_<ARCH>=tinyconfig line for the new architecture, don't use the
> > 'defconfig' any more, let's take a look at the powerpc/powerpc64 lines below:
> > 
> >     # default kernel configurations that appear to be usable
> >     DEFCONFIG_i386       = defconfig
> >     DEFCONFIG_x86_64     = defconfig
> >     DEFCONFIG_x86        = defconfig
> >     DEFCONFIG_arm64      = defconfig
> >     DEFCONFIG_arm        = multi_v7_defconfig
> >     DEFCONFIG_mips       = malta_defconfig
> >     DEFCONFIG_powerpc    = tinyconfig
> >     DEFCONFIG_powerpc64  = tinyconfig
> >     DEFCONFIG_riscv      = defconfig
> >     DEFCONFIG_s390       = defconfig
> >     DEFCONFIG_loongarch  = defconfig
> >     DEFCONFIG            = $(DEFCONFIG_$(XARCH))
> > 
> > Of course, we need to customize the EXTCONFIG for them (about ~5 options for
> > all of the architectures):
> > 
> >     # extra kernel configs by architecture
> >     EXTCONFIG_powerpc    = $(addprefix -e ,COMPAT_32BIT_TIME PPC_PMAC PPC_OF_BOOT_TRAMPOLINE SERIAL_PMACZILOG SERIAL_PMACZILOG_TTYS SERIAL_PMACZILOG_CONSOLE)
> >     EXTCONFIG_powerpc64  = $(addprefix -e ,PPC64 CPU_LITTLE_ENDIAN PPC_POWERNV HVC_OPAL)
> >     EXTCONFIG_XARCH      = $(EXTCONFIG_$(XARCH))
> 
> These could also be put into dedicated config files. Then they don't
> clutter the makefile and it's easier to maintain.
> 
> nolibc.config:       Generic configs on top of tinyconfig
> nolibc.arm64.config: Arch-specific configs for arm64
>

Thanks, xxx.config files should work well with the merge tool:
scripts/kconfig/merge_config.sh.

Since in all of the supported architectures, the maximum extra options
required by tinyconfig is 8, the others is around 5 options (some even
less), all of them can be put in one line, at the same time, the
initramfs source can use the same EXTCONFIG_COMMON logic, so, the
oneline EXTCONFIG_<ARCH> is used without any new files.

Will take a look at the xxx.config files method.

But unfortunately, powerpc defconfig doesn't print by default, it also
require extra console options, to apply the xxx.config files method, we
need wait more time to restruct the powerpc support.

> Also the extra parameter could also be passed via command line arguments to make.
> This way we don't have to modify the configuration options at all.
> The user can provide a config (or we use a tinyconfig) and everything
> required by nolibc-test is enabled on top when building.
> 
> make CONFIG_COMPAT_32BIT_TIME=y ...
>

This doesn't work sometimes especially when we want to disable an
option, that's why we eventually use scripts/config instead.

> tools/testing/selftests/wireguard/qemu/ seems to be doing something
> similar as nolibc-test.
> 
> > The extra common options (based on default kernel tinyconfig) are also required
> > to make nolibc-test.c passes without failures (~2 skips are procfs related, so,
> > procfs is not added) are minimal:
> > 
> >     # extra kernel configs shared among architectures
> >     EXTCONFIG_COMMON     = -e BLK_DEV_INITRD --set-str INITRAMFS_SOURCE $(CURDIR)/initramfs
> >     EXTCONFIG_COMMON    += -e BINFMT_ELF
> >     EXTCONFIG_COMMON    += -e PRINTK -e TTY
> > 
> > Compare to defconfig, tinyconfig not only allows test all of the nolibc
> > functions, but also is faster (around ~1-2 minutes, defconfig may cost ~30
> > minutes and even more) and brings us with smaller image size.
> > 
> > To only test nolibc itself, I do think tinyconfig with the above
> > extconfig support is enough, even if we need more, we can update the
> > EXTCONFIG_COMMON and EXTCONFIG_<ARCH> in the future.
> 
> IMO tinyconfig is enough, defconfig doesn't seem to be necessary then.
>

As Willy replied in another thread, tinyconfig may fail sometimes
because many developers only use defconfig for tests, I did find
tinyconfig fail on powerpc [1] and riscv64 [2], although later, both of
them have been fixed.

I reported the 2nd one, the 1st one has just been fixed up by somebody
else before I plan to send the patch, so, our nolibc tinyconfig support
may function as a very good coverage test for kernel ;-)

[1]: https://lore.kernel.org/lkml/20230629124500.1.I55e2f4e7903d686c4484cb23c033c6a9e1a9d4c4@changeid/
[2]: https://lore.kernel.org/linux-riscv/20230716001506.3506041-1-guoren@kernel.org/

> > I have prepared tinyconfig for all of the supported architectures
> > locally, If you agree with only reserve the DEFCONFIG_<ARCH>=tinyconfig
> > line, I will send a series of patchset to add tinyconfig for every
> > architecture with it, at last, it will become:
> > 
> >     # default kernel configurations that appear to be usable
> >     DEFCONFIG_i386       = tinyconfig
> >     DEFCONFIG_x86_64     = tinyconfig
> >     DEFCONFIG_x86        = tinyconfig
> >     DEFCONFIG_arm64      = tinyconfig
> >     DEFCONFIG_arm        = tinyconfig
> >     DEFCONFIG_mips       = tinyconfig
> >     DEFCONFIG_powerpc    = tinyconfig
> >     DEFCONFIG_powerpc64  = tinyconfig
> >     DEFCONFIG_riscv      = tinyconfig
> >     DEFCONFIG_s390       = tinyconfig
> >     DEFCONFIG_loongarch  = tinyconfig
> >     DEFCONFIG            = $(DEFCONFIG_$(XARCH))
> > 
> > So, perhaps it is better to simply use tinyconfig as the default DEFCONFIG, and
> > therefore there is no need to add powerpc and powerpc64 specific lines:
> > 
> >     # default kernel configurations that appear to be usable
> >     DEFCONFIG_i386       = defconfig
> >     DEFCONFIG_x86_64     = defconfig
> >     DEFCONFIG_x86        = defconfig
> >     DEFCONFIG_arm64      = defconfig
> >     DEFCONFIG_arm        = multi_v7_defconfig
> >     DEFCONFIG_mips       = malta_defconfig
> >     DEFCONFIG_riscv      = defconfig
> >     DEFCONFIG_s390       = defconfig
> >     DEFCONFIG_loongarch  = defconfig
> >     DEFCONFIG            = $(or $(DEFCONFIG_$(XARCH)),tinyconfig)
> > 
> > To support tinyconfig for a new architecture, we can simply remove the
> > 'DEFCONFIG_<ARCH> = defconfig' line and get the core options from
> > defconfig to customize the EXTCONFIG_ARCH, with tinyconfig, it is very
> > fast and easy to verify the run target for a new architecture.
> > 
> > At last, we will have many EXTCONFIG_<ARCH> lines and only a DEFCONFIG line:
> > 
> >     # default kernel configurations that appear to be usable
> >     DEFCONFIG            = $(or $(DEFCONFIG_$(XARCH)),tinyconfig)
> > 
> > Or at last, we remove the above line and the defconfig target and only reserve
> > a tinyconfig target:
> > 
> >     tinyconfig:
> > 	$(Q)$(MAKE_KERNEL) tinyconfig prepare
> > 
> > Welcome your suggestion.
> 
> Looks fine to me either way.

Ok, so, let's reserve defconfig as a backup although tinyconfig should
work at most of the time.

Best regards,
Zhangjin

Powered by blists - more mailing lists