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;
>> + }
>
> (...)
>
