Re: [PATCH v6 14/21] s390: vfio-ap: implement mediated device open callback

[Halil Pasic <pasic@linux.ibm.com>]

On 07/12/2018 06:03 PM, Tony Krowiak wrote:
>>> +static int vfio_ap_mdev_open(struct mdev_device *mdev)
>>> +{
>>> +    struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
>>> +    struct ap_matrix_dev *matrix_dev =
>>> +        to_ap_matrix_dev(mdev_parent_dev(mdev));
>>> +    unsigned long events;
>>> +    int ret;
>>> +
>>> +    if (!try_module_get(THIS_MODULE))
>>> +        return -ENODEV;
>>> +
>>> +    ret = vfio_ap_verify_queues_reserved(matrix_dev, matrix_mdev->name,
>>> +                         &matrix_mdev->matrix);
>>> +    if (ret)
>>> +        goto out_err;
>>> +
>>> +    matrix_mdev->group_notifier.notifier_call = vfio_ap_mdev_group_notifier;
>>> +    events = VFIO_GROUP_NOTIFY_SET_KVM;
>>> +
>>> +    ret = vfio_register_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
>>> +                     &events, &matrix_mdev->group_notifier);
>>> +    if (ret)
>>> +        goto out_err;
>>> +
>>> +    ret = kvm_ap_validate_crypto_setup(matrix_mdev->kvm);
>>
>> At this point you assume that your vfio_ap_mdev_group_notifier callback
>> was called with VFIO_GROUP_NOTIFY_SET_KVM and that you do have
>> matrix_mdev->kvm set up properly.
>>
>> Based on how callbacks usually work this seems rather strange. It's
>> probably cleaner to set up he cyrcb (including all the validation
>> that needs to be done immediately before) in the callback
>> (vfio_ap_mdev_group_notifier).
>>
>> If that is not viable I think we need a comment here explaining why is this
>> OK (at least).
> 
> This was originally in the callback and moved out, to the best of my recollection,
> because the validation at that time was done on the CRYCB and if that validation
> failed, there was no way to notify the client (QEMU) that configuration of the
> guest's CRYCB failed from the notification callback. This works - at least as far
> as I can tell from testing - because the registration of the notifier invokes the
> notification callback if KVM has already been set and that appears to be the case.
> You are correct, however; we probably shouldn't bank on that given that
> I don't think we can guarantee that to be the case 100% of the time. Consequently,
> I will move this back into the notification callback and since consistency checking
> is now being done on the mdev devices instead of the CRYCB, we don't need KVM at open
> time.

Sounds good to me. Making the open fail was not a good way to communicate this
error condition to userspace anyway.


Regards,
Halil