diff mbox series

[v3,3/7] selftests/nolibc: add extra configs customize support

Message ID 8e9e5ac6283c6ec2ecf10a70ce55b219028497c1.1690468707.git.falcon@tinylab.org (mailing list archive)
State New
Headers show
Series tools/nolibc: add 32/64-bit powerpc support | expand

Commit Message

Zhangjin Wu July 27, 2023, 3:02 p.m. UTC 
The default DEFCONFIG_<ARCH> may not always work for all architectures,
some architectures require to add extra kernel config options, this
allows to add extra options in the defconfig target.

Based on the .config generated from DEFCONFIG_<ARCH>, It allows to
customize extra kernel config options via both the common common.config
and the architecture specific <ARCH>.config, at last trigger
'allnoconfig' to let them take effect with missing config options as
disabled.

The scripts/kconfig/merge_config.sh tool is used to merge the extra
config files.

Suggested-by: Thomas Weißschuh <linux@weissschuh.net>
Link: https://lore.kernel.org/lkml/67eb70d4-c9ff-4afc-bac7-7f36cc2c81bc@t-8ch.de/
Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>
Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
---
 tools/testing/selftests/nolibc/Makefile | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Thomas Weißschuh July 29, 2023, 12:36 p.m. UTC | #1 
On 2023-07-27 23:02:02+0800, Zhangjin Wu wrote:
> The default DEFCONFIG_<ARCH> may not always work for all architectures,
> some architectures require to add extra kernel config options, this
> allows to add extra options in the defconfig target.
> 
> Based on the .config generated from DEFCONFIG_<ARCH>, It allows to
> customize extra kernel config options via both the common common.config
> and the architecture specific <ARCH>.config, at last trigger
> 'allnoconfig' to let them take effect with missing config options as
> disabled.
> 
> The scripts/kconfig/merge_config.sh tool is used to merge the extra
> config files.
> 
> Suggested-by: Thomas Weißschuh <linux@weissschuh.net>
> Link: https://lore.kernel.org/lkml/67eb70d4-c9ff-4afc-bac7-7f36cc2c81bc@t-8ch.de/
> Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>
> Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
> ---
>  tools/testing/selftests/nolibc/Makefile | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
> index f42adef87e12..9576f1a0a98d 100644
> --- a/tools/testing/selftests/nolibc/Makefile
> +++ b/tools/testing/selftests/nolibc/Makefile
> @@ -39,6 +39,9 @@ DEFCONFIG_s390       = defconfig
>  DEFCONFIG_loongarch  = defconfig
>  DEFCONFIG            = $(DEFCONFIG_$(ARCH))
>  
> +# extra kernel config files under configs/, include common + architecture specific
> +EXTCONFIG            = common.config $(ARCH).config

As this series seems to need a respin anyways:

extconfig means "extended config", correct?
That is fairly nondescript.

I would prefer something like "NOLIBC_TEST_CONFIG" and something like
"make nolibctestconfig" to make an existing config ready for
nolibc-test.

