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: <0a13397a-86e0-7c25-0044-7a5733f61730@linux.ibm.com>
Date:   Wed, 15 Jun 2022 11:59:36 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.0
Content-Language: en-US
To:     Claudio Imbrenda <imbrenda@linux.ibm.com>, kvm@vger.kernel.org
Cc:     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>
From:   Janosch Frank <frankja@linux.ibm.com>
Subject: Re: [PATCH v11 14/19] KVM: s390: pv: cleanup leftover protected VMs
 if needed
In-Reply-To: <20220603065645.10019-15-imbrenda@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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/

> 
> 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?
Maybe add that in a more prominent way and rework the subject:

KVM: s390: pv: cleanup leftover PV VM shells on VM shutdown

> 
> 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?
Also I'd turn that around: pv_vm_leftover

> + * 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.

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

>