From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH] KVM: MMU: Segregate mmu pages created with different cr4.pge settings Date: Tue, 06 Jan 2009 16:29:59 +0200 Message-ID: <49636AE7.4090108@redhat.com> References: <20081221184146.8E00B250012@cleopatra.tlv.redhat.com> <49621FA9.5080903@suse.de> <49633564.7070403@redhat.com> <20090106141151.GA3701@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Alexander Graf , "kvm@vger.kernel.org" , joerg.roedel@amd.com To: Marcelo Tosatti Return-path: Received: from mx2.redhat.com ([66.187.237.31]:56462 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752032AbZAFOaF (ORCPT ); Tue, 6 Jan 2009 09:30:05 -0500 In-Reply-To: <20090106141151.GA3701@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: Marcelo Tosatti wrote: > On Tue, Jan 06, 2009 at 12:41:40PM +0200, Avi Kivity wrote: > >> Alexander Graf wrote: >> >>> Avi Kivity wrote: >>> >>> >>>> From: Avi Kivity >>>> >>>> Don't allow a vcpu with cr4.pge cleared to use a shadow page created with >>>> cr4.pge set; this might cause a cr3 switch not to sync ptes that have the >>>> global bit set (the global bit has no effect if !cr4.pge). >>>> >>>> This can only occur on smp with different cr4.pge settings for different >>>> vcpus (since a cr4 change will resync the shadow ptes), but there's no >>>> cost to being correct here. >>>> >>>> Signed-off-by: Avi Kivity >>>> >>>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h >>>> index f49bfd0..ab8ef1d 100644 >>>> --- a/arch/x86/include/asm/kvm_host.h >>>> +++ b/arch/x86/include/asm/kvm_host.h >>>> @@ -183,6 +183,7 @@ union kvm_mmu_page_role { >>>> unsigned metaphysical:1; >>>> unsigned access:3; >>>> unsigned invalid:1; >>>> + unsigned cr4_pge:1; >>>> }; >>>> }; >>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >>>> index c4da7fb..aa4575c 100644 >>>> --- a/arch/x86/kvm/x86.c >>>> +++ b/arch/x86/kvm/x86.c >>>> @@ -363,6 +363,7 @@ void kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) >>>> } >>>> kvm_x86_ops->set_cr4(vcpu, cr4); >>>> vcpu->arch.cr4 = cr4; >>>> + vcpu->arch.mmu.base_role.cr4_pge = !!(cr4 & X86_CR4_PGE); >>>> >>>> >>> This line broke VMware ESX bootup using NPT on one VCPU for me. If I >>> comment it out and apply my patches to actually make ESX run, it boots >>> again. >>> >>> >> I think this might be the problem: >> >> >>> static int mmu_need_write_protect(struct kvm_vcpu *vcpu, gfn_t gfn, >>> bool can_unsync) >>> { >>> struct kvm_mmu_page *shadow; >>> >>> shadow = kvm_mmu_lookup_page(vcpu->kvm, gfn); >>> if (shadow) { >>> if (shadow->role.level != PT_PAGE_TABLE_LEVEL) >>> return 1; >>> if (shadow->unsync) >>> return 0; >>> if (can_unsync && oos_shadow) >>> return kvm_unsync_page(vcpu, shadow); >>> return 1; >>> } >>> return 0; >>> } >>> >>> >> lookup_page() is not deterministic if there are multiple shadows for a >> page, and the patch increases multiple shadows. >> >> Marcelo, shouldn't there be a for_each in there? >> > > static int kvm_unsync_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp) { > > unsigned index; > struct hlist_head *bucket; > struct kvm_mmu_page *s; > struct hlist_node *node, *n; > > index = kvm_page_table_hashfn(sp->gfn); > bucket = &vcpu->kvm->arch.mmu_page_hash[index]; > /* don't unsync if pagetable is shadowed with multiple roles */ > hlist_for_each_entry_safe(s, node, n, bucket, hash_link) { > if (s->gfn != sp->gfn || s->role.metaphysical) > continue; > if (s->role.word != sp->role.word) > return 1; > } > > This one? > Yes... Looks like kvm_unsync_page can be folded into mmu_need_write_protect (after which we can drop lookup_page(), which is not a good API). But that's after we solve the current problem. Looks like the addition of a second role for non-pge mode confuses the mmu. After the second page is created, mmu_need_write_protect() will return 1, but previously existing sptes can still be writable? Looks like we need to call rmap_write_protect() when the new page is created. -- error compiling committee.c: too many arguments to function