> +
>  # optional tests to run (default = all)
>  TEST =
>  
> @@ -161,6 +164,8 @@ initramfs: nolibc-test
>  
>  defconfig:
>  	$(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare
> +	$(Q)$(srctree)/scripts/kconfig/merge_config.sh -O "$(srctree)" -m "$(srctree)/.config" $(foreach c,$(EXTCONFIG),$(wildcard $(CURDIR)/configs/$c))
> +	$(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) KCONFIG_ALLCONFIG="$(srctree)/.config" allnoconfig
>  
>  kernel: initramfs
>  	$(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs
> -- 
> 2.25.1
>
Zhangjin Wu July 29, 2023, 2:39 p.m. UTC | #2 
> On 2023-07-27 23:02:02+0800, Zhangjin Wu wrote:
> > The default DEFCONFIG_<ARCH> may not always work for all architectures,
> > some architectures require to add extra kernel config options, this
> > allows to add extra options in the defconfig target.
> > 
> > Based on the .config generated from DEFCONFIG_<ARCH>, It allows to
> > customize extra kernel config options via both the common common.config
> > and the architecture specific <ARCH>.config, at last trigger
> > 'allnoconfig' to let them take effect with missing config options as
> > disabled.
> > 
> > The scripts/kconfig/merge_config.sh tool is used to merge the extra
> > config files.
> > 
> > Suggested-by: Thomas Weißschuh <linux@weissschuh.net>
> > Link: https://lore.kernel.org/lkml/67eb70d4-c9ff-4afc-bac7-7f36cc2c81bc@t-8ch.de/
> > Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>
> > Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
> > ---
> >  tools/testing/selftests/nolibc/Makefile | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
> > index f42adef87e12..9576f1a0a98d 100644
> > --- a/tools/testing/selftests/nolibc/Makefile
> > +++ b/tools/testing/selftests/nolibc/Makefile
> > @@ -39,6 +39,9 @@ DEFCONFIG_s390       = defconfig
> >  DEFCONFIG_loongarch  = defconfig
> >  DEFCONFIG            = $(DEFCONFIG_$(ARCH))
> >  
> > +# extra kernel config files under configs/, include common + architecture specific
> > +EXTCONFIG            = common.config $(ARCH).config
> 
> As this series seems to need a respin anyways:
> 
> extconfig means "extended config", correct?
> That is fairly nondescript.
>

It is more about 'extra' as commented (or 'additional'), for both
defconfig (may) and tinyconfig (must) require more options to make boot
and print work for nolibc-test.
         
    defconfig ------\
                     \
                      \
                      EXTCONFIG ----> a working .config for nolibc-test
                      /
                     /
    tinyconfig------/

> I would prefer something like "NOLIBC_TEST_CONFIG"
>

Using NOLIBC_TEST_CONFIG is ok, but with this name, do we still only put
the 'additional' options there? or we simply use EXTRA_CONFIG instead?

    # extra kernel config files under configs/, include common + architecture specific
    EXTRA_CONFIG       = common.config $(ARCH).config

From the name, NOLIBC_TEST_CONFIG should be a standalone config file to
include all necessary options? but as Willy suggested, he want to
reserve defconfig as an optional target, and tinyconfig does may be more
easier to fail than defconfig, if only consider tinyconfig, it is ok for
us to put all of the .config generated from tinyconfig + extra config to
NOLIBC_TEST_CONFIG.

   NOLIBC_TEST_CONFIG = tinyconfig + common.config + $(ARCH).config

But it may be harder to maintain a standalone config than an additional
config file.

> something like "make nolibctestconfig" to make an existing config ready for
> nolibc-test.

Do you mean rename 'defconfig' to 'nolibctestconfig'? or something
nolibc-test-config:

    nolibc-test-config:
	$(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare
	$(Q)$(srctree)/scripts/kconfig/merge_config.sh -O "$(srctree)" -m "$(srctree)/.config" $(foreach c,$(EXTRA_CONFIG),$(wildcard $(CURDIR)/configs/$c))
	$(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) KCONFIG_ALLCONFIG="$(srctree)/.config" allnoconfig

It looks too long ;-)

Currently, we use 'defconfig' by default and we use 'make defconfig
DEFCONFIG=tinyconfig' to switch to tinyconfig, in the next weeks, when
all of the nolibc supported architectures have tinyconfig support, it is
able to switch 'tinyconfig' as the default config target.

    PHONY += $(KERNEL_CONFIG)
    $(KERNEL_CONFIG):
    	$(Q)if [ ! -f "$(KERNEL_CONFIG)" ]; then $(MAKE) --no-print-directory defconfig DEFCONFIG=tinyconfig; fi

    kernel: $(KERNEL_CONFIG)
    	$(Q)$(MAKE) --no-print-directory initramfs
    	$(Q)$(MAKE_KERNEL) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs

Welcome more discussion.

Thanks,
Zhangjin

> > +
> >  # optional tests to run (default = all)
> >  TEST =
> >  
> > @@ -161,6 +164,8 @@ initramfs: nolibc-test
> >  
> >  defconfig:
> >  	$(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare
> > +	$(Q)$(srctree)/scripts/kconfig/merge_config.sh -O "$(srctree)" -m "$(srctree)/.config" $(foreach c,$(EXTCONFIG),$(wildcard $(CURDIR)/configs/$c))
> > +	$(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) KCONFIG_ALLCONFIG="$(srctree)/.config" allnoconfig
> >  
> >  kernel: initramfs
> >  	$(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs
> > -- 
> > 2.25.1
> > 
>
Willy Tarreau July 29, 2023, 4:29 p.m. UTC | #3 
On Sat, Jul 29, 2023 at 10:39:33PM +0800, Zhangjin Wu wrote:
> > > diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
> > > index f42adef87e12..9576f1a0a98d 100644
> > > --- a/tools/testing/selftests/nolibc/Makefile
> > > +++ b/tools/testing/selftests/nolibc/Makefile
> > > @@ -39,6 +39,9 @@ DEFCONFIG_s390       = defconfig
> > >  DEFCONFIG_loongarch  = defconfig
> > >  DEFCONFIG            = $(DEFCONFIG_$(ARCH))
> > >  
> > > +# extra kernel config files under configs/, include common + architecture specific
> > > +EXTCONFIG            = common.config $(ARCH).config
> > 
> > As this series seems to need a respin anyways:
> > 
> > extconfig means "extended config", correct?
> > That is fairly nondescript.
> >
> 
> It is more about 'extra' as commented (or 'additional'), for both
> defconfig (may) and tinyconfig (must) require more options to make boot
> and print work for nolibc-test.
>          
>     defconfig ------\
>                      \
>                       \
>                       EXTCONFIG ----> a working .config for nolibc-test
>                       /
>                      /
>     tinyconfig------/
> 
> > I would prefer something like "NOLIBC_TEST_CONFIG"
> >
> 
> Using NOLIBC_TEST_CONFIG is ok, but with this name, do we still only put
> the 'additional' options there? or we simply use EXTRA_CONFIG instead?
> 
>     # extra kernel config files under configs/, include common + architecture specific
>     EXTRA_CONFIG       = common.config $(ARCH).config

Either are fine to me. The most important is to mention that these
configs are appended to the config during the defconfig and tinyconfig
targets.

Also I find it odd to use $(ARCH) here, I would have expected $(XARCH)
since you probably want to distinguish ppc64 from ppc for example.

> > something like "make nolibctestconfig" to make an existing config ready for
> > nolibc-test.
> 
> Do you mean rename 'defconfig' to 'nolibctestconfig'? or something
> nolibc-test-config:
> 
>     nolibc-test-config:
> 	$(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare
> 	$(Q)$(srctree)/scripts/kconfig/merge_config.sh -O "$(srctree)" -m "$(srctree)/.config" $(foreach c,$(EXTRA_CONFIG),$(wildcard $(CURDIR)/configs/$c))
> 	$(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) KCONFIG_ALLCONFIG="$(srctree)/.config" allnoconfig
> 
> It looks too long ;-)

I think that as long as we don't claim to call topdir's makefile targets
from this directory, we can reuse some similarly named targets which are
documented in "make help" and are non-ambiguous.

> Currently, we use 'defconfig' by default and we use 'make defconfig
> DEFCONFIG=tinyconfig' to switch to tinyconfig, in the next weeks, when
> all of the nolibc supported architectures have tinyconfig support, it is
> able to switch 'tinyconfig' as the default config target.

As long as it doesn't require to locally maintain too many options, I
think I'm fine with that. But we'll see.

Willy
Zhangjin Wu July 29, 2023, 4:54 p.m. UTC | #4 
> On Sat, Jul 29, 2023 at 10:39:33PM +0800, Zhangjin Wu wrote:
> > > > diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
> > > > index f42adef87e12..9576f1a0a98d 100644
> > > > --- a/tools/testing/selftests/nolibc/Makefile
> > > > +++ b/tools/testing/selftests/nolibc/Makefile
> > > > @@ -39,6 +39,9 @@ DEFCONFIG_s390       = defconfig
> > > >  DEFCONFIG_loongarch  = defconfig
> > > >  DEFCONFIG            = $(DEFCONFIG_$(ARCH))
> > > >  
> > > > +# extra kernel config files under configs/, include common + architecture specific
> > > > +EXTCONFIG            = common.config $(ARCH).config
> > > 
> > > As this series seems to need a respin anyways:
> > > 
> > > extconfig means "extended config", correct?
> > > That is fairly nondescript.
> > >
> > 
> > It is more about 'extra' as commented (or 'additional'), for both
> > defconfig (may) and tinyconfig (must) require more options to make boot
> > and print work for nolibc-test.
> >          
> >     defconfig ------\
> >                      \
> >                       \
> >                       EXTCONFIG ----> a working .config for nolibc-test
> >                       /
> >                      /
> >     tinyconfig------/
> > 
> > > I would prefer something like "NOLIBC_TEST_CONFIG"
> > >
> > 
> > Using NOLIBC_TEST_CONFIG is ok, but with this name, do we still only put
> > the 'additional' options there? or we simply use EXTRA_CONFIG instead?
> > 
> >     # extra kernel config files under configs/, include common + architecture specific
> >     EXTRA_CONFIG       = common.config $(ARCH).config
> 
> Either are fine to me. The most important is to mention that these
> configs are appended to the config during the defconfig and tinyconfig
> targets.
>

Ok, will update the comment to something like this:

     # extra configs/ files appended to .config during the defconfig and tinyconfig targets
     # include common parts + architecture specific parts
     EXTRA_CONFIG       = common.config $(ARCH).config


> Also I find it odd to use $(ARCH) here, I would have expected $(XARCH)
> since you probably want to distinguish ppc64 from ppc for example.
>

Yes, we do, but the XARCH and ARCH mmapping patch is the 4th, will
update this to XARCH, this one is the 3th one, do we need to add this
one after the 4th one?

> > > something like "make nolibctestconfig" to make an existing config ready for
> > > nolibc-test.
> > 
> > Do you mean rename 'defconfig' to 'nolibctestconfig'? or something
> > nolibc-test-config:
> > 
> >     nolibc-test-config:
> > 	$(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare
> > 	$(Q)$(srctree)/scripts/kconfig/merge_config.sh -O "$(srctree)" -m "$(srctree)/.config" $(foreach c,$(EXTRA_CONFIG),$(wildcard $(CURDIR)/configs/$c))
> > 	$(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) KCONFIG_ALLCONFIG="$(srctree)/.config" allnoconfig
> > 
> > It looks too long ;-)
> 
> I think that as long as we don't claim to call topdir's makefile targets
> from this directory, we can reuse some similarly named targets which are
> documented in "make help" and are non-ambiguous.

Seems 'nolibc-test-config' is really more meaningful than 'defconfig', especially
when we want to use tinyconfig through it?

    $ make nolibc-test-config DEFCONFIG=tinyconfig

> 
> > Currently, we use 'defconfig' by default and we use 'make defconfig
> > DEFCONFIG=tinyconfig' to switch to tinyconfig, in the next weeks, when
> > all of the nolibc supported architectures have tinyconfig support, it is
> > able to switch 'tinyconfig' as the default config target.
> 
> As long as it doesn't require to locally maintain too many options, I
> think I'm fine with that. But we'll see.

Ok.

Thanks,
Zhangjin

> 
> Willy
Willy Tarreau July 29, 2023, 5:10 p.m. UTC | #5 
On Sun, Jul 30, 2023 at 12:54:45AM +0800, Zhangjin Wu wrote:
> > Also I find it odd to use $(ARCH) here, I would have expected $(XARCH)
> > since you probably want to distinguish ppc64 from ppc for example.
> >
> 
> Yes, we do, but the XARCH and ARCH mmapping patch is the 4th, will
> update this to XARCH, this one is the 3th one, do we need to add this
> one after the 4th one?

OK indeed it's the 4th one that will modify this one then, no need
to reorder.

> > > > something like "make nolibctestconfig" to make an existing config ready for
> > > > nolibc-test.
> > > 
> > > Do you mean rename 'defconfig' to 'nolibctestconfig'? or something
> > > nolibc-test-config:
> > > 
> > >     nolibc-test-config:
> > > 	$(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare
> > > 	$(Q)$(srctree)/scripts/kconfig/merge_config.sh -O "$(srctree)" -m "$(srctree)/.config" $(foreach c,$(EXTRA_CONFIG),$(wildcard $(CURDIR)/configs/$c))
> > > 	$(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) KCONFIG_ALLCONFIG="$(srctree)/.config" allnoconfig
> > > 
> > > It looks too long ;-)
> > 
> > I think that as long as we don't claim to call topdir's makefile targets
> > from this directory, we can reuse some similarly named targets which are
> > documented in "make help" and are non-ambiguous.
> 
> Seems 'nolibc-test-config' is really more meaningful than 'defconfig', especially
> when we want to use tinyconfig through it?
> 
>     $ make nolibc-test-config DEFCONFIG=tinyconfig

As a user, I'd ask "why not make tinyconfig" ? But see my other message,
now I'm having strong doubts about the relevance of tinyconfig if it works
as bad as you described it.

Willy
Zhangjin Wu July 30, 2023, 4:54 a.m. UTC | #6 
> On Sun, Jul 30, 2023 at 12:54:45AM +0800, Zhangjin Wu wrote:
> > > Also I find it odd to use $(ARCH) here, I would have expected $(XARCH)
> > > since you probably want to distinguish ppc64 from ppc for example.
> > >
> > 
> > Yes, we do, but the XARCH and ARCH mmapping patch is the 4th, will
> > update this to XARCH, this one is the 3rd one, do we need to add this
> > one after the 4th one?
> 
> OK indeed it's the 4th one that will modify this one then, no need
> to reorder.
> 
> > > > > something like "make nolibctestconfig" to make an existing config ready for
> > > > > nolibc-test.
> > > > 
> > > > Do you mean rename 'defconfig' to 'nolibctestconfig'? or something
> > > > nolibc-test-config:
> > > > 
> > > >     nolibc-test-config:
> > > > 	$(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare
> > > > 	$(Q)$(srctree)/scripts/kconfig/merge_config.sh -O "$(srctree)" -m "$(srctree)/.config" $(foreach c,$(EXTRA_CONFIG),$(wildcard $(CURDIR)/configs/$c))
> > > > 	$(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) KCONFIG_ALLCONFIG="$(srctree)/.config" allnoconfig
> > > > 
> > > > It looks too long ;-)
> > > 
> > > I think that as long as we don't claim to call topdir's makefile targets
> > > from this directory, we can reuse some similarly named targets which are
> > > documented in "make help" and are non-ambiguous.
> > 
> > Seems 'nolibc-test-config' is really more meaningful than 'defconfig', especially
> > when we want to use tinyconfig through it?
> > 
> >     $ make nolibc-test-config DEFCONFIG=tinyconfig
> 
> As a user, I'd ask "why not make tinyconfig" ? But see my other message,
> now I'm having strong doubts about the relevance of tinyconfig if it works
> as bad as you described it.
>

I have added a nolibc tinyconfig target before, as the same reason,
based on your suggestion, I have removed the tinyconfig target and even
moved the extconfig to this defconfig to avoid add more similar or extra
complex targets in nolibc Makefile. before, tinyconfig + extconfig
together work for nolibc-test, so, tinyconfig is the same as the
top-level one, it should be removed as your suggested.

But since now, we are ready to get a real different target from the
top-level one, we may be able to have different targets for
'defconfig+EXTRA_CONFIG' and 'tinyconfig+EXTRA_CONFIG' like this:

    nolibc-test-config:
            $(Q)echo $(MAKE_KERNEL) mrproper $(or $(CONFIG),defconfig)
            $(Q)echo $(srctree)/scripts/kconfig/merge_config.sh -Q -O "$(objtree)" -m "$(KERNEL_CONFIG)" $(foreach c,$(EXTRA_CONFIG),$(wildcard $(CURDIR)/configs/$c))
            $(Q)echo $(MAKE_KERNEL) KCONFIG_ALLCONFIG="$(KERNEL_CONFIG)" allnoconfig
            $(Q)echo $(MAKE_KERNEL) prepare

    nolibc-test-defconfig nolibc-test-tinyconfig: nolibc-test-config
    nolibc-test-tinyconfig: CONFIG=tinyconfig

The complexity here is we have planned to support both defconfig and
tinyconfig, what about this solution?

Thanks,
Zhangjin

> Willy
Zhangjin Wu July 30, 2023, 6:01 a.m. UTC | #7 
Hi, Willy, Thomas

> > On Sun, Jul 30, 2023 at 12:54:45AM +0800, Zhangjin Wu wrote:
> > > > Also I find it odd to use $(ARCH) here, I would have expected $(XARCH)
> > > > since you probably want to distinguish ppc64 from ppc for example.
> > > >
> > > 
> > > Yes, we do, but the XARCH and ARCH mmapping patch is the 4th, will
> > > update this to XARCH, this one is the 3rd one, do we need to add this
> > > one after the 4th one?
> > 
> > OK indeed it's the 4th one that will modify this one then, no need
> > to reorder.
> > 
> > > > > > something like "make nolibctestconfig" to make an existing config ready for
> > > > > > nolibc-test.
> > > > > 
> > > > > Do you mean rename 'defconfig' to 'nolibctestconfig'? or something
> > > > > nolibc-test-config:
> > > > > 
> > > > >     nolibc-test-config:
> > > > > 	$(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare
> > > > > 	$(Q)$(srctree)/scripts/kconfig/merge_config.sh -O "$(srctree)" -m "$(srctree)/.config" $(foreach c,$(EXTRA_CONFIG),$(wildcard $(CURDIR)/configs/$c))
> > > > > 	$(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) KCONFIG_ALLCONFIG="$(srctree)/.config" allnoconfig
> > > > > 
> > > > > It looks too long ;-)
> > > > 
> > > > I think that as long as we don't claim to call topdir's makefile targets
> > > > from this directory, we can reuse some similarly named targets which are
> > > > documented in "make help" and are non-ambiguous.
> > > 
> > > Seems 'nolibc-test-config' is really more meaningful than 'defconfig', especially
> > > when we want to use tinyconfig through it?
> > > 
> > >     $ make nolibc-test-config DEFCONFIG=tinyconfig
> > 
> > As a user, I'd ask "why not make tinyconfig" ? But see my other message,
> > now I'm having strong doubts about the relevance of tinyconfig if it works
> > as bad as you described it.
> >
> 
> I have added a nolibc tinyconfig target before, as the same reason,
> based on your suggestion, I have removed the tinyconfig target and even
> moved the extconfig to this defconfig to avoid add more similar or extra
> complex targets in nolibc Makefile. before, tinyconfig + extconfig
> together work for nolibc-test, so, tinyconfig is the same as the
> top-level one, it should be removed as your suggested.
> 
> But since now, we are ready to get a real different target from the
> top-level one, we may be able to have different targets for
> 'defconfig+EXTRA_CONFIG' and 'tinyconfig+EXTRA_CONFIG' like this:
> 
>     nolibc-test-config:
>             $(Q)echo $(MAKE_KERNEL) mrproper $(or $(CONFIG),defconfig)

It should be $(or $(CONFIG),$(DEFCONFIG))

>             $(Q)echo $(srctree)/scripts/kconfig/merge_config.sh -Q -O "$(objtree)" -m "$(KERNEL_CONFIG)" $(foreach c,$(EXTRA_CONFIG),$(wildcard $(CURDIR)/configs/$c))
>             $(Q)echo $(MAKE_KERNEL) KCONFIG_ALLCONFIG="$(KERNEL_CONFIG)" allnoconfig
>             $(Q)echo $(MAKE_KERNEL) prepare
> 
>     nolibc-test-defconfig nolibc-test-tinyconfig: nolibc-test-config
>     nolibc-test-tinyconfig: CONFIG=tinyconfig
>

The above two are not really required in this powerpc series, let's delay them
to the next tinyconfig part1 series.

In this series, we only need:

     nolibc-test-config:
             $(Q)$(MAKE_KERNEL) mrproper $(or $(CONFIG),$(DEFCONFIG))
             $(Q)$(srctree)/scripts/kconfig/merge_config.sh -Q -O "$(objtree)" -m "$(KERNEL_CONFIG)" $(foreach c,$(EXTRA_CONFIG),$(wildcard $(CURDIR)/configs/$c))
             $(Q)$(MAKE_KERNEL) KCONFIG_ALLCONFIG="$(KERNEL_CONFIG)" allnoconfig
             $(Q)$(MAKE_KERNEL) prepare

And this:

    # extra configs/ files appended to .config during the nolibc-test-config target
    # include common + architecture specific
    EXTRA_CONFIG         = common.config $(XARCH).config

And the update of the help target too.

If both of you are happy with this? let's do it ;-)

Best regards,
Zhangjin

> The complexity here is we have planned to support both defconfig and
> tinyconfig, what about this solution?
> 
> Thanks,
> Zhangjin
>
> > Willy
Willy Tarreau July 30, 2023, 6:18 a.m. UTC | #8 
Hi Zhangjin,

On Sun, Jul 30, 2023 at 02:01:31PM +0800, Zhangjin Wu wrote:
> > > > Seems 'nolibc-test-config' is really more meaningful than 'defconfig', especially
> > > > when we want to use tinyconfig through it?
> > > > 
> > > >     $ make nolibc-test-config DEFCONFIG=tinyconfig
> > > 
> > > As a user, I'd ask "why not make tinyconfig" ? But see my other message,
> > > now I'm having strong doubts about the relevance of tinyconfig if it works
> > > as bad as you described it.
> > >
> > 
> > I have added a nolibc tinyconfig target before, as the same reason,
> > based on your suggestion, I have removed the tinyconfig target and even
> > moved the extconfig to this defconfig to avoid add more similar or extra
> > complex targets in nolibc Makefile. before, tinyconfig + extconfig
> > together work for nolibc-test, so, tinyconfig is the same as the
> > top-level one, it should be removed as your suggested.
> > 
> > But since now, we are ready to get a real different target from the
> > top-level one, we may be able to have different targets for
> > 'defconfig+EXTRA_CONFIG' and 'tinyconfig+EXTRA_CONFIG' like this:
> > 
> >     nolibc-test-config:
> >             $(Q)echo $(MAKE_KERNEL) mrproper $(or $(CONFIG),defconfig)
> 
> It should be $(or $(CONFIG),$(DEFCONFIG))
> 
> >             $(Q)echo $(srctree)/scripts/kconfig/merge_config.sh -Q -O "$(objtree)" -m "$(KERNEL_CONFIG)" $(foreach c,$(EXTRA_CONFIG),$(wildcard $(CURDIR)/configs/$c))
> >             $(Q)echo $(MAKE_KERNEL) KCONFIG_ALLCONFIG="$(KERNEL_CONFIG)" allnoconfig
> >             $(Q)echo $(MAKE_KERNEL) prepare
> > 
> >     nolibc-test-defconfig nolibc-test-tinyconfig: nolibc-test-config
> >     nolibc-test-tinyconfig: CONFIG=tinyconfig
> >
> 
> The above two are not really required in this powerpc series, let's delay them
> to the next tinyconfig part1 series.
> 
> In this series, we only need:
> 
>      nolibc-test-config:
>              $(Q)$(MAKE_KERNEL) mrproper $(or $(CONFIG),$(DEFCONFIG))
>              $(Q)$(srctree)/scripts/kconfig/merge_config.sh -Q -O "$(objtree)" -m "$(KERNEL_CONFIG)" $(foreach c,$(EXTRA_CONFIG),$(wildcard $(CURDIR)/configs/$c))
>              $(Q)$(MAKE_KERNEL) KCONFIG_ALLCONFIG="$(KERNEL_CONFIG)" allnoconfig
>              $(Q)$(MAKE_KERNEL) prepare
> 
> And this:
> 
>     # extra configs/ files appended to .config during the nolibc-test-config target
>     # include common + architecture specific
>     EXTRA_CONFIG         = common.config $(XARCH).config
> 
> And the update of the help target too.
> 
> If both of you are happy with this? let's do it ;-)

Well, I understand "nolibc-test-config" as "the config suitable to
correctly run nolibc-test". In that case I agree. We can indeed start
from the defconfig, and if we manage to refine it we can later map it
to anything else that's known to work. And if we eventually split
nolibc-test into multiple tests (libc, syscalls, ipc, network, whatever)
then we might as well adopt different configs in the future. I'd also
want to keep defconfig on its own because it relies on each arch's
defconfig and is a simple way to verify they work (as they should).
As I said, till now I haven't met issues with defconfig so probably
we don't need to patch it, but we could revisit that option later if
needed.

I'm still having that question about your "make allnoconfig" at the
end, which for me simply replaces the config you've just created, thus
which makes no sense. There might be a trick that I'm missing but if so
it should be explained there because the only situation where anybody
should start a question with "why" when reading code is when they
discovered a bug.

Also please split the mrproper and defconfig on two distinc lines, as
there's no point anymore in keeping them as a single one.

Since you're using the extra options for nolibc-test-config, I think
they should be called "nolibc-test-common.config" and
"nolibc-test-$XARCH.config" so that we don't restart with tons of
"if" and macros when adding a new target.

Thomas, are you also OK with this ?

Thanks,
Willy
Zhangjin Wu July 30, 2023, 11:21 a.m. UTC | #9 
> On Sun, Jul 30, 2023 at 02:01:31PM +0800, Zhangjin Wu wrote:
> > > > > Seems 'nolibc-test-config' is really more meaningful than 'defconfig', especially
> > > > > when we want to use tinyconfig through it?
> > > > > 
> > > > >     $ make nolibc-test-config DEFCONFIG=tinyconfig
[...]
> > 
> > And this:
> > 
> >     # extra configs/ files appended to .config during the nolibc-test-config target
> >     # include common + architecture specific
> >     EXTRA_CONFIG         = common.config $(XARCH).config
> > 
> > And the update of the help target too.
> > 
> > If both of you are happy with this? let's do it ;-)
> 
> Well, I understand "nolibc-test-config" as "the config suitable to
> correctly run nolibc-test". In that case I agree. We can indeed start
> from the defconfig, and if we manage to refine it we can later map it
> to anything else that's known to work. And if we eventually split
> nolibc-test into multiple tests (libc, syscalls, ipc, network, whatever)
> then we might as well adopt different configs in the future. I'd also

Agree, with this info, I do like nolibc-test-config, let's align all
related with nolibc-test- prefix and NOLIBC_TEST_CONFIG too ;-)

I have thought about a simpler 'config' target for both tinyconfig and
defconfig before, but I'm worried about the same issue mentioned by
Willy, it may conflict with the one from top-level Makefile.

With exact nolibc-test prefix, it is better.

> want to keep defconfig on its own because it relies on each arch's
> defconfig and is a simple way to verify they work (as they should).
> As I said, till now I haven't met issues with defconfig so probably
> we don't need to patch it, but we could revisit that option later if
> needed.
>

No Willy, this patch here is not originally for tinyconfig, is for
pmac32_defconfig used by the default ppc32 qemu machine has no builtin
console, it is configured as a module:

    CONFIG_SERIAL_8250=m
    CONFIG_SERIAL_PMACZILOG=m
    CONFIG_SERIAL_PMACZILOG_TTYS=y

This is why we also need to add such extra logic also for defconfig
target. Another solution is ask the kernel maintainer to switch the =m
to =y, but as we discussed before, it is better to depends on the kernel
part, so, an extra logic is used here for better flexibility and less
dependency.

If we still to reserve the 'defconfig' target, is this ok for you?

    defconfig: nolibc-test-config

    Or

    nolibc-test-defconfig: nolibc-test-config

Which one do you like? If 'defconfig', the one for tinyconfig may also
better to use 'tinyconfig':

    tinyconfig defconfig: nolibc-test-config
    tinyconfig: CONFIG=tinyconfig

Otherwise,

    nolibc-test-tinyconfig nolibc-test-defconfig: nolibc-test-config
    nolibc-test-tinyconfig: CONFIG=tinyconfig

I'm ok with anyone of them (note, the tinyconfig related parts will not
be really added here), what about you?

> I'm still having that question about your "make allnoconfig" at the
> end, which for me simply replaces the config you've just created, thus
> which makes no sense. There might be a trick that I'm missing but if so
> it should be explained there because the only situation where anybody
> should start a question with "why" when reading code is when they
> discovered a bug.
>

Yeah, the name of allnoconfig doesn't confuse a little, to simplify it a
lot, let's simply use 'olddefconfig' (as 'oldconfig' you suggested in
another replay, but without prompt and silently set new options as
default) instead?

> Also please split the mrproper and defconfig on two distinc lines, as
> there's no point anymore in keeping them as a single one.
>

Yeah, it is better.

> Since you're using the extra options for nolibc-test-config, I think
> they should be called "nolibc-test-common.config" and
> "nolibc-test-$XARCH.config" so that we don't restart with tons of
> "if" and macros when adding a new target.

Done.

Btw, since this one does have some relationship with another three
patches (two are from tinyconfig part1, like the XARCH you mentioned two
times), in order to make them more fast forward, I have reorder them to
make things clear.

* selftests/nolibc: fix up O= option support

    Fix up the wrong srctree previously used, then, we can use objtree
    directly in this patch.

* selftests/nolibc: add macros to reduce duplicated changes

    Some of the macros are heavily used in our new nolibc-test-config
    target, for we require to split mrproper and prepare into more
    lines, shorter macros prepared earlier does help this.

* selftests/nolibc: add XARCH and ARCH mapping support

    Add this one earlier, then, our patch can use the XARCH directly.

As you suggested, If no other opposite, another two about CROSS_COMPILE
will be moved to this powerpc series too:

* selftests/nolibc: allow customize CROSS_COMPILE by architecture
* selftests/nolibc: customize CROSS_COMPILE for 32/64-bit powerpc

At last, here is it?

    # extra configs/ files appended to .config during the nolibc-test-config target
    # include common + architecture specific
    NOLIBC_TEST_CONFIG   = nolibc-test-common.config nolibc-test-$(XARCH).config

    nolibc-test-config:
	$(Q)$(MAKE_KERNEL) mrproper
	$(Q)$(MAKE_KERNEL) $(or $(CONFIG),$(DEFCONFIG)) prepare
	$(Q)$(srctree)/scripts/kconfig/merge_config.sh -Q -O "$(objtree)" -m "$(KERNEL_CONFIG)" $(foreach c,$(NOLIBC_TEST_CONFIG),$(wildcard $(CURDIR)/configs/$c))
	$(Q)$(MAKE_KERNEL) olddefconfig
	$(Q)$(MAKE_KERNEL) prepare

    defconfig: nolibc-test-config

The last line still depends on your confirm.

Without more issues, I will renew this patchset as v4, thanks very much!

(will update the XARCH patch to get your confirm in another reply too)

Best regards,
Zhangjin

> Thomas, are you also OK with this ?
> 
> Thanks,
> Willy
Zhangjin Wu July 30, 2023, 6:02 p.m. UTC | #10 
Hi,

> At last, here is it?
> 
>     # extra configs/ files appended to .config during the nolibc-test-config target
>     # include common + architecture specific
>     NOLIBC_TEST_CONFIG   = nolibc-test-common.config nolibc-test-$(XARCH).config
> 
>     nolibc-test-config:
> 	$(Q)$(MAKE_KERNEL) mrproper
> 	$(Q)$(MAKE_KERNEL) $(or $(CONFIG),$(DEFCONFIG)) prepare

The 'prepare' should be removed, we have one in the end.

> 	$(Q)$(srctree)/scripts/kconfig/merge_config.sh -Q -O "$(objtree)" -m "$(KERNEL_CONFIG)" $(foreach c,$(NOLIBC_TEST_CONFIG),$(wildcard $(CURDIR)/configs/$c))
> 	$(Q)$(MAKE_KERNEL) olddefconfig

Oh, sorry, test shows, 'allnoconfig' worth a comment ;-)

