Re: [PATCH v2 01/39] xen/riscv: disable unnecessary configs

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Thu, 2023-12-07 at 15:11 +0100, Jan Beulich wrote:
> On 07.12.2023 14:44, Oleksii wrote:
> > On Thu, 2023-12-07 at 11:00 +0100, Jan Beulich wrote:
> > > On 07.12.2023 10:22, Oleksii wrote:
> > > > On Tue, 2023-12-05 at 16:38 +0100, Jan Beulich wrote:
> > > > > > On 24.11.2023 11:30, Oleksii Kurochko wrote:
> > > > > > > > The patch also fixes the build script as conf util
> > > > > > > > expects
> > > > > > > > to have each config on separate line.
> > > > > > 
> > > > > > The approach doesn't really scale (it's already odd that
> > > > > > you
> > > > > > add
> > > > > > the
> > > > > > (apparently) same set four times. There's also zero
> > > > > > justification
> > > > > > for
> > > > > > this kind of an adjustment (as per discussion elsewhere I
> > > > > > don't
> > > > > > think
> > > > > > we should go this route, and hence arguments towards
> > > > > > convincing
> > > > > > me
> > > > > > [and
> > > > > > perhaps others] would be needed here).
> > > > I agree that this may not be the best approach, but it seems
> > > > like
> > > > we
> > > > don't have too many options to turn off a config for
> > > > randconfig.
> > > > 
> > > > To be honest, in my opinion (IMO), randconfig should either be
> > > > removed
> > > > or allowed to fail until most of the functionality is ready.
> > > > Otherwise,
> > > > there should be a need to introduce HAS_* or depend on
> > > > 'SUPPORTED_ARCHS' for each config, or introduce a lot of stubs.
> > > > 
> > > > Could you please suggest a better option?
> > > 
> > > As to dropping randconfig tests, I'd like to refer you to Andrew,
> > > who
> > > is of the opinion that it was wrong to drop them for ppc. (I'm
> > > agreeing
> > > with him when taking a theoretical perspective, but I'm not happy
> > > with
> > > the practical consequences.)
> > > 
> > > As to a better approach: Instead of listing the same set of
> > > options
> > > several times, can't there be a template config which is used to
> > > force
> > > randconfig to not touch certain settings? In fact at least for
> > > non-
> > > randconfig purposes I thought tiny64_defconfig /
> > > riscv64_defconfig
> > > already serve kind of a similar purpose. Imo the EXTRA_*CONFIG
> > > overrides
> > > are there for at most very few special case settings, not for
> > > purposes
> > > like you use them here.
> > The template will be the really a good option.
> > 
> > What do you think about the following patch which introduces arch-
> > specific allrandom.config?
> > 
> > diff --git a/xen/Makefile b/xen/Makefile
> > index ca571103c8..cb1eca76c2 100644
> > --- a/xen/Makefile
> > +++ b/xen/Makefile
> > @@ -336,11 +336,14 @@ ifeq ($(config-build),y)
> >  # *config targets only - make sure prerequisites are updated, and
> > descend
> >  # in tools/kconfig to make the *config target
> >  
> > +ARCH_ALLRANDOM_CONFIG :=
> > $(srctree)/arch/$(SRCARCH)/configs/allrandom.config
> > +
> >  # Create a file for KCONFIG_ALLCONFIG which depends on the
> > environment.
> >  # This will be use by kconfig targets
> > allyesconfig/allmodconfig/allnoconfig/randconfig
> >  filechk_kconfig_allconfig = \
> >      $(if $(findstring n,$(XEN_HAS_CHECKPOLICY)), echo
> > 'CONFIG_XSM_FLASK_POLICY=n';) \
> > -    $(if $(KCONFIG_ALLCONFIG), cat $(KCONFIG_ALLCONFIG);) \
> > +    $(if $(KCONFIG_ALLCONFIG), cat $(KCONFIG_ALLCONFIG); \
> > +    $(if $(wildcard $(ARCH_ALLRANDOM_CONFIG)), cat
> > $(ARCH_ALLRANDOM_CONFIG);) ) \
> >      :
> 
> Something along these lines may be okay, but why would the name be
> "allrandom" when the config is used elsewhere as well?
The naming is not optimal. "unused.config" or "ignored.config" would be
a better choice.

>  Further, besides
> keeping randconfig and all*config from creating unusable configs, it
> will at least want considering whether in other cases that set of
> fixed
> values shouldn't be used as well then.
If I understood you correctly, the other case is *defconfig targets.
Therefore, the following targets might also need to be updated by
merging "unused.config" with {defconfig,%_defconfig}:


defconfig: $(obj)/conf
ifneq ($(wildcard
$(srctree)/arch/$(SRCARCH)/configs/$(KBUILD_DEFCONFIG)),)
        @$(kecho) "*** Default configuration is based on
'$(KBUILD_DEFCONFIG)'"
        $(Q)$< $(silent) --
defconfig=arch/$(SRCARCH)/configs/$(KBUILD_DEFCONFIG) $(Kconfig)
else
        @$(kecho) "*** Default configuration is based on target
'$(KBUILD_DEFCONFIG)'"
        $(Q)$(MAKE) -f $(srctree)/Makefile $(KBUILD_DEFCONFIG)
endif

%_defconfig: $(obj)/conf
        $(Q)$< $(silent) --defconfig=arch/$(SRCARCH)/configs/$@
$(Kconfig)

However, I believe it's possible that for *defconfig, a configuration
should be set to N, but in randconfig, it is still acceptable to be set
to Y.

~ Oleksii