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
