Re: [PATCH 04/22] kvm: mmu: Allocate and free TDP MMU roots

[Sean Christopherson]

Heads up, you may get this multiple times, our mail servers got "upgraded"
recently and are giving me troubles...

On Mon, Oct 12, 2020 at 03:59:35PM -0700, Ben Gardon wrote:
> On Tue, Sep 29, 2020 at 11:06 PM Sean Christopherson
> <sean.j.christopherson@xxxxxxxxx> wrote:
> > > @@ -3691,7 +3690,13 @@ static int mmu_alloc_direct_roots(struct kvm_vcpu *vcpu)
> > >       unsigned i;
> > >
> > >       if (shadow_root_level >= PT64_ROOT_4LEVEL) {
> > > -             root = mmu_alloc_root(vcpu, 0, 0, shadow_root_level, true);
> > > +             if (vcpu->kvm->arch.tdp_mmu_enabled) {
> >
> > I believe this will break 32-bit NPT.  Or at a minimum, look weird.  It'd
> > be better to explicitly disable the TDP MMU on 32-bit KVM, then this becomes
> >
> >         if (vcpu->kvm->arch.tdp_mmu_enabled) {
> >
> >         } else if (shadow_root_level >= PT64_ROOT_4LEVEL) {
> >
> >         } else {
> >
> >         }
> >
>
> How does this break 32-bit NPT? I'm not sure I understand how we would
> get into a bad state here because I'm not familiar with the specifics
> of 32 bit NPT.

32-bit NPT will have a max TDP level of PT32E_ROOT_LEVEL (3), i.e. will
fail the "shadow_root_level >= PT64_ROOT_4LEVEL" check, and thus won't get
to the tdp_mmu_enabled check.  That would likely break as some parts of KVM
would see tdp_mmu_enabled, but this root allocation would continue using
the legacy MMU.

It's somewhat of a moot point, because IIRC there are other things that will
break with 32-bit KVM, i.e. TDP MMU will be 64-bit only.  But burying that
assumption/dependency in these flows is weird.

> > > +                     root = kvm_tdp_mmu_get_vcpu_root_hpa(vcpu);
> > > +             } else {
> > > +                     root = mmu_alloc_root(vcpu, 0, 0, shadow_root_level,
> > > +                                           true);
> > > +             }