Messages in this thread | | | Date | Wed, 15 Jun 2022 12:57:39 +0200 | Subject | Re: [PATCH v11 14/19] KVM: s390: pv: cleanup leftover protected VMs if needed | From | Janosch Frank <> |
| |
On 6/15/22 12:19, Claudio Imbrenda wrote: > On Wed, 15 Jun 2022 11:59:36 +0200 > Janosch Frank <frankja@linux.ibm.com> wrote: > >> On 6/3/22 08:56, Claudio Imbrenda wrote: >>> In upcoming patches it will be possible to start tearing down a >>> protected VM, and finish the teardown concurrently in a different >>> thread. >> >> s/,/ >> s/the/its/ > > will fix > >> >>> >>> Protected VMs that are pending for tear down ("leftover") need to be >>> cleaned properly when the userspace process (e.g. qemu) terminates. >>> >>> This patch makes sure that all "leftover" protected VMs are always >>> properly torn down. >> >> So we're handling the kvm_arch_destroy_vm() case here, right? > > yes > >> Maybe add that in a more prominent way and rework the subject: >> >> KVM: s390: pv: cleanup leftover PV VM shells on VM shutdown > > ok, I'll change the description and rework the subject > >> >>> >>> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> >>> --- >>> arch/s390/include/asm/kvm_host.h | 2 + >>> arch/s390/kvm/kvm-s390.c | 2 + >>> arch/s390/kvm/pv.c | 109 ++++++++++++++++++++++++++++--- >>> 3 files changed, 104 insertions(+), 9 deletions(-) >>> >>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h >>> index 5824efe5fc9d..cca8e05e0a71 100644 >>> --- a/arch/s390/include/asm/kvm_host.h >>> +++ b/arch/s390/include/asm/kvm_host.h >>> @@ -924,6 +924,8 @@ struct kvm_s390_pv { >>> u64 guest_len; >>> unsigned long stor_base; >>> void *stor_var; >>> + void *prepared_for_async_deinit; >>> + struct list_head need_cleanup; >>> struct mmu_notifier mmu_notifier; >>> }; >>> >>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >>> index fe1fa896def7..369de8377116 100644 >>> --- a/arch/s390/kvm/kvm-s390.c >>> +++ b/arch/s390/kvm/kvm-s390.c >>> @@ -2890,6 +2890,8 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) >>> kvm_s390_vsie_init(kvm); >>> if (use_gisa) >>> kvm_s390_gisa_init(kvm); >>> + INIT_LIST_HEAD(&kvm->arch.pv.need_cleanup); >>> + kvm->arch.pv.prepared_for_async_deinit = NULL; >>> KVM_EVENT(3, "vm 0x%pK created by pid %u", kvm, current->pid); >>> >>> return 0; >>> diff --git a/arch/s390/kvm/pv.c b/arch/s390/kvm/pv.c >>> index 6cffea26c47f..8471c17d538c 100644 >>> --- a/arch/s390/kvm/pv.c >>> +++ b/arch/s390/kvm/pv.c >>> @@ -17,6 +17,19 @@ >>> #include <linux/mmu_notifier.h> >>> #include "kvm-s390.h" >>> >>> +/** >>> + * @struct leftover_pv_vm >> >> Any other ideas on naming these VMs? > > not really > >> Also I'd turn that around: pv_vm_leftover > > I mean, it's a leftover protected VM, it felt more natural to name it > that way > >> >>> + * Represents a "leftover" protected VM that is still registered with the >>> + * Ultravisor, but which does not correspond any longer to an active KVM VM. >>> + */ >>> +struct leftover_pv_vm { >>> + struct list_head list; >>> + unsigned long old_gmap_table; >>> + u64 handle; >>> + void *stor_var; >>> + unsigned long stor_base; >>> +}; >>> + >> >> I think we should switch this patch and the next one and add this struct >> to the next patch. The list work below makes more sense once the next >> patch has been read. > > but the next patch will leave leftovers in some circumstances, and > those won't be cleaned up without this patch. > > having this patch first means that when the next patch is applied, the > leftovers are already taken care of
Then I opt for squashing the patch.
Without the next patch prepared_for_async_deinit will always be NULL and this code is completely unneeded, no?
> >>> static void kvm_s390_clear_pv_state(struct kvm *kvm) >>> { >>> kvm->arch.pv.handle = 0; >>> @@ -158,23 +171,88 @@ static int kvm_s390_pv_alloc_vm(struct kvm *kvm) >>> return -ENOMEM; >>> } >>> >> >>> >> >
| |