[...]
>
> +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 = -EINVAL;
> + if (kvm_s390_pv_is_protected(kvm))
> + break;
Isn't this racy? I think there has to be a way to make sure the PV state
can't change. Is there any and I am missing something obvious? (is
suspect we need the kvm->lock)
> +
> + 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) {
> + kvm_s390_pv_dealloc_vm(kvm);
> + kvm_s390_vcpu_unblock_all(kvm);
> + mutex_unlock(&kvm->lock);
> + break;
> + }
> + r = kvm_s390_pv_create_vm(kvm, &cmd->rc, &cmd->rrc);
> + kvm_s390_vcpu_unblock_all(kvm);
> + mutex_unlock(&kvm->lock);
> + break;
> + }
> + case KVM_PV_VM_DESTROY: {
> + r = -EINVAL;
> + if (!kvm_s390_pv_is_protected(kvm))
> + break;
> +
dito
> + /* 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, &cmd->rc, &cmd->rrc);
> + if (!r)
> + kvm_s390_pv_dealloc_vm(kvm);
> + kvm_s390_vcpu_unblock_all(kvm);
> + mutex_unlock(&kvm->lock);
> + break;
> + }
> + case KVM_PV_VM_SET_SEC_PARMS: {
I'd name this "KVM_PV_VM_SET_PARMS" instead.
> + struct kvm_s390_pv_sec_parm parms = {};
> + void *hdr;
> +
> + r = -EINVAL;
> + if (!kvm_s390_pv_is_protected(kvm))
> + break;
> +
dito
> + r = -EFAULT;
> + if (copy_from_user(&parms, argp, sizeof(parms)))
> + break;
> +
> + /* Currently restricted to 8KB */
> + r = -EINVAL;
> + if (parms.length > PAGE_SIZE * 2)
> + break;
> +
> + r = -ENOMEM;
> + hdr = vmalloc(parms.length);
> + if (!hdr)
> + break;
> +
> + r = -EFAULT;
> + if (!copy_from_user(hdr, (void __user *)parms.origin,
> + parms.length))
> + r = kvm_s390_pv_set_sec_parms(kvm, hdr, parms.length,
> + &cmd->rc, &cmd->rrc);
> +
> + vfree(hdr);
> + break;
> + }
> + case KVM_PV_VM_UNPACK: {
> + struct kvm_s390_pv_unp unp = {};
> +
> + r = -EINVAL;
> + if (!kvm_s390_pv_is_protected(kvm))
> + break;
> +
dito
> + r = -EFAULT;
> + if (copy_from_user(&unp, argp, sizeof(unp)))
> + break;
> +
> + r = kvm_s390_pv_unpack(kvm, unp.addr, unp.size, unp.tweak,
> + &cmd->rc, &cmd->rrc);
> + break;
> + }
> + case KVM_PV_VM_VERIFY: {
> + r = -EINVAL;
> + if (!kvm_s390_pv_is_protected(kvm))> + break;
dito
> +
> + r = uv_cmd_nodata(kvm_s390_pv_handle(kvm),
> + UVC_CMD_VERIFY_IMG, &cmd->rc, &cmd->rrc);
> + KVM_UV_EVENT(kvm, 3, "PROTVIRT VERIFY: rc %x rrc %x", cmd->rc,
> + cmd->rrc);
> + break;
> + }
> + default:
> + return -ENOTTY;
> + }
> + return r;
> +}
> +
> long kvm_arch_vm_ioctl(struct file *filp,
> unsigned int ioctl, unsigned long arg)
> {
> @@ -2262,6 +2376,25 @@ long kvm_arch_vm_ioctl(struct file *filp,
> mutex_unlock(&kvm->slots_lock);
> break;
> }
> + case KVM_S390_PV_COMMAND: {
> + struct kvm_pv_cmd args;
> +
> + r = 0;
> + if (!is_prot_virt_host()) {
> + r = -EINVAL;
> + break;
> + }
> + if (copy_from_user(&args, argp, sizeof(args))) {
> + r = -EFAULT;
> + break;
> + }
> + r = kvm_s390_handle_pv(kvm, &args);
> + if (copy_to_user(argp, &args, sizeof(args))) {
> + r = -EFAULT;
> + break;
> + }
> + break;
> + }
> default:
> r = -ENOTTY;
> }
> @@ -2525,6 +2658,8 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>
> void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
> {
> + u16 rc, rrc;
> +
> VCPU_EVENT(vcpu, 3, "%s", "free cpu");
> trace_kvm_s390_destroy_vcpu(vcpu->vcpu_id);
> kvm_s390_clear_local_irqs(vcpu);
> @@ -2537,6 +2672,8 @@ void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
>
> if (vcpu->kvm->arch.use_cmma)
> kvm_s390_vcpu_unsetup_cmma(vcpu);
> + if (kvm_s390_pv_handle_cpu(vcpu))
> + kvm_s390_pv_destroy_cpu(vcpu, &rc, &rrc);
> free_page((unsigned long)(vcpu->arch.sie_block));
> }
>
> @@ -2558,10 +2695,15 @@ 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);
> + if (kvm_s390_pv_is_protected(kvm)) {
> + 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 +2799,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 +3053,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 +3121,9 @@ static int kvm_s390_vcpu_setup(struct kvm_vcpu *vcpu)
>
> kvm_s390_vcpu_crypto_setup(vcpu);
>
> + if (kvm_s390_pv_is_protected(vcpu->kvm))
> + rc = kvm_s390_pv_create_cpu(vcpu, &uvrc, &uvrrc);
With an explicit KVM_PV_VCPU_CREATE, this does not belong here. When
hotplugging CPUs, user space has to do that manually. But as I said
already, this user space API could be improved. (below)
> +
> return rc;
> }
>
> @@ -4352,6 +4501,38 @@ long kvm_arch_vcpu_async_ioctl(struct file *filp,
> return -ENOIOCTLCMD;
> }
>
> +static int kvm_s390_handle_pv_vcpu(struct kvm_vcpu *vcpu,
> + struct kvm_pv_cmd *cmd)
> +{
> + int r = 0;
> +
> + if (!kvm_s390_pv_is_protected(vcpu->kvm))
> + return -EINVAL;
> +
> + if (cmd->flags)
> + return -EINVAL;
> +
> + switch (cmd->cmd) {
> + case KVM_PV_VCPU_CREATE: {
> + if (kvm_s390_pv_handle_cpu(vcpu))
> + return -EINVAL;
> +
> + r = kvm_s390_pv_create_cpu(vcpu, &cmd->rc, &cmd->rrc);
> + break;
> + }
> + case KVM_PV_VCPU_DESTROY: {
> + if (!kvm_s390_pv_handle_cpu(vcpu))
> + return -EINVAL;
> +
> + r = kvm_s390_pv_destroy_cpu(vcpu, &cmd->rc, &cmd->rrc);
> + break;
> + }
> + default:
> + r = -ENOTTY;
> + }
> + return r;
> +}
> +
> long kvm_arch_vcpu_ioctl(struct file *filp,
> unsigned int ioctl, unsigned long arg)
> {
> @@ -4493,6 +4674,25 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> irq_state.len);
> break;
> }
> + case KVM_S390_PV_COMMAND_VCPU: {
> + struct kvm_pv_cmd args;
> +
> + r = 0;
> + if (!is_prot_virt_host()) {
> + r = -EINVAL;
> + break;
> + }
> + if (copy_from_user(&args, argp, sizeof(args))) {
> + r = -EFAULT;
> + break;
> + }
> + r = kvm_s390_handle_pv_vcpu(vcpu, &args);
> + if (copy_to_user(argp, &args, sizeof(args))) {
> + r = -EFAULT;
> + break;
> + }
> + break;
> + }
> default:
> r = -ENOTTY;
Can we please discuss why we can't
- Get rid of KVM_S390_PV_COMMAND_VCPU
- Do the allocation in KVM_PV_VM_CREATE
- Rename KVM_PV_VM_CREATE -> KVM_PV_ENABLE
- Rename KVM_PV_VM_DESTROY -> KVM_PV_DISABLE
This user space API is unnecessary complicated and confusing.
--
Thanks,
David / dhildenb
