Re: [PATCH v13 1/6] KVM: s390: pv: asynchronous destroy for reboot

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, seiden@linux.ibm.com, nrb@linux.ibm.com
Subject: Re: [PATCH v13 1/6] KVM: s390: pv: asynchronous destroy for reboot
Date: Fri, 12 Aug 2022 16:02:47 +0200	[thread overview]
Message-ID: <20220812160247.57527886@p-imbrenda> (raw)
In-Reply-To: <b726199f-6c07-fd9a-fd1e-016e6d98971e@linux.ibm.com>

On Thu, 11 Aug 2022 18:26:13 +0200
Janosch Frank <frankja@linux.ibm.com> wrote:

[...]

> > +	case KVM_PV_ASYNC_CLEANUP_PREPARE:
> > +		r = -EINVAL;
> > +		if (!kvm_s390_pv_is_protected(kvm) || !async_destroy)
> > +			break;
> > +
> > +		r = kvm_s390_cpus_from_pv(kvm, &cmd->rc, &cmd->rrc);
> > +		/*
> > +		 * If a CPU could not be destroyed, destroy VM will also fail.
> > +		 * There is no point in trying to destroy it. Instead return
> > +		 * the rc and rrc from the first CPU that failed destroying.
> > +		 */
> > +		if (r)
> > +			break;
> > +		r = kvm_s390_pv_set_aside(kvm, &cmd->rc, &cmd->rrc);
> > +
> > +		/* no need to block service interrupts any more */
> > +		clear_bit(IRQ_PEND_EXT_SERVICE, &kvm->arch.float_int.masked_irqs);
> > +		break;
> > +	case KVM_PV_ASYNC_CLEANUP_PERFORM:
> > +		/* This must not be called while holding kvm->lock */  
> 
> Two things:
> I know that we don't need to check async_destroy since it will find 
> nothing to cleanup because the command above is fenced. But I'd still 
> appreciate the same check here.

will add

> 
> Consider adding this to the comment:
> ", this is asserted inside the function."

will add

> 
> > +		r = kvm_s390_pv_deinit_aside_vm(kvm, &cmd->rc, &cmd->rrc);
> > +		break;
> >   	case KVM_PV_DISABLE: {
> >   		r = -EINVAL;
> >   		if (!kvm_s390_pv_is_protected(kvm))
> > @@ -2553,7 +2581,7 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
> >   		 */
> >   		if (r)
> >   			break;
> > -		r = kvm_s390_pv_deinit_vm(kvm, &cmd->rc, &cmd->rrc);
> > +		r = kvm_s390_pv_deinit_cleanup_all(kvm, &cmd->rc, &cmd->rrc);
> >   
> >   		/* no need to block service interrupts any more */
> >   		clear_bit(IRQ_PEND_EXT_SERVICE, &kvm->arch.float_int.masked_irqs);
> > @@ -2703,6 +2731,9 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
> >   	default:
> >   		r = -ENOTTY;
> >   	}
> > +	if (needslock)
> > +		mutex_unlock(&kvm->lock);
> > +
> >   	return r;
> >   }
> >   
> > @@ -2907,9 +2938,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
> >   			r = -EINVAL;
> >   			break;
> >   		}
> > -		mutex_lock(&kvm->lock);
> > +		/* must be called without kvm->lock */  
> 
> ...as it will acquire and release it by itself.

none of the other switch cases acquire kvm->lock, I actually think the
comment is redundant as it is, I don't think we need to expand it
further.

> 
> >   		r = kvm_s390_handle_pv(kvm, &args);
> > -		mutex_unlock(&kvm->lock);
> >   		if (copy_to_user(argp, &args, sizeof(args))) {
> >   			r = -EFAULT;
> >   			break;
> > @@ -3228,6 +3258,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.set_aside = NULL;
> >   	KVM_EVENT(3, "vm 0x%pK created by pid %u", kvm, current->pid);
> >   
> >   	return 0;
> > @@ -3272,11 +3304,9 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
> >   	/*
> >   	 * We are already at the end of life and kvm->lock is not taken.
> >   	 * This is ok as the file descriptor is closed by now and nobody
> > -	 * can mess with the pv state. To avoid lockdep_assert_held from
> > -	 * complaining we do not use kvm_s390_pv_is_protected.
> > +	 * can mess with the pv state.
> >   	 */
> > -	if (kvm_s390_pv_get_handle(kvm))
> > -		kvm_s390_pv_deinit_vm(kvm, &rc, &rrc);
> > +	kvm_s390_pv_deinit_cleanup_all(kvm, &rc, &rrc);
> >   	/*
> >   	 * Remove the mmu notifier only when the whole KVM VM is torn down,
> >   	 * and only if one was registered to begin with. If the VM is  
> [...]
> > +
> > +/**
> > + * kvm_s390_pv_set_aside - Set aside a protected VM for later teardown.
> > + * @kvm: the VM
> > + * @rc: return value for the RC field of the UVCB
> > + * @rrc: return value for the RRC field of the UVCB
> > + *
> > + * Set aside the protected VM for a subsequent teardown. The VM will be able
> > + * to continue immediately as a non-secure VM, and the information needed to
> > + * properly tear down the protected VM is set aside. If another protected VM
> > + * was already set aside without starting its teardown, this function will
> > + * fail.
> > + * The CPUs of the protected VM need to be destroyed beforehand.
> > + *
> > + * Context: kvm->lock needs to be held
> > + *
> > + * Return: 0 in case of success, -EINVAL if another protected VM was already set
> > + * aside, -ENOMEM if the system ran out of memory.
> > + */
> > +int kvm_s390_pv_set_aside(struct kvm *kvm, u16 *rc, u16 *rrc)
> > +{
> > +	struct pv_vm_to_be_destroyed *priv;
> > +
> > +	/*
> > +	 * If another protected VM was already prepared, refuse.  
> 
> s/prepared/set aside/
> or
> prepared for teardown

prepared for teardown; will fix

> 
> > +	 * A normal deinitialization has to be performed instead.
> > +	 */
> > +	if (kvm->arch.pv.set_aside)
> > +		return -EINVAL;
> > +	priv = kmalloc(sizeof(*priv), GFP_KERNEL | __GFP_ZERO);  
> 
> kzalloc()?

oops, yes

> 
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->stor_var = kvm->arch.pv.stor_var;
> > +	priv->stor_base = kvm->arch.pv.stor_base;
> > +	priv->handle = kvm_s390_pv_get_handle(kvm);
> > +	priv->old_gmap_table = (unsigned long)kvm->arch.gmap->table;
> > +	WRITE_ONCE(kvm->arch.gmap->guest_handle, 0);
> > +	if (s390_replace_asce(kvm->arch.gmap)) {
> > +		kfree(priv);
> > +		return -ENOMEM;
> >   	}
> >   
> > +	kvm_s390_destroy_lower_2g(kvm);
> > +	kvm_s390_clear_pv_state(kvm);
> > +	kvm->arch.pv.set_aside = priv;
> > +
> > +	*rc = 1;  
> 
> UVC_RC_EXECUTED	

will fix

> 
> > +	*rrc = 42;  
> 
> I'd prefer setting the rrc to 0.

I'd like to convey the information that the "successful" execution was
actually faked

> 
> > +	return 0;
> > +}  