linux-kernel - Re: [PATCH 6/7] KVM: x86/MMU: Move rmap zap operations to rmap.c

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 9 Dec 2022 14:44:46 -0800
From:   David Matlack <dmatlack@...gle.com>
To:     Ben Gardon <bgardon@...gle.com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        Paolo Bonzini <pbonzini@...hat.com>,
        Peter Xu <peterx@...hat.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Vipin Sharma <vipinsh@...gle.com>
Subject: Re: [PATCH 6/7] KVM: x86/MMU: Move rmap zap operations to rmap.c

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@...gle.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.