On 11/11/19 5:25 PM, Cornelia Huck wrote:
> 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?
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 (!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...
That's why I created the uv dbf :-)
Well, it shouldn't happen at all so maybe a WARN will be a good option
>
>>
>>>
>>>> + }
>>>> + 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:
signature.asc
Description: OpenPGP digital signature
