From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [PATCH RFC] KVM MMU: fix hashing for TDP and non-paging modes Date: Mon, 26 Apr 2010 18:30:00 -0300 Message-ID: <20100426213000.GK21425@amt.cnet> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org To: Eric Northup Return-path: Received: from mx1.redhat.com ([209.132.183.28]:49643 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753664Ab0DZVaU (ORCPT ); Mon, 26 Apr 2010 17:30:20 -0400 Content-Disposition: inline In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: On Thu, Apr 22, 2010 at 02:15:14PM -0700, Eric Northup wrote: > I've been reading the x86's mmu.c recently and had been wondering > about something. Avi's recent mmu documentation (thanks!) seems to > have confirmed my understanding of how the shadow paging is supposed > to be working. In TDP mode, when mmu_alloc_roots() calls > kvm_mmu_get_page(), why does it pass (vcpu->arch.cr3 >> PAGE_SHIFT) or > (vcpu->arch.mmu.pae_root[i]) as gfn? > > It seems to me that in TDP mode, gfn should be either zero for the > root page table, or 0/1GB/2GB/3GB (for PAE page tables). > > The existing behavior can lead to multiple, semantically-identical TDP > roots being created by mmu_alloc_roots, depending on the VCPU's CR3 at > the time that mmu_alloc_roots was called. But the nested page tables > should be* independent of the VCPU state. That wastes some memory and > causes extra page faults while populating the extra copies of the page > tables. > > *assuming that we aren't modeling per-VCPU state that might change the > physical address map as seen by that VCPU, such as setting the APIC > base to an address overlapping RAM. > > All feedback would be welcome, since I'm new to this system! A > strawman patch follows. > > thanks, > -Eric > > -- > > For TDP mode, avoid creating multiple page table roots for the single > guest-to-host physical address map by fixing the inputs used for the > shadow page table hash in mmu_alloc_roots(). > > Signed-off-by: Eric Northup > --- > arch/x86/kvm/mmu.c | 12 ++++++++---- > 1 files changed, 8 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c > index ddfa865..9696d65 100644 > --- a/arch/x86/kvm/mmu.c > +++ b/arch/x86/kvm/mmu.c > @@ -2059,10 +2059,12 @@ static int mmu_alloc_roots(struct kvm_vcpu *vcpu) > hpa_t root = vcpu->arch.mmu.root_hpa; > > ASSERT(!VALID_PAGE(root)); > - if (tdp_enabled) > - direct = 1; > if (mmu_check_root(vcpu, root_gfn)) > return 1; > + if (tdp_enabled) { > + direct = 1; > + root_gfn = 0; > + } > sp = kvm_mmu_get_page(vcpu, root_gfn, 0, > PT64_ROOT_LEVEL, direct, > ACC_ALL, NULL); > @@ -2072,8 +2074,6 @@ static int mmu_alloc_roots(struct kvm_vcpu *vcpu) > return 0; > } > direct = !is_paging(vcpu); > - if (tdp_enabled) > - direct = 1; > for (i = 0; i < 4; ++i) { > hpa_t root = vcpu->arch.mmu.pae_root[i]; > > @@ -2089,6 +2089,10 @@ static int mmu_alloc_roots(struct kvm_vcpu *vcpu) > root_gfn = 0; > if (mmu_check_root(vcpu, root_gfn)) > return 1; > + if (tdp_enabled) { > + direct = 1; > + root_gfn = i << 30; > + } > sp = kvm_mmu_get_page(vcpu, root_gfn, i << 30, > PT32_ROOT_LEVEL, direct, > ACC_ALL, NULL); There is no need to allocate 4 different roots for TDP tables if kvm_x86_ops->get_tdp_level() == PT64_ROOT_LEVEL.