On Fri, Jul 30, 2021 at 10:42:51AM +0800, Yu Zhang wrote:
> 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().
Oh. We do not need this, setting these flags to 0 shall be fine, because role flags
of the SP are all from kvm_mmu_get_page(). Sorry for the noise...
B.R.
Yu
> >
> > > > 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
>
