Re: A question of TDP unloading.

[Sean Christopherson <seanjc@google.com>]

On Thu, Jul 29, 2021, Yu Zhang wrote:
> On Wed, Jul 28, 2021 at 06:37:38PM +0000, Sean Christopherson wrote:
> > On Wed, Jul 28, 2021, Yu Zhang wrote:
> In the caller, force_tdp_unload was set to false for CR0/CR4/EFER changes. For SMM and
> cpuid updates, it is set to true.
> 
> With this change, I can successfully boot a VM(and of course, number of unloadings is
> greatly reduced). But access test case in kvm-unit-test hangs, after CR4.SMEP is flipped.
> I'm trying to figure out why...

Hrm, I'll look into when I get around to making this into a proper patch.

Note, there's at least once bug, as is_root_usable() will compare the full role
against a root shadow page's modified role.  A common helper to derive the page
role for a direct/TDP page from an existing mmu_role is likely the way to go, as
kvm_tdp_mmu_get_vcpu_root_hpa() would want the same functionality.

> > I'll put this on my todo list, I've been looking for an excuse to update the
> > cr0/cr4/efer flows anyways :-).  If it works, the changes should be relatively
> > minor, if it works...
> > 
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index a8cdfd8d45c4..700664fe163e 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -2077,8 +2077,20 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
> >         role = vcpu->arch.mmu->mmu_role.base;
> >         role.level = level;
> >         role.direct = direct;
> > -       if (role.direct)
> > +       if (role.direct) {
> >                 role.gpte_is_8_bytes = true;
> > +
> > +               /*
> > +                * Guest PTE permissions do not impact SPTE permissions for
> > +                * direct MMUs.  Either there are no guest PTEs (CR0.PG=0) or
> > +                * guest PTE permissions are enforced by the CPU (TDP enabled).
> > +                */
> > +               WARN_ON_ONCE(access != ACC_ALL);
> > +               role.efer_nx = 0;
> > +               role.cr0_wp = 0;
> > +               role.smep_andnot_wp = 0;
> > +               role.smap_andnot_wp = 0;
> > +       }
> 
> How about we do this in kvm_calc_mmu_role_common()? :-)

No, because the role in struct kvm_mmu does need the correct bits, even for TDP,
as the role is used to detect whether or not the context needs to be re-initialized,
e.g. it would get a false negative on a cr0_wp change, not go through
update_permission_bitmask(), and use the wrong page permissions when walking the
guest page tables.