Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Date:   Mon, 9 Oct 2023 10:43:43 -0700
In-Reply-To: <20231006205415.3501535-1-kuba@kernel.org>
Mime-Version: 1.0
References: <20231006205415.3501535-1-kuba@kernel.org>
Message-ID: <ZSQ7z8gqIemJQXI6@google.com>
Subject: Re: [PATCH] KVM: deprecate KVM_WERROR in favor of general WERROR
From:   Sean Christopherson <seanjc@google.com>
To:     Jakub Kicinski <kuba@kernel.org>
Cc:     pbonzini@redhat.com, workflows@vger.kernel.org,
        linux-kernel@vger.kernel.org, kvm@vger.kernel.org
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

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.