On 5/19/22 12:23 PM, Tony Krowiak wrote:
I made a few comments, but other than that this looks good to
me:
Reviewed-by: Tony Krowiak <akrowiak@xxxxxxxxxxxxx>
...
I'm not sure what version of the code on which the patch was rebased,
Was on top of Jason's vfio_group_locking series, but now would apply on
vfio-next since Alex pulled that series in to vfio-next.
but in the
latest master branch from our repository the kvm_get_kvm(kvm) function is
called inside of the if block below. I'm fine with moving outside of the
block, but
I don't see a corresponding removal of it from inside the block.
Yeah, I didn't notice those there. v3 will simply remove my get/put
additions and leave yours as-is.
...
vfio_ap_mdev_group_notifier;
- events = VFIO_GROUP_NOTIFY_SET_KVM;
+ if (!vdev->kvm)
+ return -EPERM;
Perhaps -EINVAL or -EFAULT?
Whichever you'd prefer? If I don't hear back I'll just use -EINVAL in v3.
- ret = vfio_register_notifier(vdev, VFIO_GROUP_NOTIFY, &events,
- &matrix_mdev->group_notifier);
+ ret = vfio_ap_mdev_set_kvm(matrix_mdev, vdev->kvm);
if (ret)
return ret;
@@ -1415,12 +1400,11 @@ static int vfio_ap_mdev_open_device(struct
vfio_device *vdev)
ret = vfio_register_notifier(vdev, VFIO_IOMMU_NOTIFY, &events,
&matrix_mdev->iommu_notifier);
if (ret)
- goto out_unregister_group;
+ goto err_kvm;
return 0;
-out_unregister_group:
- vfio_unregister_notifier(vdev, VFIO_GROUP_NOTIFY,
- &matrix_mdev->group_notifier);
+err_kvm:
+ vfio_ap_mdev_unset_kvm(matrix_mdev);
return ret;
}
@@ -1431,8 +1415,6 @@ static void vfio_ap_mdev_close_device(struct
vfio_device *vdev)
vfio_unregister_notifier(vdev, VFIO_IOMMU_NOTIFY,
&matrix_mdev->iommu_notifier);
- vfio_unregister_notifier(vdev, VFIO_GROUP_NOTIFY,
- &matrix_mdev->group_notifier);
vfio_ap_mdev_unset_kvm(matrix_mdev);
I'm not sure if this matters, but the vfio_ap_mdev_unset_kvm(matrix_mdev)
function uses the KVM pointer stored in matrix_mdev->kvm. I can't imagine
the KVM pointer stored in vdev->kvm being different than matrix_mdev->kvm,
With this patch matrix_mdev->kvm is set from the value in vdev->kvm
during vfio_ap_mdev_set_kvm (basically, doing the work that the notifier
was doing but instead of getting it from notifier data get it from the
vfio_device)
but thought I should point it out. Previously, this function was called
by the
notifier handler which did not have access to the KVM pointer which is
why it
was retrieved from matrix_mdev->kvm. Even if the vdev->kvm and
matrix_mdev->kvm did not match, we should probably go ahead and call
the unset function anyway to remove access to AP resources for the guest
and
reset the queues.
