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>
