| Message ID | 20211018174137.579907-1-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [GIT,PULL] KVM fixes for Linux 5.15-rc7 | expand |
On Mon, Oct 18, 2021 at 7:42 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > * avoid warning with -Wbitwise-instead-of-logical Christ. Please no. Guys, you can't just mindlessly shut off warnings without even thinking about the code. Apparently the compiler gives completely insane warning "fixes" suggestions, and somebody just completely mindlessly followed that compiler badness. The way to do a logical "or" (instead of a bitwise one on two boolean expressions) is to use "||". Instead, the code was changed to completely insane (int) boolexpr1 | (int) boolexpr2 thing, which is entirely illegible and pointless, and no sane person should ever write code like that. In other words, the *proper* fix to a warning is to look at the code, and *unsderstand* the code and the warning, instead of some mindless conversion to just avoid a warning. NEVER EVER do mindless changes to source code because the compiler tells you to. Apparently the clang people wrote a particularly bad warning "explanation", and that's on clang. I'm not going to pull this. The clang warning fix is wrong, and then another commit literally disables accounting for another non-fatal run-time warning. Again - warnings are not an excuse to just "mindlessly shut up the warning". They need some thought. None of this kind of "I'll do wrong things just to make the warning go away" garbage that this pull request has two very different examples of. I'm adding some clang people, because apparently that note: cast one or both operands to int to silence this warning thing came from clang. Somebody in the clang community really needs to re-think their "informational" messages. Giving people those kinds of insane suggestions is a disservice to everybody. Clang should fix their stupid "note" before release. Please, guys. Linus
On 18/10/21 19:57, Linus Torvalds wrote: > The way to do a logical "or" (instead of a bitwise one on two boolean > expressions) is to use "||". > > Instead, the code was changed to completely insane > > (int) boolexpr1 | (int) boolexpr2 > > thing, which is entirely illegible and pointless, and no sane person > should ever write code like that. > > In other words, the*proper* fix to a warning is to look at the code, > and*unsderstand* the code and the warning, instead of some mindless > conversion to just avoid a warning. The code is not wrong, there is a comment explaining it: * Use a bitwise-OR instead of a logical-OR to aggregate the reserved * bits and EPT's invalid memtype/XWR checks to avoid an extra Jcc * (this is extremely unlikely to be short-circuited as true). Paolo
On Mon, Oct 18, 2021 at 8:03 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > The code is not wrong, there is a comment explaining it: > > * Use a bitwise-OR instead of a logical-OR to aggregate the reserved > * bits and EPT's invalid memtype/XWR checks to avoid an extra Jcc > * (this is extremely unlikely to be short-circuited as true). That makes very little sense. It seems to be avoiding a 'jcc' and replace it with a 'setcc' and an 'or'. Which is likely both bigger and slower. If the jcc were unpredictable, maybe that would be one thing. But at least from a quick look, that doesn't even seem likely The other use of that function has a "WARN_ONCE()" on it, so presumably it normally doesn't ever trigger either of the boolean conditions. Very strange code. Linus
On 18/10/21 20:15, Linus Torvalds wrote: >> * Use a bitwise-OR instead of a logical-OR to aggregate the reserved >> * bits and EPT's invalid memtype/XWR checks to avoid an extra Jcc >> * (this is extremely unlikely to be short-circuited as true). > That makes very little sense. > > It seems to be avoiding a 'jcc' and replace it with a 'setcc' and an > 'or'. Which is likely both bigger and slower. The compiler knows that the setcc is unnecessary, because a!=0|b!=0 is the same as (a|b)!=0. > If the jcc were unpredictable, maybe that would be one thing. But at > least from a quick look, that doesn't even seem likely > > The other use of that function has a "WARN_ONCE()" on it, so > presumably it normally doesn't ever trigger either of the boolean > conditions. Yes, and that was why it used a "|". Anyway, not worth arguing for or against; I'll just change it to logical OR. Paolo
Linus, The following changes since commit 7b0035eaa7dab9fd33d6658ad6a755024bdce26c: KVM: selftests: Ensure all migrations are performed when test is affined (2021-09-30 04:25:57 -0400) are available in the Git repository at: https://git.kernel.org/pub/scm/virt/kvm/kvm.git tags/for-linus for you to fetch changes up to 4934c52be82cb235d756c27c1cc769a0498607fb: KVM: SEV-ES: reduce ghcb_sa_len to 32 bits (2021-10-18 06:49:26 -0400) ---------------------------------------------------------------- Tools: * kvm_stat: do not show halt_wait_ns since it is not a cumulative statistic x86: * clean ups and fixes for bus lock vmexit and lazy allocation of rmaps * avoid warning with -Wbitwise-instead-of-logical * avoid warning due to huge kvmalloc. Temporarily disables accounting, will be fixed in 5.16 and backported to stable 5.15 * two fixes for SEV-ES (one more coming as soon as I get reviews) * fix for static_key underflow ARM: * Properly refcount pages used as a concatenated stage-2 PGD * Fix missing unlock when detecting the use of MTE+VM_SHARED ---------------------------------------------------------------- Christian Borntraeger (1): KVM: kvm_stat: do not show halt_wait_ns Hao Xiang (1): KVM: VMX: Remove redundant handling of bus lock vmexit Janosch Frank (1): KVM: s390: Function documentation fixes Paolo Bonzini (7): Merge tag 'kvm-s390-master-5.15-1' of git://git.kernel.org/pub/scm/linux/kernel/git/kvms390/linux into kvm-master KVM: SEV-ES: fix length of string I/O Merge tag 'kvmarm-fixes-5.15-2' of git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm into HEAD KVM: replace large kvmalloc allocation with vmalloc KVM: X86: fix lazy allocation of rmaps KVM: x86: avoid warning with -Wbitwise-instead-of-logical KVM: SEV-ES: reduce ghcb_sa_len to 32 bits Peter Gonda (1): KVM: SEV-ES: Set guest_state_protected after VMSA update Quentin Perret (3): KVM: arm64: Fix host stage-2 PGD refcount KVM: arm64: Report corrupted refcount at EL2 KVM: arm64: Release mmap_lock when using VM_SHARED with MTE Sean Christopherson (2): Revert "KVM: x86: Open code necessary bits of kvm_lapic_set_base() at vCPU RESET" KVM: x86: WARN if APIC HW/SW disable static keys are non-zero on unload arch/arm64/kvm/hyp/include/nvhe/gfp.h | 1 + arch/arm64/kvm/hyp/nvhe/mem_protect.c | 13 ++++++++++++- arch/arm64/kvm/hyp/nvhe/page_alloc.c | 15 +++++++++++++++ arch/arm64/kvm/mmu.c | 6 ++++-- arch/s390/kvm/gaccess.c | 12 ++++++++++++ arch/s390/kvm/intercept.c | 4 +++- arch/x86/kvm/lapic.c | 20 +++++++++++++------- arch/x86/kvm/mmu/page_track.c | 3 +-- arch/x86/kvm/mmu/spte.h | 7 +++++-- arch/x86/kvm/svm/sev.c | 9 +++++++-- arch/x86/kvm/svm/svm.h | 2 +- arch/x86/kvm/vmx/vmx.c | 15 +++++++++------ arch/x86/kvm/x86.c | 7 ++++--- include/linux/vmalloc.h | 10 ++++++++++ tools/kvm/kvm_stat/kvm_stat | 2 +- virt/kvm/kvm_main.c | 4 ++-- 16 files changed, 100 insertions(+), 30 deletions(-)