Received-SPF: pass (google.com: domain of linux-kernel+bounces-205587-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-9-rick.p.edgecombe@intel.com>
In-Reply-To: <20240530210714.364118-9-rick.p.edgecombe@intel.com>
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Fri, 7 Jun 2024 10:10:28 +0200
Message-ID: <CABgObfZr_YNcymua7ejapiL0+M=0CQhroheerj-1_YYxisp=Ug@mail.gmail.com>
Subject: Re: [PATCH v2 08/15] KVM: x86/tdp_mmu: Introduce KVM MMU root types
 to specify page table type
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, 
	Isaku Yamahata <isaku.yamahata@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:
> +enum kvm_tdp_mmu_root_types {
> +       KVM_VALID_ROOTS =3D BIT(0),
> +
> +       KVM_ANY_ROOTS =3D 0,
> +       KVM_ANY_VALID_ROOTS =3D KVM_VALID_ROOTS,

I would instead define it as

    KVM_INVALID_ROOTS =3D BIT(0),
    KVM_VALID_ROOTS =3D BIT(1),
    KVM_ALL_ROOTS =3D KVM_VALID_ROOTS | KVM_INVALID_ROOTS,

and then

  if (root->role.invalid)
    return types & KVM_INVALID_ROOTS;
  else
    return types & KVM_VALID_ROOTS;

Then in the next patch you can do

     KVM_INVALID_ROOTS =3D BIT(0),
-    KVM_VALID_ROOTS =3D BIT(1),
+   KVM_DIRECT_ROOTS =3D BIT(1),
+   KVM_MIRROR_ROOTS =3D BIT(2),
+   KVM_VALID_ROOTS =3D KVM_DIRECT_ROOTS | KVM_MIRROR_ROOTS,
     KVM_ALL_ROOTS =3D KVM_VALID_ROOTS | KVM_INVALID_ROOTS,

and likewise

  if (root->role.invalid)
    return types & KVM_INVALID_ROOTS;
  else if (likely(!is_mirror_sp(root)))
    return types & KVM_DIRECT_ROOTS;
  else
    return types & KVM_MIRROR_ROOTS;

This removes the need for KVM_ANY_VALID_ROOTS (btw I don't know if
it's me, but ALL sounds more grammatical than ANY in this context). So
the resulting code should be a bit clearer.

Apart from this small tweak, the overall idea is really good.

Paolo

> +};
> +
>  bool kvm_tdp_mmu_zap_leafs(struct kvm *kvm, gfn_t start, gfn_t end, bool=
 flush);
>  bool kvm_tdp_mmu_zap_sp(struct kvm *kvm, struct kvm_mmu_page *sp);
>  void kvm_tdp_mmu_zap_all(struct kvm *kvm);
> --
> 2.34.1
>