On Tue, Aug 30, 2022 at 3:12 AM Huang, Kai <kai.huang@xxxxxxxxx> wrote:
>
> On Fri, 2022-08-26 at 16:12 -0700, David Matlack wrote:
> > Change tdp_mmu to a read-only parameter and drop the per-vm
> > tdp_mmu_enabled. For 32-bit KVM, make tdp_mmu_enabled a const bool so
> > that the compiler can continue omitting calls to the TDP MMU.
> >
> > The TDP MMU was introduced in 5.10 and has been enabled by default since
> > 5.15. At this point there are no known functionality gaps between the
> > TDP MMU and the shadow MMU, and the TDP MMU uses less memory and scales
> > better with the number of vCPUs. In other words, there is no good reason
> > to disable the TDP MMU on a live system.
> >
> > Do not drop tdp_mmu=N support (i.e. do not force 64-bit KVM to always
> > use the TDP MMU) since tdp_mmu=N is still used to get test coverage of
> > KVM's shadow MMU TDP support, which is used in 32-bit KVM.
> >
> > Signed-off-by: David Matlack <dmatlack@xxxxxxxxxx>
> > ---
> > arch/x86/include/asm/kvm_host.h | 9 ------
> > arch/x86/kvm/mmu.h | 11 +++----
> > arch/x86/kvm/mmu/mmu.c | 54 ++++++++++++++++++++++-----------
> > arch/x86/kvm/mmu/tdp_mmu.c | 9 ++----
> > 4 files changed, 44 insertions(+), 39 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index 2c96c43c313a..d76059270a43 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -1262,15 +1262,6 @@ struct kvm_arch {
> > struct task_struct *nx_lpage_recovery_thread;
> >
> > #ifdef CONFIG_X86_64
> > - /*
> > - * Whether the TDP MMU is enabled for this VM. This contains a
> > - * snapshot of the TDP MMU module parameter from when the VM was
> > - * created and remains unchanged for the life of the VM. If this is
> > - * true, TDP MMU handler functions will run for various MMU
> > - * operations.
> > - */
> > - bool tdp_mmu_enabled;
> > -
> > /*
> > * List of struct kvm_mmu_pages being used as roots.
> > * All struct kvm_mmu_pages in the list should have
> > diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
> > index 6bdaacb6faa0..dd014bece7f0 100644
> > --- a/arch/x86/kvm/mmu.h
> > +++ b/arch/x86/kvm/mmu.h
> > @@ -230,15 +230,14 @@ static inline bool kvm_shadow_root_allocated(struct kvm *kvm)
> > }
> >
> > #ifdef CONFIG_X86_64
> > -static inline bool is_tdp_mmu_enabled(struct kvm *kvm) { return kvm->arch.tdp_mmu_enabled; }
> > -#else
> > -static inline bool is_tdp_mmu_enabled(struct kvm *kvm) { return false; }
> > -#endif
> > -
> > +extern bool tdp_mmu_enabled;
> > static inline bool kvm_memslots_have_rmaps(struct kvm *kvm)
> > {
> > - return !is_tdp_mmu_enabled(kvm) || kvm_shadow_root_allocated(kvm);
> > + return !tdp_mmu_enabled || kvm_shadow_root_allocated(kvm);
> > }
> > +#else
> > +static inline bool kvm_memslots_have_rmaps(struct kvm *kvm) { return true; }
> > +#endif
> >
> > static inline gfn_t gfn_to_index(gfn_t gfn, gfn_t base_gfn, int level)
> > {
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index e418ef3ecfcb..7caf51023d47 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -98,6 +98,16 @@ module_param_named(flush_on_reuse, force_flush_and_sync_on_reuse, bool, 0644);
> > */
> > bool tdp_enabled = false;
> >
> > +bool __read_mostly tdp_mmu_allowed;
>
> This can be __ro_after_init since it is only set in kvm_mmu_x86_module_init()
> which is tagged with __init.
Indeed, thanks.
>
> > +
> > +#ifdef CONFIG_X86_64
> > +bool __read_mostly tdp_mmu_enabled = true;
> > +module_param_named(tdp_mmu, tdp_mmu_enabled, bool, 0444);
> > +#else
> > +/* TDP MMU is not supported on 32-bit KVM. */
> > +const bool tdp_mmu_enabled;
> > +#endif
> > +
>
> I am not sure by using 'const bool' the compile will always omit the function
> call? I did some experiment on my 64-bit system and it seems if we don't use
> any -O option then the generated code still does function call.
>
> How about just (if it works):
>
> #define tdp_mmu_enabled false
I can give it a try. By the way, I wonder if the existing code
compiles without -O. The existing code relies on a static inline
function returning false on 32-bit KVM, which doesn't seem like it
would be any easier for the compiler to optimize out than a const
bool. But who knows.
I considered biting the bullet and using conditional compilation
instead of relying on the compiler to optimize calls out, but I didn't
want to blow up the series.
>
> ?
>
> --
> Thanks,
> -Kai
>
>
