| Message ID | b4e85f8f58787b4d179022973ce25673d6b56e36.1700761381.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enable build of full Xen for RISC-V | expand |
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). > --- 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). Jan
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? > > > > > > --- 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? > > ~ Oleksii
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. >>>>> --- 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. Jan
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@gmail.com/ https://lore.kernel.org/xen-devel/d42a34866edc70a12736b5c6976aa1b44b4ebd8a.1701453087.git.oleksii.kurochko@gmail.com/ > > > > > > > --- 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
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
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
On 07.12.2023 15:51, Oleksii wrote: > 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. I don't think "unused" or "ignored" are properly describing what such a file would hold. I was vaguely playing with "fixed" or "forced", fwiw. >> 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. I'm not sure *defconfig should be affected by what's in the "locked down" config, assuming they're consistent with what may not be changed. But of course one could also model this incrementally, such that defconfigs don't need to repeat what the "locked down" one has. Anyway, I think this needs sending an RFC with the right people in Cc, such that this can be properly discussed. Jan
diff --git a/automation/gitlab-ci/build.yaml b/automation/gitlab-ci/build.yaml index 32af30cced..f4ef81839a 100644 --- 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 @@ -529,6 +561,38 @@ archlinux-current-gcc-riscv64-debug: 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-randconfig: extends: .gcc-riscv64-cross-build @@ -538,6 +602,35 @@ archlinux-current-gcc-riscv64-randconfig: RANDCONFIG: y EXTRA_FIXED_RANDCONFIG: 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-randconfig: extends: .gcc-riscv64-cross-build-debug @@ -547,6 +640,35 @@ archlinux-current-gcc-riscv64-debug-randconfig: RANDCONFIG: y EXTRA_FIXED_RANDCONFIG: 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 # Power cross-build debian-bullseye-gcc-ppc64le: diff --git a/automation/scripts/build b/automation/scripts/build index b3c71fb6fb..88287b9c46 100755 --- a/automation/scripts/build +++ b/automation/scripts/build @@ -14,7 +14,7 @@ if [[ "${RANDCONFIG}" == "y" ]]; then # Append job-specific fixed configuration if [[ -n "${EXTRA_FIXED_RANDCONFIG}" ]]; then - echo "${EXTRA_FIXED_RANDCONFIG}" >> xen/tools/kconfig/allrandom.config + sed "s/ /\n/g" <<< "${EXTRA_FIXED_RANDCONFIG}" > xen/tools/kconfig/allrandom.config fi make -j$(nproc) -C xen KCONFIG_ALLCONFIG=tools/kconfig/allrandom.config randconfig @@ -28,7 +28,7 @@ else echo "CONFIG_DEBUG=${debug}" >> xen/.config if [[ -n "${EXTRA_XEN_CONFIG}" ]]; then - echo "${EXTRA_XEN_CONFIG}" >> xen/.config + sed "s/ /\n/g" <<< "${EXTRA_XEN_CONFIG}" >> xen/.config fi make -j$(nproc) -C xen olddefconfig diff --git a/xen/arch/riscv/configs/tiny64_defconfig b/xen/arch/riscv/configs/tiny64_defconfig index 3c9a2ff941..ce37c5ae00 100644 --- a/xen/arch/riscv/configs/tiny64_defconfig +++ b/xen/arch/riscv/configs/tiny64_defconfig @@ -6,6 +6,24 @@ # CONFIG_HYPFS is not set # CONFIG_GRANT_TABLE is not set # CONFIG_SPECULATIVE_HARDEN_ARRAY is not set +# CONFIG_ARGO is not set +# CONFIG_HYPFS_CONFIG is not set +# CONFIG_CORE_PARKING is not set +# CONFIG_DEBUG_TRACE is not set +# CONFIG_IOREQ_SERVER is not set +# CONFIG_CRASH_DEBUG is not setz +# CONFIG_KEXEC is not set +# CONFIG_LIVEPATCH is not set +# CONFIG_MEM_ACCESS is not set +# CONFIG_NUMA is not set +# CONFIG_PERF_COUNTERS is not set +# CONFIG_HAS_PMAP is not set +# CONFIG_TRACEBUFFER is not set +# CONFIG_XENOPROF is not set +# CONFIG_COMPAT is not set +# CONFIG_COVERAGE is not set +# CONFIG_UBSAN is not set +# CONFIG_NEEDS_LIBELF is not set CONFIG_RISCV_64=y CONFIG_DEBUG=y
The patch also fixes the build script as conf util expects to have each config on separate line. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in V2: - update the commit message. - remove xen/arch/riscv/Kconfig changes. --- automation/gitlab-ci/build.yaml | 122 ++++++++++++++++++++++++ automation/scripts/build | 4 +- xen/arch/riscv/configs/tiny64_defconfig | 18 ++++ 3 files changed, 142 insertions(+), 2 deletions(-)