Re: [RFC 04/37] KVM: s390: protvirt: Add initial lifecycle handling

[Cornelia Huck <cohuck@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 8857 bytes --]

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?

Also, can you maybe skip calling this function if we use the esca
already?

> >> +		if (!r)
> >> +			r = kvm_s390_pv_create_vm(kvm);
> >> +		kvm_s390_vcpu_unblock_all(kvm);
> >> +		mutex_unlock(&kvm->lock);
> >> +		break;
> >> +	}
> >> +	case KVM_PV_VM_DESTROY: {
> >> +		/* All VCPUs have to be destroyed before this call. */
> >> +		mutex_lock(&kvm->lock);
> >> +		kvm_s390_vcpu_block_all(kvm);
> >> +		r = kvm_s390_pv_destroy_vm(kvm);
> >> +		if (!r)
> >> +			kvm_s390_pv_dealloc_vm(kvm);
> >> +		kvm_s390_vcpu_unblock_all(kvm);
> >> +		mutex_unlock(&kvm->lock);
> >> +		break;
> >> +	}  
> > 
> > Would be helpful to have some code that shows when/how these are called
> > - do you have any plans to post something soon?  
> 
> Qemu patches will be in internal review soonish and afterwards I'll post
> them upstream

Great, looking forward to this :)

> 
> > 
> > (...)
> >   
> >> @@ -2529,6 +2642,9 @@ void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
> >>  
> >>  	if (vcpu->kvm->arch.use_cmma)
> >>  		kvm_s390_vcpu_unsetup_cmma(vcpu);
> >> +	if (IS_ENABLED(CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST) &&
> >> +	    kvm_s390_pv_handle_cpu(vcpu))  
> > 
> > I was a bit confused by that function name... maybe
> > kvm_s390_pv_cpu_get_handle()?  
> 
> Sure
> 
> > 
> > Also, if this always returns 0 if the config option is off, you
> > probably don't need to check for that option?  
> 
> Hmm, if we decide to remove the config option altogether then it's not
> needed anyway and I think that's what Christian wants.

That would be fine with me as well (I have not yet thought about all
implications there, though.)

> 
> >   
> >> +		kvm_s390_pv_destroy_cpu(vcpu);
> >>  	free_page((unsigned long)(vcpu->arch.sie_block));
> >>  
> >>  	kvm_vcpu_uninit(vcpu);
> >> @@ -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...

> 
> >   
> >> +	}
> >> +	debug_unregister(kvm->arch.dbf);
> >>  	free_page((unsigned long)kvm->arch.sie_page2);
> >>  	if (!kvm_is_ucontrol(kvm))
> >>  		gmap_remove(kvm->arch.gmap);  
> > 
> > (...)
> >   
> >> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
> >> index 6d9448dbd052..0d61dcc51f0e 100644
> >> --- a/arch/s390/kvm/kvm-s390.h
> >> +++ b/arch/s390/kvm/kvm-s390.h
> >> @@ -196,6 +196,53 @@ static inline int kvm_s390_user_cpu_state_ctrl(struct kvm *kvm)
> >>  	return kvm->arch.user_cpu_state_ctrl != 0;
> >>  }
> >>  
> >> +#ifdef CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST
> >> +/* implemented in pv.c */
> >> +void kvm_s390_pv_unpin(struct kvm *kvm);
> >> +void kvm_s390_pv_dealloc_vm(struct kvm *kvm);
> >> +int kvm_s390_pv_alloc_vm(struct kvm *kvm);
> >> +int kvm_s390_pv_create_vm(struct kvm *kvm);
> >> +int kvm_s390_pv_create_cpu(struct kvm_vcpu *vcpu);
> >> +int kvm_s390_pv_destroy_vm(struct kvm *kvm);
> >> +int kvm_s390_pv_destroy_cpu(struct kvm_vcpu *vcpu);
> >> +int kvm_s390_pv_set_sec_parms(struct kvm *kvm, void *hdr, u64 length);
> >> +int kvm_s390_pv_unpack(struct kvm *kvm, unsigned long addr, unsigned long size,
> >> +		       unsigned long tweak);
> >> +int kvm_s390_pv_verify(struct kvm *kvm);
> >> +
> >> +static inline bool kvm_s390_pv_is_protected(struct kvm *kvm)
> >> +{
> >> +	return !!kvm->arch.pv.handle;
> >> +}
> >> +
> >> +static inline u64 kvm_s390_pv_handle(struct kvm *kvm)  
> > 
> > This function name is less confusing than the one below, but maybe also
> > rename this to kvm_s390_pv_get_handle() for consistency?  
> 
> kvm_s390_pv_kvm_handle?

