Re: [PATCH v11 14/19] KVM: s390: pv: cleanup leftover protected VMs if needed

Janosch Frank
2022-06-15T10:57:52Z

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;
>>> }
>>>
>>
>>>
>>
>