Received-SPF: pass (google.com: domain of linux-kernel+bounces-204663-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240530210714.364118-1-rick.p.edgecombe@intel.com> <20240530210714.364118-6-rick.p.edgecombe@intel.com>
In-Reply-To: <20240530210714.364118-6-rick.p.edgecombe@intel.com>
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Thu, 6 Jun 2024 18:10:49 +0200
Message-ID: <CABgObfYgF9gcokAg2gOjXqfuxj_oDBmEu4HfDaRc0CLSkmWhoA@mail.gmail.com>
Subject: Re: [PATCH v2 05/15] KVM: x86/mmu: Make kvm_tdp_mmu_alloc_root()
 return void
To: Rick Edgecombe <rick.p.edgecombe@intel.com>
Cc: seanjc@google.com, kvm@vger.kernel.org, kai.huang@intel.com, 
	dmatlack@google.com, erdemaktas@google.com, isaku.yamahata@gmail.com, 
	linux-kernel@vger.kernel.org, sagis@google.com, yan.y.zhao@intel.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, May 30, 2024 at 11:07=E2=80=AFPM Rick Edgecombe
<rick.p.edgecombe@intel.com> wrote:
>
> The kvm_tdp_mmu_alloc_root() function currently always returns 0. This
> allows for the caller, mmu_alloc_direct_roots(), to call
> kvm_tdp_mmu_alloc_root() and also return 0 in one line:
>    return kvm_tdp_mmu_alloc_root(vcpu);
>
> So it is useful even though the return value of kvm_tdp_mmu_alloc_root()
> is always the same. However, in future changes, kvm_tdp_mmu_alloc_root()
> will be called twice in mmu_alloc_direct_roots(). This will force the
> first call to either awkwardly handle the return value that will always
> be zero or ignore it. So change kvm_tdp_mmu_alloc_root() to return void.
> Do it in a separate change so the future change will be cleaner.
>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

> ---
> TDX MMU Prep:
>  - New patch
> ---
>  arch/x86/kvm/mmu/mmu.c     | 6 ++++--
>  arch/x86/kvm/mmu/tdp_mmu.c | 3 +--
>  arch/x86/kvm/mmu/tdp_mmu.h | 2 +-
>  3 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 5070ba7c6e89..12178945922f 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -3700,8 +3700,10 @@ static int mmu_alloc_direct_roots(struct kvm_vcpu =
*vcpu)
>         unsigned i;
>         int r;
>
> -       if (tdp_mmu_enabled)
> -               return kvm_tdp_mmu_alloc_root(vcpu);
> +       if (tdp_mmu_enabled) {
> +               kvm_tdp_mmu_alloc_root(vcpu);
> +               return 0;
> +       }
>
>         write_lock(&vcpu->kvm->mmu_lock);
>         r =3D make_mmu_pages_available(vcpu);
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index e7cd4921afe7..2770230a5636 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -224,7 +224,7 @@ static void tdp_mmu_init_child_sp(struct kvm_mmu_page=
 *child_sp,
>         tdp_mmu_init_sp(child_sp, iter->sptep, iter->gfn, role);
>  }
>
> -int kvm_tdp_mmu_alloc_root(struct kvm_vcpu *vcpu)
> +void kvm_tdp_mmu_alloc_root(struct kvm_vcpu *vcpu)
>  {
>         struct kvm_mmu *mmu =3D vcpu->arch.mmu;
>         union kvm_mmu_page_role role =3D mmu->root_role;
> @@ -285,7 +285,6 @@ int kvm_tdp_mmu_alloc_root(struct kvm_vcpu *vcpu)
>          */
>         mmu->root.hpa =3D __pa(root->spt);
>         mmu->root.pgd =3D 0;
> -       return 0;
>  }
>
>  static void handle_changed_spte(struct kvm *kvm, int as_id, gfn_t gfn,
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.h b/arch/x86/kvm/mmu/tdp_mmu.h
> index 58b55e61bd33..437ddd4937a9 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.h
> +++ b/arch/x86/kvm/mmu/tdp_mmu.h
> @@ -10,7 +10,7 @@
>  void kvm_mmu_init_tdp_mmu(struct kvm *kvm);
>  void kvm_mmu_uninit_tdp_mmu(struct kvm *kvm);
>
> -int kvm_tdp_mmu_alloc_root(struct kvm_vcpu *vcpu);
> +void kvm_tdp_mmu_alloc_root(struct kvm_vcpu *vcpu);
>
>  __must_check static inline bool kvm_tdp_mmu_get_root(struct kvm_mmu_page=
 *root)
>  {
> --
> 2.34.1
>