On Tue, Sep 27, 2022 at 2:19 AM Huang, Kai <kai.huang@xxxxxxxxx> wrote: > > > > > > +bool __ro_after_init tdp_mmu_allowed; > > + > > [...] > > > @@ -5662,6 +5669,9 @@ void kvm_configure_mmu(bool enable_tdp, int tdp_forced_root_level, > > tdp_root_level = tdp_forced_root_level; > > max_tdp_level = tdp_max_root_level; > > > > +#ifdef CONFIG_X86_64 > > + tdp_mmu_enabled = tdp_mmu_allowed && tdp_enabled; > > +#endif > > > > [...] > > > @@ -6661,6 +6671,13 @@ void __init kvm_mmu_x86_module_init(void) > > if (nx_huge_pages == -1) > > __set_nx_huge_pages(get_nx_auto_mode()); > > > > + /* > > + * Snapshot userspace's desire to enable the TDP MMU. Whether or not the > > + * TDP MMU is actually enabled is determined in kvm_configure_mmu() > > + * when the vendor module is loaded. > > + */ > > + tdp_mmu_allowed = tdp_mmu_enabled; > > + > > kvm_mmu_spte_module_init(); > > } > > > > Sorry last time I didn't review deeply, but I am wondering why do we need > 'tdp_mmu_allowed' at all? The purpose of having 'allow_mmio_caching' is because > kvm_mmu_set_mmio_spte_mask() is called twice, and 'enable_mmio_caching' can be > disabled in the first call, so it can be against user's desire in the second > call. However it appears for 'tdp_mmu_enabled' we don't need 'tdp_mmu_allowed', > as kvm_configure_mmu() is only called once by VMX or SVM, if I read correctly. tdp_mmu_allowed is needed because kvm_intel and kvm_amd are separate modules from kvm. So kvm_configure_mmu() can be called multiple times (each time kvm_intel or kvm_amd is loaded). > > So, should we just do below in kvm_configure_mmu()? > > #ifdef CONFIG_X86_64 > if (!tdp_enabled) > tdp_mmu_enabled = false; > #endif > > > -- > Thanks, > -Kai > >
