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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

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);) ) \
     :

If this patch is OK then it can be reused for patches:
https://lore.kernel.org/xen-devel/cdc20255540a66ba0b6946ac6d48c11029cd3385.1701453087.git.oleksii.kurochko@xxxxxxxxx/
https://lore.kernel.org/xen-devel/d42a34866edc70a12736b5c6976aa1b44b4ebd8a.1701453087.git.oleksii.kurochko@xxxxxxxxx/

> 
> > > > > > --- a/automation/gitlab-ci/build.yaml
> > > > > > +++ b/automation/gitlab-ci/build.yaml
> > > > > > @@ -522,6 +522,38 @@ archlinux-current-gcc-riscv64:
> > > > > >      CONTAINER: archlinux:current-riscv64
> > > > > >      KBUILD_DEFCONFIG: tiny64_defconfig
> > > > > >      HYPERVISOR_ONLY: y
> > > > > > +    EXTRA_XEN_CONFIG:
> > > > > > +      CONFIG_COVERAGE=n
> > > > > > +      CONFIG_GRANT_TABLE=n
> > > > > > +      CONFIG_SCHED_CREDIT=n
> > > > > > +      CONFIG_SCHED_CREDIT2=n
> > > > > > +      CONFIG_SCHED_RTDS=n
> > > > > > +      CONFIG_SCHED_NULL=n
> > > > > > +      CONFIG_SCHED_ARINC653=n
> > > > > > +      CONFIG_TRACEBUFFER=n
> > > > > > +      CONFIG_HYPFS=n
> > > > > > +      CONFIG_GRANT_TABLE=n
> > > > > > +      CONFIG_SPECULATIVE_HARDEN_ARRAY=n
> > > > > > +      CONFIG_ARGO=n
> > > > > > +      CONFIG_HYPFS_CONFIG=n
> > > > > > +      CONFIG_CORE_PARKING=n
> > > > > > +      CONFIG_DEBUG_TRACE=n
> > > > > > +      CONFIG_IOREQ_SERVER=n
> > > > > > +      CONFIG_CRASH_DEBUG=n
> > > > > > +      CONFIG_KEXEC=n
> > > > > > +      CONFIG_LIVEPATCH=n
> > > > > > +      CONFIG_MEM_ACCESS=n
> > > > > > +      CONFIG_NUMA=n
> > > > > > +      CONFIG_PERF_COUNTERS=n
> > > > > > +      CONFIG_HAS_PMAP=n
> > > > > > +      CONFIG_TRACEBUFFER=n
> > > > > > +      CONFIG_XENOPROF=n
> > > > > > +      CONFIG_COMPAT=n
> > > > > > +      CONFIG_COVERAGE=n
> > > > > > +      CONFIG_UBSAN=n
> > > > > > +      CONFIG_NEEDS_LIBELF=n
> > > > > > +      CONFIG_XSM=n
> > > > > > +
> > > > > >  
> > > > > >  archlinux-current-gcc-riscv64-debug:
> > > > > >    extends: .gcc-riscv64-cross-build-debug
> > > > 
> > > > I think I've said so elsewhere before: Please avoid introducing
> > > > double
> > > > blank lines, unless entirely unavoidable (for reasons I cannot
> > > > think
> > > > of).
> > Sorry for that; I am not doing that on purpose. It's just a big
> > patch
> > series, and I missed that double blank. I will try to be more
> > attentive.
> > 
> > Do you think it makes sense to add a script to perform basic code
> > style
> > checks, similar to what Linux has?
> 
> Such a script would be nice, but it doesn't exist and re-using
> existing
> checkers has also proven controversial. There's actually an ongoing
> discussion on this topic which you may want to follow.
Yes, I would like to follow. I'll search the topic in ML.
Thanks.

~ Oleksii