'allnoconfig' is ~2x faster than 'olddefconfig', it is more
deterministic for it set all new symbols (the ones not specified in
.config) with no.

    // scripts/kconfig/Makefile

        @echo  '  oldconfig       - Update current config utilising a provided .config as base'
        @echo  '  defconfig       - New config with default from ARCH supplied defconfig'
        @echo  '  allnoconfig     - New config where all options are answered with no'
        @echo  '  allyesconfig    - New config where all options are accepted with yes'
        @echo  '  olddefconfig    - Same as oldconfig but sets new symbols to their'
        @echo  '                    default value without prompting'


here is the result:

    // with 'allnoconfig'
    $ sudo sh -c 'echo 3 > /proc/sys/vm/drop_caches'; arch=ppc; time make tinyconfig kernel -C tools/testing/selftests/nolibc CONFIG=tinyconfig XARCH=$arch O=$PWD/kernel-$arch
    real	3m37.337s
    user	3m11.576s
    sys         0m16.899s

    // with 'olddefconfig'
    $ sudo sh -c 'echo 3 > /proc/sys/vm/drop_caches'; arch=ppc; time make tinyconfig kernel -C tools/testing/selftests/nolibc CONFIG=tinyconfig XARCH=$arch O=$PWD/kernel-$arch 
    real	5m28.759s
    user	4m47.873s
    sys         0m30.115s

    // with 'defconfig'
    

