On 21.02.20 12:45, David Hildenbrand wrote: > >> +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; >> + > > To make this nicer, can we simply merge alloc+create into init > > /* FMT 4 SIE needs esca */ > r = sca_switch_to_extended(kvm); > if (r) > break; > > r = kvm_s390_pv_init_vm(); > if (r) > break; > > r = kvm_s390_cpus_to_pv(kvm, &cmd->rc, &cmd->rrc); > if (r) > kvm_s390_pv_deinit_vm(); > break; > > I remember the split dates back to an earlier UAPI interface. > > Similarly from deinit. > > The you can just make deinit never fail and handle that freeing > special-case in there and add a comment. We want to tell userspace that PV_DISABLE failed, so I need the return. But I can still simplify things a bit. Will send an 3.2. I still have the alloc/dealloc as static helpers inside pv.c