Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Date:   Tue, 17 Oct 2023 14:31:28 -0700
In-Reply-To: <20231017093335.18216-1-likexu@tencent.com>
Mime-Version: 1.0
References: <20231017093335.18216-1-likexu@tencent.com>
Message-ID: <ZS79MFkVB6A1N9AA@google.com>
Subject: Re: [PATCH] KVM: x86: Clean up included but non-essential header declarations
From:   Sean Christopherson <seanjc@google.com>
To:     Like Xu <like.xu.linux@gmail.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Maxim Levitsky <mlevitsk@redhat.com>,
        Ingo Molnar <mingo@redhat.com>, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

On Tue, Oct 17, 2023, Like Xu wrote:
> From: Like Xu <likexu@tencent.com>
> Removing these declarations as part of KVM code refactoring can also (when
> the compiler isn't so smart) reduce compile time, compile warnings, and the

Really, warnings?  On what W= level?  W=1 builds just fine with KVM_WERROR=y.
If any of the "supported" warn levels triggers a warn=>error, then we'll fix it.

> size of compiled artefacts, and more importantly can help developers better
> consider decoupling when adding/refactoring unmerged code, thus relieving
> some of the burden on the code review process.

Can you provide an example?  KVM certainly has its share of potential circular
dependency pitfalls, e.g. it's largely why we have the ugly and seemingly
arbitrary split between x86.h and asm/kvm_host.h.  But outside of legitimate
collisions like that, I can't think of a single instance where superfluous existing
includes caused problems.  On the other hand, I distinctly recall multiple
instances where a header didn't include what it used and broke the build when the
buggy header was included in a new file.

> Specific header declaration is supposed to be retained if it makes more
> sense for reviewers to understand. No functional changes intended.
> 
> [*] https://lore.kernel.org/all/YdIfz+LMewetSaEB@gmail.com/

This patch violates one of the talking points of Ingo's work:

 - "Make headers standalone": over 80 headers don't build standalone and
   depend on various accidental indirect dependencies they gain through
   other headers, especially once headers get their unnecessary dependencies
   removed. So there's over 80 commits changing these headers.

I think it's also worth noting that Ingo boosted build times by eliminating
includes in common "high use" headers, not by playing whack-a-mole with "private"
headers and C files.

> [**] https://include-what-you-use.org/
> 
> Signed-off-by: Like Xu <likexu@tencent.com>
> ---
>  arch/x86/kvm/cpuid.c           |  3 ---
>  arch/x86/kvm/cpuid.h           |  1 -
>  arch/x86/kvm/emulate.c         |  2 --
>  arch/x86/kvm/hyperv.c          |  3 ---
>  arch/x86/kvm/i8259.c           |  1 -
>  arch/x86/kvm/ioapic.c          | 10 ----------
>  arch/x86/kvm/irq.c             |  3 ---
>  arch/x86/kvm/irq.h             |  3 ---
>  arch/x86/kvm/irq_comm.c        |  2 --
>  arch/x86/kvm/lapic.c           |  8 --------
>  arch/x86/kvm/mmu.h             |  1 -
>  arch/x86/kvm/mmu/mmu.c         | 11 -----------
>  arch/x86/kvm/mmu/spte.c        |  1 -
>  arch/x86/kvm/mmu/tdp_iter.h    |  1 -
>  arch/x86/kvm/mmu/tdp_mmu.c     |  3 ---
>  arch/x86/kvm/mmu/tdp_mmu.h     |  4 ----
>  arch/x86/kvm/smm.c             |  1 -
>  arch/x86/kvm/smm.h             |  3 ---
>  arch/x86/kvm/svm/avic.c        |  2 --
>  arch/x86/kvm/svm/hyperv.h      |  2 --
>  arch/x86/kvm/svm/nested.c      |  2 --
>  arch/x86/kvm/svm/pmu.c         |  4 ----
>  arch/x86/kvm/svm/sev.c         |  7 -------
>  arch/x86/kvm/svm/svm.c         | 29 -----------------------------
>  arch/x86/kvm/svm/svm.h         |  3 ---
>  arch/x86/kvm/vmx/hyperv.c      |  4 ----
>  arch/x86/kvm/vmx/hyperv.h      |  7 -------
>  arch/x86/kvm/vmx/nested.c      |  2 --
>  arch/x86/kvm/vmx/nested.h      |  1 -
>  arch/x86/kvm/vmx/pmu_intel.c   |  1 -
>  arch/x86/kvm/vmx/posted_intr.c |  1 -
>  arch/x86/kvm/vmx/sgx.h         |  5 -----
>  arch/x86/kvm/vmx/vmx.c         | 11 -----------
>  arch/x86/kvm/x86.c             | 17 -----------------
>  arch/x86/kvm/xen.c             |  1 -
>  virt/kvm/async_pf.c            |  2 --
>  virt/kvm/binary_stats.c        |  1 -
>  virt/kvm/coalesced_mmio.h      |  2 --
>  virt/kvm/eventfd.c             |  2 --
>  virt/kvm/irqchip.c             |  1 -
>  virt/kvm/kvm_main.c            | 13 -------------
>  virt/kvm/pfncache.c            |  1 -
>  virt/kvm/vfio.c                |  2 --
>  43 files changed, 184 deletions(-)

NAK, I am not taking a wholesale purge of includes.  I have no objection to
removing truly unnecessary includes, e.g. there are definitely some includes that
are no longer necessary due to code being moved around.  But changes like the
removal of all includes from tdp_mmu.h and smm.h are completely bogus.  If anything,
smm.h clearly needs more includes, because it is certainly not including everything
it is using.

> diff --git a/arch/x86/kvm/mmu/tdp_mmu.h b/arch/x86/kvm/mmu/tdp_mmu.h
> index 733a3aef3a96..66afdf3e262a 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.h
> +++ b/arch/x86/kvm/mmu/tdp_mmu.h
> @@ -3,10 +3,6 @@
>  #ifndef __KVM_X86_MMU_TDP_MMU_H
>  #define __KVM_X86_MMU_TDP_MMU_H
>  
> -#include <linux/kvm_host.h>
> -
> -#include "spte.h"
> -
>  void kvm_mmu_init_tdp_mmu(struct kvm *kvm);
>  void kvm_mmu_uninit_tdp_mmu(struct kvm *kvm);

...

> diff --git a/arch/x86/kvm/smm.h b/arch/x86/kvm/smm.h
> index a1cf2ac5bd78..3e067ce1ea1d 100644
> --- a/arch/x86/kvm/smm.h
> +++ b/arch/x86/kvm/smm.h
> @@ -2,11 +2,8 @@
>  #ifndef ASM_KVM_SMM_H
>  #define ASM_KVM_SMM_H
>  
> -#include <linux/build_bug.h>
> -
>  #ifdef CONFIG_KVM_SMM
>  
> -
>  /*
>   * 32 bit KVM's emulated SMM layout. Based on Intel P6 layout
>   * (https://www.sandpile.org/x86/smm.htm).