Re: [PATCH] KVM: deprecate KVM_WERROR in favor of general WERROR

Sean Christopherson
2023-10-09T17:43:51Z

On Fri, Oct 06, 2023, Jakub Kicinski wrote:
> Setting WERROR for random subsystems make life really hard
> for subsystems which want to build-test their stuff with W=1.
> WERROR for the entire kernel now exists and can be used
> instead. W=1 people probably know how to deal with the global
> W=1 already, tracking all per-subsystem WERRORs is too much...

I assume s/W=1/WERROR=y in this line?

> Link: https://lore.kernel.org/all/0da9874b6e9fcbaaa5edeb345d7e2a7c859fc818.1696271334.git.thomas.lendacky@amd.com/
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> Documentation/process/maintainer-kvm-x86.rst | 2 +-
> arch/x86/kvm/Kconfig | 14 --------------
> arch/x86/kvm/Makefile | 1 -
> 3 files changed, 1 insertion(+), 16 deletions(-)
>
> diff --git a/Documentation/process/maintainer-kvm-x86.rst b/Documentation/process/maintainer-kvm-x86.rst
> index 9183bd449762..cd70c0351108 100644
> --- a/Documentation/process/maintainer-kvm-x86.rst
> +++ b/Documentation/process/maintainer-kvm-x86.rst
> @@ -243,7 +243,7 @@ context and disambiguate the reference.
> Testing
> -------
> At a bare minimum, *all* patches in a series must build cleanly for KVM_INTEL=m
> -KVM_AMD=m, and KVM_WERROR=y. Building every possible combination of Kconfigs
> +KVM_AMD=m, and WERROR=y. Building every possible combination of Kconfigs
> isn't feasible, but the more the merrier. KVM_SMM, KVM_XEN, PROVE_LOCKING, and
> X86_64 are particularly interesting knobs to turn.
>
> diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
> index ed90f148140d..12929324ac3e 100644
> --- a/arch/x86/kvm/Kconfig
> +++ b/arch/x86/kvm/Kconfig
> @@ -63,20 +63,6 @@ config KVM
>
> If unsure, say N.
>
> -config KVM_WERROR
> - bool "Compile KVM with -Werror"
> - # KASAN may cause the build to fail due to larger frames
> - default y if X86_64 && !KASAN

Hrm, I am loath to give up KVM's targeted -Werror as it allows for more aggresive
enabling, e.g. enabling CONFIG_WERROR for i386 builds with other defaults doesn't
work because of CONFIG_FRAME_WARN=1024. That in turns means making WERROR=y a
requirement in maintainer-kvm-x86.rst is likely unreasonable.

And arguably KVM_WERROR is doing its job by flagging the linked W=1 error. The
problem there lies more in my build testing, which I'll go fix by adding a W=1
configuration or three. As the changelog notes, I highly doubt W=1 builds work
with WERROR, whereas keeping KVM x86 warning-free even with W=1 is feasible.

> - # We use the dependency on !COMPILE_TEST to not be enabled
> - # blindly in allmodconfig or allyesconfig configurations
> - depends on KVM
> - depends on (X86_64 && !KASAN) || !COMPILE_TEST

On a related topic, this is comically stale as WERROR is on by default for both
allmodconfig and allyesconfig, which work because they trigger 64-bit builds.
And KASAN on x86 is 64-bit only.

Rather than yank out KVM_WERROR entirely, what if we make default=n and trim the
depends down to "KVM && EXPERT && !KASAN"? E.g.

diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
index 8452ed0228cb..c2466304aa6a 100644
--- a/arch/x86/kvm/Kconfig
+++ b/arch/x86/kvm/Kconfig
@@ -65,13 +65,12 @@ config KVM

config KVM_WERROR
bool "Compile KVM with -Werror"
- # KASAN may cause the build to fail due to larger frames
- default y if X86_64 && !KASAN
- # We use the dependency on !COMPILE_TEST to not be enabled
- # blindly in allmodconfig or allyesconfig configurations
- depends on KVM
- depends on (X86_64 && !KASAN) || !COMPILE_TEST
- depends on EXPERT
+ # Disallow KVM's -Werror if KASAN=y, e.g. to guard against randomized
+ # configs from selecting KVM_WERROR=y. KASAN builds generates warnings
+ # for the default FRAME_WARN, i.e. KVM_WERROR=y with KASAN=y requires
+ # special tuning. Building KVM with -Werror and KASAN is still doable
+ * via enabling the kernel-wide WERROR=y.
+ depends on KVM && EXPERT && !KASAN
help
Add -Werror to the build flags for KVM.