On Mon, 11 Nov 2019 17:39:15 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 11/11/19 5:25 PM, Cornelia Huck wrote:
> > On Fri, 8 Nov 2019 08:36:35 +0100
> > Janosch Frank <frankja@linux.ibm.com> wrote:
> >   
> >> On 11/7/19 5:29 PM, Cornelia Huck wrote:  
> >>> On Thu, 24 Oct 2019 07:40:26 -0400
> >>> Janosch Frank <frankja@linux.ibm.com> wrote:  
> >   
> >>>> @@ -2157,6 +2164,96 @@ static int kvm_s390_set_cmma_bits(struct kvm *kvm,
> >>>>  	return r;
> >>>>  }
> >>>>  
> >>>> +#ifdef CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST
> >>>> +static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
> >>>> +{
> >>>> +	int r = 0;
> >>>> +	void __user *argp = (void __user *)cmd->data;
> >>>> +
> >>>> +	switch (cmd->cmd) {
> >>>> +	case KVM_PV_VM_CREATE: {
> >>>> +		r = kvm_s390_pv_alloc_vm(kvm);
> >>>> +		if (r)
> >>>> +			break;
> >>>> +
> >>>> +		mutex_lock(&kvm->lock);
> >>>> +		kvm_s390_vcpu_block_all(kvm);
> >>>> +		/* FMT 4 SIE needs esca */
> >>>> +		r = sca_switch_to_extended(kvm);  
> > 
> > Looking at this again: this function calls kvm_s390_vcpu_block_all()
> > (which probably does not hurt), but then kvm_s390_vcpu_unblock_all()...
> > don't we want to keep the block across pv_create_vm() as well?  
> 
> Yeah
> 
> > 
> > Also, can you maybe skip calling this function if we use the esca
> > already?  
> 
> Did I forget to include that in the patchset?
> I extended sca_switch_to_extended() to just return in that case.

If you did, I likely missed it; way too much stuff to review :)

> 
> >   
> >>>> +		if (!r)
> >>>> +			r = kvm_s390_pv_create_vm(kvm);
> >>>> +		kvm_s390_vcpu_unblock_all(kvm);
> >>>> +		mutex_unlock(&kvm->lock);
> >>>> +		break;
> >>>> +	}

(...)

> >>>> @@ -2555,8 +2671,13 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
> >>>>  {
> >>>>  	kvm_free_vcpus(kvm);
> >>>>  	sca_dispose(kvm);
> >>>> -	debug_unregister(kvm->arch.dbf);
> >>>>  	kvm_s390_gisa_destroy(kvm);
> >>>> +	if (IS_ENABLED(CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST) &&
> >>>> +	    kvm_s390_pv_is_protected(kvm)) {
> >>>> +		kvm_s390_pv_destroy_vm(kvm);
> >>>> +		kvm_s390_pv_dealloc_vm(kvm);    
> >>>
> >>> It seems the pv vm can be either destroyed via the ioctl above or in
> >>> the course of normal vm destruction. When is which way supposed to be
> >>> used? Also, it seems kvm_s390_pv_destroy_vm() can fail -- can that be a
> >>> problem in this code path?    
> >>
> >> On a reboot we need to tear down the protected VM and boot from
> >> unprotected mode again. If the VM shuts down we go through this cleanup
> >> path. If it fails the kernel will loose the memory that was allocated to
> >> start the VM.  
> > 
> > Shouldn't you at least log a moan in that case? Hopefully, this happens
> > very rarely, but the dbf will be gone...  
> 
> That's why I created the uv dbf :-)

Again, way too easy to get lost in these changes :)

> Well, it shouldn't happen at all so maybe a WARN will be a good option

Yeah, if it this is one of these "should not happen" things, a WARN
sounds good.

> 
> >   
> >>  
> >>>     
> >>>> +	}
> >>>> +	debug_unregister(kvm->arch.dbf);
> >>>>  	free_page((unsigned long)kvm->arch.sie_page2);
> >>>>  	if (!kvm_is_ucontrol(kvm))
> >>>>  		gmap_remove(kvm->arch.gmap);