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:   Fri, 1 Apr 2022 17:13:02 +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 v9 13/18] KVM: s390: pv: cleanup leftover protected VMs
 if needed
Message-ID: <20220401171302.60415a07@p-imbrenda>
In-Reply-To: <827cfa86-bad4-8c31-8038-8db9a011fee9@linux.ibm.com>
References: <20220330122605.247613-1-imbrenda@linux.ibm.com>
        <20220330122605.247613-14-imbrenda@linux.ibm.com>
        <827cfa86-bad4-8c31-8038-8db9a011fee9@linux.ibm.com>
Organization: IBM
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 31 Mar 2022 16:02:55 +0200
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 3/30/22 14:26, 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.
> > 
> > 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.
> > 
> > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> > ---
> >   arch/s390/include/asm/kvm_host.h |  2 +
> >   arch/s390/kvm/kvm-s390.c         |  1 +
> >   arch/s390/kvm/pv.c               | 69 ++++++++++++++++++++++++++++++++
> >   3 files changed, 72 insertions(+)
> > 
> > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> > index 1bccb8561ba9..50e3516cbc03 100644
> > --- a/arch/s390/include/asm/kvm_host.h
> > +++ b/arch/s390/include/asm/kvm_host.h
> > @@ -922,6 +922,8 @@ struct kvm_s390_pv {
> >   	u64 guest_len;
> >   	unsigned long stor_base;
> >   	void *stor_var;
> > +	void *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 446f89db93a1..3637f556ff33 100644
> > --- a/arch/s390/kvm/kvm-s390.c
> > +++ b/arch/s390/kvm/kvm-s390.c
> > @@ -2788,6 +2788,7 @@ 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.sync_deinit = NULL;

isn't the struct allocated with __GFP_ZERO ?

> 
> >   	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 be3b467f8feb..56412617dd01 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 deferred_priv
> > + * Represents a "leftover" protected VM that does not correspond to any
> > + * active KVM VM.  
> 
> Maybe something like:
> ...that is still registered with the Ultravisor but isn't registered 
> with KVM anymore.

will fix

> 
> > + */
> > +struct deferred_priv {
> > +	struct list_head list;
> > +	unsigned long old_table;
> > +	u64 handle;
> > +	void *stor_var;
> > +	unsigned long stor_base;
> > +};
> > +
> >   static void kvm_s390_clear_pv_state(struct kvm *kvm)
> >   {
> >   	kvm->arch.pv.handle = 0;
> > @@ -163,6 +176,60 @@ static int kvm_s390_pv_alloc_vm(struct kvm *kvm)
> >   	return -ENOMEM;
> >   }
> >   
> > +/**
> > + * kvm_s390_pv_cleanup_deferred - Clean up one leftover protected VM.
> > + * @kvm the KVM that was associated with this leftover protected VM
> > + * @deferred details about the leftover protected VM that needs a clean up
> > + * Return: 0 in case of success, otherwise 1
> > + */
> > +static int kvm_s390_pv_cleanup_deferred(struct kvm *kvm, struct deferred_priv *deferred)
> > +{
> > +	u16 rc, rrc;
> > +	int cc;
> > +
> > +	cc = uv_cmd_nodata(deferred->handle, UVC_CMD_DESTROY_SEC_CONF, &rc, &rrc);
> > +	KVM_UV_EVENT(kvm, 3, "PROTVIRT DESTROY VM: rc %x rrc %x", rc, rrc);
> > +	WARN_ONCE(cc, "protvirt destroy vm failed rc %x rrc %x", rc, rrc);
> > +	if (cc)
> > +		return cc;
> > +	/*
> > +	 * Intentionally leak unusable memory. If the UVC fails, the memory
> > +	 * used for the VM and its metadata is permanently unusable.
> > +	 * This can only happen in case of a serious KVM or hardware bug; it
> > +	 * is not expected to happen in normal operation.
> > +	 */
> > +	free_pages(deferred->stor_base, get_order(uv_info.guest_base_stor_len));
> > +	free_pages(deferred->old_table, CRST_ALLOC_ORDER);
> > +	vfree(deferred->stor_var);
> > +	return 0;
> > +}
> > +
> > +/**
> > + * kvm_s390_pv_cleanup_leftovers - Clean up all leftover protected VMs.
> > + * @kvm the KVM whose leftover protected VMs are to be cleaned up
> > + * Return: 0 in case of success, otherwise 1
> > + */
> > +static int kvm_s390_pv_cleanup_leftovers(struct kvm *kvm)
> > +{
> > +	struct deferred_priv *deferred;
> > +	int cc = 0;
> > +
> > +	if (kvm->arch.pv.async_deinit)
> > +		list_add(kvm->arch.pv.async_deinit, &kvm->arch.pv.need_cleanup);
> > +
> > +	while (!list_empty(&kvm->arch.pv.need_cleanup)) {
> > +		deferred = list_first_entry(&kvm->arch.pv.need_cleanup, typeof(*deferred), list);
> > +		if (kvm_s390_pv_cleanup_deferred(kvm, deferred))
> > +			cc = 1;
> > +		else
> > +			atomic_dec(&kvm->mm->context.protected_count);
> > +		list_del(&deferred->list);
> > +		kfree(deferred);
> > +	}
> > +	kvm->arch.pv.async_deinit = NULL;
> > +	return cc;
> > +}
> > +
> >   /* this should not fail, but if it does, we must not free the donated memory */
> >   int kvm_s390_pv_deinit_vm(struct kvm *kvm, u16 *rc, u16 *rrc)
> >   {
> > @@ -190,6 +257,8 @@ int kvm_s390_pv_deinit_vm(struct kvm *kvm, u16 *rc, u16 *rrc)
> >   	KVM_UV_EVENT(kvm, 3, "PROTVIRT DESTROY VM: rc %x rrc %x", *rc, *rrc);
> >   	WARN_ONCE(cc, "protvirt destroy vm failed rc %x rrc %x", *rc, *rrc);
> >   
> > +	cc |= kvm_s390_pv_cleanup_leftovers(kvm);
> > +
> >   	return cc ? -EIO : 0;
> >   }
> >     
>