On Fri, 8 Nov 2019 08:36:35 +0100
Janosch Frank <frankja@xxxxxxxxxxxxx> wrote:
> On 11/7/19 5:29 PM, Cornelia Huck wrote:
> > On Thu, 24 Oct 2019 07:40:26 -0400
> > Janosch Frank <frankja@xxxxxxxxxxxxx> 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:
pgpOadijGRXFM.pgp
Description: OpenPGP digital signature
