Re: [PATCH RFC] KVM MMU: fix hashing for TDP and non-paging modes

[Marcelo Tosatti <mtosatti@redhat.com>]

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@google.com>
> ---
>  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.