Re: [PATCH v6 3/7] s390: ap: setup relation betwen KVM and mediated device

From: Pierre Morel
Date: Thu Mar 28 2019 - 12:27:53 EST


On 28/03/2019 17:12, Tony Krowiak wrote:
On 3/22/19 10:43 AM, Pierre Morel wrote:
When the mediated device is open we setup the relation with KVM unset it
when the mediated device is released.

s/open we setup/open, we set up/
s/with KVM unset/with KVM and unset/


We lock the matrix mediated device to avoid any change until the
open is done.
We make sure that KVM is present when opening the mediated device
otherwise we return an error.

s/mediated device/mediated device,/


Increase kvm's refcount to ensure the KVM structures are still available
during the use of the mediated device by the guest.

Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxx>
---
 drivers/s390/crypto/vfio_ap_ops.c | 143 +++++++++++++++++++++-----------------
 1 file changed, 79 insertions(+), 64 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 77f7bac..bdb36e0 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -787,74 +787,24 @@ static const struct attribute_group *vfio_ap_mdev_attr_groups[] = {
ÂÂÂÂÂ NULL
 };
-/**
- * vfio_ap_mdev_set_kvm
- *
- * @matrix_mdev: a mediated matrix device
- * @kvm: reference to KVM instance
- *
- * Verifies no other mediated matrix device has @kvm and sets a reference to
- * it in @matrix_mdev->kvm.
- *
- * Return 0 if no other mediated matrix device has a reference to @kvm;
- * otherwise, returns an -EPERM.
- */
-static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct kvm *kvm)
-{
-ÂÂÂ struct ap_matrix_mdev *m;
-
-ÂÂÂ mutex_lock(&matrix_dev->lock);
-
-ÂÂÂ list_for_each_entry(m, &matrix_dev->mdev_list, node) {
-ÂÂÂÂÂÂÂ if ((m != matrix_mdev) && (m->kvm == kvm)) {
-ÂÂÂÂÂÂÂÂÂÂÂ mutex_unlock(&matrix_dev->lock);
-ÂÂÂÂÂÂÂÂÂÂÂ return -EPERM;
-ÂÂÂÂÂÂÂ }
-ÂÂÂ }
-
-ÂÂÂ matrix_mdev->kvm = kvm;
-ÂÂÂ mutex_unlock(&matrix_dev->lock);
-
-ÂÂÂ return 0;
-}
-
 static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long action, void *data)
 {
-ÂÂÂ int ret;
ÂÂÂÂÂ 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);
-
-ÂÂÂ if (!data) {
-ÂÂÂÂÂÂÂ matrix_mdev->kvm = NULL;
-ÂÂÂÂÂÂÂ return NOTIFY_OK;
-ÂÂÂ }
-
-ÂÂÂ ret = vfio_ap_mdev_set_kvm(matrix_mdev, data);
-ÂÂÂ if (ret)
-ÂÂÂÂÂÂÂ return 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;
-
-ÂÂÂ kvm_arch_crypto_set_masks(matrix_mdev->kvm, matrix_mdev->matrix.apm,
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ matrix_mdev->matrix.aqm,
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ matrix_mdev->matrix.adm);
+ÂÂÂ matrix_mdev->kvm = data;
ÂÂÂÂÂ return NOTIFY_OK;
 }
-static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev)
+static int vfio_ap_mdev_reset_queues(struct ap_matrix_mdev *matrix_mdev)
 {
ÂÂÂÂÂ int ret;
ÂÂÂÂÂ int rc = 0;
-ÂÂÂ struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
ÂÂÂÂÂ struct vfio_ap_queue *q;
ÂÂÂÂÂ list_for_each_entry(q, &matrix_mdev->qlist, list) {
@@ -871,41 +821,106 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev)
ÂÂÂÂÂ return rc;
 }
+/**
+ * vfio_ap_mdev_set_kvm
+ *
+ * @matrix_mdev: a mediated matrix device
+ *
+ * - Verifies that the hook is free and install the PQAP hook
+ * - Copy the matrix masks inside the CRYCB
+ * - Increment the KVM rerference count
+ *
+ * Return 0 if no other mediated matrix device has a reference to @kvm;
+ * otherwise, returns an -EPERM.
+ */
+static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev)
+{
+ÂÂÂ if (matrix_mdev->kvm->arch.crypto.pqap_hook)
+ÂÂÂÂÂÂÂ return -EPERM;

How would this happen; in other words, why are we checking this?

I check this to verify that no other AP mediated device is already in use by this VM.

Regards,
Pierre



--
Pierre Morel
Linux/KVM/QEMU in BÃblingen - Germany