On 11/7/19 5:29 PM, Cornelia Huck wrote:
> On Thu, 24 Oct 2019 07:40:26 -0400
> Janosch Frank <frankja@xxxxxxxxxxxxx> wrote:
>
>> Let's add a KVM interface to create and destroy protected VMs.
>>
>> Signed-off-by: Janosch Frank <frankja@xxxxxxxxxxxxx>
>> ---
>> arch/s390/include/asm/kvm_host.h | 24 +++-
>> arch/s390/include/asm/uv.h | 110 ++++++++++++++
>> arch/s390/kvm/Makefile | 2 +-
>> arch/s390/kvm/kvm-s390.c | 173 +++++++++++++++++++++-
>> arch/s390/kvm/kvm-s390.h | 47 ++++++
>> arch/s390/kvm/pv.c | 237 +++++++++++++++++++++++++++++++
>> include/uapi/linux/kvm.h | 33 +++++
>
> Any new ioctls and caps probably want a mention in
> Documentation/virt/kvm/api.txt :)
Noted
>
>> 7 files changed, 622 insertions(+), 4 deletions(-)
>> create mode 100644 arch/s390/kvm/pv.c
>
> (...)
>
>> @@ -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);
>> + 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
>
> (...)
>
>> @@ -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.
>
>> + 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.
>
>> + }
>> + 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?
>
>> +{
>> + 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.
>
>> + 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
