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

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

On 04.02.20 13:13, David Hildenbrand wrote:
> [...]
> 
>> +#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);
>> +		kvm_s390_vcpu_unblock_all(kvm);
>> +		mutex_unlock(&kvm->lock);
>> +		break;
>> +	}
> 
> I think KVM_PV_VM_ENABLE/KVM_PV_VM_DISABLE would be a better fit. You're
> not creating/deleting VMs, aren't you? All you're doing is allocating
> some data and performing some kind of a mode switch.

I kind of like the idea. Need to talk to Janosch about this. 
> [...]
> 
>>  	VM_EVENT(kvm, 3, "create cpu %d at 0x%pK, sie block at 0x%pK", id, vcpu,
>>  		 vcpu->arch.sie_block);
>>  	trace_kvm_s390_create_vcpu(id, vcpu, vcpu->arch.sie_block);
>> @@ -4353,6 +4502,37 @@ long kvm_arch_vcpu_async_ioctl(struct file *filp,
>>  	return -ENOIOCTLCMD;
>>  }
>>  
>> +#ifdef CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST
>> +static int kvm_s390_handle_pv_vcpu(struct kvm_vcpu *vcpu,
>> +				   struct kvm_pv_cmd *cmd)
>> +{
>> +	int r = 0;
>> +
>> +	if (!kvm_s390_pv_is_protected(vcpu->kvm))
>> +		return -EINVAL;
>> +
>> +	switch (cmd->cmd) {
>> +	case KVM_PV_VCPU_CREATE: {
>> +		if (kvm_s390_pv_handle_cpu(vcpu))
>> +			return -EINVAL;
>> +
>> +		r = kvm_s390_pv_create_cpu(vcpu);
>> +		break;
>> +	}
>> +	case KVM_PV_VCPU_DESTROY: {
>> +		if (!kvm_s390_pv_handle_cpu(vcpu))
>> +			return -EINVAL;
>> +
>> +		r = kvm_s390_pv_destroy_cpu(vcpu);
>> +		break;
>> +	}
>> +	default:
>> +		r = -ENOTTY;
>> +	}
>> +	return r;
>> +}
> 
> I asked this already and didn't get an answer (lost in the flood of
> comments :) )
> 
> Can't we simply convert all VCPUs via KVM_PV_VM_CREATE and destoy them
> via KVM_PV_VM_DESTROY? Then you can easily handle hotplug as well in the
> kernel when a new VCPU is created and PV is active - oh and I see you
> are already doing that in kvm_arch_vcpu_create(). So that screams for
> doing either this a) completely triggered by user space or b) completely
> in the kernel. I prefer the latter. One interface less required.
> 
> I would assume that no VCPU is allowed to be running inside KVM while

right.

> performing the PV switch, which would make this even easier.

Same as above. I like the idea. Will need to talk to Janosch.