Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 9 Dec 2022 14:44:46 -0800
From:   David Matlack <dmatlack@google.com>
To:     Ben Gardon <bgardon@google.com>
Cc:     linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        Paolo Bonzini <pbonzini@redhat.com>,
        Peter Xu <peterx@redhat.com>,
        Sean Christopherson <seanjc@google.com>,
        Vipin Sharma <vipinsh@google.com>
Subject: Re: [PATCH 6/7] KVM: x86/MMU: Move rmap zap operations to rmap.c
Message-ID: <Y5O6XiJSjGmpMl3R@google.com>
References: <20221206173601.549281-1-bgardon@google.com>
 <20221206173601.549281-7-bgardon@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221206173601.549281-7-bgardon@google.com>
Precedence: bulk

On Tue, Dec 06, 2022 at 05:36:00PM +0000, Ben Gardon wrote:
> Move the various rmap zap functions to rmap.c. These functions are less
> "pure" rmap operations in that they also contain some SPTE manipulation,
> however they're mostly about rmap / pte list manipulation.
> 
> No functional change intended.
> 
> Signed-off-by: Ben Gardon <bgardon@google.com>
> ---
[...]
> -static void kvm_zap_one_rmap_spte(struct kvm *kvm,
> -				  struct kvm_rmap_head *rmap_head, u64 *sptep)
> -{
> -	mmu_spte_clear_track_bits(kvm, sptep);
> -	pte_list_remove(sptep, rmap_head);
> -}
> -
> -/* Return true if at least one SPTE was zapped, false otherwise */
> -static bool kvm_zap_all_rmap_sptes(struct kvm *kvm,
> -				   struct kvm_rmap_head *rmap_head)
> -{
> -	struct pte_list_desc *desc, *next;
> -	int i;
> -
> -	if (!rmap_head->val)
> -		return false;
> -
> -	if (!(rmap_head->val & 1)) {
> -		mmu_spte_clear_track_bits(kvm, (u64 *)rmap_head->val);
> -		goto out;
> -	}
> -
> -	desc = (struct pte_list_desc *)(rmap_head->val & ~1ul);
> -
> -	for (; desc; desc = next) {
> -		for (i = 0; i < desc->spte_count; i++)
> -			mmu_spte_clear_track_bits(kvm, desc->sptes[i]);
> -		next = desc->more;
> -		free_pte_list_desc(desc);
> -	}
> -out:
> -	/* rmap_head is meaningless now, remember to reset it */
> -	rmap_head->val = 0;
> -	return true;
> -}

I don't like moving the rmap zap functions into rmap.c, because they are
more mmu.c functions, as you note in the commit description. e.g. It's
odd to have kvm_zap_all_rmap_sptes() in rmap.c but not, say
__rmap_clear_dirty().

I get your point though that kvm_zap_all_rmap_sptes() has to know
intimate details of the pte_list_desc structure. It would be nice to
keep those details isolated to rmap.c.

What about keeping the zap functions mmu.c and just provide a better API
for kvm_zap_all_rmap_sptes() to process the rmap entries?

e.g.

mmu.c:

static bool kvm_zap_all_rmap_sptes(struct kvm *kvm,
				   struct kvm_rmap_head *rmap_head)
{
	struct rmap_iterator iter;
	bool flush = false;
	u64 *sptep;

	for_each_rmap_spte(rmap_head, &iter, sptep) {
		mmu_spte_clear_track_bits(kvm, sptep);
		flush = true;
	}

	pte_list_free_all(rmap_head);  // <-- implemented in rmap.c
	return flush;
}

This should be about as efficient as the current approach (same big-O
notation at least) and maintain the separation of pte_list_desc
internals in rmap.c.