| Message ID | 20231006205415.3501535-1-kuba@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: deprecate KVM_WERROR in favor of general WERROR | expand |
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>
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.
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.
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.
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?
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.
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.
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?
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
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
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(-)