Both merge_tools.sh and tinyconfig target use 'allnoconfig', the usage
is clear enough, no risk:

    scripts/kconfig/merge_config.sh:

    # Use the merged file as the starting point for:
    # alldefconfig: Fills in any missing symbols with Kconfig default
    # allnoconfig: Fills in any missing symbols with # CONFIG_* is not set
    make KCONFIG_ALLCONFIG=$TMP_FILE $OUTPUT_ARG $ALLTARGET


    scripts/kconfig/Makefile:

    tinyconfig:
        $(Q)KCONFIG_ALLCONFIG=kernel/configs/tiny-base.config $(MAKE) -f $(srctree)/Makefile allnoconfig
        $(Q)$(MAKE) -f $(srctree)/Makefile tiny.config


And also since I have carefully test 'allnoconfig' for all of the nolibc
supported architectures, it is not good to waste time to test
'olddefconfig'. 'allnoconfig' is also more deterministic than
'olddefconfig' since it only enable the options specified by us
explicitly, so, no new symbols will be randomly enabled.

I plan to add more comments before 'nolibc-test-config':

    # kernel config for nolibc-test
    #
    # - delete the current configuration and all generated files via 'mrproper' target
    # - generate .config via '$(CONFIG)' or '$(DEFCONFIG_$(XARCH))' target
    # - merge extra config options from $(NOLIBC_TEST_CONFIG) files to .config
    # - use merged .config as base and fills in any missing symbols with '# CONFIG_* is not set' via 'allnoconfig' target
    # - prepare things we need to do before we recursively start building the kernel via 'prepare' target
    #

    nolibc-test-config:
    	$(Q)$(MAKE_KERNEL) mrproper
    	$(Q)$(MAKE_KERNEL) $(or $(CONFIG),$(DEFCONFIG))
    	$(Q)$(srctree)/scripts/kconfig/merge_config.sh -Q -O "$(objtree)" -m "$(KERNEL_CONFIG)" $(foreach c,$(NOLIBC_TEST_CONFIG),$(wildcard $(CURDIR)/configs/$c))
    	$(Q)$(MAKE_KERNEL) KCONFIG_ALLCONFIG=$(KERNEL_CONFIG) allnoconfig
    	$(Q)$(MAKE_KERNEL) prepare

