Re: A question of TDP unloading.

[Yu Zhang <yu.c.zhang@linux.intel.com>]

On Thu, Jul 29, 2021 at 09:04:56PM +0000, Sean Christopherson wrote:
> 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.

So, if we know there are some bits meaningless in SP, could we use a 
ignored_mask, each time we try to compare the full role.word? This may
be also needed in kvm_mmu_get_page().

> 
> > > 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.

Oh yes. Regardless of what flags really matter in a SP, all of them are useful for mmu
context. Thanks for correcting me.

B.R.
Yu