On Mon, 2022-10-03 at 11:58 -0700, Isaku Yamahata wrote: > On Tue, Sep 27, 2022 at 09:10:43PM +0000, > "Huang, Kai" <kai.huang@xxxxxxxxx> wrote: > > > On Tue, 2022-09-27 at 09:14 -0700, David Matlack wrote: > > > 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). > > > > > > > > > > Indeed. :) > > > > Reviewed-by: Kai Huang <kai.huang@xxxxxxxxx> > > kvm_arch_init() which is called early during the module initialization before > kvm_configure_mmu() via kvm_arch_hardware_setup() checks if the vendor module > (kvm_intel or kvm_amd) was already loaded. If yes, it results in -EEXIST. > > So kvm_configure_mmu() won't be called twice. > Hi Isaku, Please consider module reload. -- Thanks, -Kai
