Re: [RFCv2 08/37] KVM: s390: protvirt: Add initial lifecycle handling

[Christian Borntraeger <borntraeger@de.ibm.com>]

On 04.02.20 17:27, Cornelia Huck wrote:

>> +struct uv_cb_cgc {
> 
> Given that we now have a bunch of structs of the form uv_cb_TLA, can we
> add a comment to each for what uv call they are?

ack.

> 
>> +	struct uv_cb_header header;
>> +	u64 reserved08[2];
>> +	u64 guest_handle;
>> +	u64 conf_base_stor_origin;
>> +	u64 conf_var_stor_origin;
>> +	u64 reserved30;
>> +	u64 guest_stor_origin;
>> +	u64 guest_stor_len;
>> +	u64 guest_sca;
>> +	u64 guest_asce;
>> +	u64 reserved60[5];
>> +} __packed __aligned(8);
> 
> (...)
> 
>> +#ifdef CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST
>> +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;
>> +
>> +		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)
>> +			r = kvm_s390_pv_create_vm(kvm);
> 
> If sca_switch_to_extended() fails, you don't call
> kvm_s390_pv_dealloc_vm(). Also, kvm_s390_pv_create_vm() _does_ call
> _dealloc_vm() on failure, which seems a bit surprising. I'd probably
> move the _dealloc_vm() out of the error path of _create_vm() and call
> it here for r != 0.

Indeed this is fishy. Will fix. I might also want to split this patch into
an ultravisor related part and a kvm user interface part. then this would
have been more obvious and it could be easier to try Davids proposal regarding
enable/disable.   Its not that easyto split though, not sure if this works out. 
> 
>> +		kvm_s390_vcpu_unblock_all(kvm);
>> +		mutex_unlock(&kvm->lock);
>> +		break;
>> +	}
> 
> (...)
>