Re: [PATCH v2 09/42] KVM: s390: protvirt: Add initial vm and cpu lifecycle handling

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 17.02.20 11:56, David Hildenbrand wrote:
> [...]
>>  
>> +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;
> 
> Isn't this racy? I think there has to be a way to make sure the PV state
> can't change. Is there any and I am missing something obvious? (is
> suspect we need the kvm->lock)


Yes, kvm->lock around kvm_s390_handle_pv is safer.

Something like


diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 932f7f32e82f..87dc6caa2181 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2422,7 +2422,9 @@ long kvm_arch_vm_ioctl(struct file *filp,
                        r = -EFAULT;
                        break;
                }
+               mutex_lock(&kvm->lock);
                r = kvm_s390_handle_pv(kvm, &args);
+               mutex_unlock(&kvm->lock);
                if (copy_to_user(argp, &args, sizeof(args))) {
                        r = -EFAULT;
                        break;



[...]


>> +	case KVM_PV_VM_SET_SEC_PARMS: {
> 
> I'd name this "KVM_PV_VM_SET_PARMS" instead.

[...]

>> @@ -2975,6 +3121,9 @@ static int kvm_s390_vcpu_setup(struct kvm_vcpu *vcpu)
>>  
>>  	kvm_s390_vcpu_crypto_setup(vcpu);
>>  
>> +	if (kvm_s390_pv_is_protected(vcpu->kvm))
>> +		rc = kvm_s390_pv_create_cpu(vcpu, &uvrc, &uvrrc);
> 
> With an explicit KVM_PV_VCPU_CREATE, this does not belong here. When
> hotplugging CPUs, user space has to do that manually. But as I said
> already, this user space API could be improved. (below)

With your proposed API this would stay.

[...]

>> @@ -4493,6 +4674,25 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>>  					   irq_state.len);
>>  		break;
>>  	}
>> +	case KVM_S390_PV_COMMAND_VCPU: {
>> +		struct kvm_pv_cmd args;
>> +
>> +		r = 0;
>> +		if (!is_prot_virt_host()) {
>> +			r = -EINVAL;
>> +			break;
>> +		}
>> +		if (copy_from_user(&args, argp, sizeof(args))) {
>> +			r = -EFAULT;
>> +			break;
>> +		}
>> +		r = kvm_s390_handle_pv_vcpu(vcpu, &args);
>> +		if (copy_to_user(argp, &args, sizeof(args))) {
>> +			r = -EFAULT;
>> +			break;
>> +		}
>> +		break;
>> +	}
>>  	default:
>>  		r = -ENOTTY;
> 
> 
> Can we please discuss why we can't
> 
> - Get rid of KVM_S390_PV_COMMAND_VCPU
> - Do the allocation in KVM_PV_VM_CREATE
> - Rename KVM_PV_VM_CREATE -> KVM_PV_ENABLE
> - Rename KVM_PV_VM_DESTROY -> KVM_PV_DISABLE
> 
> This user space API is unnecessary complicated and confusing.

I will have a look if this is feasible.




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux