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 <digitaleric@xxxxxxxxxx>
> ---
> 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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
