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

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

[Jakub Kicinski]

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

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
-	# 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
-	help
-	  Add -Werror to the build flags for KVM.
-
-	  If in doubt, say "N".
-
 config KVM_INTEL
 	tristate "KVM for Intel (and compatible) processors support"
 	depends on KVM && IA32_FEAT_CTL
diff --git a/arch/x86/kvm/Makefile b/arch/x86/kvm/Makefile
index 80e3fe184d17..8e6afde7c680 100644
--- a/arch/x86/kvm/Makefile
+++ b/arch/x86/kvm/Makefile
@@ -1,7 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 
 ccflags-y += -I $(srctree)/arch/x86/kvm
-ccflags-$(CONFIG_KVM_WERROR) += -Werror
 
 ifeq ($(CONFIG_FRAME_POINTER),y)
 OBJECT_FILES_NON_STANDARD_vmenter.o := y
-- 
2.41.0

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

[Kees Cook]

On Fri, Oct 06, 2023 at 01:54:15PM -0700, 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...
> 
> Link: https://lore.kernel.org/all/0da9874b6e9fcbaaa5edeb345d7e2a7c859fc818.1696271334.git.thomas.lendacky@amd.com/
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Yeah, best to have just the global option.

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

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

[Sean Christopherson]

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.

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

[Jakub Kicinski]

On Mon, 9 Oct 2023 10:43:43 -0700 Sean Christopherson wrote:
> 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?

Yes, sorry about that.

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

IMO setting WERROR is a bit perverse. The way I see it WERROR is a
crutch for people who don't have the time / infra to properly build
test changes they send to Linus. Or wait for build bots to do their job.
We do have sympathy for these folks, we are mostly volunteers after
all. At the same time someone's under-investment should not be causing
pain to those of us who _do_ build test stuff carefully.

