Re: [PATCH 03/22] kvm: mmu: Init / Uninit the TDP MMU

[Ben Gardon]

On Tue, Sep 29, 2020 at 10:35 PM Sean Christopherson
<sean.j.christopherson@xxxxxxxxx> wrote:
>
> Nit on all the shortlogs, can you use "KVM: x86/mmu" instead of "kvm: mmu"?

Will do.

>
> On Fri, Sep 25, 2020 at 02:22:43PM -0700, Ben Gardon wrote:
> > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> > new file mode 100644
> > index 0000000000000..8241e18c111e6
> > --- /dev/null
> > +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> > @@ -0,0 +1,34 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#include "tdp_mmu.h"
> > +
> > +static bool __read_mostly tdp_mmu_enabled = true;
> > +module_param_named(tdp_mmu, tdp_mmu_enabled, bool, 0644);
>
> Do y'all actually toggle tdp_mmu_enabled while VMs are running?  I can see
> having a per-VM capability, or a read-only module param, but a writable
> module param is... interesting.

We don't use this much, but it is useful when running tests to be able
to go back and forth between running with and without the TDP MMU. I
should have added a note that the module parameter is mostly for
development purposes.

>
> > +static bool is_tdp_mmu_enabled(void)
> > +{
> > +     if (!READ_ONCE(tdp_mmu_enabled))
> > +             return false;
> > +
> > +     if (WARN_ONCE(!tdp_enabled,
> > +                   "Creating a VM with TDP MMU enabled requires TDP."))
>
> This should be enforced, i.e. clear tdp_mmu_enabled if !tdp_enabled.  As is,
> it's a user triggerable WARN, which is not good, e.g. with PANIC_ON_WARN.

That's a good point.

>
> > +             return false;
> > +
> > +     return true;
> > +}