Re: [RFC PATCH 06/15] KVM: x86/mmu: Derive page role from parent

From: David Matlack <dmatlack@google.com>
To: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	kvm@vger.kernel.org, Ben Gardon <bgardon@google.com>,
	Joerg Roedel <joro@8bytes.org>, Jim Mattson <jmattson@google.com>,
	Wanpeng Li <wanpengli@tencent.com>,
	Vitaly Kuznetsov <vkuznets@redhat.com>,
	Janis Schoetterl-Glausch <scgl@linux.vnet.ibm.com>,
	Junaid Shahid <junaids@google.com>,
	Oliver Upton <oupton@google.com>,
	Harish Barathvajasankar <hbarath@google.com>,
	Peter Xu <peterx@redhat.com>, Peter Shier <pshier@google.com>
Subject: Re: [RFC PATCH 06/15] KVM: x86/mmu: Derive page role from parent
Date: Wed, 1 Dec 2021 13:56:46 -0800	[thread overview]
Message-ID: <CALzav=cRAENeD+nLZ0H=1gKPX1Fim-Y542wMTkiVR9GsPF6xWQ@mail.gmail.com> (raw)
In-Reply-To: <YabFqf0fZqe9RZii@google.com>

On Tue, Nov 30, 2021 at 4:45 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Tue, Nov 30, 2021, David Matlack wrote:
> > > I have a similar patch for the old MMU, but it was also replacing
> > > shadow_root_level with shadow_root_role.  I'll see if I can adapt it to
> > > the TDP MMU, since the shadow_root_role is obviously the same for both.
> >
> > While I was writing this patch it got me wondering if we can do an
> > even more general refactor and replace root_hpa and shadow_root_level
> > with a pointer to the root kvm_mmu_page struct. But I didn't get a
> > chance to look into it further.
>
> For TDP MUU, yes, as root_hpa == __pa(sp->spt) in all cases.  For the legacy/full
> MMU, not without additional refactoring since root_hpa doesn't point at a kvm_mmu_page
> when KVM shadows a non-paging guest with PAE paging (uses pae_root), or when KVM
> shadows nested NPT and the guest is using fewer paging levels that the host (uses
> pml5_root or pml4_root).
>
>         if (mmu->shadow_root_level == PT64_ROOT_5LEVEL)
>                 mmu->root_hpa = __pa(mmu->pml5_root);
>         else if (mmu->shadow_root_level == PT64_ROOT_4LEVEL)
>                 mmu->root_hpa = __pa(mmu->pml4_root);
>         else
>                 mmu->root_hpa = __pa(mmu->pae_root);
>
> That's definitely a solvable problem, e.g. it wouldn't be a problem to burn a few
> kvm_mmu_page for the special root.  The biggest issue is probably the sheer amount
> of code that would need to be updated.  I do think it would be a good change, but
> I think we'd want to do it in a release that isn't expected to have many other MMU
> changes.

Thanks for the explanation! I had a feeling this refactor would start
getting hairy when I ventured outside of the TDP MMU.

>
> shadow_root_level can also be replaced by mmu_role.base.level.  I've never bothered
> to do the replacement because there's zero memory savings and it would undoubtedly
> take me some time to retrain my brain :-)