Rather than tweak stuff I'd prefer if we could agree that local -Werror
is anti-social :(

The global WERROR seems to be a good compromise.

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

[Sean Christopherson]

On Mon, Oct 09, 2023, Jakub Kicinski wrote:
> On Mon, 9 Oct 2023 10:43:43 -0700 Sean Christopherson wrote:
> > On Fri, Oct 06, 2023, Jakub Kicinski wrote:
> > 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.
> 
> IMO setting WERROR is a bit perverse. The way I see it WERROR is a
> crutch for people who don't have the time / infra to properly build
> test changes they send to Linus. Or wait for build bots to do their job.

KVM_WERROR reduces the probability of issues in patches being sent to *me*.  The
reality is that most contributors do not have the knowledge and/or resources to
"properly" build test changes without specific guidance on what/how to test, or
what configs to prioritize.

Nor is it realistic to expect that build bots will detect every issue in every
possible configuration in every patch that's posted.

Call -Werror a crutch if you will, but for me it's a crutch that I'm more than
willing to lean on in order to increase the overall quality of KVM x86 submissions.

> We do have sympathy for these folks, we are mostly volunteers after
> all. At the same time someone's under-investment should not be causing
> pain to those of us who _do_ build test stuff carefully.

This is a bit over the top.  Yeah, I need to add W=1 to my build scripts, but that's
not a lack of investment, just an oversight.  Though in this case it likely wouldn't
have made any difference since Paolo grabbed the patches directly and might have
even bypassed linux-next.  But again I would argue that's bad process, not a lack
of investment.

> Rather than tweak stuff I'd prefer if we could agree that local -Werror
> is anti-social :(
> 
> The global WERROR seems to be a good compromise.

I disagree.  WERROR simply doesn't provide the same coverage.  E.g. it can't be
enabled for i386 without tuning FRAME_WARN, which (a) won't be at all obvious to
the average contributor and (b) increasing FRAME_WARN effectively reduces the
test coverage of KVM i386.

For KVM x86, I want the rules for contributing to be clearly documented, and as
simple as possible.  I don't see a sane way to achieve that with WERROR=y.

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

[Jakub Kicinski]

On Mon, 9 Oct 2023 12:33:53 -0700 Sean Christopherson wrote:
> > We do have sympathy for these folks, we are mostly volunteers after
> > all. At the same time someone's under-investment should not be causing
> > pain to those of us who _do_ build test stuff carefully.  
> 
> This is a bit over the top.  Yeah, I need to add W=1 to my build scripts, but that's
> not a lack of investment, just an oversight.  Though in this case it likely wouldn't
> have made any difference since Paolo grabbed the patches directly and might have
> even bypassed linux-next.  But again I would argue that's bad process, not a lack
> of investment.

If you do invest in build testing automation, why can't your automation
count warnings rather than depend on WERROR? I don't understand.

> > Rather than tweak stuff I'd prefer if we could agree that local -Werror
> > is anti-social :(
> > 
> > The global WERROR seems to be a good compromise.  
> 
> I disagree.  WERROR simply doesn't provide the same coverage.  E.g. it can't be
> enabled for i386 without tuning FRAME_WARN, which (a) won't be at all obvious to
> the average contributor and (b) increasing FRAME_WARN effectively reduces the
> test coverage of KVM i386.
> 
> For KVM x86, I want the rules for contributing to be clearly documented, and as
> simple as possible.  I don't see a sane way to achieve that with WERROR=y.

Linus, you created the global WERROR option. Do you have an opinion
on whether random subsystems should create their own WERROR flags?
W=1 warning got in thru KVM and since they have a KVM_WERROR which
defaults to enabled it broke build testing in networking.
Randomly sprinkled -Werrors are fragile. Can we ask people to stop
using them now that the global ERROR exists?

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

[Jani Nikula]

On Mon, 09 Oct 2023, Jakub Kicinski <kuba@kernel.org> wrote:
> On Mon, 9 Oct 2023 12:33:53 -0700 Sean Christopherson wrote:
>> > We do have sympathy for these folks, we are mostly volunteers after
>> > all. At the same time someone's under-investment should not be causing
>> > pain to those of us who _do_ build test stuff carefully.  
>> 
>> This is a bit over the top.  Yeah, I need to add W=1 to my build scripts, but that's
>> not a lack of investment, just an oversight.  Though in this case it likely wouldn't
>> have made any difference since Paolo grabbed the patches directly and might have
>> even bypassed linux-next.  But again I would argue that's bad process, not a lack
>> of investment.
>
> If you do invest in build testing automation, why can't your automation
> count warnings rather than depend on WERROR? I don't understand.

Because having both CI and the subsystem/driver developers enable a
local WERROR actually works in keeping the subsystem/driver clean of
warnings.

For i915, we also enable W=1 warnings and kernel-doc -Werror with it,
keeping all of them warning clean. I don't much appreciate calling that
anti-social.

>
>> > Rather than tweak stuff I'd prefer if we could agree that local -Werror
>> > is anti-social :(
>> > 
>> > The global WERROR seems to be a good compromise.  
>> 
>> I disagree.  WERROR simply doesn't provide the same coverage.  E.g. it can't be
>> enabled for i386 without tuning FRAME_WARN, which (a) won't be at all obvious to
>> the average contributor and (b) increasing FRAME_WARN effectively reduces the
>> test coverage of KVM i386.
>> 
>> For KVM x86, I want the rules for contributing to be clearly documented, and as
>> simple as possible.  I don't see a sane way to achieve that with WERROR=y.
>
> Linus, you created the global WERROR option. Do you have an opinion
> on whether random subsystems should create their own WERROR flags?
> W=1 warning got in thru KVM and since they have a KVM_WERROR which
> defaults to enabled it broke build testing in networking.
> Randomly sprinkled -Werrors are fragile. Can we ask people to stop
> using them now that the global ERROR exists?

The DRM_I915_WERROR config depends on EXPERT and !COMPILE_TEST, and to
my knowledge this has never caused issues outside of i915 developers and
CI.

Maybe the fix to KVM_ERROR config should be

-	depends on (X86_64 && !KASAN) || !COMPILE_TEST
-	depends on (X86_64 && !KASAN) && !COMPILE_TEST


BR,
Jani.


-- 
Jani Nikula, Intel

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

[Jakub Kicinski]

On Tue, 10 Oct 2023 11:04:18 +0300 Jani Nikula wrote:
> > If you do invest in build testing automation, why can't your automation
> > count warnings rather than depend on WERROR? I don't understand.  
> 
> Because having both CI and the subsystem/driver developers enable a
> local WERROR actually works in keeping the subsystem/driver clean of
> warnings.
> 
> For i915, we also enable W=1 warnings and kernel-doc -Werror with it,
> keeping all of them warning clean. I don't much appreciate calling that
> anti-social.

Anti-social is not the right word, that's fair.

Werror makes your life easier while increasing the blast radius 
of your mistakes. So you're trading off your convenience for risk
of breakage to others. Note that you can fix issues locally very
quickly and move on. Others have to wait to get your patches thru
Linus.

> >> I disagree.  WERROR simply doesn't provide the same coverage.  E.g. it can't be
> >> enabled for i386 without tuning FRAME_WARN, which (a) won't be at all obvious to
> >> the average contributor and (b) increasing FRAME_WARN effectively reduces the
> >> test coverage of KVM i386.
> >> 
> >> For KVM x86, I want the rules for contributing to be clearly documented, and as
> >> simple as possible.  I don't see a sane way to achieve that with WERROR=y.  
> 
> The DRM_I915_WERROR config depends on EXPERT and !COMPILE_TEST, and to
> my knowledge this has never caused issues outside of i915 developers and
> CI.

Ack, I think you do it right. I was trying to establish a precedent
so that we can delete these as soon as they cause an issue, not sooner.

Whatever tho, there's no accounting for taste.

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

[Sean Christopherson]

On Tue, Oct 10, 2023, Jakub Kicinski wrote:
> On Tue, 10 Oct 2023 11:04:18 +0300 Jani Nikula wrote:
> > > If you do invest in build testing automation, why can't your automation
> > > count warnings rather than depend on WERROR? I don't understand.  
> > 
> > Because having both CI and the subsystem/driver developers enable a
> > local WERROR actually works in keeping the subsystem/driver clean of
> > warnings.
> > 
> > For i915, we also enable W=1 warnings and kernel-doc -Werror with it,
> > keeping all of them warning clean. I don't much appreciate calling that
> > anti-social.
> 
> Anti-social is not the right word, that's fair.
> 
> Werror makes your life easier while increasing the blast radius 
> of your mistakes. So you're trading off your convenience for risk
> of breakage to others. Note that you can fix issues locally very
> quickly and move on. Others have to wait to get your patches thru
> Linus.
> 
> > >> I disagree.  WERROR simply doesn't provide the same coverage.  E.g. it can't be
> > >> enabled for i386 without tuning FRAME_WARN, which (a) won't be at all obvious to
> > >> the average contributor and (b) increasing FRAME_WARN effectively reduces the
> > >> test coverage of KVM i386.
> > >> 
> > >> For KVM x86, I want the rules for contributing to be clearly documented, and as
> > >> simple as possible.  I don't see a sane way to achieve that with WERROR=y.  
> > 
> > The DRM_I915_WERROR config depends on EXPERT and !COMPILE_TEST, and to
> > my knowledge this has never caused issues outside of i915 developers and
> > CI.
> 
> Ack, I think you do it right. I was trying to establish a precedent
> so that we can delete these as soon as they cause an issue, not sooner.

So isn't the underlying problem simply that KVM_WERROR is enabled by default for
some configurations?  If that's the case, then my proposal to make KVM_WERROR
always off by default, and "depends on KVM && EXPERT && !KASAN", would make this
go away, no?

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

[Paolo Bonzini]

On Wed, Oct 11, 2023 at 1:46 AM Sean Christopherson <seanjc@google.com> wrote:
> > > The DRM_I915_WERROR config depends on EXPERT and !COMPILE_TEST, and to
> > > my knowledge this has never caused issues outside of i915 developers and
> > > CI.
> >
> > Ack, I think you do it right. I was trying to establish a precedent
> > so that we can delete these as soon as they cause an issue, not sooner.
>
> So isn't the underlying problem simply that KVM_WERROR is enabled by default for
> some configurations?  If that's the case, then my proposal to make KVM_WERROR
> always off by default, and "depends on KVM && EXPERT && !KASAN", would make this
> go away, no?

No objection to adding EXPERT. Adding W=1 when build-testing KVM
patches is a good
idea, you will still get the occasional patch from someone who didn't
have it but that's fine.

I added KVM_WERROR a relatively long time ago after a warning scrolled
away too quickly (a
harmless one, but also a kind that honestly shouldn't have made it to
Linus). At the time there
were still too many warnings to enable WERROR globally, and I feel
that now we're on the
same boat with W=1. I think we should keep KVM_WERROR (which was based on
DRM_I915_WERROR indeed) and maintainers should just add W=1 when build-testing
KVM patches.

Paolo