On 21.02.20 14:03, Christian Borntraeger wrote:
>
>
> 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.
Yes, makes sense. But having handling wrapped in a single function makes
sense.
> Will send an 3.2. I still have the alloc/dealloc as static helpers inside
> pv.c
Ack.
--
Thanks,
David / dhildenb