kvm_s390_pv_kvm_get_handle() would mirror the cpu function :) </bikeshed>

> 
> >   
> >> +{
> >> +	return kvm->arch.pv.handle;
> >> +}
> >> +
> >> +static inline u64 kvm_s390_pv_handle_cpu(struct kvm_vcpu *vcpu)
> >> +{
> >> +	return vcpu->arch.pv.handle;
> >> +}
> >> +#else
> >> +static inline void kvm_s390_pv_unpin(struct kvm *kvm) {}
> >> +static inline void kvm_s390_pv_dealloc_vm(struct kvm *kvm) {}
> >> +static inline int kvm_s390_pv_alloc_vm(struct kvm *kvm) { return 0; }
> >> +static inline int kvm_s390_pv_create_vm(struct kvm *kvm) { return 0; }
> >> +static inline int kvm_s390_pv_create_cpu(struct kvm_vcpu *vcpu) { return 0; }
> >> +static inline int kvm_s390_pv_destroy_vm(struct kvm *kvm) { return 0; }
> >> +static inline int kvm_s390_pv_destroy_cpu(struct kvm_vcpu *vcpu) { return 0; }
> >> +static inline int kvm_s390_pv_set_sec_parms(struct kvm *kvm,
> >> +					    u64 origin, u64 length) { return 0; }
> >> +static inline int kvm_s390_pv_unpack(struct kvm *kvm, unsigned long addr,
> >> +				     unsigned long size,  unsigned long tweak)
> >> +{ return 0; }
> >> +static inline int kvm_s390_pv_verify(struct kvm *kvm) { return 0; }
> >> +static inline bool kvm_s390_pv_is_protected(struct kvm *kvm) { return 0; }
> >> +static inline u64 kvm_s390_pv_handle(struct kvm *kvm) { return 0; }
> >> +static inline u64 kvm_s390_pv_handle_cpu(struct kvm_vcpu *vcpu) { return 0; }
> >> +#endif
> >> +
> >>  /* implemented in interrupt.c */
> >>  int kvm_s390_handle_wait(struct kvm_vcpu *vcpu);
> >>  void kvm_s390_vcpu_wakeup(struct kvm_vcpu *vcpu);  
> > 
> > (...)
> >   
> >> +int kvm_s390_pv_create_cpu(struct kvm_vcpu *vcpu)
> >> +{
> >> +	int rc;
> >> +	struct uv_cb_csc uvcb = {
> >> +		.header.cmd = UVC_CMD_CREATE_SEC_CPU,
> >> +		.header.len = sizeof(uvcb),
> >> +	};
> >> +
> >> +	/* EEXIST and ENOENT? */  
> > 
> > ?  
> 
> I was asking myself if EEXIST or ENOENT would be better error values
> than EINVAL.

EEXIST might be better, but I don't really like ENOENT.

> 
> >   
> >> +	if (kvm_s390_pv_handle_cpu(vcpu))
> >> +		return -EINVAL;
> >> +
> >> +	vcpu->arch.pv.stor_base = __get_free_pages(GFP_KERNEL,
> >> +						   get_order(uv_info.guest_cpu_stor_len));
> >> +	if (!vcpu->arch.pv.stor_base)
> >> +		return -ENOMEM;
> >> +
> >> +	/* Input */
> >> +	uvcb.guest_handle = kvm_s390_pv_handle(vcpu->kvm);
> >> +	uvcb.num = vcpu->arch.sie_block->icpua;
> >> +	uvcb.state_origin = (u64)vcpu->arch.sie_block;
> >> +	uvcb.stor_origin = (u64)vcpu->arch.pv.stor_base;
> >> +
> >> +	rc = uv_call(0, (u64)&uvcb);
> >> +	VCPU_EVENT(vcpu, 3, "PROTVIRT CREATE VCPU: cpu %d handle %llx rc %x rrc %x",
> >> +		   vcpu->vcpu_id, uvcb.cpu_handle, uvcb.header.rc,
> >> +		   uvcb.header.rrc);
> >> +
> >> +	/* Output */
> >> +	vcpu->arch.pv.handle = uvcb.cpu_handle;
> >> +	vcpu->arch.sie_block->pv_handle_cpu = uvcb.cpu_handle;
> >> +	vcpu->arch.sie_block->pv_handle_config = kvm_s390_pv_handle(vcpu->kvm);
> >> +	vcpu->arch.sie_block->sdf = 2;
> >> +	if (!rc)
> >> +		return 0;
> >> +
> >> +	kvm_s390_pv_destroy_cpu(vcpu);
> >> +	return -EINVAL;
> >> +}  
> > 
> > (...)
> > 
> > Only a quick readthrough, as this patch is longish.
> >   
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]