> 	$(Q)$(MAKE_KERNEL) prepare
> 
>     defconfig: nolibc-test-config
> 
> The last line still depends on your confirm.
> 
> Without more issues, I will renew this patchset as v4, thanks very much!
> 
> (will update the XARCH patch to get your confirm in another reply too)
> 
> Best regards,
> Zhangjin
diff mbox series

Patch

diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
index f42adef87e12..9576f1a0a98d 100644
--- a/tools/testing/selftests/nolibc/Makefile
+++ b/tools/testing/selftests/nolibc/Makefile
@@ -39,6 +39,9 @@  DEFCONFIG_s390       = defconfig
 DEFCONFIG_loongarch  = defconfig
 DEFCONFIG            = $(DEFCONFIG_$(ARCH))
 
+# extra kernel config files under configs/, include common + architecture specific
+EXTCONFIG            = common.config $(ARCH).config
+
 # optional tests to run (default = all)
 TEST =
 
@@ -161,6 +164,8 @@  initramfs: nolibc-test
 
 defconfig:
 	$(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare
+	$(Q)$(srctree)/scripts/kconfig/merge_config.sh -O "$(srctree)" -m "$(srctree)/.config" $(foreach c,$(EXTCONFIG),$(wildcard $(CURDIR)/configs/$c))
+	$(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) KCONFIG_ALLCONFIG="$(srctree)/.config" allnoconfig
 
 kernel: initramfs
 	$(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs