[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f23f0869-fd33-339a-4833-1249dd4c1670@redhat.com>
Date: Mon, 15 Oct 2018 13:51:13 +0200
From: Paolo Bonzini <pbonzini@...hat.com>
To: lantianyu1986@...il.com
Cc: Lan Tianyu <Tianyu.Lan@...rosoft.com>, kys@...rosoft.com,
haiyangz@...rosoft.com, sthemmin@...rosoft.com, tglx@...utronix.de,
mingo@...hat.com, hpa@...or.com, x86@...nel.org,
rkrcmar@...hat.com, devel@...uxdriverproject.org,
linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
michael.h.kelley@...rosoft.com, vkuznets@...hat.com
Subject: Re: [PATCH V4 11/15] KVM/MMU: Replace tlb flush function with range
list flush function
On 13/10/2018 16:54, lantianyu1986@...il.com wrote:
> while (mmu_unsync_walk(parent, &pages)) {
> bool protected = false;
> + LIST_HEAD(flush_list);
>
> - for_each_sp(pages, sp, parents, i)
> + for_each_sp(pages, sp, parents, i) {
> protected |= rmap_write_protect(vcpu, sp->gfn);
> + kvm_mmu_queue_flush_request(sp, &flush_list);
> + }
Here you already know that the page has to be flushed, because you are
dealing with shadow page tables and those always use 4K pages. So the
check on is_last_page is unnecessary.
>
> pte_access, PT_PAGE_TABLE_LEVEL,
> gfn, spte_to_pfn(sp->spt[i]),
> true, false, host_writable);
> + if (set_spte_ret && kvm_available_flush_tlb_with_range())
> + kvm_mmu_queue_flush_request(sp, &flush_list);
> }
This is wrong, I think. sp is always the same throughout the loop, so
you are adding it multiple times to flush_list.
Instead, you need to add a separate range for each virtual address (in
this case L2 GPA) that is synced; but for each PTE that you call
set_spte here for, you could be syncing multiple L2 GPAs if a single
page is reused multiple times by the guest's EPT page tables.
And actually I may be missing something, but doesn't this apply to all
call sites? For mmu_sync_children you can do the flush in
__rmap_write_protect and return false, similar to the first part of the
series, but not for kvm_mmu_commit_zap_page and sync_page.
Can you simplify this series to only have hv_remote_flush_tlb_with_range
and remove all the flush_list stuff? That first part is safe and well
understood, because it uses the rmap and so it's clear that you have L2
GPAs at hand. Most of the remarks I made on the Hyper-V API will still
apply.
Paolo
> if (set_spte_ret & SET_SPTE_NEED_REMOTE_TLB_FLUSH)
> - kvm_flush_remote_tlbs(vcpu->kvm);
> + kvm_flush_remote_tlbs_with_list(vcpu->kvm, &flush_list);
>
> return nr_present;
Powered by blists - more mailing lists