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.
