Re: [PATCH v3.1 09/37] KVM: s390: protvirt: Add initial vm and cpu lifecycle handling

Christian Borntraeger
2020-02-21T13:03:53Z

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