From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752679AbbKOViW (ORCPT ); Sun, 15 Nov 2015 16:38:22 -0500 Received: from mx1.redhat.com ([209.132.183.28]:53537 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752079AbbKOViU (ORCPT ); Sun, 15 Nov 2015 16:38:20 -0500 Date: Sat, 14 Nov 2015 07:20:47 -0200 From: Marcelo Tosatti To: Takuya Yoshikawa Cc: pbonzini@redhat.com, kvm@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 05/10] KVM: x86: MMU: Use for_each_rmap_spte macro instead of pte_list_walk() Message-ID: <20151114092047.GA25627@amt.cnet> References: <20151112204849.ba920599a8426d7196a0df73@lab.ntt.co.jp> <20151112205245.6bd773737cfa78422dac5a79@lab.ntt.co.jp> <20151113214727.GA29103@amt.cnet> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151113214727.GA29103@amt.cnet> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Nov 13, 2015 at 07:47:28PM -0200, Marcelo Tosatti wrote: > On Thu, Nov 12, 2015 at 08:52:45PM +0900, Takuya Yoshikawa wrote: > > kvm_mmu_mark_parents_unsync() alone uses pte_list_walk(), witch does > > nearly the same as the for_each_rmap_spte macro. The only difference > > is that is_shadow_present_pte() checks cannot be placed there because > > kvm_mmu_mark_parents_unsync() can be called with a new parent pointer > > whose entry is not set yet. > > > > By calling mark_unsync() separately for the parent and adding the parent > > pointer to the parent_ptes chain later in kvm_mmu_get_page(), the macro > > works with no problem. > > > > Signed-off-by: Takuya Yoshikawa > > --- > > arch/x86/kvm/mmu.c | 36 +++++++++++++----------------------- > > 1 file changed, 13 insertions(+), 23 deletions(-) > > > > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c > > index e8cfdc4..1691171 100644 > > --- a/arch/x86/kvm/mmu.c > > +++ b/arch/x86/kvm/mmu.c > > @@ -1007,26 +1007,6 @@ static void pte_list_remove(u64 *spte, unsigned long *pte_list) > > } > > } > > > > -typedef void (*pte_list_walk_fn) (u64 *spte); > > -static void pte_list_walk(unsigned long *pte_list, pte_list_walk_fn fn) > > -{ > > - struct pte_list_desc *desc; > > - int i; > > - > > - if (!*pte_list) > > - return; > > - > > - if (!(*pte_list & 1)) > > - return fn((u64 *)*pte_list); > > - > > - desc = (struct pte_list_desc *)(*pte_list & ~1ul); > > - while (desc) { > > - for (i = 0; i < PTE_LIST_EXT && desc->sptes[i]; ++i) > > - fn(desc->sptes[i]); > > - desc = desc->more; > > - } > > -} > > - > > static unsigned long *__gfn_to_rmap(gfn_t gfn, int level, > > struct kvm_memory_slot *slot) > > { > > @@ -1741,7 +1721,12 @@ static struct kvm_mmu_page *kvm_mmu_alloc_page(struct kvm_vcpu *vcpu, > > static void mark_unsync(u64 *spte); > > static void kvm_mmu_mark_parents_unsync(struct kvm_mmu_page *sp) > > { > > - pte_list_walk(&sp->parent_ptes, mark_unsync); > > + u64 *sptep; > > + struct rmap_iterator iter; > > + > > + for_each_rmap_spte(&sp->parent_ptes, &iter, sptep) { > > + mark_unsync(sptep); > > + } > > } > > > > static void mark_unsync(u64 *spte) > > @@ -2111,12 +2096,17 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, > > Faulting a spte, and one of the levels of sptes, either > > > spte-1 > spte-2 > spte-3 > > has present bit clear. So we're searching for a guest page to shadow, with > gfn "gfn". > > > if (sp->unsync && kvm_sync_page_transient(vcpu, sp)) > > break; > > If a shadow for gfn exists, but is unsync, sync guest-page ---to--> kvm > sptes. > > > - mmu_page_add_parent_pte(vcpu, sp, parent_pte); > > add "gfn" (actually its "struct kvm_mmu_page *sp" pointer) to > the parent. > > if (sp->unsync_children) { > > kvm_make_request(KVM_REQ_MMU_SYNC, vcpu); > > kvm_mmu_mark_parents_unsync(sp); > > kvm_mmu_mark_parents_unsync relied on the links from current level all > the way to top level to mark all levels unsync, so that on guest entry, > KVM_REQ_MMU_SYNC is processed and any level is brought from guest --> > kvm pages. This now fails, because you removed "mmu_page_add_parent_pte" > (the link is not formed all the way to root). > > Unless i am missing something, this is not correct. The actual issue is this: a higher level page that had, under its children, no out of sync pages, now, due to your addition, a child that is unsync: initial state: level1 final state: level1 -x-> level2 -x-> level3 Where -x-> are the links created by this pagefault fixing round. If _any_ page under you is unsync (not necessarily the ones this pagefault is accessing), you have to mark parents unsync.