Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <44b2b227-9757-b7a2-41a0-cbea0e2bbbdc@linux.ibm.com>
Date:   Wed, 15 Jun 2022 12:57:39 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.0
Subject: Re: [PATCH v11 14/19] KVM: s390: pv: cleanup leftover protected VMs
 if needed
Content-Language: en-US
To:     Claudio Imbrenda <imbrenda@linux.ibm.com>
Cc:     kvm@vger.kernel.org, borntraeger@de.ibm.com, thuth@redhat.com,
        pasic@linux.ibm.com, david@redhat.com, linux-s390@vger.kernel.org,
        linux-kernel@vger.kernel.org, scgl@linux.ibm.com,
        mimu@linux.ibm.com, nrb@linux.ibm.com
References: <20220603065645.10019-1-imbrenda@linux.ibm.com>
 <20220603065645.10019-15-imbrenda@linux.ibm.com>
 <0a13397a-86e0-7c25-0044-7a5733f61730@linux.ibm.com>
 <20220615121916.77b039af@p-imbrenda>
From:   Janosch Frank <frankja@linux.ibm.com>
In-Reply-To: <20220615121916.77b039af@p-imbrenda>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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