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

[Jan Beulich <jbeulich@xxxxxxxx>]

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? 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.

Jan