Message ID | 20220330165510.213111-1-pbonzini@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | KVM: MMU: propagate alloc_workqueue failure | expand |
On Wed, Mar 30, 2022 at 12:55:10PM -0400, Paolo Bonzini wrote: > If kvm->arch.tdp_mmu_zap_wq cannot be created, the failure has > to be propagated up to kvm_mmu_init_vm and kvm_arch_init_vm. > kvm_arch_init_vm also has to undo all the initialization, so > group all the MMU initialization code at the beginning and > handle cleaning up of kvm_page_track_init. > Fixes: 22b94c4b63eb ("KVM: x86/mmu: Zap invalidated roots via asynchronous worker") > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > arch/x86/include/asm/kvm_host.h | 2 +- > arch/x86/kvm/mmu/mmu.c | 11 +++++++++-- > arch/x86/kvm/mmu/tdp_mmu.c | 17 ++++++++++------- > arch/x86/kvm/mmu/tdp_mmu.h | 4 ++-- > arch/x86/kvm/x86.c | 15 ++++++++++----- > 5 files changed, 32 insertions(+), 17 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 0ddc2e67a731..469c7702fad9 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -1584,7 +1584,7 @@ void kvm_mmu_module_exit(void); > > void kvm_mmu_destroy(struct kvm_vcpu *vcpu); > int kvm_mmu_create(struct kvm_vcpu *vcpu); > -void kvm_mmu_init_vm(struct kvm *kvm); > +int kvm_mmu_init_vm(struct kvm *kvm); > void kvm_mmu_uninit_vm(struct kvm *kvm); > > void kvm_mmu_after_set_cpuid(struct kvm_vcpu *vcpu); > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > index 51671cb34fb6..857ba93b5c92 100644 > --- a/arch/x86/kvm/mmu/mmu.c > +++ b/arch/x86/kvm/mmu/mmu.c > @@ -5768,17 +5768,24 @@ static void kvm_mmu_invalidate_zap_pages_in_memslot(struct kvm *kvm, > kvm_mmu_zap_all_fast(kvm); > } > > -void kvm_mmu_init_vm(struct kvm *kvm) > +int kvm_mmu_init_vm(struct kvm *kvm) > { > struct kvm_page_track_notifier_node *node = &kvm->arch.mmu_sp_tracker; > + int r; > > + INIT_LIST_HEAD(&kvm->arch.active_mmu_pages); > + INIT_LIST_HEAD(&kvm->arch.zapped_obsolete_pages); > + INIT_LIST_HEAD(&kvm->arch.lpage_disallowed_mmu_pages); I agree with moving these but that should probably be done in a separate commit. > spin_lock_init(&kvm->arch.mmu_unsync_pages_lock); > > - kvm_mmu_init_tdp_mmu(kvm); > + r = kvm_mmu_init_tdp_mmu(kvm); > + if (r < 0) > + return r; > > node->track_write = kvm_mmu_pte_write; > node->track_flush_slot = kvm_mmu_invalidate_zap_pages_in_memslot; > kvm_page_track_register_notifier(kvm, node); > + return 0; > } > > void kvm_mmu_uninit_vm(struct kvm *kvm) > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c > index e7e7876251b3..d7c112a29fe9 100644 > --- a/arch/x86/kvm/mmu/tdp_mmu.c > +++ b/arch/x86/kvm/mmu/tdp_mmu.c > @@ -14,21 +14,24 @@ static bool __read_mostly tdp_mmu_enabled = true; > module_param_named(tdp_mmu, tdp_mmu_enabled, bool, 0644); > > /* Initializes the TDP MMU for the VM, if enabled. */ > -bool kvm_mmu_init_tdp_mmu(struct kvm *kvm) > +int kvm_mmu_init_tdp_mmu(struct kvm *kvm) > { > + struct workqueue_struct *wq; > + > if (!tdp_enabled || !READ_ONCE(tdp_mmu_enabled)) > - return false; > + return 0; > + > + wq = alloc_workqueue("kvm", WQ_UNBOUND|WQ_MEM_RECLAIM|WQ_CPU_INTENSIVE, 0); > + if (IS_ERR(wq)) > + return PTR_ERR(wq); > > /* This should not be changed for the lifetime of the VM. */ > kvm->arch.tdp_mmu_enabled = true; > - > INIT_LIST_HEAD(&kvm->arch.tdp_mmu_roots); > spin_lock_init(&kvm->arch.tdp_mmu_pages_lock); > INIT_LIST_HEAD(&kvm->arch.tdp_mmu_pages); > - kvm->arch.tdp_mmu_zap_wq = > - alloc_workqueue("kvm", WQ_UNBOUND|WQ_MEM_RECLAIM|WQ_CPU_INTENSIVE, 0); > - > - return true; > + kvm->arch.tdp_mmu_zap_wq = wq; Suggest moving this to just after checking the return value of alloc_workqueue(). > + return 1; Perhaps return 0 until we have a reason to differentiate the 2 cases. > } > > /* Arbitrarily returns true so that this may be used in if statements. */ > diff --git a/arch/x86/kvm/mmu/tdp_mmu.h b/arch/x86/kvm/mmu/tdp_mmu.h > index 5e5ef2576c81..647926541e38 100644 > --- a/arch/x86/kvm/mmu/tdp_mmu.h > +++ b/arch/x86/kvm/mmu/tdp_mmu.h > @@ -72,7 +72,7 @@ u64 *kvm_tdp_mmu_fast_pf_get_last_sptep(struct kvm_vcpu *vcpu, u64 addr, > u64 *spte); > > #ifdef CONFIG_X86_64 > -bool kvm_mmu_init_tdp_mmu(struct kvm *kvm); > +int kvm_mmu_init_tdp_mmu(struct kvm *kvm); > void kvm_mmu_uninit_tdp_mmu(struct kvm *kvm); > static inline bool is_tdp_mmu_page(struct kvm_mmu_page *sp) { return sp->tdp_mmu_page; } > > @@ -93,7 +93,7 @@ static inline bool is_tdp_mmu(struct kvm_mmu *mmu) > return sp && is_tdp_mmu_page(sp) && sp->root_count; > } > #else > -static inline bool kvm_mmu_init_tdp_mmu(struct kvm *kvm) { return false; } > +static inline int kvm_mmu_init_tdp_mmu(struct kvm *kvm) { return 0; } > static inline void kvm_mmu_uninit_tdp_mmu(struct kvm *kvm) {} > static inline bool is_tdp_mmu_page(struct kvm_mmu_page *sp) { return false; } > static inline bool is_tdp_mmu(struct kvm_mmu *mmu) { return false; } > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index fe2171b11441..89b6efb7f504 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -11629,12 +11629,13 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) > > ret = kvm_page_track_init(kvm); > if (ret) > - return ret; > + goto out; nit: This goto is unnecessary. > + > + ret = kvm_mmu_init_vm(kvm); > + if (ret) > + goto out_page_track; > > INIT_HLIST_HEAD(&kvm->arch.mask_notifier_list); > - INIT_LIST_HEAD(&kvm->arch.active_mmu_pages); > - INIT_LIST_HEAD(&kvm->arch.zapped_obsolete_pages); > - INIT_LIST_HEAD(&kvm->arch.lpage_disallowed_mmu_pages); > INIT_LIST_HEAD(&kvm->arch.assigned_dev_head); > atomic_set(&kvm->arch.noncoherent_dma_count, 0); > > @@ -11666,10 +11667,14 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) > > kvm_apicv_init(kvm); > kvm_hv_init_vm(kvm); > - kvm_mmu_init_vm(kvm); > kvm_xen_init_vm(kvm); > > return static_call(kvm_x86_vm_init)(kvm); > + > +out_page_track: > + kvm_page_track_cleanup(kvm); > +out: > + return ret; > } > > int kvm_arch_post_init_vm(struct kvm *kvm) > -- > 2.31.1 >
On 3/31/22 01:51, David Matlack wrote: >> -void kvm_mmu_init_vm(struct kvm *kvm) >> +int kvm_mmu_init_vm(struct kvm *kvm) >> { >> struct kvm_page_track_notifier_node *node = &kvm->arch.mmu_sp_tracker; >> + int r; >> >> + INIT_LIST_HEAD(&kvm->arch.active_mmu_pages); >> + INIT_LIST_HEAD(&kvm->arch.zapped_obsolete_pages); >> + INIT_LIST_HEAD(&kvm->arch.lpage_disallowed_mmu_pages); > > I agree with moving these but that should probably be done in a separate > commit. Ok. >> - kvm->arch.tdp_mmu_zap_wq = >> - alloc_workqueue("kvm", WQ_UNBOUND|WQ_MEM_RECLAIM|WQ_CPU_INTENSIVE, 0); >> - >> - return true; >> + kvm->arch.tdp_mmu_zap_wq = wq; > > Suggest moving this to just after checking the return value of > alloc_workqueue(). This is intentional, in case we have other future allocations, to avoid having to NULL out the field in the unwind path. It's a matter of taste I guess. >> + return 1; > > Perhaps return 0 until we have a reason to differentiate the 2 cases. Yeah, though I wanted to preserve the previous behavior. >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index fe2171b11441..89b6efb7f504 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -11629,12 +11629,13 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) >> >> ret = kvm_page_track_init(kvm); >> if (ret) >> - return ret; >> + goto out; > > nit: This goto is unnecessary. True, but I prefer to be consistent in using "goto" so that any future additions are careful about preserving the chain. Paolo
On Thu, Mar 31, 2022 at 2:34 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 3/31/22 01:51, David Matlack wrote: > >> -void kvm_mmu_init_vm(struct kvm *kvm) > >> +int kvm_mmu_init_vm(struct kvm *kvm) > >> { > >> struct kvm_page_track_notifier_node *node = &kvm->arch.mmu_sp_tracker; > >> + int r; > >> > >> + INIT_LIST_HEAD(&kvm->arch.active_mmu_pages); > >> + INIT_LIST_HEAD(&kvm->arch.zapped_obsolete_pages); > >> + INIT_LIST_HEAD(&kvm->arch.lpage_disallowed_mmu_pages); > > > > I agree with moving these but that should probably be done in a separate > > commit. > > Ok. > > >> - kvm->arch.tdp_mmu_zap_wq = > >> - alloc_workqueue("kvm", WQ_UNBOUND|WQ_MEM_RECLAIM|WQ_CPU_INTENSIVE, 0); > >> - > >> - return true; > >> + kvm->arch.tdp_mmu_zap_wq = wq; > > > > Suggest moving this to just after checking the return value of > > alloc_workqueue(). > > This is intentional, in case we have other future allocations, to avoid > having to NULL out the field in the unwind path. It's a matter of taste > I guess. Oh ok, that makes sense. I agree it's a matter of taste. > > >> + return 1; > > > > Perhaps return 0 until we have a reason to differentiate the 2 cases. > > Yeah, though I wanted to preserve the previous behavior. > > >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > >> index fe2171b11441..89b6efb7f504 100644 > >> --- a/arch/x86/kvm/x86.c > >> +++ b/arch/x86/kvm/x86.c > >> @@ -11629,12 +11629,13 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) > >> > >> ret = kvm_page_track_init(kvm); > >> if (ret) > >> - return ret; > >> + goto out; > > > > nit: This goto is unnecessary. > > True, but I prefer to be consistent in using "goto" so that any future > additions are careful about preserving the chain. Sounds good. > > Paolo >
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 0ddc2e67a731..469c7702fad9 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -1584,7 +1584,7 @@ void kvm_mmu_module_exit(void); void kvm_mmu_destroy(struct kvm_vcpu *vcpu); int kvm_mmu_create(struct kvm_vcpu *vcpu); -void kvm_mmu_init_vm(struct kvm *kvm); +int kvm_mmu_init_vm(struct kvm *kvm); void kvm_mmu_uninit_vm(struct kvm *kvm); void kvm_mmu_after_set_cpuid(struct kvm_vcpu *vcpu); diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index 51671cb34fb6..857ba93b5c92 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -5768,17 +5768,24 @@ static void kvm_mmu_invalidate_zap_pages_in_memslot(struct kvm *kvm, kvm_mmu_zap_all_fast(kvm); } -void kvm_mmu_init_vm(struct kvm *kvm) +int kvm_mmu_init_vm(struct kvm *kvm) { struct kvm_page_track_notifier_node *node = &kvm->arch.mmu_sp_tracker; + int r; + INIT_LIST_HEAD(&kvm->arch.active_mmu_pages); + INIT_LIST_HEAD(&kvm->arch.zapped_obsolete_pages); + INIT_LIST_HEAD(&kvm->arch.lpage_disallowed_mmu_pages); spin_lock_init(&kvm->arch.mmu_unsync_pages_lock); - kvm_mmu_init_tdp_mmu(kvm); + r = kvm_mmu_init_tdp_mmu(kvm); + if (r < 0) + return r; node->track_write = kvm_mmu_pte_write; node->track_flush_slot = kvm_mmu_invalidate_zap_pages_in_memslot; kvm_page_track_register_notifier(kvm, node); + return 0; } void kvm_mmu_uninit_vm(struct kvm *kvm) diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c index e7e7876251b3..d7c112a29fe9 100644 --- a/arch/x86/kvm/mmu/tdp_mmu.c +++ b/arch/x86/kvm/mmu/tdp_mmu.c @@ -14,21 +14,24 @@ static bool __read_mostly tdp_mmu_enabled = true; module_param_named(tdp_mmu, tdp_mmu_enabled, bool, 0644); /* Initializes the TDP MMU for the VM, if enabled. */ -bool kvm_mmu_init_tdp_mmu(struct kvm *kvm) +int kvm_mmu_init_tdp_mmu(struct kvm *kvm) { + struct workqueue_struct *wq; + if (!tdp_enabled || !READ_ONCE(tdp_mmu_enabled)) - return false; + return 0; + + wq = alloc_workqueue("kvm", WQ_UNBOUND|WQ_MEM_RECLAIM|WQ_CPU_INTENSIVE, 0); + if (IS_ERR(wq)) + return PTR_ERR(wq); /* This should not be changed for the lifetime of the VM. */ kvm->arch.tdp_mmu_enabled = true; - INIT_LIST_HEAD(&kvm->arch.tdp_mmu_roots); spin_lock_init(&kvm->arch.tdp_mmu_pages_lock); INIT_LIST_HEAD(&kvm->arch.tdp_mmu_pages); - kvm->arch.tdp_mmu_zap_wq = - alloc_workqueue("kvm", WQ_UNBOUND|WQ_MEM_RECLAIM|WQ_CPU_INTENSIVE, 0); - - return true; + kvm->arch.tdp_mmu_zap_wq = wq; + return 1; } /* Arbitrarily returns true so that this may be used in if statements. */ diff --git a/arch/x86/kvm/mmu/tdp_mmu.h b/arch/x86/kvm/mmu/tdp_mmu.h index 5e5ef2576c81..647926541e38 100644 --- a/arch/x86/kvm/mmu/tdp_mmu.h +++ b/arch/x86/kvm/mmu/tdp_mmu.h @@ -72,7 +72,7 @@ u64 *kvm_tdp_mmu_fast_pf_get_last_sptep(struct kvm_vcpu *vcpu, u64 addr, u64 *spte); #ifdef CONFIG_X86_64 -bool kvm_mmu_init_tdp_mmu(struct kvm *kvm); +int kvm_mmu_init_tdp_mmu(struct kvm *kvm); void kvm_mmu_uninit_tdp_mmu(struct kvm *kvm); static inline bool is_tdp_mmu_page(struct kvm_mmu_page *sp) { return sp->tdp_mmu_page; } @@ -93,7 +93,7 @@ static inline bool is_tdp_mmu(struct kvm_mmu *mmu) return sp && is_tdp_mmu_page(sp) && sp->root_count; } #else -static inline bool kvm_mmu_init_tdp_mmu(struct kvm *kvm) { return false; } +static inline int kvm_mmu_init_tdp_mmu(struct kvm *kvm) { return 0; } static inline void kvm_mmu_uninit_tdp_mmu(struct kvm *kvm) {} static inline bool is_tdp_mmu_page(struct kvm_mmu_page *sp) { return false; } static inline bool is_tdp_mmu(struct kvm_mmu *mmu) { return false; } diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index fe2171b11441..89b6efb7f504 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -11629,12 +11629,13 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) ret = kvm_page_track_init(kvm); if (ret) - return ret; + goto out; + + ret = kvm_mmu_init_vm(kvm); + if (ret) + goto out_page_track; INIT_HLIST_HEAD(&kvm->arch.mask_notifier_list); - INIT_LIST_HEAD(&kvm->arch.active_mmu_pages); - INIT_LIST_HEAD(&kvm->arch.zapped_obsolete_pages); - INIT_LIST_HEAD(&kvm->arch.lpage_disallowed_mmu_pages); INIT_LIST_HEAD(&kvm->arch.assigned_dev_head); atomic_set(&kvm->arch.noncoherent_dma_count, 0); @@ -11666,10 +11667,14 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) kvm_apicv_init(kvm); kvm_hv_init_vm(kvm); - kvm_mmu_init_vm(kvm); kvm_xen_init_vm(kvm); return static_call(kvm_x86_vm_init)(kvm); + +out_page_track: + kvm_page_track_cleanup(kvm); +out: + return ret; } int kvm_arch_post_init_vm(struct kvm *kvm)
If kvm->arch.tdp_mmu_zap_wq cannot be created, the failure has to be propagated up to kvm_mmu_init_vm and kvm_arch_init_vm. kvm_arch_init_vm also has to undo all the initialization, so group all the MMU initialization code at the beginning and handle cleaning up of kvm_page_track_init. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- arch/x86/include/asm/kvm_host.h | 2 +- arch/x86/kvm/mmu/mmu.c | 11 +++++++++-- arch/x86/kvm/mmu/tdp_mmu.c | 17 ++++++++++------- arch/x86/kvm/mmu/tdp_mmu.h | 4 ++-- arch/x86/kvm/x86.c | 15 ++++++++++----- 5 files changed, 32 insertions(+), 17 deletions(-)