From: Isaku Yamahata <isaku.yamahata@intel.com>

Extract tdp_mmu_root_match() to check if the root has given types and use
it for the root page table iterator.  It checks only_invalid now.

TDX KVM operates on a shared page table only (Shared-EPT), a mirrored page
table only (Secure-EPT), or both based on the operation.  KVM MMU notifier
operations only on shared page table.  KVM guest_memfd invalidation
operations only on mirrored page table, and so on.  Introduce a centralized
matching function instead of open coding matching logic in the iterator.
The next step is to extend the function to check whether the page is shared
or private

Link: https://lore.kernel.org/kvm/ZivazWQw1oCU8VBC@google.com/
Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Message-ID: <20240718211230.1492011-10-rick.p.edgecombe@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/mmu/tdp_mmu.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index 5b01038ddce8..e0ccfdd4200b 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -92,6 +92,14 @@ void kvm_tdp_mmu_put_root(struct kvm *kvm, struct kvm_mmu_page *root)
 	call_rcu(&root->rcu_head, tdp_mmu_free_sp_rcu_callback);
 }
 
+static bool tdp_mmu_root_match(struct kvm_mmu_page *root, bool only_valid)
+{
+	if (only_valid && root->role.invalid)
+		return false;
+
+	return true;
+}
+
 /*
  * Returns the next root after @prev_root (or the first root if @prev_root is
  * NULL).  A reference to the returned root is acquired, and the reference to
@@ -125,7 +133,7 @@ static struct kvm_mmu_page *tdp_mmu_next_root(struct kvm *kvm,
 						   typeof(*next_root), link);
 
 	while (next_root) {
-		if ((!only_valid || !next_root->role.invalid) &&
+		if (tdp_mmu_root_match(next_root, only_valid) &&
 		    kvm_tdp_mmu_get_root(next_root))
 			break;
 
@@ -176,7 +184,7 @@ static struct kvm_mmu_page *tdp_mmu_next_root(struct kvm *kvm,
 	list_for_each_entry(_root, &_kvm->arch.tdp_mmu_roots, link)		\
 		if (kvm_lockdep_assert_mmu_lock_held(_kvm, false) &&		\
 		    ((_as_id >= 0 && kvm_mmu_page_as_id(_root) != _as_id) ||	\
-		     ((_only_valid) && (_root)->role.invalid))) {		\
+		     !tdp_mmu_root_match((_root), (_only_valid)))) {		\
 		} else
 
 #define for_each_tdp_mmu_root(_kvm, _root, _as_id)			\
-- 
2.43.5

On Sun, Dec 22, 2024, Paolo Bonzini wrote:
> From: Isaku Yamahata <isaku.yamahata@intel.com>
> 
> Extract tdp_mmu_root_match() to check if the root has given types and use
> it for the root page table iterator.  It checks only_invalid now.
> 
> TDX KVM operates on a shared page table only (Shared-EPT), a mirrored page
> table only (Secure-EPT), or both based on the operation.  KVM MMU notifier
> operations only on shared page table.  KVM guest_memfd invalidation
> operations only on mirrored page table, and so on.  Introduce a centralized
> matching function instead of open coding matching logic in the iterator.
> The next step is to extend the function to check whether the page is shared
> or private
> 
> Link: https://lore.kernel.org/kvm/ZivazWQw1oCU8VBC@google.com/
> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> Message-ID: <20240718211230.1492011-10-rick.p.edgecombe@intel.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/mmu/tdp_mmu.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index 5b01038ddce8..e0ccfdd4200b 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -92,6 +92,14 @@ void kvm_tdp_mmu_put_root(struct kvm *kvm, struct kvm_mmu_page *root)
>  	call_rcu(&root->rcu_head, tdp_mmu_free_sp_rcu_callback);
>  }
>  
> +static bool tdp_mmu_root_match(struct kvm_mmu_page *root, bool only_valid)
> +{
> +	if (only_valid && root->role.invalid)
> +		return false;
> +
> +	return true;

Ugh, this is almost as bad as

	if (x)
		return true;

	return false;

Just do

	return !only_valid || root->role.invalid;

And I vote to drop the helper, "match" makes me think about things like roles
and addresses, not just if a root is valid.

> +}
> +
>  /*
>   * Returns the next root after @prev_root (or the first root if @prev_root is
>   * NULL).  A reference to the returned root is acquired, and the reference to
> @@ -125,7 +133,7 @@ static struct kvm_mmu_page *tdp_mmu_next_root(struct kvm *kvm,
>  						   typeof(*next_root), link);
>  
>  	while (next_root) {
> -		if ((!only_valid || !next_root->role.invalid) &&
> +		if (tdp_mmu_root_match(next_root, only_valid) &&

E.g. this can just as easily be 

		    !only_valid || root->role.invalid &&

which is amusingly even fewer characters :-D

>  		    kvm_tdp_mmu_get_root(next_root))
>  			break;
>  
> @@ -176,7 +184,7 @@ static struct kvm_mmu_page *tdp_mmu_next_root(struct kvm *kvm,
>  	list_for_each_entry(_root, &_kvm->arch.tdp_mmu_roots, link)		\
>  		if (kvm_lockdep_assert_mmu_lock_held(_kvm, false) &&		\
>  		    ((_as_id >= 0 && kvm_mmu_page_as_id(_root) != _as_id) ||	\
> -		     ((_only_valid) && (_root)->role.invalid))) {		\
> +		     !tdp_mmu_root_match((_root), (_only_valid)))) {		\
>  		} else
>  
>  #define for_each_tdp_mmu_root(_kvm, _root, _as_id)			\
> -- 
> 2.43.5
> 
>

On Sat, Jan 18, 2025 at 2:05 AM Sean Christopherson <seanjc@google.com> wrote:
> > +static bool tdp_mmu_root_match(struct kvm_mmu_page *root, bool only_valid)
> > +{
> > +     if (only_valid && root->role.invalid)
> > +             return false;
> > +
> > +     return true;
>
> Ugh, this is almost as bad as
>
>         if (x)
>                 return true;
>
>         return false;
>
> Just do
>
>         return !only_valid || root->role.invalid;
>
> And I vote to drop the helper, "match" makes me think about things like roles
> and addresses, not just if a root is valid.

This is not the definitive shape of the function; by the end of the
series it becomes

static bool tdp_mmu_root_match(struct kvm_mmu_page *root,
                               enum kvm_tdp_mmu_root_types types)
{
        if (WARN_ON_ONCE(!(types & KVM_VALID_ROOTS)))
                return false;

        if (root->role.invalid && !(types & KVM_INVALID_ROOTS))
                return false;

        if (likely(!is_mirror_sp(root)))
                return types & KVM_DIRECT_ROOTS;
        return types & KVM_MIRROR_ROOTS;
}

(where the first "if" could also be just a WARN_ON_ONCE without the
anticipated return, since KVM_VALID_ROOTS == KVM_DIRECT_ROOTS |
KVM_MIRROR_ROOTS)

Paolo