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

[David Hildenbrand <david@redhat.com>]

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