On 12/21/20 11:05 PM, Halil Pasic wrote:
On Mon, 21 Dec 2020 13:56:25 -0500 Tony Krowiak akrowiak@linux.ibm.com wrote:
The vfio_ap device driver registers a group notifier with VFIO when the file descriptor for a VFIO mediated device for a KVM guest is opened to receive notification that the KVM pointer is set (VFIO_GROUP_NOTIFY_SET_KVM event). When the KVM pointer is set, the vfio_ap driver takes the following actions:
- Stashes the KVM pointer in the vfio_ap_mdev struct that holds the state of the mediated device.
- Calls the kvm_get_kvm() function to increment its reference counter.
- Sets the function pointer to the function that handles interception of the instruction that enables/disables interrupt processing.
- Sets the masks in the KVM guest's CRYCB to pass AP resources through to the guest.
In order to avoid memory leaks, when the notifier is called to receive notification that the KVM pointer has been set to NULL, the vfio_ap device driver should reverse the actions taken when the KVM pointer was set.
Fixes: 258287c994de ("s390: vfio-ap: implement mediated device open callback") Cc: stable@vger.kernel.org Signed-off-by: Tony Krowiak akrowiak@linux.ibm.com Reviewed-by: Halil Pasic pasic@linux.ibm.com Reviewed-by: Cornelia Huck cohuck@redhat.com
[..]
static int vfio_ap_mdev_group_notifier(struct notifier_block *nb, unsigned long action, void *data) {
- int ret;
- int ret, notify_rc = NOTIFY_DONE; struct ap_matrix_mdev *matrix_mdev;
if (action != VFIO_GROUP_NOTIFY_SET_KVM) return NOTIFY_OK; matrix_mdev = container_of(nb, struct ap_matrix_mdev, group_notifier);
- mutex_lock(&matrix_dev->lock);
if (!data) {
matrix_mdev->kvm = NULL;return NOTIFY_OK;
if (matrix_mdev->kvm)vfio_ap_mdev_unset_kvm(matrix_mdev);notify_rc = NOTIFY_OK;goto notify_done;ret = vfio_ap_mdev_set_kvm(matrix_mdev, data); if (ret)
return NOTIFY_DONE;
goto notify_done;/* If there is no CRYCB pointer, then we can't copy the masks */ if (!matrix_mdev->kvm->arch.crypto.crycbd)
return NOTIFY_DONE;
goto notify_done;kvm_arch_crypto_set_masks(matrix_mdev->kvm, matrix_mdev->matrix.apm, matrix_mdev->matrix.aqm, matrix_mdev->matrix.adm);
- return NOTIFY_OK;
Shouldn't there be an
- notify_rc = NOTIFY_OK;
here? I mean you initialize notify_rc to NOTIFY_DONE, in the !data branch on success you set notify_rc to NOTIFY_OK, but in the !!data branch it just stays NOTIFY_DONE. Or am I missing something?
I don't think it matters much since NOTIFY_OK and NOTIFY_DONE have no further effect on processing of the notification queue, but I believe you are correct, this is a change from what we originally had. I can restore the original return values if you'd prefer.
Otherwise LGTM!
Regards, Halil
+notify_done:
- mutex_unlock(&matrix_dev->lock);
- return notify_rc; }
[..]