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;
Date:   Wed, 15 Jun 2022 12:19:16 +0200
From:   Claudio Imbrenda <imbrenda@linux.ibm.com>
To:     Janosch Frank <frankja@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
Subject: Re: [PATCH v11 14/19] KVM: s390: pv: cleanup leftover protected VMs
 if needed
Message-ID: <20220615121916.77b039af@p-imbrenda>
In-Reply-To: <0a13397a-86e0-7c25-0044-7a5733f61730@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>
Organization: IBM
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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

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