> +static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
> +{
> + int r = 0;
> + u16 dummy;
> + void __user *argp = (void __user *)cmd->data;
> +
> + switch (cmd->cmd) {
> + case KVM_PV_ENABLE: {
> + r = -EINVAL;
> + if (kvm_s390_pv_is_protected(kvm))
> + break;
> +
> + r = kvm_s390_pv_alloc_vm(kvm);
> + if (r)
> + break;
> +
> + /* FMT 4 SIE needs esca */
> + r = sca_switch_to_extended(kvm);
> + if (r) {
> + kvm_s390_pv_dealloc_vm(kvm);
> + kvm_s390_vcpu_unblock_all(kvm);
You forgot to remove that.
> + mutex_unlock(&kvm->lock);
That's certainly wrong as well.
> + break;
> + }
> + r = kvm_s390_pv_create_vm(kvm, &cmd->rc, &cmd->rrc);
> + if (!r)
> + r = kvm_s390_cpus_to_pv(kvm, &cmd->rc, &cmd->rrc);
> + if (r)
> + kvm_s390_pv_destroy_vm(kvm, &dummy, &dummy);
Should there be a kvm_s390_pv_dealloc_vm() as well?
> +
> + break;
> + }
> + case KVM_PV_DISABLE: {
> + r = -EINVAL;
> + if (!kvm_s390_pv_is_protected(kvm))
> + break;
> +
> + kvm_s390_cpus_from_pv(kvm, &cmd->rc, &cmd->rrc);
> + r = kvm_s390_pv_destroy_vm(kvm, &cmd->rc, &cmd->rrc);
> + if (!r)
> + kvm_s390_pv_dealloc_vm(kvm);
Hm, if destroy fails, the CPUs would already have been removed.
Is there a way to make kvm_s390_pv_destroy_vm() never fail? The return
value is always ignored except here ... which looks wrong.
> + break;
> + }
[...]
> @@ -2558,10 +2724,21 @@ static void kvm_free_vcpus(struct kvm *kvm)
>
> void kvm_arch_destroy_vm(struct kvm *kvm)
> {
> + u16 rc, rrc;
> kvm_free_vcpus(kvm);
> sca_dispose(kvm);
> - debug_unregister(kvm->arch.dbf);
> kvm_s390_gisa_destroy(kvm);
> + /*
> + * We are already at the end of life and kvm->lock is not taken.
> + * This is ok as the file descriptor is closed by now and nobody
> + * can mess with the pv state. To avoid lockdep_assert_held from
> + * complaining we do not use kvm_s390_pv_is_protected.
> + */
> + if (kvm_s390_pv_get_handle(kvm)) {
I'd prefer something like kvm_s390_pv_is_protected_unlocked(), but I
guess for these few use cases, this is fine.
> + kvm_s390_pv_destroy_vm(kvm, &rc, &rrc);
> + kvm_s390_pv_dealloc_vm(kvm);
> + }
> + debug_unregister(kvm->arch.dbf);
> free_page((unsigned long)kvm->arch.sie_page2);
> if (!kvm_is_ucontrol(kvm))
> gmap_remove(kvm->arch.gmap);
> @@ -2657,6 +2834,9 @@ static int sca_switch_to_extended(struct kvm *kvm)
> unsigned int vcpu_idx;
> u32 scaol, scaoh;
>
> + if (kvm->arch.use_esca)
> + return 0;
> +
> new_sca = alloc_pages_exact(sizeof(*new_sca), GFP_KERNEL|__GFP_ZERO);
> if (!new_sca)
> return -ENOMEM;
> @@ -2908,6 +3088,7 @@ static void kvm_s390_vcpu_setup_model(struct kvm_vcpu *vcpu)
> static int kvm_s390_vcpu_setup(struct kvm_vcpu *vcpu)
> {
> int rc = 0;
> + u16 uvrc, uvrrc;
>
> atomic_set(&vcpu->arch.sie_block->cpuflags, CPUSTAT_ZARCH |
> CPUSTAT_SM |
> @@ -2975,6 +3156,11 @@ static int kvm_s390_vcpu_setup(struct kvm_vcpu *vcpu)
>
> kvm_s390_vcpu_crypto_setup(vcpu);
>
> + mutex_lock(&vcpu->kvm->lock);
> + if (kvm_s390_pv_is_protected(vcpu->kvm))
> + rc = kvm_s390_pv_create_cpu(vcpu, &uvrc, &uvrrc);
> + mutex_unlock(&vcpu->kvm->lock);
Do we have to cleanup anything? (e.g., cmma page) I *think*
kvm_arch_vcpu_destroy() is not called when kvm_arch_vcpu_create() fails ...
> +
> return rc;
> }
>
> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
> index 83dabb18e4d9..d62de29b2d6c 100644
> --- a/arch/s390/kvm/kvm-s390.h
> +++ b/arch/s390/kvm/kvm-s390.h
> @@ -15,6 +15,7 @@
> #include <linux/hrtimer.h>
> #include <linux/kvm.h>
> #include <linux/kvm_host.h>
> +#include <linux/lockdep.h>
> #include <asm/facility.h>
> #include <asm/processor.h>
> #include <asm/sclp.h>
> @@ -207,6 +208,40 @@ static inline int kvm_s390_user_cpu_state_ctrl(struct kvm *kvm)
> return kvm->arch.user_cpu_state_ctrl != 0;
> }
>
> +/* implemented in pv.c */
> +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, u16 *rc, u16 *rrc);
> +int kvm_s390_pv_create_cpu(struct kvm_vcpu *vcpu, u16 *rc, u16 *rrc);
> +int kvm_s390_pv_destroy_vm(struct kvm *kvm, u16 *rc, u16 *rrc);
> +void kvm_s390_pv_destroy_cpu(struct kvm_vcpu *vcpu, u16 *rc, u16 *rrc);
> +int kvm_s390_pv_set_sec_parms(struct kvm *kvm, void *hdr, u64 length, u16 *rc,
> + u16 *rrc);
> +int kvm_s390_pv_unpack(struct kvm *kvm, unsigned long addr, unsigned long size,
> + unsigned long tweak, u16 *rc, u16 *rrc);
> +
> +static inline u64 kvm_s390_pv_get_handle(struct kvm *kvm)
> +{
> + return kvm->arch.pv.handle;
> +}
> +
> +static inline u64 kvm_s390_pv_cpu_get_handle(struct kvm_vcpu *vcpu)
> +{
> + return vcpu->arch.pv.handle;
> +}
> +
> +static inline bool kvm_s390_pv_is_protected(struct kvm *kvm)
Could have been "kvm_s390_is_protected" or "kvm_s390_is_pv", but also
fine with me. (maybe I even suggested that one without caring about that
detail :) )
[...]
> +
> /* implemented in interrupt.c */
> int kvm_s390_handle_wait(struct kvm_vcpu *vcpu);
> void kvm_s390_vcpu_wakeup(struct kvm_vcpu *vcpu);
> diff --git a/arch/s390/kvm/pv.c b/arch/s390/kvm/pv.c
> new file mode 100644
> index 000000000000..67ea9a18ed8f
> --- /dev/null
> +++ b/arch/s390/kvm/pv.c
> @@ -0,0 +1,256 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Hosting Secure Execution virtual machines
> + *
> + * Copyright IBM Corp. 2019
> + * Author(s): Janosch Frank <frankja@xxxxxxxxxxxxx>
I'd assume you're an author as well at this point ;)
[...]
> +
> +int kvm_s390_pv_set_sec_parms(struct kvm *kvm, void *hdr, u64 length, u16 *rc,
> + u16 *rrc)
> +{
> + struct uv_cb_ssc uvcb = {
> + .header.cmd = UVC_CMD_SET_SEC_CONF_PARAMS,
> + .header.len = sizeof(uvcb),
> + .sec_header_origin = (u64)hdr,
> + .sec_header_len = length,
> + .guest_handle = kvm_s390_pv_get_handle(kvm),
> + };
> + int cc;
> +
> + cc = uv_call(0, (u64)&uvcb);
int cc = ... could be done.
> + *rc = uvcb.header.rc;
> + *rrc = uvcb.header.rrc;
> + KVM_UV_EVENT(kvm, 3, "PROTVIRT VM SET PARMS: rc %x rrc %x",
> + *rc, *rrc);
> + if (cc)
> + return -EINVAL;
> + return 0;
> +}
> +
> +static int unpack_one(struct kvm *kvm, unsigned long addr, u64 tweak[2],
> + u16 *rc, u16 *rrc)
> +{
> + struct uv_cb_unp uvcb = {
> + .header.cmd = UVC_CMD_UNPACK_IMG,
> + .header.len = sizeof(uvcb),
> + .guest_handle = kvm_s390_pv_get_handle(kvm),
> + .gaddr = addr,
> + .tweak[0] = tweak[0],
> + .tweak[1] = tweak[1],
> + };
> + int ret;
> +
> + ret = gmap_make_secure(kvm->arch.gmap, addr, &uvcb);
... similarly, with ret.
> + *rc = uvcb.header.rc;
> + *rrc = uvcb.header.rrc;
> +
> + if (ret && ret != -EAGAIN)
> + KVM_UV_EVENT(kvm, 3, "PROTVIRT VM UNPACK: failed addr %llx with rc %x rrc %x",
> + uvcb.gaddr, *rc, *rrc);
> + return ret;
> +}
> +
> +int kvm_s390_pv_unpack(struct kvm *kvm, unsigned long addr, unsigned long size,
> + unsigned long tweak, u16 *rc, u16 *rrc)
> +{
> + u64 tw[2] = {tweak, 0};
I have no idea what tweaks are in this context. So I have to trust you
guys on the implementation, because I don't understand it.
Especially, why can't we simply have
s/tweak/tweak/
offset = 0;
while (offset < size) {
...
ret = unpack_one(kvm, addr, tweak, offset, rc, rrc);
^ no idea what tweak is
...
... offset += PAGE_SIZE;
}
But maybe I am missing what the whole array is about.
> + int ret = 0;
> +
> + if (addr & ~PAGE_MASK || !size || size & ~PAGE_MASK)
> + return -EINVAL;
> +
> + KVM_UV_EVENT(kvm, 3, "PROTVIRT VM UNPACK: start addr %lx size %lx",
> + addr, size);
> +
> + while (tw[1] < size) {> + ret = unpack_one(kvm, addr, tw, rc, rrc);
> + if (ret == -EAGAIN) {
> + cond_resched();
> + if (fatal_signal_pending(current))
> + break;
> + continue;
> + }
> + if (ret)
> + break;
> + addr += PAGE_SIZE;
> + tw[1] += PAGE_SIZE;
> + }
> + if (!ret)
> + KVM_UV_EVENT(kvm, 3, "%s", "PROTVIRT VM UNPACK: successful");
> + return ret;
> +}
[...]
> +enum pv_cmd_id {
> + KVM_PV_ENABLE,
> + KVM_PV_DISABLE,
> + KVM_PV_VM_SET_SEC_PARMS,
> + KVM_PV_VM_UNPACK,
> + KVM_PV_VM_VERIFY,
I wonder if we should just drop "_VM" from all of these ...
[...]
--
Thanks,
David / dhildenb
