linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification
@ 2021-07-19 19:35 Tony Krowiak
  2021-07-19 19:35 ` [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer Tony Krowiak
                   ` (2 more replies)
  0 siblings, 3 replies; 42+ messages in thread
From: Tony Krowiak @ 2021-07-19 19:35 UTC (permalink / raw)
  To: linux-s390, linux-kernel
  Cc: borntraeger, cohuck, pasic, jjherne, jgg, alex.williamson,
	kwankhede, david

This series is actually only comprised of a single patch to replace the
open coded locks used during VFIO_GROUP_NOTIFY_SET_KVM notification. The
first patch is included because it is a pre-req slotted to be merged but is
not yet available in the kernel.

Tony Krowiak (2):
  s390/vfio-ap: r/w lock for PQAP interception handler function pointer
  s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM
    notification

 arch/s390/include/asm/kvm_host.h      |   8 +-
 arch/s390/kvm/kvm-s390.c              |  28 +++++-
 arch/s390/kvm/priv.c                  |  10 +-
 drivers/s390/crypto/vfio_ap_ops.c     | 127 +++++++++-----------------
 drivers/s390/crypto/vfio_ap_private.h |   4 +-
 5 files changed, 77 insertions(+), 100 deletions(-)

-- 
2.30.2


^ permalink raw reply	[flat|nested] 42+ messages in thread

* [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer
  2021-07-19 19:35 [PATCH 0/2] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification Tony Krowiak
@ 2021-07-19 19:35 ` Tony Krowiak
  2021-08-18 17:03   ` Christian Borntraeger
  2021-07-19 19:35 ` [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification Tony Krowiak
  2021-08-02 13:10 ` [PATCH 0/2] s390/vfio-ap: do not open code " Tony Krowiak
  2 siblings, 1 reply; 42+ messages in thread
From: Tony Krowiak @ 2021-07-19 19:35 UTC (permalink / raw)
  To: linux-s390, linux-kernel
  Cc: borntraeger, cohuck, pasic, jjherne, jgg, alex.williamson,
	kwankhede, david

The function pointer to the interception handler for the PQAP instruction
can get changed during the interception process. Let's add a
semaphore to struct kvm_s390_crypto to control read/write access to the
function pointer contained therein.

The semaphore must be locked for write access by the vfio_ap device driver
when notified that the KVM pointer has been set or cleared. It must be
locked for read access by the interception framework when the PQAP
instruction is intercepted.

Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
---
 arch/s390/include/asm/kvm_host.h      |  8 +++-----
 arch/s390/kvm/kvm-s390.c              |  1 +
 arch/s390/kvm/priv.c                  | 10 ++++++----
 drivers/s390/crypto/vfio_ap_ops.c     | 23 +++++++++++++++++------
 drivers/s390/crypto/vfio_ap_private.h |  2 +-
 5 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 9b4473f76e56..f18849d259e6 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -798,14 +798,12 @@ struct kvm_s390_cpu_model {
 	unsigned short ibc;
 };
 
-struct kvm_s390_module_hook {
-	int (*hook)(struct kvm_vcpu *vcpu);
-	struct module *owner;
-};
+typedef int (*crypto_hook)(struct kvm_vcpu *vcpu);
 
 struct kvm_s390_crypto {
 	struct kvm_s390_crypto_cb *crycb;
-	struct kvm_s390_module_hook *pqap_hook;
+	struct rw_semaphore pqap_hook_rwsem;
+	crypto_hook *pqap_hook;
 	__u32 crycbd;
 	__u8 aes_kw;
 	__u8 dea_kw;
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index b655a7d82bf0..a08f242a9f27 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2630,6 +2630,7 @@ static void kvm_s390_crypto_init(struct kvm *kvm)
 {
 	kvm->arch.crypto.crycb = &kvm->arch.sie_page2->crycb;
 	kvm_s390_set_crycb_format(kvm);
+	init_rwsem(&kvm->arch.crypto.pqap_hook_rwsem);
 
 	if (!test_kvm_facility(kvm, 76))
 		return;
diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
index 9928f785c677..6bed9406c1f3 100644
--- a/arch/s390/kvm/priv.c
+++ b/arch/s390/kvm/priv.c
@@ -610,6 +610,7 @@ static int handle_io_inst(struct kvm_vcpu *vcpu)
 static int handle_pqap(struct kvm_vcpu *vcpu)
 {
 	struct ap_queue_status status = {};
+	crypto_hook pqap_hook;
 	unsigned long reg0;
 	int ret;
 	uint8_t fc;
@@ -657,15 +658,16 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
 	 * Verify that the hook callback is registered, lock the owner
 	 * and call the hook.
 	 */
+	down_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem);
 	if (vcpu->kvm->arch.crypto.pqap_hook) {
-		if (!try_module_get(vcpu->kvm->arch.crypto.pqap_hook->owner))
-			return -EOPNOTSUPP;
-		ret = vcpu->kvm->arch.crypto.pqap_hook->hook(vcpu);
-		module_put(vcpu->kvm->arch.crypto.pqap_hook->owner);
+		pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook;
+		ret = pqap_hook(vcpu);
 		if (!ret && vcpu->run->s.regs.gprs[1] & 0x00ff0000)
 			kvm_s390_set_psw_cc(vcpu, 3);
+		up_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem);
 		return ret;
 	}
+	up_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem);
 	/*
 	 * A vfio_driver must register a hook.
 	 * No hook means no driver to enable the SIE CRYCB and no queues.
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 122c85c22469..742277bc8d1c 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -352,8 +352,7 @@ static int vfio_ap_mdev_create(struct mdev_device *mdev)
 	vfio_ap_matrix_init(&matrix_dev->info, &matrix_mdev->matrix);
 	init_waitqueue_head(&matrix_mdev->wait_for_kvm);
 	mdev_set_drvdata(mdev, matrix_mdev);
-	matrix_mdev->pqap_hook.hook = handle_pqap;
-	matrix_mdev->pqap_hook.owner = THIS_MODULE;
+	matrix_mdev->pqap_hook = handle_pqap;
 	mutex_lock(&matrix_dev->lock);
 	list_add(&matrix_mdev->node, &matrix_dev->mdev_list);
 	mutex_unlock(&matrix_dev->lock);
@@ -1115,15 +1114,20 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
 		}
 
 		kvm_get_kvm(kvm);
+		matrix_mdev->kvm = kvm;
 		matrix_mdev->kvm_busy = true;
 		mutex_unlock(&matrix_dev->lock);
+
+		down_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem);
+		kvm->arch.crypto.pqap_hook = &matrix_mdev->pqap_hook;
+		up_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem);
+
 		kvm_arch_crypto_set_masks(kvm,
 					  matrix_mdev->matrix.apm,
 					  matrix_mdev->matrix.aqm,
 					  matrix_mdev->matrix.adm);
+
 		mutex_lock(&matrix_dev->lock);
-		kvm->arch.crypto.pqap_hook = &matrix_mdev->pqap_hook;
-		matrix_mdev->kvm = kvm;
 		matrix_mdev->kvm_busy = false;
 		wake_up_all(&matrix_mdev->wait_for_kvm);
 	}
@@ -1189,10 +1193,17 @@ static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
 	if (matrix_mdev->kvm) {
 		matrix_mdev->kvm_busy = true;
 		mutex_unlock(&matrix_dev->lock);
-		kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
+
+		if (matrix_mdev->kvm->arch.crypto.crycbd) {
+			down_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem);
+			matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
+			up_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem);
+
+			kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
+		}
+
 		mutex_lock(&matrix_dev->lock);
 		vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
-		matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
 		kvm_put_kvm(matrix_mdev->kvm);
 		matrix_mdev->kvm = NULL;
 		matrix_mdev->kvm_busy = false;
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index f82a6396acae..e12218e5a629 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -86,7 +86,7 @@ struct ap_matrix_mdev {
 	bool kvm_busy;
 	wait_queue_head_t wait_for_kvm;
 	struct kvm *kvm;
-	struct kvm_s390_module_hook pqap_hook;
+	crypto_hook pqap_hook;
 	struct mdev_device *mdev;
 };
 
-- 
2.30.2


^ permalink raw reply	[flat|nested] 42+ messages in thread

* [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification
  2021-07-19 19:35 [PATCH 0/2] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification Tony Krowiak
  2021-07-19 19:35 ` [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer Tony Krowiak
@ 2021-07-19 19:35 ` Tony Krowiak
  2021-07-21 14:45   ` Halil Pasic
  2021-07-21 19:37   ` Jason J. Herne
  2021-08-02 13:10 ` [PATCH 0/2] s390/vfio-ap: do not open code " Tony Krowiak
  2 siblings, 2 replies; 42+ messages in thread
From: Tony Krowiak @ 2021-07-19 19:35 UTC (permalink / raw)
  To: linux-s390, linux-kernel
  Cc: borntraeger, cohuck, pasic, jjherne, jgg, alex.williamson,
	kwankhede, david

It was pointed out during an unrelated patch review that locks should not
be open coded - i.e., writing the algorithm of a standard lock in a
function instead of using a lock from the standard library. The setting and
testing of a busy flag and sleeping on a wait_event is the same thing
a lock does. The open coded locks are invisible to lockdep, so potential
locking problems are not detected.

This patch removes the open coded locks used during
VFIO_GROUP_NOTIFY_SET_KVM notification. The busy flag
and wait queue were introduced to resolve a possible circular locking
dependency reported by lockdep when starting a secure execution guest
configured with AP adapters and domains. Reversing the order in which
the kvm->lock mutex and matrix_dev->lock mutex are locked resolves the
issue reported by lockdep, thus enabling the removal of the open coded
locks.

Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
---
 arch/s390/kvm/kvm-s390.c              |  27 +++++-
 drivers/s390/crypto/vfio_ap_ops.c     | 132 ++++++++------------------
 drivers/s390/crypto/vfio_ap_private.h |   2 -
 3 files changed, 63 insertions(+), 98 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index a08f242a9f27..4d2ef3a3286e 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2559,12 +2559,24 @@ static void kvm_s390_set_crycb_format(struct kvm *kvm)
 		kvm->arch.crypto.crycbd |= CRYCB_FORMAT1;
 }
 
+/*
+ * kvm_arch_crypto_set_masks
+ *
+ * @kvm: a pointer to the object containing the crypto masks
+ * @apm: the mask identifying the accessible AP adapters
+ * @aqm: the mask identifying the accessible AP domains
+ * @adm: the mask identifying the accessible AP control domains
+ *
+ * Set the masks that identify the adapters, domains and control domains to
+ * which the KVM guest is granted access.
+ *
+ * Note: The kvm->lock mutex must be locked by the caller.
+ */
 void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm,
 			       unsigned long *aqm, unsigned long *adm)
 {
 	struct kvm_s390_crypto_cb *crycb = kvm->arch.crypto.crycb;
 
-	mutex_lock(&kvm->lock);
 	kvm_s390_vcpu_block_all(kvm);
 
 	switch (kvm->arch.crypto.crycbd & CRYCB_FORMAT_MASK) {
@@ -2595,13 +2607,21 @@ void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm,
 	/* recreate the shadow crycb for each vcpu */
 	kvm_s390_sync_request_broadcast(kvm, KVM_REQ_VSIE_RESTART);
 	kvm_s390_vcpu_unblock_all(kvm);
-	mutex_unlock(&kvm->lock);
 }
 EXPORT_SYMBOL_GPL(kvm_arch_crypto_set_masks);
 
+/*
+ * kvm_arch_crypto_set_masks
+ *
+ * @kvm: a pointer to the object containing the crypto masks
+ *
+ * Clear the masks that identify the adapters, domains and control domains to
+ * which the KVM guest is granted access.
+ *
+ * Note: The kvm->lock mutex must be locked by the caller.
+ */
 void kvm_arch_crypto_clear_masks(struct kvm *kvm)
 {
-	mutex_lock(&kvm->lock);
 	kvm_s390_vcpu_block_all(kvm);
 
 	memset(&kvm->arch.crypto.crycb->apcb0, 0,
@@ -2613,7 +2633,6 @@ void kvm_arch_crypto_clear_masks(struct kvm *kvm)
 	/* recreate the shadow crycb for each vcpu */
 	kvm_s390_sync_request_broadcast(kvm, KVM_REQ_VSIE_RESTART);
 	kvm_s390_vcpu_unblock_all(kvm);
-	mutex_unlock(&kvm->lock);
 }
 EXPORT_SYMBOL_GPL(kvm_arch_crypto_clear_masks);
 
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 742277bc8d1c..a9c041d3b95f 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -294,15 +294,6 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
 	matrix_mdev = container_of(vcpu->kvm->arch.crypto.pqap_hook,
 				   struct ap_matrix_mdev, pqap_hook);
 
-	/*
-	 * If the KVM pointer is in the process of being set, wait until the
-	 * process has completed.
-	 */
-	wait_event_cmd(matrix_mdev->wait_for_kvm,
-		       !matrix_mdev->kvm_busy,
-		       mutex_unlock(&matrix_dev->lock),
-		       mutex_lock(&matrix_dev->lock));
-
 	/* If the there is no guest using the mdev, there is nothing to do */
 	if (!matrix_mdev->kvm)
 		goto out_unlock;
@@ -350,7 +341,6 @@ static int vfio_ap_mdev_create(struct mdev_device *mdev)
 
 	matrix_mdev->mdev = mdev;
 	vfio_ap_matrix_init(&matrix_dev->info, &matrix_mdev->matrix);
-	init_waitqueue_head(&matrix_mdev->wait_for_kvm);
 	mdev_set_drvdata(mdev, matrix_mdev);
 	matrix_mdev->pqap_hook = handle_pqap;
 	mutex_lock(&matrix_dev->lock);
@@ -619,11 +609,8 @@ static ssize_t assign_adapter_store(struct device *dev,
 
 	mutex_lock(&matrix_dev->lock);
 
-	/*
-	 * If the KVM pointer is in flux or the guest is running, disallow
-	 * un-assignment of adapter
-	 */
-	if (matrix_mdev->kvm_busy || matrix_mdev->kvm) {
+	/* If the KVM guest is running, disallow assignment of adapter */
+	if (matrix_mdev->kvm) {
 		ret = -EBUSY;
 		goto done;
 	}
@@ -692,11 +679,8 @@ static ssize_t unassign_adapter_store(struct device *dev,
 
 	mutex_lock(&matrix_dev->lock);
 
-	/*
-	 * If the KVM pointer is in flux or the guest is running, disallow
-	 * un-assignment of adapter
-	 */
-	if (matrix_mdev->kvm_busy || matrix_mdev->kvm) {
+	/* If the KVM guest is running, disallow unassignment of adapter */
+	if (matrix_mdev->kvm) {
 		ret = -EBUSY;
 		goto done;
 	}
@@ -782,11 +766,8 @@ static ssize_t assign_domain_store(struct device *dev,
 
 	mutex_lock(&matrix_dev->lock);
 
-	/*
-	 * If the KVM pointer is in flux or the guest is running, disallow
-	 * assignment of domain
-	 */
-	if (matrix_mdev->kvm_busy || matrix_mdev->kvm) {
+	/* If the KVM guest is running, disallow assignment of domain */
+	if (matrix_mdev->kvm) {
 		ret = -EBUSY;
 		goto done;
 	}
@@ -850,11 +831,8 @@ static ssize_t unassign_domain_store(struct device *dev,
 
 	mutex_lock(&matrix_dev->lock);
 
-	/*
-	 * If the KVM pointer is in flux or the guest is running, disallow
-	 * un-assignment of domain
-	 */
-	if (matrix_mdev->kvm_busy || matrix_mdev->kvm) {
+	/* If the KVM guest is running, disallow unassignment of domain */
+	if (matrix_mdev->kvm) {
 		ret = -EBUSY;
 		goto done;
 	}
@@ -904,11 +882,8 @@ static ssize_t assign_control_domain_store(struct device *dev,
 
 	mutex_lock(&matrix_dev->lock);
 
-	/*
-	 * If the KVM pointer is in flux or the guest is running, disallow
-	 * assignment of control domain.
-	 */
-	if (matrix_mdev->kvm_busy || matrix_mdev->kvm) {
+	/* If the KVM guest is running, disallow assignment of control domain */
+	if (matrix_mdev->kvm) {
 		ret = -EBUSY;
 		goto done;
 	}
@@ -963,11 +938,8 @@ static ssize_t unassign_control_domain_store(struct device *dev,
 
 	mutex_lock(&matrix_dev->lock);
 
-	/*
-	 * If the KVM pointer is in flux or the guest is running, disallow
-	 * un-assignment of control domain.
-	 */
-	if (matrix_mdev->kvm_busy || matrix_mdev->kvm) {
+	/* If a KVM guest is running, disallow unassignment of control domain */
+	if (matrix_mdev->kvm) {
 		ret = -EBUSY;
 		goto done;
 	}
@@ -1108,28 +1080,30 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
 	struct ap_matrix_mdev *m;
 
 	if (kvm->arch.crypto.crycbd) {
+		down_write(&kvm->arch.crypto.pqap_hook_rwsem);
+		kvm->arch.crypto.pqap_hook = &matrix_mdev->pqap_hook;
+		up_write(&kvm->arch.crypto.pqap_hook_rwsem);
+
+		mutex_lock(&kvm->lock);
+		mutex_lock(&matrix_dev->lock);
+
 		list_for_each_entry(m, &matrix_dev->mdev_list, node) {
-			if (m != matrix_mdev && m->kvm == kvm)
+			if (m != matrix_mdev && m->kvm == kvm) {
+				mutex_unlock(&kvm->lock);
+				mutex_unlock(&matrix_dev->lock);
 				return -EPERM;
+			}
 		}
 
 		kvm_get_kvm(kvm);
 		matrix_mdev->kvm = kvm;
-		matrix_mdev->kvm_busy = true;
-		mutex_unlock(&matrix_dev->lock);
-
-		down_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem);
-		kvm->arch.crypto.pqap_hook = &matrix_mdev->pqap_hook;
-		up_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem);
-
 		kvm_arch_crypto_set_masks(kvm,
 					  matrix_mdev->matrix.apm,
 					  matrix_mdev->matrix.aqm,
 					  matrix_mdev->matrix.adm);
 
-		mutex_lock(&matrix_dev->lock);
-		matrix_mdev->kvm_busy = false;
-		wake_up_all(&matrix_mdev->wait_for_kvm);
+		mutex_unlock(&kvm->lock);
+		mutex_unlock(&matrix_dev->lock);
 	}
 
 	return 0;
@@ -1179,35 +1153,24 @@ static int vfio_ap_mdev_iommu_notifier(struct notifier_block *nb,
  * done under the @matrix_mdev->lock.
  *
  */
-static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
+static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev,
+				   struct kvm *kvm)
 {
-	/*
-	 * If the KVM pointer is in the process of being set, wait until the
-	 * process has completed.
-	 */
-	wait_event_cmd(matrix_mdev->wait_for_kvm,
-		       !matrix_mdev->kvm_busy,
-		       mutex_unlock(&matrix_dev->lock),
-		       mutex_lock(&matrix_dev->lock));
-
-	if (matrix_mdev->kvm) {
-		matrix_mdev->kvm_busy = true;
-		mutex_unlock(&matrix_dev->lock);
-
-		if (matrix_mdev->kvm->arch.crypto.crycbd) {
-			down_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem);
-			matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
-			up_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem);
-
-			kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
-		}
+	if (kvm->arch.crypto.crycbd) {
+		down_write(&kvm->arch.crypto.pqap_hook_rwsem);
+		kvm->arch.crypto.pqap_hook = NULL;
+		up_write(&kvm->arch.crypto.pqap_hook_rwsem);
 
+		mutex_lock(&kvm->lock);
 		mutex_lock(&matrix_dev->lock);
+
+		kvm_arch_crypto_clear_masks(kvm);
 		vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
-		kvm_put_kvm(matrix_mdev->kvm);
+		kvm_put_kvm(kvm);
 		matrix_mdev->kvm = NULL;
-		matrix_mdev->kvm_busy = false;
-		wake_up_all(&matrix_mdev->wait_for_kvm);
+
+		mutex_unlock(&kvm->lock);
+		mutex_unlock(&matrix_dev->lock);
 	}
 }
 
@@ -1220,16 +1183,13 @@ static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
 	if (action != VFIO_GROUP_NOTIFY_SET_KVM)
 		return NOTIFY_OK;
 
-	mutex_lock(&matrix_dev->lock);
 	matrix_mdev = container_of(nb, struct ap_matrix_mdev, group_notifier);
 
 	if (!data)
-		vfio_ap_mdev_unset_kvm(matrix_mdev);
+		vfio_ap_mdev_unset_kvm(matrix_mdev, matrix_mdev->kvm);
 	else if (vfio_ap_mdev_set_kvm(matrix_mdev, data))
 		notify_rc = NOTIFY_DONE;
 
-	mutex_unlock(&matrix_dev->lock);
-
 	return notify_rc;
 }
 
@@ -1363,14 +1323,11 @@ static void vfio_ap_mdev_release(struct mdev_device *mdev)
 {
 	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
 
-	mutex_lock(&matrix_dev->lock);
-	vfio_ap_mdev_unset_kvm(matrix_mdev);
-	mutex_unlock(&matrix_dev->lock);
-
 	vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
 				 &matrix_mdev->iommu_notifier);
 	vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
 				 &matrix_mdev->group_notifier);
+	vfio_ap_mdev_unset_kvm(matrix_mdev, matrix_mdev->kvm);
 	module_put(THIS_MODULE);
 }
 
@@ -1412,15 +1369,6 @@ static ssize_t vfio_ap_mdev_ioctl(struct mdev_device *mdev,
 			break;
 		}
 
-		/*
-		 * If the KVM pointer is in the process of being set, wait until
-		 * the process has completed.
-		 */
-		wait_event_cmd(matrix_mdev->wait_for_kvm,
-			       !matrix_mdev->kvm_busy,
-			       mutex_unlock(&matrix_dev->lock),
-			       mutex_lock(&matrix_dev->lock));
-
 		ret = vfio_ap_mdev_reset_queues(mdev);
 		break;
 	default:
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index e12218e5a629..22d2e0ca3ae5 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -83,8 +83,6 @@ struct ap_matrix_mdev {
 	struct ap_matrix matrix;
 	struct notifier_block group_notifier;
 	struct notifier_block iommu_notifier;
-	bool kvm_busy;
-	wait_queue_head_t wait_for_kvm;
 	struct kvm *kvm;
 	crypto_hook pqap_hook;
 	struct mdev_device *mdev;
-- 
2.30.2


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification
  2021-07-19 19:35 ` [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification Tony Krowiak
@ 2021-07-21 14:45   ` Halil Pasic
  2021-07-22 13:09     ` Tony Krowiak
  2021-07-21 19:37   ` Jason J. Herne
  1 sibling, 1 reply; 42+ messages in thread
From: Halil Pasic @ 2021-07-21 14:45 UTC (permalink / raw)
  To: Tony Krowiak
  Cc: linux-s390, linux-kernel, borntraeger, cohuck, pasic, jjherne,
	jgg, alex.williamson, kwankhede, david

On Mon, 19 Jul 2021 15:35:03 -0400
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> It was pointed out during an unrelated patch review that locks should not

[..]

> -static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
> +static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev,
> +				   struct kvm *kvm)
>  {
> -	/*
> -	 * If the KVM pointer is in the process of being set, wait until the
> -	 * process has completed.
> -	 */
> -	wait_event_cmd(matrix_mdev->wait_for_kvm,
> -		       !matrix_mdev->kvm_busy,
> -		       mutex_unlock(&matrix_dev->lock),
> -		       mutex_lock(&matrix_dev->lock));
> -
> -	if (matrix_mdev->kvm) {

We used to check if matrix_mdev->kvm is null, but ...

> -		matrix_mdev->kvm_busy = true;
> -		mutex_unlock(&matrix_dev->lock);
> -
> -		if (matrix_mdev->kvm->arch.crypto.crycbd) {
> -			down_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem);
> -			matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
> -			up_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem);
> -
> -			kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
> -		}
> +	if (kvm->arch.crypto.crycbd) {

... now we just try to dereference it. And ..

> +		down_write(&kvm->arch.crypto.pqap_hook_rwsem);
> +		kvm->arch.crypto.pqap_hook = NULL;
> +		up_write(&kvm->arch.crypto.pqap_hook_rwsem);
> 
> +		mutex_lock(&kvm->lock);
>  		mutex_lock(&matrix_dev->lock);
> +
> +		kvm_arch_crypto_clear_masks(kvm);
>  		vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
> -		kvm_put_kvm(matrix_mdev->kvm);
> +		kvm_put_kvm(kvm);
>  		matrix_mdev->kvm = NULL;
> -		matrix_mdev->kvm_busy = false;
> -		wake_up_all(&matrix_mdev->wait_for_kvm);
> +
> +		mutex_unlock(&kvm->lock);
> +		mutex_unlock(&matrix_dev->lock);
>  	}
>  }
> 

[..]

> @@ -1363,14 +1323,11 @@ static void vfio_ap_mdev_release(struct mdev_device *mdev)
>  {
>  	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
> 
> -	mutex_lock(&matrix_dev->lock);
> -	vfio_ap_mdev_unset_kvm(matrix_mdev);
> -	mutex_unlock(&matrix_dev->lock);
> -

.. before access to the matrix_mdev->kvm used to be protected by
the matrix_dev->lock ...

>  	vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
>  				 &matrix_mdev->iommu_notifier);
>  	vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
>  				 &matrix_mdev->group_notifier);
> +	vfio_ap_mdev_unset_kvm(matrix_mdev, matrix_mdev->kvm);

... but it is not any more. BTW I don't think the code is guaranteed
to fetch ->kvm just once.

Can you please explain why can we get away with being more
lax when dealing with matrix_mdev->kvm?

Regards,
Halil

[..]

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification
  2021-07-19 19:35 ` [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification Tony Krowiak
  2021-07-21 14:45   ` Halil Pasic
@ 2021-07-21 19:37   ` Jason J. Herne
  2021-07-22 13:16     ` Tony Krowiak
  1 sibling, 1 reply; 42+ messages in thread
From: Jason J. Herne @ 2021-07-21 19:37 UTC (permalink / raw)
  To: Tony Krowiak, linux-s390, linux-kernel
  Cc: borntraeger, cohuck, pasic, jgg, alex.williamson, kwankhede, david

On 7/19/21 3:35 PM, Tony Krowiak wrote:
> It was pointed out during an unrelated patch review that locks should not
> be open coded - i.e., writing the algorithm of a standard lock in a
> function instead of using a lock from the standard library. The setting and
> testing of a busy flag and sleeping on a wait_event is the same thing
> a lock does. The open coded locks are invisible to lockdep, so potential
> locking problems are not detected.
> 
> This patch removes the open coded locks used during
> VFIO_GROUP_NOTIFY_SET_KVM notification. The busy flag
> and wait queue were introduced to resolve a possible circular locking
> dependency reported by lockdep when starting a secure execution guest
> configured with AP adapters and domains. Reversing the order in which
> the kvm->lock mutex and matrix_dev->lock mutex are locked resolves the
> issue reported by lockdep, thus enabling the removal of the open coded
> locks.
> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> ---
>   arch/s390/kvm/kvm-s390.c              |  27 +++++-
>   drivers/s390/crypto/vfio_ap_ops.c     | 132 ++++++++------------------
>   drivers/s390/crypto/vfio_ap_private.h |   2 -
>   3 files changed, 63 insertions(+), 98 deletions(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index a08f242a9f27..4d2ef3a3286e 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2559,12 +2559,24 @@ static void kvm_s390_set_crycb_format(struct kvm *kvm)
>   		kvm->arch.crypto.crycbd |= CRYCB_FORMAT1;
>   }
>   
> +/*
> + * kvm_arch_crypto_set_masks
> + *
> + * @kvm: a pointer to the object containing the crypto masks

This should probably say "a pointer to the target guest's KVM struct" or something to that 
effect. Same comment for the comment above kvm_arch_crypto_clear_masks.

> + * @apm: the mask identifying the accessible AP adapters
> + * @aqm: the mask identifying the accessible AP domains
> + * @adm: the mask identifying the accessible AP control domains
> + *
> + * Set the masks that identify the adapters, domains and control domains to
> + * which the KVM guest is granted access.
> + *
> + * Note: The kvm->lock mutex must be locked by the caller.
> + */
>   void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm,
>   			       unsigned long *aqm, unsigned long *adm)
>   {
>   	struct kvm_s390_crypto_cb *crycb = kvm->arch.crypto.crycb;
>   
> -	mutex_lock(&kvm->lock);
>   	kvm_s390_vcpu_block_all(kvm);
>   
>   	switch (kvm->arch.crypto.crycbd & CRYCB_FORMAT_MASK) {
> @@ -2595,13 +2607,21 @@ void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm,
>   	/* recreate the shadow crycb for each vcpu */
>   	kvm_s390_sync_request_broadcast(kvm, KVM_REQ_VSIE_RESTART);
>   	kvm_s390_vcpu_unblock_all(kvm);
> -	mutex_unlock(&kvm->lock);
>   }
>   EXPORT_SYMBOL_GPL(kvm_arch_crypto_set_masks);
>   
> +/*
> + * kvm_arch_crypto_set_masks

Copy/paste error here. Rename to kvm_arch_crypto_CLEAR_masks

I did not find anything else in my review. However, I'm still very new to this code, so 
take that with a grain of salt :).

Also, I could not apply this to master. If there is a next version do you mind rebasing? 
Seeing the patch in full context would be helpful.


-- 
-- Jason J. Herne (jjherne@linux.ibm.com)

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification
  2021-07-21 14:45   ` Halil Pasic
@ 2021-07-22 13:09     ` Tony Krowiak
  2021-07-23 14:26       ` Halil Pasic
  0 siblings, 1 reply; 42+ messages in thread
From: Tony Krowiak @ 2021-07-22 13:09 UTC (permalink / raw)
  To: Halil Pasic
  Cc: linux-s390, linux-kernel, borntraeger, cohuck, pasic, jjherne,
	jgg, alex.williamson, kwankhede, david



On 7/21/21 10:45 AM, Halil Pasic wrote:
> On Mon, 19 Jul 2021 15:35:03 -0400
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>
>> It was pointed out during an unrelated patch review that locks should not
> [..]
>
>> -static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
>> +static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev,
>> +				   struct kvm *kvm)
>>   {
>> -	/*
>> -	 * If the KVM pointer is in the process of being set, wait until the
>> -	 * process has completed.
>> -	 */
>> -	wait_event_cmd(matrix_mdev->wait_for_kvm,
>> -		       !matrix_mdev->kvm_busy,
>> -		       mutex_unlock(&matrix_dev->lock),
>> -		       mutex_lock(&matrix_dev->lock));
>> -
>> -	if (matrix_mdev->kvm) {
> We used to check if matrix_mdev->kvm is null, but ...
>
>> -		matrix_mdev->kvm_busy = true;
>> -		mutex_unlock(&matrix_dev->lock);
>> -
>> -		if (matrix_mdev->kvm->arch.crypto.crycbd) {
>> -			down_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem);
>> -			matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
>> -			up_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem);
>> -
>> -			kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
>> -		}
>> +	if (kvm->arch.crypto.crycbd) {
> ... now we just try to dereference it. And ..

We used to check matrix_mdev->kvm, now the kvm pointer is passed into
the function; however, having said that, the pointer passed in should be
checked before de-referencing it.

>
>> +		down_write(&kvm->arch.crypto.pqap_hook_rwsem);
>> +		kvm->arch.crypto.pqap_hook = NULL;
>> +		up_write(&kvm->arch.crypto.pqap_hook_rwsem);
>>
>> +		mutex_lock(&kvm->lock);
>>   		mutex_lock(&matrix_dev->lock);
>> +
>> +		kvm_arch_crypto_clear_masks(kvm);
>>   		vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
>> -		kvm_put_kvm(matrix_mdev->kvm);
>> +		kvm_put_kvm(kvm);
>>   		matrix_mdev->kvm = NULL;
>> -		matrix_mdev->kvm_busy = false;
>> -		wake_up_all(&matrix_mdev->wait_for_kvm);
>> +
>> +		mutex_unlock(&kvm->lock);
>> +		mutex_unlock(&matrix_dev->lock);
>>   	}
>>   }
>>
> [..]
>
>> @@ -1363,14 +1323,11 @@ static void vfio_ap_mdev_release(struct mdev_device *mdev)
>>   {
>>   	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
>>
>> -	mutex_lock(&matrix_dev->lock);
>> -	vfio_ap_mdev_unset_kvm(matrix_mdev);
>> -	mutex_unlock(&matrix_dev->lock);
>> -
> .. before access to the matrix_mdev->kvm used to be protected by
> the matrix_dev->lock ...
>
>>   	vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
>>   				 &matrix_mdev->iommu_notifier);
>>   	vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
>>   				 &matrix_mdev->group_notifier);
>> +	vfio_ap_mdev_unset_kvm(matrix_mdev, matrix_mdev->kvm);
> ... but it is not any more. BTW I don't think the code is guaranteed
> to fetch ->kvm just once.

There are a couple of things to point out here:
1. The vfio_ap_mdev_unset_kvm function() is the only place where the
     matrix_mdev->kvm pointer is cleared. That function is called here
     as well as by the group notifier callback for VFIO_GROUP_NOTIFY_SET_KVM
     events. If you notice in the code above, the group notifier is 
unregistered
     before calling the unset function, so either the notifier will have 
already
     been invoked and the pointer cleared (which is why you are correct
     that the KVM pointer passed in needs to get checked in the unset 
function),
     or will get cleared here.
2. The release callback is invoked when the mdev fd is closed by userspace.
     The remove callback is the only place where the matrix_mdev is 
freed. The
     remove callback is not called until the mdev fd is released, so it 
is guaranteed
     the matrix_mdev will exist when the release callback is invoked.
3. The matrix_dev->lock is then taken in the vfio_ap_mdev_unset_kvm function
     before doing any operations that modify the matrix_mdev.

> Can you please explain why can we get away with being more
> lax when dealing with matrix_mdev->kvm?

See above.

>
> Regards,
> Halil
>
> [..]


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification
  2021-07-21 19:37   ` Jason J. Herne
@ 2021-07-22 13:16     ` Tony Krowiak
  0 siblings, 0 replies; 42+ messages in thread
From: Tony Krowiak @ 2021-07-22 13:16 UTC (permalink / raw)
  To: jjherne, linux-s390, linux-kernel
  Cc: borntraeger, cohuck, pasic, jgg, alex.williamson, kwankhede, david



On 7/21/21 3:37 PM, Jason J. Herne wrote:
> On 7/19/21 3:35 PM, Tony Krowiak wrote:
>> It was pointed out during an unrelated patch review that locks should 
>> not
>> be open coded - i.e., writing the algorithm of a standard lock in a
>> function instead of using a lock from the standard library. The 
>> setting and
>> testing of a busy flag and sleeping on a wait_event is the same thing
>> a lock does. The open coded locks are invisible to lockdep, so potential
>> locking problems are not detected.
>>
>> This patch removes the open coded locks used during
>> VFIO_GROUP_NOTIFY_SET_KVM notification. The busy flag
>> and wait queue were introduced to resolve a possible circular locking
>> dependency reported by lockdep when starting a secure execution guest
>> configured with AP adapters and domains. Reversing the order in which
>> the kvm->lock mutex and matrix_dev->lock mutex are locked resolves the
>> issue reported by lockdep, thus enabling the removal of the open coded
>> locks.
>>
>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>> ---
>>   arch/s390/kvm/kvm-s390.c              |  27 +++++-
>>   drivers/s390/crypto/vfio_ap_ops.c     | 132 ++++++++------------------
>>   drivers/s390/crypto/vfio_ap_private.h |   2 -
>>   3 files changed, 63 insertions(+), 98 deletions(-)
>>
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index a08f242a9f27..4d2ef3a3286e 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -2559,12 +2559,24 @@ static void kvm_s390_set_crycb_format(struct 
>> kvm *kvm)
>>           kvm->arch.crypto.crycbd |= CRYCB_FORMAT1;
>>   }
>>   +/*
>> + * kvm_arch_crypto_set_masks
>> + *
>> + * @kvm: a pointer to the object containing the crypto masks
>
> This should probably say "a pointer to the target guest's KVM struct" 
> or something to that effect. Same comment for the comment above 
> kvm_arch_crypto_clear_masks.

Will do.

>
>> + * @apm: the mask identifying the accessible AP adapters
>> + * @aqm: the mask identifying the accessible AP domains
>> + * @adm: the mask identifying the accessible AP control domains
>> + *
>> + * Set the masks that identify the adapters, domains and control 
>> domains to
>> + * which the KVM guest is granted access.
>> + *
>> + * Note: The kvm->lock mutex must be locked by the caller.
>> + */
>>   void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm,
>>                      unsigned long *aqm, unsigned long *adm)
>>   {
>>       struct kvm_s390_crypto_cb *crycb = kvm->arch.crypto.crycb;
>>   -    mutex_lock(&kvm->lock);
>>       kvm_s390_vcpu_block_all(kvm);
>>         switch (kvm->arch.crypto.crycbd & CRYCB_FORMAT_MASK) {
>> @@ -2595,13 +2607,21 @@ void kvm_arch_crypto_set_masks(struct kvm 
>> *kvm, unsigned long *apm,
>>       /* recreate the shadow crycb for each vcpu */
>>       kvm_s390_sync_request_broadcast(kvm, KVM_REQ_VSIE_RESTART);
>>       kvm_s390_vcpu_unblock_all(kvm);
>> -    mutex_unlock(&kvm->lock);
>>   }
>>   EXPORT_SYMBOL_GPL(kvm_arch_crypto_set_masks);
>>   +/*
>> + * kvm_arch_crypto_set_masks
>
> Copy/paste error here. Rename to kvm_arch_crypto_CLEAR_masks

I will fix it.

>
> I did not find anything else in my review. However, I'm still very new 
> to this code, so take that with a grain of salt :).

The grain of salt has been ingested.

>
> Also, I could not apply this to master. If there is a next version do 
> you mind rebasing? Seeing the patch in full context would be helpful.

This was rebased on the latest master branch at the time then re-tested. 
This is something I always
do prior to submitting patches, so is it possible you were using an 
older version of master?

>
>
>


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification
  2021-07-22 13:09     ` Tony Krowiak
@ 2021-07-23 14:26       ` Halil Pasic
  2021-07-23 21:24         ` Tony Krowiak
  0 siblings, 1 reply; 42+ messages in thread
From: Halil Pasic @ 2021-07-23 14:26 UTC (permalink / raw)
  To: Tony Krowiak
  Cc: linux-s390, linux-kernel, borntraeger, cohuck, pasic, jjherne,
	jgg, alex.williamson, kwankhede, david

On Thu, 22 Jul 2021 09:09:26 -0400
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> On 7/21/21 10:45 AM, Halil Pasic wrote:
> > On Mon, 19 Jul 2021 15:35:03 -0400
> > Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> >  
> >> It was pointed out during an unrelated patch review that locks should not  
> > [..]
> >  
> >> -static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
> >> +static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev,
> >> +				   struct kvm *kvm)
> >>   {
> >> -	/*
> >> -	 * If the KVM pointer is in the process of being set, wait until the
> >> -	 * process has completed.
> >> -	 */
> >> -	wait_event_cmd(matrix_mdev->wait_for_kvm,
> >> -		       !matrix_mdev->kvm_busy,
> >> -		       mutex_unlock(&matrix_dev->lock),
> >> -		       mutex_lock(&matrix_dev->lock));
> >> -
> >> -	if (matrix_mdev->kvm) {  
> > We used to check if matrix_mdev->kvm is null, but ...
> >  
> >> -		matrix_mdev->kvm_busy = true;
> >> -		mutex_unlock(&matrix_dev->lock);
> >> -
> >> -		if (matrix_mdev->kvm->arch.crypto.crycbd) {
> >> -			down_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem);
> >> -			matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
> >> -			up_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem);
> >> -
> >> -			kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
> >> -		}
> >> +	if (kvm->arch.crypto.crycbd) {  
> > ... now we just try to dereference it. And ..  
> 
> We used to check matrix_mdev->kvm, now the kvm pointer is passed into
> the function; however, having said that, the pointer passed in should be
> checked before de-referencing it.
> 
> >  
> >> +		down_write(&kvm->arch.crypto.pqap_hook_rwsem);
> >> +		kvm->arch.crypto.pqap_hook = NULL;
> >> +		up_write(&kvm->arch.crypto.pqap_hook_rwsem);
> >>
> >> +		mutex_lock(&kvm->lock);
> >>   		mutex_lock(&matrix_dev->lock);
> >> +
> >> +		kvm_arch_crypto_clear_masks(kvm);
> >>   		vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
> >> -		kvm_put_kvm(matrix_mdev->kvm);
> >> +		kvm_put_kvm(kvm);
> >>   		matrix_mdev->kvm = NULL;
> >> -		matrix_mdev->kvm_busy = false;
> >> -		wake_up_all(&matrix_mdev->wait_for_kvm);
> >> +
> >> +		mutex_unlock(&kvm->lock);
> >> +		mutex_unlock(&matrix_dev->lock);
> >>   	}
> >>   }
> >>  
> > [..]
> >  
> >> @@ -1363,14 +1323,11 @@ static void vfio_ap_mdev_release(struct mdev_device *mdev)
> >>   {
> >>   	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
> >>
> >> -	mutex_lock(&matrix_dev->lock);
> >> -	vfio_ap_mdev_unset_kvm(matrix_mdev);
> >> -	mutex_unlock(&matrix_dev->lock);
> >> -  
> > .. before access to the matrix_mdev->kvm used to be protected by
> > the matrix_dev->lock ...
> >  
> >>   	vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
> >>   				 &matrix_mdev->iommu_notifier);
> >>   	vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
> >>   				 &matrix_mdev->group_notifier);
> >> +	vfio_ap_mdev_unset_kvm(matrix_mdev, matrix_mdev->kvm);  
> > ... but it is not any more. BTW I don't think the code is guaranteed
> > to fetch ->kvm just once.  
> 
> There are a couple of things to point out here:
> 1. The vfio_ap_mdev_unset_kvm function() is the only place where the
>      matrix_mdev->kvm pointer is cleared. That function is called here
>      as well as by the group notifier callback for VFIO_GROUP_NOTIFY_SET_KVM
>      events. If you notice in the code above, the group notifier is 
> unregistered
>      before calling the unset function, so either the notifier will have 
> already
>      been invoked and the pointer cleared (which is why you are correct
>      that the KVM pointer passed in needs to get checked in the unset 
> function),
>      or will get cleared here.

Hm, vfio_unregister_notifier() indeed seems to guarantee, that by the
time it returns no notifer is running. I didn't know that. But this
blocking notifier chain uses an rwsem. So mutual exclusion with
vfio_ap_mdev_open() is guaranteed, than it is indeed guaranteed. A quick
glance at the code didn't tell me if vfio/mdev guarantees that. 

I mean it would make sense to me to make the init and the cleanup
mutually exclusive, but I'm reluctant to just assume it is like that.
Can you please point me into the right direction?


> 2. The release callback is invoked when the mdev fd is closed by userspace.
>      The remove callback is the only place where the matrix_mdev is 
> freed. The
>      remove callback is not called until the mdev fd is released, so it 
> is guaranteed
>      the matrix_mdev will exist when the release callback is invoked.
> 3. The matrix_dev->lock is then taken in the vfio_ap_mdev_unset_kvm function
>      before doing any operations that modify the matrix_mdev.

Yeah but both the reader, and the writer needs to use the same lock to
have the protected by the lock type of situation. That is why I asked
about the place where you read matrix_mdev members outside the
matrix_dev->lock.

Regards,
Halil

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification
  2021-07-23 14:26       ` Halil Pasic
@ 2021-07-23 21:24         ` Tony Krowiak
  2021-07-26 20:36           ` Halil Pasic
  0 siblings, 1 reply; 42+ messages in thread
From: Tony Krowiak @ 2021-07-23 21:24 UTC (permalink / raw)
  To: Halil Pasic
  Cc: linux-s390, linux-kernel, borntraeger, cohuck, pasic, jjherne,
	jgg, alex.williamson, kwankhede, david



On 7/23/21 10:26 AM, Halil Pasic wrote:
> On Thu, 22 Jul 2021 09:09:26 -0400
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>
>> On 7/21/21 10:45 AM, Halil Pasic wrote:
>>> On Mon, 19 Jul 2021 15:35:03 -0400
>>> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>>>   
>>>> It was pointed out during an unrelated patch review that locks should not
>>> [..]
>>>   
>>>> -static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
>>>> +static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev,
>>>> +				   struct kvm *kvm)
>>>>    {
>>>> -	/*
>>>> -	 * If the KVM pointer is in the process of being set, wait until the
>>>> -	 * process has completed.
>>>> -	 */
>>>> -	wait_event_cmd(matrix_mdev->wait_for_kvm,
>>>> -		       !matrix_mdev->kvm_busy,
>>>> -		       mutex_unlock(&matrix_dev->lock),
>>>> -		       mutex_lock(&matrix_dev->lock));
>>>> -
>>>> -	if (matrix_mdev->kvm) {
>>> We used to check if matrix_mdev->kvm is null, but ...
>>>   
>>>> -		matrix_mdev->kvm_busy = true;
>>>> -		mutex_unlock(&matrix_dev->lock);
>>>> -
>>>> -		if (matrix_mdev->kvm->arch.crypto.crycbd) {
>>>> -			down_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem);
>>>> -			matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
>>>> -			up_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem);
>>>> -
>>>> -			kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
>>>> -		}
>>>> +	if (kvm->arch.crypto.crycbd) {
>>> ... now we just try to dereference it. And ..
>> We used to check matrix_mdev->kvm, now the kvm pointer is passed into
>> the function; however, having said that, the pointer passed in should be
>> checked before de-referencing it.
>>
>>>   
>>>> +		down_write(&kvm->arch.crypto.pqap_hook_rwsem);
>>>> +		kvm->arch.crypto.pqap_hook = NULL;
>>>> +		up_write(&kvm->arch.crypto.pqap_hook_rwsem);
>>>>
>>>> +		mutex_lock(&kvm->lock);
>>>>    		mutex_lock(&matrix_dev->lock);
>>>> +
>>>> +		kvm_arch_crypto_clear_masks(kvm);
>>>>    		vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
>>>> -		kvm_put_kvm(matrix_mdev->kvm);
>>>> +		kvm_put_kvm(kvm);
>>>>    		matrix_mdev->kvm = NULL;
>>>> -		matrix_mdev->kvm_busy = false;
>>>> -		wake_up_all(&matrix_mdev->wait_for_kvm);
>>>> +
>>>> +		mutex_unlock(&kvm->lock);
>>>> +		mutex_unlock(&matrix_dev->lock);
>>>>    	}
>>>>    }
>>>>   
>>> [..]
>>>   
>>>> @@ -1363,14 +1323,11 @@ static void vfio_ap_mdev_release(struct mdev_device *mdev)
>>>>    {
>>>>    	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
>>>>
>>>> -	mutex_lock(&matrix_dev->lock);
>>>> -	vfio_ap_mdev_unset_kvm(matrix_mdev);
>>>> -	mutex_unlock(&matrix_dev->lock);
>>>> -
>>> .. before access to the matrix_mdev->kvm used to be protected by
>>> the matrix_dev->lock ...
>>>   
>>>>    	vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
>>>>    				 &matrix_mdev->iommu_notifier);
>>>>    	vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
>>>>    				 &matrix_mdev->group_notifier);
>>>> +	vfio_ap_mdev_unset_kvm(matrix_mdev, matrix_mdev->kvm);
>>> ... but it is not any more. BTW I don't think the code is guaranteed
>>> to fetch ->kvm just once.
>> There are a couple of things to point out here:
>> 1. The vfio_ap_mdev_unset_kvm function() is the only place where the
>>       matrix_mdev->kvm pointer is cleared. That function is called here
>>       as well as by the group notifier callback for VFIO_GROUP_NOTIFY_SET_KVM
>>       events. If you notice in the code above, the group notifier is
>> unregistered
>>       before calling the unset function, so either the notifier will have
>> already
>>       been invoked and the pointer cleared (which is why you are correct
>>       that the KVM pointer passed in needs to get checked in the unset
>> function),
>>       or will get cleared here.
> Hm, vfio_unregister_notifier() indeed seems to guarantee, that by the
> time it returns no notifer is running. I didn't know that. But this
> blocking notifier chain uses an rwsem. So mutual exclusion with
> vfio_ap_mdev_open() is guaranteed, than it is indeed guaranteed. A quick
> glance at the code didn't tell me if vfio/mdev guarantees that.
>
> I mean it would make sense to me to make the init and the cleanup
> mutually exclusive, but I'm reluctant to just assume it is like that.
> Can you please point me into the right direction?

I'm not quite sure what you're asking for here, but I'll give it a
shot. The notifier is registered by the vfio_ap_mdev_open()
callback which is invoked in response to the opening of the mdev fd.
Since mdev fd can't be closed unless and until it's open,
there is no way for the vfio_ap_mdev_release() callback, which
is invoked when the mdev fd is closed, to execute at the same
time as the vfio_ap_mdev_open callback. Since the release
callback unregisters the notifier and both the registration and
unregistration are done under the same rwsem, I don't see how
they can be anything but mutually exclusive. Am I missing something
here?

>
>
>> 2. The release callback is invoked when the mdev fd is closed by userspace.
>>       The remove callback is the only place where the matrix_mdev is
>> freed. The
>>       remove callback is not called until the mdev fd is released, so it
>> is guaranteed
>>       the matrix_mdev will exist when the release callback is invoked.
>> 3. The matrix_dev->lock is then taken in the vfio_ap_mdev_unset_kvm function
>>       before doing any operations that modify the matrix_mdev.
> Yeah but both the reader, and the writer needs to use the same lock to
> have the protected by the lock type of situation. That is why I asked
> about the place where you read matrix_mdev members outside the
> matrix_dev->lock.

With this patch, the matrix_mdev is always written or read with
the matrix_dev->lock mutex locked. By moving the locking of the
kvm->lock mutex out of the functions that set/clear the masks
in the guest's CRYCB, we are now able to lock the kvm->lock
mutex before locking the matrix_dev->lock mutex in both the
vfio_ap_mdev_set_kvm() and vfio_ap_mdev_unset_kvm()
functions. This precludes the need to unlock the
matrix_dev->lock mutex while the masks are being set or
cleared and alleviates the lockdep splat for which the open coded
locks were created.

>
> Regards,
> Halil


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification
  2021-07-23 21:24         ` Tony Krowiak
@ 2021-07-26 20:36           ` Halil Pasic
  2021-07-26 22:03             ` Jason Gunthorpe
  0 siblings, 1 reply; 42+ messages in thread
From: Halil Pasic @ 2021-07-26 20:36 UTC (permalink / raw)
  To: Tony Krowiak
  Cc: linux-s390, linux-kernel, borntraeger, cohuck, pasic, jjherne,
	jgg, alex.williamson, kwankhede, david

On Fri, 23 Jul 2021 17:24:16 -0400
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> On 7/23/21 10:26 AM, Halil Pasic wrote:
> > On Thu, 22 Jul 2021 09:09:26 -0400
> > Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> >  
> >> On 7/21/21 10:45 AM, Halil Pasic wrote:  
> >>> On Mon, 19 Jul 2021 15:35:03 -0400
> >>> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> >>>     
> >>>> It was pointed out during an unrelated patch review that locks should not  
> >>> [..]
> >>>     
> >>>> -static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
> >>>> +static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev,
> >>>> +				   struct kvm *kvm)
> >>>>    {
> >>>> -	/*
> >>>> -	 * If the KVM pointer is in the process of being set, wait until the
> >>>> -	 * process has completed.
> >>>> -	 */
> >>>> -	wait_event_cmd(matrix_mdev->wait_for_kvm,
> >>>> -		       !matrix_mdev->kvm_busy,
> >>>> -		       mutex_unlock(&matrix_dev->lock),
> >>>> -		       mutex_lock(&matrix_dev->lock));
> >>>> -
> >>>> -	if (matrix_mdev->kvm) {  
> >>> We used to check if matrix_mdev->kvm is null, but ...
> >>>     
> >>>> -		matrix_mdev->kvm_busy = true;
> >>>> -		mutex_unlock(&matrix_dev->lock);
> >>>> -
> >>>> -		if (matrix_mdev->kvm->arch.crypto.crycbd) {
> >>>> -			down_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem);
> >>>> -			matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
> >>>> -			up_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem);
> >>>> -
> >>>> -			kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
> >>>> -		}
> >>>> +	if (kvm->arch.crypto.crycbd) {  
> >>> ... now we just try to dereference it. And ..  
> >> We used to check matrix_mdev->kvm, now the kvm pointer is passed into
> >> the function; however, having said that, the pointer passed in should be
> >> checked before de-referencing it.
> >>  
> >>>     
> >>>> +		down_write(&kvm->arch.crypto.pqap_hook_rwsem);
> >>>> +		kvm->arch.crypto.pqap_hook = NULL;
> >>>> +		up_write(&kvm->arch.crypto.pqap_hook_rwsem);
> >>>>
> >>>> +		mutex_lock(&kvm->lock);
> >>>>    		mutex_lock(&matrix_dev->lock);
> >>>> +
> >>>> +		kvm_arch_crypto_clear_masks(kvm);
> >>>>    		vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
> >>>> -		kvm_put_kvm(matrix_mdev->kvm);
> >>>> +		kvm_put_kvm(kvm);
> >>>>    		matrix_mdev->kvm = NULL;
> >>>> -		matrix_mdev->kvm_busy = false;
> >>>> -		wake_up_all(&matrix_mdev->wait_for_kvm);
> >>>> +
> >>>> +		mutex_unlock(&kvm->lock);
> >>>> +		mutex_unlock(&matrix_dev->lock);
> >>>>    	}
> >>>>    }
> >>>>     
> >>> [..]
> >>>     
> >>>> @@ -1363,14 +1323,11 @@ static void vfio_ap_mdev_release(struct mdev_device *mdev)
> >>>>    {
> >>>>    	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
> >>>>
> >>>> -	mutex_lock(&matrix_dev->lock);
> >>>> -	vfio_ap_mdev_unset_kvm(matrix_mdev);
> >>>> -	mutex_unlock(&matrix_dev->lock);
> >>>> -  
> >>> .. before access to the matrix_mdev->kvm used to be protected by
> >>> the matrix_dev->lock ...
> >>>     
> >>>>    	vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
> >>>>    				 &matrix_mdev->iommu_notifier);
> >>>>    	vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
> >>>>    				 &matrix_mdev->group_notifier);
> >>>> +	vfio_ap_mdev_unset_kvm(matrix_mdev, matrix_mdev->kvm);  
> >>> ... but it is not any more. BTW I don't think the code is guaranteed
> >>> to fetch ->kvm just once.  
> >> There are a couple of things to point out here:
> >> 1. The vfio_ap_mdev_unset_kvm function() is the only place where the
> >>       matrix_mdev->kvm pointer is cleared. That function is called here
> >>       as well as by the group notifier callback for VFIO_GROUP_NOTIFY_SET_KVM
> >>       events. If you notice in the code above, the group notifier is
> >> unregistered
> >>       before calling the unset function, so either the notifier will have
> >> already
> >>       been invoked and the pointer cleared (which is why you are correct
> >>       that the KVM pointer passed in needs to get checked in the unset
> >> function),
> >>       or will get cleared here.  
> > Hm, vfio_unregister_notifier() indeed seems to guarantee, that by the
> > time it returns no notifer is running. I didn't know that. But this
> > blocking notifier chain uses an rwsem. So mutual exclusion with
> > vfio_ap_mdev_open() is guaranteed, than it is indeed guaranteed. A quick
> > glance at the code didn't tell me if vfio/mdev guarantees that.
> >
> > I mean it would make sense to me to make the init and the cleanup
> > mutually exclusive, but I'm reluctant to just assume it is like that.
> > Can you please point me into the right direction?  
> 
> I'm not quite sure what you're asking for here, but I'll give it a
> shot. The notifier is registered by the vfio_ap_mdev_open()
> callback which is invoked in response to the opening of the mdev fd.

vfio_ap_mdev_open <-vfio_group_get_device_fd

> Since mdev fd can't be closed unless and until it's open,
> there is no way for the vfio_ap_mdev_release() callback, which
> is invoked when the mdev fd is closed, to execute at the same
> time as the vfio_ap_mdev_open callback. 

A plain old file you can open several times over (and thus close
several times over as well). So if you imagine something like:
thread A               thread B
--------               --------
open()
close()                open()
open()                 close()
close()

You may end up with open and close running interleaved. What I'
trying to say is, to my best knowledge, normally there is no
you have to close it before you open it again rule for files.

Does that make sense?

If there is no special mechanism to prevent such a scenario for the
mdev device, I guess, the second open (B) if you like would try to
register a group notifier. I'm not aware of such an mechanism. I 
had a brief look at vfio_group_get_device_fd but couldn't find any
such logic there.

Maybe Connie can actually help us with this one?



> Since the release
> callback unregisters the notifier and both the registration and
> unregistration are done under the same rwsem, I don't see how
> they can be anything but mutually exclusive. Am I missing something
> here?
> 
> >
> >  
> >> 2. The release callback is invoked when the mdev fd is closed by userspace.
> >>       The remove callback is the only place where the matrix_mdev is
> >> freed. The
> >>       remove callback is not called until the mdev fd is released, so it
> >> is guaranteed
> >>       the matrix_mdev will exist when the release callback is invoked.
> >> 3. The matrix_dev->lock is then taken in the vfio_ap_mdev_unset_kvm function
> >>       before doing any operations that modify the matrix_mdev.  
> > Yeah but both the reader, and the writer needs to use the same lock to
> > have the protected by the lock type of situation. That is why I asked
> > about the place where you read matrix_mdev members outside the
> > matrix_dev->lock.  
> 
> With this patch, the matrix_mdev is always written or read with
> the matrix_dev->lock mutex locked.

Sorry I don't understand. How is the struct matrix_mdev.kvm read
with the matrix_dev->lock mutex locked in:

static void vfio_ap_mdev_release(struct mdev_device *mdev)                      
{                                                                               
        struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);            
                                                                                
        vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,             
                                 &matrix_mdev->iommu_notifier);                 
        vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,             
                                 &matrix_mdev->group_notifier);                 
        vfio_ap_mdev_unset_kvm(matrix_mdev, matrix_mdev->kvm);   <==== HERE!
        module_put(THIS_MODULE);                                                
} 
> By moving the locking of the
> kvm->lock mutex out of the functions that set/clear the masks
> in the guest's CRYCB, we are now able to lock the kvm->lock
> mutex before locking the matrix_dev->lock mutex in both the
> vfio_ap_mdev_set_kvm() and vfio_ap_mdev_unset_kvm()
> functions. This precludes the need to unlock the
> matrix_dev->lock mutex while the masks are being set or
> cleared and alleviates the lockdep splat for which the open coded
> locks were created.


> 
> >
> > Regards,
> > Halil  
> 


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification
  2021-07-26 20:36           ` Halil Pasic
@ 2021-07-26 22:03             ` Jason Gunthorpe
  2021-07-26 22:43               ` Halil Pasic
  2021-07-27  6:54               ` Christoph Hellwig
  0 siblings, 2 replies; 42+ messages in thread
From: Jason Gunthorpe @ 2021-07-26 22:03 UTC (permalink / raw)
  To: Halil Pasic
  Cc: Tony Krowiak, linux-s390, linux-kernel, borntraeger, cohuck,
	pasic, jjherne, alex.williamson, kwankhede, david

On Mon, Jul 26, 2021 at 10:36:28PM +0200, Halil Pasic wrote:

> You may end up with open and close running interleaved. What I'
> trying to say is, to my best knowledge, normally there is no
> you have to close it before you open it again rule for files.

This is an existing bug in this driver, I've fixed in the reflck series.

open_device/close_device will not run concurrently, or out of order,
afer it is fixed.

Jason

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification
  2021-07-26 22:03             ` Jason Gunthorpe
@ 2021-07-26 22:43               ` Halil Pasic
  2021-07-28 13:43                 ` Tony Krowiak
  2021-07-27  6:54               ` Christoph Hellwig
  1 sibling, 1 reply; 42+ messages in thread
From: Halil Pasic @ 2021-07-26 22:43 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Tony Krowiak, linux-s390, linux-kernel, borntraeger, cohuck,
	pasic, jjherne, alex.williamson, kwankhede, david

On Mon, 26 Jul 2021 19:03:17 -0300
Jason Gunthorpe <jgg@nvidia.com> wrote:

> > You may end up with open and close running interleaved. What I'
> > trying to say is, to my best knowledge, normally there is no
> > you have to close it before you open it again rule for files.  
> 
> This is an existing bug in this driver, I've fixed in the reflck series.
> 
> open_device/close_device will not run concurrently, or out of order,
> afer it is fixed.

Well if that is the case then provided your fix precedes this patch:

Acked-by: Halil Pasic <pasic@linux.ibm.com>

I'm not entirely happy with this. I did not thoroughly investigate the
implications of reversing the locking order of the vfio-ap lock (driver
global) and the kvm lock (guest specific). I will trust Tony and hope
our KVM maintainers will scream if this is bad from interference and
delay perspective.

Regards,
Halil

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification
  2021-07-26 22:03             ` Jason Gunthorpe
  2021-07-26 22:43               ` Halil Pasic
@ 2021-07-27  6:54               ` Christoph Hellwig
  1 sibling, 0 replies; 42+ messages in thread
From: Christoph Hellwig @ 2021-07-27  6:54 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Halil Pasic, Tony Krowiak, linux-s390, linux-kernel, borntraeger,
	cohuck, pasic, jjherne, alex.williamson, kwankhede, david, kvm

On Mon, Jul 26, 2021 at 07:03:17PM -0300, Jason Gunthorpe wrote:
> On Mon, Jul 26, 2021 at 10:36:28PM +0200, Halil Pasic wrote:
> 
> > You may end up with open and close running interleaved. What I'
> > trying to say is, to my best knowledge, normally there is no
> > you have to close it before you open it again rule for files.
> 
> This is an existing bug in this driver, I've fixed in the reflck series.
> 
> open_device/close_device will not run concurrently, or out of order,
> afer it is fixed.

Btw, while I've got all your attention, I've been struggling a bit with
how that SET_KVM notifier is supposed to work.

The i915 gvt code simplify assumes the notification registration hits
the case of KVM already being active, and gets away with that as at
least qemu ensures that the KVM_DEV_VFIO_GROUP_ADD has been called before
the device FDs are opened.  Is that something we could generalize and
never allow to actually notify for SET_KVM with non-null data beeing
called at runtime and avoid the locking entirely?

Similarly the removal case is a little weird: with Jason's work to only
call a release function on the last reference drop which solves a lot
of concurrency issues nicely  this still creates a nasty corner case
with a sideband release under weird locking rules.   What prevents the
vfio core from simply holding a refefeence to the struct kvm as long as
the device is open to close that hole?

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification
  2021-07-26 22:43               ` Halil Pasic
@ 2021-07-28 13:43                 ` Tony Krowiak
  2021-07-28 19:42                   ` Halil Pasic
  0 siblings, 1 reply; 42+ messages in thread
From: Tony Krowiak @ 2021-07-28 13:43 UTC (permalink / raw)
  To: Halil Pasic, Jason Gunthorpe
  Cc: linux-s390, linux-kernel, borntraeger, cohuck, pasic, jjherne,
	alex.williamson, kwankhede, david, pbonzini, David Hildenbrand,
	frankja, imbrenda



On 7/26/21 6:43 PM, Halil Pasic wrote:
> On Mon, 26 Jul 2021 19:03:17 -0300
> Jason Gunthorpe <jgg@nvidia.com> wrote:
>
>>> You may end up with open and close running interleaved. What I'
>>> trying to say is, to my best knowledge, normally there is no
>>> you have to close it before you open it again rule for files.
>> This is an existing bug in this driver, I've fixed in the reflck series.
>>
>> open_device/close_device will not run concurrently, or out of order,
>> afer it is fixed.
> Well if that is the case then provided your fix precedes this patch:
>
> Acked-by: Halil Pasic <pasic@linux.ibm.com>
>
> I'm not entirely happy with this. I did not thoroughly investigate the
> implications of reversing the locking order of the vfio-ap lock (driver
> global) and the kvm lock (guest specific). I will trust Tony and hope
> our KVM maintainers will scream if this is bad from interference and
> delay perspective.

This solution was suggested by Jason G and it does in fact resolve
the lockdep splat encountered when starting an SE guest with
access to crypto resources. There is a chance that the KVM lock
can get held while waiting for the lock on the matrix_dev->mutex,
but this does not seem like a grave concern to me. That would
only happen during VFIO_GROUP_NOTIFY_SET_KVM notification - either
when the guest is being started or terminated, or when the mdev
fd is opened or closed. According to Jason, once he integrates his reflck
series, the open/close will happen only once.

I've copied the KVM maintainers on this response, so hopefully one of
them will provide some input.

>
> Regards,
> Halil


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification
  2021-07-28 13:43                 ` Tony Krowiak
@ 2021-07-28 19:42                   ` Halil Pasic
  2021-07-30 13:33                     ` Tony Krowiak
  0 siblings, 1 reply; 42+ messages in thread
From: Halil Pasic @ 2021-07-28 19:42 UTC (permalink / raw)
  To: Tony Krowiak
  Cc: Jason Gunthorpe, linux-s390, linux-kernel, borntraeger, cohuck,
	pasic, jjherne, alex.williamson, kwankhede, david, pbonzini,
	frankja, imbrenda

On Wed, 28 Jul 2021 09:43:03 -0400
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> This solution was suggested by Jason G and it does in fact resolve
> the lockdep splat encountered when starting an SE guest with
> access to crypto resources. There is a chance that the KVM lock
> can get held while waiting for the lock on the matrix_dev->mutex,
> but this does not seem like a grave concern to me. 

Yes I agree. I was thinking along the lines: matrix modifications
via the sysfs take the matrix_dev->lock so the level of contention
may depend on what userspace is doing...

Regards,
Halil

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification
  2021-07-28 19:42                   ` Halil Pasic
@ 2021-07-30 13:33                     ` Tony Krowiak
  0 siblings, 0 replies; 42+ messages in thread
From: Tony Krowiak @ 2021-07-30 13:33 UTC (permalink / raw)
  To: Halil Pasic
  Cc: Jason Gunthorpe, linux-s390, linux-kernel, borntraeger, cohuck,
	pasic, jjherne, alex.williamson, kwankhede, david, pbonzini,
	frankja, imbrenda



On 7/28/21 3:42 PM, Halil Pasic wrote:
> On Wed, 28 Jul 2021 09:43:03 -0400
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>
>> This solution was suggested by Jason G and it does in fact resolve
>> the lockdep splat encountered when starting an SE guest with
>> access to crypto resources. There is a chance that the KVM lock
>> can get held while waiting for the lock on the matrix_dev->mutex,
>> but this does not seem like a grave concern to me.
> Yes I agree. I was thinking along the lines: matrix modifications
> via the sysfs take the matrix_dev->lock so the level of contention
> may depend on what userspace is doing...

The probe/remove functions also take the matrix_dev->lock
as does the handle_pqap() function. In any case, while all of
those are possible, in our implementation of AP queue
pass-through, the two functions that take the KVM lock
are invoked when the guest is starting or shutting down,
or when the mdev is hot plugged/unplugged. For the cases of
guest startup/shutdown, it would seem that holding the
kvm->lock while waiting for the matrix_dev->lock shouldn't
be a big problem since the guest will either not be fully up
yet or on its way down. I suppose the hot plug/unplug case
could potentially cause the guest vcpus to pause while processing,
but how often do you anticipate a hot plug to take place?

>
> Regards,
> Halil


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 0/2] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification
  2021-07-19 19:35 [PATCH 0/2] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification Tony Krowiak
  2021-07-19 19:35 ` [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer Tony Krowiak
  2021-07-19 19:35 ` [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification Tony Krowiak
@ 2021-08-02 13:10 ` Tony Krowiak
  2021-08-02 13:53   ` Halil Pasic
  2 siblings, 1 reply; 42+ messages in thread
From: Tony Krowiak @ 2021-08-02 13:10 UTC (permalink / raw)
  To: linux-s390, linux-kernel
  Cc: borntraeger, cohuck, pasic, jjherne, jgg, alex.williamson,
	kwankhede, david, pbonzini, david, frankja, imbrenda

PING!

This patch will pre-req version 17 of a patch series I have waiting in 
the wings,
so I'd like to get this one merged ASAP. In particular, if a KVM 
maintainer can
take a look at the comments concerning the taking of the kvm->lock 
before the
matrix_mdev->lock it would be greatly appreciated. Those comments begin with
Message ID <20210727004329.3bcc7d4f.pasic@linux.ibm.com> from Halil Pasic.

On 7/19/21 3:35 PM, Tony Krowiak wrote:
> This series is actually only comprised of a single patch to replace the
> open coded locks used during VFIO_GROUP_NOTIFY_SET_KVM notification. The
> first patch is included because it is a pre-req slotted to be merged but is
> not yet available in the kernel.
>
> Tony Krowiak (2):
>    s390/vfio-ap: r/w lock for PQAP interception handler function pointer
>    s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM
>      notification
>
>   arch/s390/include/asm/kvm_host.h      |   8 +-
>   arch/s390/kvm/kvm-s390.c              |  28 +++++-
>   arch/s390/kvm/priv.c                  |  10 +-
>   drivers/s390/crypto/vfio_ap_ops.c     | 127 +++++++++-----------------
>   drivers/s390/crypto/vfio_ap_private.h |   4 +-
>   5 files changed, 77 insertions(+), 100 deletions(-)
>


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 0/2] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification
  2021-08-02 13:10 ` [PATCH 0/2] s390/vfio-ap: do not open code " Tony Krowiak
@ 2021-08-02 13:53   ` Halil Pasic
  2021-08-02 16:32     ` Tony Krowiak
  0 siblings, 1 reply; 42+ messages in thread
From: Halil Pasic @ 2021-08-02 13:53 UTC (permalink / raw)
  To: Tony Krowiak
  Cc: linux-s390, linux-kernel, borntraeger, cohuck, pasic, jjherne,
	jgg, alex.williamson, kwankhede, david, pbonzini, frankja,
	imbrenda

On Mon, 2 Aug 2021 09:10:26 -0400
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> PING!
> 
> This patch will pre-req version 17 of a patch series I have waiting in 
> the wings,
> so I'd like to get this one merged ASAP. In particular, if a KVM 
> maintainer can
> take a look at the comments concerning the taking of the kvm->lock 
> before the
> matrix_mdev->lock it would be greatly appreciated. Those comments begin with
> Message ID <20210727004329.3bcc7d4f.pasic@linux.ibm.com> from Halil Pasic.

As far as I'm concerned, we can move forward with this. Was this
supposed to go in via Alex's tree?

> 
> On 7/19/21 3:35 PM, Tony Krowiak wrote:
> > This series is actually only comprised of a single patch to replace the
> > open coded locks used during VFIO_GROUP_NOTIFY_SET_KVM notification. The
> > first patch is included because it is a pre-req slotted to be merged but is
> > not yet available in the kernel.
> >
> > Tony Krowiak (2):
> >    s390/vfio-ap: r/w lock for PQAP interception handler function pointer
> >    s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM
> >      notification
> >
> >   arch/s390/include/asm/kvm_host.h      |   8 +-
> >   arch/s390/kvm/kvm-s390.c              |  28 +++++-
> >   arch/s390/kvm/priv.c                  |  10 +-
> >   drivers/s390/crypto/vfio_ap_ops.c     | 127 +++++++++-----------------
> >   drivers/s390/crypto/vfio_ap_private.h |   4 +-
> >   5 files changed, 77 insertions(+), 100 deletions(-)
> >  
> 


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 0/2] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification
  2021-08-02 13:53   ` Halil Pasic
@ 2021-08-02 16:32     ` Tony Krowiak
  2021-08-03 13:08       ` Jason Gunthorpe
  2021-08-18 15:59       ` Christian Borntraeger
  0 siblings, 2 replies; 42+ messages in thread
From: Tony Krowiak @ 2021-08-02 16:32 UTC (permalink / raw)
  To: Halil Pasic
  Cc: linux-s390, linux-kernel, borntraeger, cohuck, pasic, jjherne,
	jgg, alex.williamson, kwankhede, david, pbonzini, frankja,
	imbrenda



On 8/2/21 9:53 AM, Halil Pasic wrote:
> On Mon, 2 Aug 2021 09:10:26 -0400
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>
>> PING!
>>
>> This patch will pre-req version 17 of a patch series I have waiting in
>> the wings,
>> so I'd like to get this one merged ASAP. In particular, if a KVM
>> maintainer can
>> take a look at the comments concerning the taking of the kvm->lock
>> before the
>> matrix_mdev->lock it would be greatly appreciated. Those comments begin with
>> Message ID <20210727004329.3bcc7d4f.pasic@linux.ibm.com> from Halil Pasic.
> As far as I'm concerned, we can move forward with this. Was this
> supposed to go in via Alex's tree?

I am not certain, Christian queued the previous patches related to
this on:


https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git/log/?h=fixes

Jason G., since this will need to be integrated with your other patches,
where should this be queued?

>
>> On 7/19/21 3:35 PM, Tony Krowiak wrote:
>>> This series is actually only comprised of a single patch to replace the
>>> open coded locks used during VFIO_GROUP_NOTIFY_SET_KVM notification. The
>>> first patch is included because it is a pre-req slotted to be merged but is
>>> not yet available in the kernel.
>>>
>>> Tony Krowiak (2):
>>>     s390/vfio-ap: r/w lock for PQAP interception handler function pointer
>>>     s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM
>>>       notification
>>>
>>>    arch/s390/include/asm/kvm_host.h      |   8 +-
>>>    arch/s390/kvm/kvm-s390.c              |  28 +++++-
>>>    arch/s390/kvm/priv.c                  |  10 +-
>>>    drivers/s390/crypto/vfio_ap_ops.c     | 127 +++++++++-----------------
>>>    drivers/s390/crypto/vfio_ap_private.h |   4 +-
>>>    5 files changed, 77 insertions(+), 100 deletions(-)
>>>   


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 0/2] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification
  2021-08-02 16:32     ` Tony Krowiak
@ 2021-08-03 13:08       ` Jason Gunthorpe
  2021-08-03 13:34         ` Tony Krowiak
  2021-08-18 15:59       ` Christian Borntraeger
  1 sibling, 1 reply; 42+ messages in thread
From: Jason Gunthorpe @ 2021-08-03 13:08 UTC (permalink / raw)
  To: Tony Krowiak
  Cc: Halil Pasic, linux-s390, linux-kernel, borntraeger, cohuck,
	pasic, jjherne, alex.williamson, kwankhede, david, pbonzini,
	frankja, imbrenda

On Mon, Aug 02, 2021 at 12:32:12PM -0400, Tony Krowiak wrote:
> On 8/2/21 9:53 AM, Halil Pasic wrote:
> > On Mon, 2 Aug 2021 09:10:26 -0400
> > Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> > 
> > > PING!
> > > 
> > > This patch will pre-req version 17 of a patch series I have waiting in
> > > the wings,
> > > so I'd like to get this one merged ASAP. In particular, if a KVM
> > > maintainer can
> > > take a look at the comments concerning the taking of the kvm->lock
> > > before the
> > > matrix_mdev->lock it would be greatly appreciated. Those comments begin with
> > > Message ID <20210727004329.3bcc7d4f.pasic@linux.ibm.com> from Halil Pasic.
> > As far as I'm concerned, we can move forward with this. Was this
> > supposed to go in via Alex's tree?
> 
> I am not certain, Christian queued the previous patches related to
> this on:
> 
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git/log/?h=fixes
> 
> Jason G., since this will need to be integrated with your other patches,
> where should this be queued?

I prefer changes to the vfio stuff go to Alex as we are changing it a
lot as well this cycle.

Jason

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 0/2] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification
  2021-08-03 13:08       ` Jason Gunthorpe
@ 2021-08-03 13:34         ` Tony Krowiak
  0 siblings, 0 replies; 42+ messages in thread
From: Tony Krowiak @ 2021-08-03 13:34 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Halil Pasic, linux-s390, linux-kernel, borntraeger, cohuck,
	pasic, jjherne, alex.williamson, kwankhede, david, pbonzini,
	frankja, imbrenda



On 8/3/21 9:08 AM, Jason Gunthorpe wrote:
> On Mon, Aug 02, 2021 at 12:32:12PM -0400, Tony Krowiak wrote:
>> On 8/2/21 9:53 AM, Halil Pasic wrote:
>>> On Mon, 2 Aug 2021 09:10:26 -0400
>>> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>>>
>>>> PING!
>>>>
>>>> This patch will pre-req version 17 of a patch series I have waiting in
>>>> the wings,
>>>> so I'd like to get this one merged ASAP. In particular, if a KVM
>>>> maintainer can
>>>> take a look at the comments concerning the taking of the kvm->lock
>>>> before the
>>>> matrix_mdev->lock it would be greatly appreciated. Those comments begin with
>>>> Message ID <20210727004329.3bcc7d4f.pasic@linux.ibm.com> from Halil Pasic.
>>> As far as I'm concerned, we can move forward with this. Was this
>>> supposed to go in via Alex's tree?
>> I am not certain, Christian queued the previous patches related to
>> this on:
>>
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git/log/?h=fixes
>>
>> Jason G., since this will need to be integrated with your other patches,
>> where should this be queued?
> I prefer changes to the vfio stuff go to Alex as we are changing it a
> lot as well this cycle.

Thanks Jason.

>
> Jason


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 0/2] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification
  2021-08-02 16:32     ` Tony Krowiak
  2021-08-03 13:08       ` Jason Gunthorpe
@ 2021-08-18 15:59       ` Christian Borntraeger
  2021-08-18 16:39         ` Alex Williamson
  1 sibling, 1 reply; 42+ messages in thread
From: Christian Borntraeger @ 2021-08-18 15:59 UTC (permalink / raw)
  To: Tony Krowiak, Halil Pasic, alex.williamson
  Cc: linux-s390, linux-kernel, cohuck, pasic, jjherne, jgg, kwankhede,
	david, pbonzini, frankja, imbrenda

On 02.08.21 18:32, Tony Krowiak wrote:
> 
> 
> On 8/2/21 9:53 AM, Halil Pasic wrote:
>> On Mon, 2 Aug 2021 09:10:26 -0400
>> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>>
>>> PING!
>>>
>>> This patch will pre-req version 17 of a patch series I have waiting in
>>> the wings,
>>> so I'd like to get this one merged ASAP. In particular, if a KVM
>>> maintainer can
>>> take a look at the comments concerning the taking of the kvm->lock
>>> before the
>>> matrix_mdev->lock it would be greatly appreciated. Those comments begin with
>>> Message ID <20210727004329.3bcc7d4f.pasic@linux.ibm.com> from Halil Pasic.
>> As far as I'm concerned, we can move forward with this. Was this
>> supposed to go in via Alex's tree?
> 
> I am not certain, Christian queued the previous patches related to
> this on:
> 
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git/log/?h=fixes
> 
> Jason G., since this will need to be integrated with your other patches,
> where should this be queued?


This previous patch (s390/vfio-ap: clean up mdev resources when remove callback invoked) is
already in master.
Can you respin the series with all Acks and RBs?

Alex, can you then take these 2 patches via your tree? Thanks

Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
for this series.

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 0/2] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification
  2021-08-18 15:59       ` Christian Borntraeger
@ 2021-08-18 16:39         ` Alex Williamson
  2021-08-18 16:50           ` Christian Borntraeger
  2021-08-19 15:30           ` Cornelia Huck
  0 siblings, 2 replies; 42+ messages in thread
From: Alex Williamson @ 2021-08-18 16:39 UTC (permalink / raw)
  To: Christian Borntraeger
  Cc: Tony Krowiak, Halil Pasic, linux-s390, linux-kernel, cohuck,
	pasic, jjherne, jgg, kwankhede, david, pbonzini, frankja,
	imbrenda

On Wed, 18 Aug 2021 17:59:51 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 02.08.21 18:32, Tony Krowiak wrote:
> > 
> > 
> > On 8/2/21 9:53 AM, Halil Pasic wrote:  
> >> On Mon, 2 Aug 2021 09:10:26 -0400
> >> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> >>  
> >>> PING!
> >>>
> >>> This patch will pre-req version 17 of a patch series I have waiting in
> >>> the wings,
> >>> so I'd like to get this one merged ASAP. In particular, if a KVM
> >>> maintainer can
> >>> take a look at the comments concerning the taking of the kvm->lock
> >>> before the
> >>> matrix_mdev->lock it would be greatly appreciated. Those comments begin with
> >>> Message ID <20210727004329.3bcc7d4f.pasic@linux.ibm.com> from Halil Pasic.  
> >> As far as I'm concerned, we can move forward with this. Was this
> >> supposed to go in via Alex's tree?  
> > 
> > I am not certain, Christian queued the previous patches related to
> > this on:
> > 
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git/log/?h=fixes
> > 
> > Jason G., since this will need to be integrated with your other patches,
> > where should this be queued?  
> 
> 
> This previous patch (s390/vfio-ap: clean up mdev resources when remove callback invoked) is
> already in master.
> Can you respin the series with all Acks and RBs?
> 
> Alex, can you then take these 2 patches via your tree? Thanks
> 
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> for this series.


I see some review feedback that seems to suggest a new version would be
posted:

https://lore.kernel.org/linux-s390/0f03ab0b-2dfd-e1c1-fe43-be2a59030a71@linux.ibm.com/

I also see in this thread:

https://lore.kernel.org/linux-s390/20210721164550.5402fe1c.pasic@linux.ibm.com/

that Halil's concern's around open/close races are addressed by Jason's
device_open/close series that's already in my next branch and he
provided an Ack, but there's still the above question regarding the
kvm->lock that was looking for a review from... I'm not sure, maybe
Connie or Paolo.  Christian, is this specifically what you're ack'ing?

It can ultimately go in through my tree, but not being familiar with
this code I'd hope for more closure.  Thanks,

Alex


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 0/2] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification
  2021-08-18 16:39         ` Alex Williamson
@ 2021-08-18 16:50           ` Christian Borntraeger
  2021-08-18 22:52             ` Halil Pasic
  2021-08-19 15:30           ` Cornelia Huck
  1 sibling, 1 reply; 42+ messages in thread
From: Christian Borntraeger @ 2021-08-18 16:50 UTC (permalink / raw)
  To: Alex Williamson
  Cc: Tony Krowiak, Halil Pasic, linux-s390, linux-kernel, cohuck,
	pasic, jjherne, jgg, kwankhede, david, pbonzini, frankja,
	imbrenda



On 18.08.21 18:39, Alex Williamson wrote:
> On Wed, 18 Aug 2021 17:59:51 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 02.08.21 18:32, Tony Krowiak wrote:
>>>
>>>
>>> On 8/2/21 9:53 AM, Halil Pasic wrote:
>>>> On Mon, 2 Aug 2021 09:10:26 -0400
>>>> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>>>>   
>>>>> PING!
>>>>>
>>>>> This patch will pre-req version 17 of a patch series I have waiting in
>>>>> the wings,
>>>>> so I'd like to get this one merged ASAP. In particular, if a KVM
>>>>> maintainer can
>>>>> take a look at the comments concerning the taking of the kvm->lock
>>>>> before the
>>>>> matrix_mdev->lock it would be greatly appreciated. Those comments begin with
>>>>> Message ID <20210727004329.3bcc7d4f.pasic@linux.ibm.com> from Halil Pasic.
>>>> As far as I'm concerned, we can move forward with this. Was this
>>>> supposed to go in via Alex's tree?
>>>
>>> I am not certain, Christian queued the previous patches related to
>>> this on:
>>>
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git/log/?h=fixes
>>>
>>> Jason G., since this will need to be integrated with your other patches,
>>> where should this be queued?
>>
>>
>> This previous patch (s390/vfio-ap: clean up mdev resources when remove callback invoked) is
>> already in master.
>> Can you respin the series with all Acks and RBs?
>>
>> Alex, can you then take these 2 patches via your tree? Thanks
>>
>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> for this series.
> 
> 
> I see some review feedback that seems to suggest a new version would be
> posted:
> 
> https://lore.kernel.org/linux-s390/0f03ab0b-2dfd-e1c1-fe43-be2a59030a71@linux.ibm.com/
> 
> I also see in this thread:
> 
> https://lore.kernel.org/linux-s390/20210721164550.5402fe1c.pasic@linux.ibm.com/
> 
> that Halil's concern's around open/close races are addressed by Jason's
> device_open/close series that's already in my next branch and he
> provided an Ack, but there's still the above question regarding the
> kvm->lock that was looking for a review from... I'm not sure, maybe
> Connie or Paolo.  Christian, is this specifically what you're ack'ing?

My understanding was that Halil was ok in the end?
I will do a review myself then if that helps.
> 
> It can ultimately go in through my tree, but not being familiar with
> this code I'd hope for more closure.  Thanks,
> 
> Alex
> 

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer
  2021-07-19 19:35 ` [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer Tony Krowiak
@ 2021-08-18 17:03   ` Christian Borntraeger
  2021-08-18 23:25     ` Halil Pasic
  2021-08-19 13:20     ` Tony Krowiak
  0 siblings, 2 replies; 42+ messages in thread
From: Christian Borntraeger @ 2021-08-18 17:03 UTC (permalink / raw)
  To: Tony Krowiak, linux-s390, linux-kernel
  Cc: cohuck, pasic, jjherne, jgg, alex.williamson, kwankhede, david

On 19.07.21 21:35, Tony Krowiak wrote:
> The function pointer to the interception handler for the PQAP instruction
> can get changed during the interception process. Let's add a
> semaphore to struct kvm_s390_crypto to control read/write access to the
> function pointer contained therein.
> 
> The semaphore must be locked for write access by the vfio_ap device driver
> when notified that the KVM pointer has been set or cleared. It must be
> locked for read access by the interception framework when the PQAP
> instruction is intercepted.
> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   arch/s390/include/asm/kvm_host.h      |  8 +++-----
>   arch/s390/kvm/kvm-s390.c              |  1 +
>   arch/s390/kvm/priv.c                  | 10 ++++++----
>   drivers/s390/crypto/vfio_ap_ops.c     | 23 +++++++++++++++++------
>   drivers/s390/crypto/vfio_ap_private.h |  2 +-
>   5 files changed, 28 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index 9b4473f76e56..f18849d259e6 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -798,14 +798,12 @@ struct kvm_s390_cpu_model {
>   	unsigned short ibc;
>   };
>   
> -struct kvm_s390_module_hook {
> -	int (*hook)(struct kvm_vcpu *vcpu);
> -	struct module *owner;
> -};
> +typedef int (*crypto_hook)(struct kvm_vcpu *vcpu);
>   
>   struct kvm_s390_crypto {
>   	struct kvm_s390_crypto_cb *crycb;
> -	struct kvm_s390_module_hook *pqap_hook;
> +	struct rw_semaphore pqap_hook_rwsem;
> +	crypto_hook *pqap_hook;
>   	__u32 crycbd;
>   	__u8 aes_kw;
>   	__u8 dea_kw;
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index b655a7d82bf0..a08f242a9f27 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2630,6 +2630,7 @@ static void kvm_s390_crypto_init(struct kvm *kvm)
>   {
>   	kvm->arch.crypto.crycb = &kvm->arch.sie_page2->crycb;
>   	kvm_s390_set_crycb_format(kvm);
> +	init_rwsem(&kvm->arch.crypto.pqap_hook_rwsem);
>   
>   	if (!test_kvm_facility(kvm, 76))
>   		return;
> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
> index 9928f785c677..6bed9406c1f3 100644
> --- a/arch/s390/kvm/priv.c
> +++ b/arch/s390/kvm/priv.c
> @@ -610,6 +610,7 @@ static int handle_io_inst(struct kvm_vcpu *vcpu)
>   static int handle_pqap(struct kvm_vcpu *vcpu)
>   {
>   	struct ap_queue_status status = {};
> +	crypto_hook pqap_hook;
>   	unsigned long reg0;
>   	int ret;
>   	uint8_t fc;
> @@ -657,15 +658,16 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
>   	 * Verify that the hook callback is registered, lock the owner
>   	 * and call the hook.
>   	 */
> +	down_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem);
>   	if (vcpu->kvm->arch.crypto.pqap_hook) {
> -		if (!try_module_get(vcpu->kvm->arch.crypto.pqap_hook->owner))
> -			return -EOPNOTSUPP;
> -		ret = vcpu->kvm->arch.crypto.pqap_hook->hook(vcpu);
> -		module_put(vcpu->kvm->arch.crypto.pqap_hook->owner);
> +		pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook;

Dont we have to check for NULL here? If not can you add a comment why?

Otherwise this looks good.


> +		ret = pqap_hook(vcpu);
[...]

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 0/2] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification
  2021-08-18 16:50           ` Christian Borntraeger
@ 2021-08-18 22:52             ` Halil Pasic
  0 siblings, 0 replies; 42+ messages in thread
From: Halil Pasic @ 2021-08-18 22:52 UTC (permalink / raw)
  To: Christian Borntraeger
  Cc: Alex Williamson, Tony Krowiak, linux-s390, linux-kernel, cohuck,
	pasic, jjherne, jgg, kwankhede, david, pbonzini, frankja,
	imbrenda

On Wed, 18 Aug 2021 18:50:47 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> > that Halil's concern's around open/close races are addressed by Jason's
> > device_open/close series that's already in my next branch and he
> > provided an Ack, but there's still the above question regarding the
> > kvm->lock that was looking for a review from... I'm not sure, maybe
> > Connie or Paolo.  Christian, is this specifically what you're ack'ing?  
> 
> My understanding was that Halil was ok in the end?

Yes, I'm OK with it provided it is merged after Jason's patch that makes
it a non-issue.

Regards,
Halil

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer
  2021-08-18 17:03   ` Christian Borntraeger
@ 2021-08-18 23:25     ` Halil Pasic
  2021-08-19  6:56       ` Christian Borntraeger
  2021-08-19 13:36       ` Tony Krowiak
  2021-08-19 13:20     ` Tony Krowiak
  1 sibling, 2 replies; 42+ messages in thread
From: Halil Pasic @ 2021-08-18 23:25 UTC (permalink / raw)
  To: Christian Borntraeger
  Cc: Tony Krowiak, linux-s390, linux-kernel, cohuck, pasic, jjherne,
	jgg, alex.williamson, kwankhede, david

On Wed, 18 Aug 2021 19:03:33 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 19.07.21 21:35, Tony Krowiak wrote:
> > The function pointer to the interception handler for the PQAP instruction
> > can get changed during the interception process. Let's add a
> > semaphore to struct kvm_s390_crypto to control read/write access to the
> > function pointer contained therein.
> > 
> > The semaphore must be locked for write access by the vfio_ap device driver
> > when notified that the KVM pointer has been set or cleared. It must be
> > locked for read access by the interception framework when the PQAP
> > instruction is intercepted.
> > 
> > Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> > ---
> >   arch/s390/include/asm/kvm_host.h      |  8 +++-----
> >   arch/s390/kvm/kvm-s390.c              |  1 +
> >   arch/s390/kvm/priv.c                  | 10 ++++++----
> >   drivers/s390/crypto/vfio_ap_ops.c     | 23 +++++++++++++++++------
> >   drivers/s390/crypto/vfio_ap_private.h |  2 +-
> >   5 files changed, 28 insertions(+), 16 deletions(-)
> > 
> > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> > index 9b4473f76e56..f18849d259e6 100644
> > --- a/arch/s390/include/asm/kvm_host.h
> > +++ b/arch/s390/include/asm/kvm_host.h
> > @@ -798,14 +798,12 @@ struct kvm_s390_cpu_model {
> >   	unsigned short ibc;
> >   };
> >   
> > -struct kvm_s390_module_hook {
> > -	int (*hook)(struct kvm_vcpu *vcpu);
> > -	struct module *owner;
> > -};
> > +typedef int (*crypto_hook)(struct kvm_vcpu *vcpu);
> >   
> >   struct kvm_s390_crypto {
> >   	struct kvm_s390_crypto_cb *crycb;
> > -	struct kvm_s390_module_hook *pqap_hook;
> > +	struct rw_semaphore pqap_hook_rwsem;
> > +	crypto_hook *pqap_hook;
> >   	__u32 crycbd;
> >   	__u8 aes_kw;
> >   	__u8 dea_kw;
> > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> > index b655a7d82bf0..a08f242a9f27 100644
> > --- a/arch/s390/kvm/kvm-s390.c
> > +++ b/arch/s390/kvm/kvm-s390.c
> > @@ -2630,6 +2630,7 @@ static void kvm_s390_crypto_init(struct kvm *kvm)
> >   {
> >   	kvm->arch.crypto.crycb = &kvm->arch.sie_page2->crycb;
> >   	kvm_s390_set_crycb_format(kvm);
> > +	init_rwsem(&kvm->arch.crypto.pqap_hook_rwsem);
> >   
> >   	if (!test_kvm_facility(kvm, 76))
> >   		return;
> > diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
> > index 9928f785c677..6bed9406c1f3 100644
> > --- a/arch/s390/kvm/priv.c
> > +++ b/arch/s390/kvm/priv.c
> > @@ -610,6 +610,7 @@ static int handle_io_inst(struct kvm_vcpu *vcpu)
> >   static int handle_pqap(struct kvm_vcpu *vcpu)
> >   {
> >   	struct ap_queue_status status = {};
> > +	crypto_hook pqap_hook;
> >   	unsigned long reg0;
> >   	int ret;
> >   	uint8_t fc;
> > @@ -657,15 +658,16 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
> >   	 * Verify that the hook callback is registered, lock the owner
> >   	 * and call the hook.
> >   	 */
> > +	down_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem);
> >   	if (vcpu->kvm->arch.crypto.pqap_hook) {                     <--- HERE
> > -		if (!try_module_get(vcpu->kvm->arch.crypto.pqap_hook->owner))
> > -			return -EOPNOTSUPP;
> > -		ret = vcpu->kvm->arch.crypto.pqap_hook->hook(vcpu);
> > -		module_put(vcpu->kvm->arch.crypto.pqap_hook->owner);
> > +		pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook;  
> 
> Dont we have to check for NULL here? If not can you add a comment why?

I believe we did the necessary check on the line I just marked with
"<--- HERE". 

I find that "*" operator confusing in this context as it doesn't do
any good for us. I believe this situation is described in 6.5.3.2.4 of
the c11 standard. For convenience I will cite from the corresponding
draft:
"The unary * operator denotes indirection. If the operand points to a
function, the result is a function designator; if it points to an
object, the result is an lvalue designating the object. If the operand
has type ‘‘pointer to type’’, the result has type ‘‘type’’. If an
invalid value has been assigned to the pointer, the behavior of the
unary * operator is undefined."

Frankly I also fail to see the benefit of introducing the local variable
named "pqap_hook", but back then I decided to not complain about style.

Regards,
Halil

> 
> 
> > +		ret = pqap_hook(vcpu);  
> [...]


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer
  2021-08-18 23:25     ` Halil Pasic
@ 2021-08-19  6:56       ` Christian Borntraeger
  2021-08-19 13:36       ` Tony Krowiak
  1 sibling, 0 replies; 42+ messages in thread
From: Christian Borntraeger @ 2021-08-19  6:56 UTC (permalink / raw)
  To: Halil Pasic
  Cc: Tony Krowiak, linux-s390, linux-kernel, cohuck, pasic, jjherne,
	jgg, alex.williamson, kwankhede, david



On 19.08.21 01:25, Halil Pasic wrote:
> On Wed, 18 Aug 2021 19:03:33 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 19.07.21 21:35, Tony Krowiak wrote:
>>> The function pointer to the interception handler for the PQAP instruction
>>> can get changed during the interception process. Let's add a
>>> semaphore to struct kvm_s390_crypto to control read/write access to the
>>> function pointer contained therein.
>>>
>>> The semaphore must be locked for write access by the vfio_ap device driver
>>> when notified that the KVM pointer has been set or cleared. It must be
>>> locked for read access by the interception framework when the PQAP
>>> instruction is intercepted.
>>>
>>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>>> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
>>> ---
>>>    arch/s390/include/asm/kvm_host.h      |  8 +++-----
>>>    arch/s390/kvm/kvm-s390.c              |  1 +
>>>    arch/s390/kvm/priv.c                  | 10 ++++++----
>>>    drivers/s390/crypto/vfio_ap_ops.c     | 23 +++++++++++++++++------
>>>    drivers/s390/crypto/vfio_ap_private.h |  2 +-
>>>    5 files changed, 28 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>>> index 9b4473f76e56..f18849d259e6 100644
>>> --- a/arch/s390/include/asm/kvm_host.h
>>> +++ b/arch/s390/include/asm/kvm_host.h
>>> @@ -798,14 +798,12 @@ struct kvm_s390_cpu_model {
>>>    	unsigned short ibc;
>>>    };
>>>    
>>> -struct kvm_s390_module_hook {
>>> -	int (*hook)(struct kvm_vcpu *vcpu);
>>> -	struct module *owner;
>>> -};
>>> +typedef int (*crypto_hook)(struct kvm_vcpu *vcpu);
>>>    
>>>    struct kvm_s390_crypto {
>>>    	struct kvm_s390_crypto_cb *crycb;
>>> -	struct kvm_s390_module_hook *pqap_hook;
>>> +	struct rw_semaphore pqap_hook_rwsem;
>>> +	crypto_hook *pqap_hook;
>>>    	__u32 crycbd;
>>>    	__u8 aes_kw;
>>>    	__u8 dea_kw;
>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>> index b655a7d82bf0..a08f242a9f27 100644
>>> --- a/arch/s390/kvm/kvm-s390.c
>>> +++ b/arch/s390/kvm/kvm-s390.c
>>> @@ -2630,6 +2630,7 @@ static void kvm_s390_crypto_init(struct kvm *kvm)
>>>    {
>>>    	kvm->arch.crypto.crycb = &kvm->arch.sie_page2->crycb;
>>>    	kvm_s390_set_crycb_format(kvm);
>>> +	init_rwsem(&kvm->arch.crypto.pqap_hook_rwsem);
>>>    
>>>    	if (!test_kvm_facility(kvm, 76))
>>>    		return;
>>> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
>>> index 9928f785c677..6bed9406c1f3 100644
>>> --- a/arch/s390/kvm/priv.c
>>> +++ b/arch/s390/kvm/priv.c
>>> @@ -610,6 +610,7 @@ static int handle_io_inst(struct kvm_vcpu *vcpu)
>>>    static int handle_pqap(struct kvm_vcpu *vcpu)
>>>    {
>>>    	struct ap_queue_status status = {};
>>> +	crypto_hook pqap_hook;
>>>    	unsigned long reg0;
>>>    	int ret;
>>>    	uint8_t fc;
>>> @@ -657,15 +658,16 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
>>>    	 * Verify that the hook callback is registered, lock the owner
>>>    	 * and call the hook.
>>>    	 */
>>> +	down_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem);
>>>    	if (vcpu->kvm->arch.crypto.pqap_hook) {                     <--- HERE
>>> -		if (!try_module_get(vcpu->kvm->arch.crypto.pqap_hook->owner))
>>> -			return -EOPNOTSUPP;
>>> -		ret = vcpu->kvm->arch.crypto.pqap_hook->hook(vcpu);
>>> -		module_put(vcpu->kvm->arch.crypto.pqap_hook->owner);
>>> +		pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook;
>>
>> Dont we have to check for NULL here? If not can you add a comment why?
> 
> I believe we did the necessary check on the line I just marked with
> "<--- HERE".

Right, I missed that.

Then
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>

> 
> I find that "*" operator confusing in this context as it doesn't do
> any good for us. I believe this situation is described in 6.5.3.2.4 of
> the c11 standard. For convenience I will cite from the corresponding
> draft:
> "The unary * operator denotes indirection. If the operand points to a
> function, the result is a function designator; if it points to an
> object, the result is an lvalue designating the object. If the operand
> has type ‘‘pointer to type’’, the result has type ‘‘type’’. If an
> invalid value has been assigned to the pointer, the behavior of the
> unary * operator is undefined."
> 
> Frankly I also fail to see the benefit of introducing the local variable
> named "pqap_hook", but back then I decided to not complain about style.

Right we can probably do better, but I would rather have the functional
fix in now.

> 
> Regards,
> Halil
> 
>>
>>
>>> +		ret = pqap_hook(vcpu);
>> [...]
> 

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer
  2021-08-18 17:03   ` Christian Borntraeger
  2021-08-18 23:25     ` Halil Pasic
@ 2021-08-19 13:20     ` Tony Krowiak
  2021-08-19 17:54       ` Alex Williamson
  1 sibling, 1 reply; 42+ messages in thread
From: Tony Krowiak @ 2021-08-19 13:20 UTC (permalink / raw)
  To: Christian Borntraeger, linux-s390, linux-kernel
  Cc: cohuck, pasic, jjherne, jgg, alex.williamson, kwankhede, david



On 8/18/21 1:03 PM, Christian Borntraeger wrote:
> On 19.07.21 21:35, Tony Krowiak wrote:
>> The function pointer to the interception handler for the PQAP 
>> instruction
>> can get changed during the interception process. Let's add a
>> semaphore to struct kvm_s390_crypto to control read/write access to the
>> function pointer contained therein.
>>
>> The semaphore must be locked for write access by the vfio_ap device 
>> driver
>> when notified that the KVM pointer has been set or cleared. It must be
>> locked for read access by the interception framework when the PQAP
>> instruction is intercepted.
>>
>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
>> ---
>>   arch/s390/include/asm/kvm_host.h      |  8 +++-----
>>   arch/s390/kvm/kvm-s390.c              |  1 +
>>   arch/s390/kvm/priv.c                  | 10 ++++++----
>>   drivers/s390/crypto/vfio_ap_ops.c     | 23 +++++++++++++++++------
>>   drivers/s390/crypto/vfio_ap_private.h |  2 +-
>>   5 files changed, 28 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/s390/include/asm/kvm_host.h 
>> b/arch/s390/include/asm/kvm_host.h
>> index 9b4473f76e56..f18849d259e6 100644
>> --- a/arch/s390/include/asm/kvm_host.h
>> +++ b/arch/s390/include/asm/kvm_host.h
>> @@ -798,14 +798,12 @@ struct kvm_s390_cpu_model {
>>       unsigned short ibc;
>>   };
>>   -struct kvm_s390_module_hook {
>> -    int (*hook)(struct kvm_vcpu *vcpu);
>> -    struct module *owner;
>> -};
>> +typedef int (*crypto_hook)(struct kvm_vcpu *vcpu);
>>     struct kvm_s390_crypto {
>>       struct kvm_s390_crypto_cb *crycb;
>> -    struct kvm_s390_module_hook *pqap_hook;
>> +    struct rw_semaphore pqap_hook_rwsem;
>> +    crypto_hook *pqap_hook;
>>       __u32 crycbd;
>>       __u8 aes_kw;
>>       __u8 dea_kw;
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index b655a7d82bf0..a08f242a9f27 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -2630,6 +2630,7 @@ static void kvm_s390_crypto_init(struct kvm *kvm)
>>   {
>>       kvm->arch.crypto.crycb = &kvm->arch.sie_page2->crycb;
>>       kvm_s390_set_crycb_format(kvm);
>> +    init_rwsem(&kvm->arch.crypto.pqap_hook_rwsem);
>>         if (!test_kvm_facility(kvm, 76))
>>           return;
>> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
>> index 9928f785c677..6bed9406c1f3 100644
>> --- a/arch/s390/kvm/priv.c
>> +++ b/arch/s390/kvm/priv.c
>> @@ -610,6 +610,7 @@ static int handle_io_inst(struct kvm_vcpu *vcpu)
>>   static int handle_pqap(struct kvm_vcpu *vcpu)
>>   {
>>       struct ap_queue_status status = {};
>> +    crypto_hook pqap_hook;
>>       unsigned long reg0;
>>       int ret;
>>       uint8_t fc;
>> @@ -657,15 +658,16 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
>>        * Verify that the hook callback is registered, lock the owner
>>        * and call the hook.
>>        */
>> + down_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem);
>>       if (vcpu->kvm->arch.crypto.pqap_hook) {
>> -        if (!try_module_get(vcpu->kvm->arch.crypto.pqap_hook->owner))
>> -            return -EOPNOTSUPP;
>> -        ret = vcpu->kvm->arch.crypto.pqap_hook->hook(vcpu);
>> - module_put(vcpu->kvm->arch.crypto.pqap_hook->owner);
>> +        pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook;
>
> Dont we have to check for NULL here? If not can you add a comment why?

Take a look above the removed lines: if (vcpu->kvm->arch.crypto.pqap_hook)

>
> Otherwise this looks good.

Also, in the cover letter I said this patch was already queued and was
included here because it pre-reqs the second patch. Is this patch not
already in Alex's tree?

>
>
>> +        ret = pqap_hook(vcpu);
> [...]


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer
  2021-08-18 23:25     ` Halil Pasic
  2021-08-19  6:56       ` Christian Borntraeger
@ 2021-08-19 13:36       ` Tony Krowiak
  2021-08-19 21:42         ` Halil Pasic
  1 sibling, 1 reply; 42+ messages in thread
From: Tony Krowiak @ 2021-08-19 13:36 UTC (permalink / raw)
  To: Halil Pasic, Christian Borntraeger
  Cc: linux-s390, linux-kernel, cohuck, pasic, jjherne, jgg,
	alex.williamson, kwankhede, david



On 8/18/21 7:25 PM, Halil Pasic wrote:
> On Wed, 18 Aug 2021 19:03:33 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>
>> On 19.07.21 21:35, Tony Krowiak wrote:
>>> The function pointer to the interception handler for the PQAP instruction
>>> can get changed during the interception process. Let's add a
>>> semaphore to struct kvm_s390_crypto to control read/write access to the
>>> function pointer contained therein.
>>>
>>> The semaphore must be locked for write access by the vfio_ap device driver
>>> when notified that the KVM pointer has been set or cleared. It must be
>>> locked for read access by the interception framework when the PQAP
>>> instruction is intercepted.
>>>
>>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>>> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
>>> ---
>>>    arch/s390/include/asm/kvm_host.h      |  8 +++-----
>>>    arch/s390/kvm/kvm-s390.c              |  1 +
>>>    arch/s390/kvm/priv.c                  | 10 ++++++----
>>>    drivers/s390/crypto/vfio_ap_ops.c     | 23 +++++++++++++++++------
>>>    drivers/s390/crypto/vfio_ap_private.h |  2 +-
>>>    5 files changed, 28 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>>> index 9b4473f76e56..f18849d259e6 100644
>>> --- a/arch/s390/include/asm/kvm_host.h
>>> +++ b/arch/s390/include/asm/kvm_host.h
>>> @@ -798,14 +798,12 @@ struct kvm_s390_cpu_model {
>>>    	unsigned short ibc;
>>>    };
>>>    
>>> -struct kvm_s390_module_hook {
>>> -	int (*hook)(struct kvm_vcpu *vcpu);
>>> -	struct module *owner;
>>> -};
>>> +typedef int (*crypto_hook)(struct kvm_vcpu *vcpu);
>>>    
>>>    struct kvm_s390_crypto {
>>>    	struct kvm_s390_crypto_cb *crycb;
>>> -	struct kvm_s390_module_hook *pqap_hook;
>>> +	struct rw_semaphore pqap_hook_rwsem;
>>> +	crypto_hook *pqap_hook;
>>>    	__u32 crycbd;
>>>    	__u8 aes_kw;
>>>    	__u8 dea_kw;
>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>> index b655a7d82bf0..a08f242a9f27 100644
>>> --- a/arch/s390/kvm/kvm-s390.c
>>> +++ b/arch/s390/kvm/kvm-s390.c
>>> @@ -2630,6 +2630,7 @@ static void kvm_s390_crypto_init(struct kvm *kvm)
>>>    {
>>>    	kvm->arch.crypto.crycb = &kvm->arch.sie_page2->crycb;
>>>    	kvm_s390_set_crycb_format(kvm);
>>> +	init_rwsem(&kvm->arch.crypto.pqap_hook_rwsem);
>>>    
>>>    	if (!test_kvm_facility(kvm, 76))
>>>    		return;
>>> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
>>> index 9928f785c677..6bed9406c1f3 100644
>>> --- a/arch/s390/kvm/priv.c
>>> +++ b/arch/s390/kvm/priv.c
>>> @@ -610,6 +610,7 @@ static int handle_io_inst(struct kvm_vcpu *vcpu)
>>>    static int handle_pqap(struct kvm_vcpu *vcpu)
>>>    {
>>>    	struct ap_queue_status status = {};
>>> +	crypto_hook pqap_hook;
>>>    	unsigned long reg0;
>>>    	int ret;
>>>    	uint8_t fc;
>>> @@ -657,15 +658,16 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
>>>    	 * Verify that the hook callback is registered, lock the owner
>>>    	 * and call the hook.
>>>    	 */
>>> +	down_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem);
>>>    	if (vcpu->kvm->arch.crypto.pqap_hook) {                     <--- HERE
>>> -		if (!try_module_get(vcpu->kvm->arch.crypto.pqap_hook->owner))
>>> -			return -EOPNOTSUPP;
>>> -		ret = vcpu->kvm->arch.crypto.pqap_hook->hook(vcpu);
>>> -		module_put(vcpu->kvm->arch.crypto.pqap_hook->owner);
>>> +		pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook;
>> Dont we have to check for NULL here? If not can you add a comment why?
> I believe we did the necessary check on the line I just marked with
> "<--- HERE".
>
> I find that "*" operator confusing in this context as it doesn't do
> any good for us. I believe this situation is described in 6.5.3.2.4 of
> the c11 standard. For convenience I will cite from the corresponding
> draft:
> "The unary * operator denotes indirection. If the operand points to a
> function, the result is a function designator; if it points to an
> object, the result is an lvalue designating the object. If the operand
> has type ‘‘pointer to type’’, the result has type ‘‘type’’. If an
> invalid value has been assigned to the pointer, the behavior of the
> unary * operator is undefined."
>
> Frankly I also fail to see the benefit of introducing the local variable
> named "pqap_hook", but back then I decided to not complain about style.

The vcpu->kvm->arch.crypto.pqap_hook is a pointer to a function
pointer. The actual function pointer is stored in matrix_mdev->pqap_hook,
the reason being that the handle_pqap function in vfio_ap_ops.c
retrieves the matrix_mdev via a container_of macro. The dereferencing
of the vcpu->kvm->arch.crypto.pqap_hook into a local variable was
to get the function pointer. There may have been a more stylish
way of doing this, but the functionality is there.

>
> Regards,
> Halil
>
>>
>>> +		ret = pqap_hook(vcpu);
>> [...]


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 0/2] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification
  2021-08-18 16:39         ` Alex Williamson
  2021-08-18 16:50           ` Christian Borntraeger
@ 2021-08-19 15:30           ` Cornelia Huck
  2021-08-20 14:24             ` Tony Krowiak
  1 sibling, 1 reply; 42+ messages in thread
From: Cornelia Huck @ 2021-08-19 15:30 UTC (permalink / raw)
  To: Alex Williamson, Christian Borntraeger
  Cc: Tony Krowiak, Halil Pasic, linux-s390, linux-kernel, pasic,
	jjherne, jgg, kwankhede, david, pbonzini, frankja, imbrenda

On Wed, Aug 18 2021, Alex Williamson <alex.williamson@redhat.com> wrote:

> On Wed, 18 Aug 2021 17:59:51 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>
>> On 02.08.21 18:32, Tony Krowiak wrote:
>> > 
>> > 
>> > On 8/2/21 9:53 AM, Halil Pasic wrote:  
>> >> On Mon, 2 Aug 2021 09:10:26 -0400
>> >> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>> >>  
>> >>> PING!
>> >>>
>> >>> This patch will pre-req version 17 of a patch series I have waiting in
>> >>> the wings,
>> >>> so I'd like to get this one merged ASAP. In particular, if a KVM
>> >>> maintainer can
>> >>> take a look at the comments concerning the taking of the kvm->lock
>> >>> before the
>> >>> matrix_mdev->lock it would be greatly appreciated. Those comments begin with
>> >>> Message ID <20210727004329.3bcc7d4f.pasic@linux.ibm.com> from Halil Pasic.  
>> >> As far as I'm concerned, we can move forward with this. Was this
>> >> supposed to go in via Alex's tree?  
>> > 
>> > I am not certain, Christian queued the previous patches related to
>> > this on:
>> > 
>> > 
>> > https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git/log/?h=fixes
>> > 
>> > Jason G., since this will need to be integrated with your other patches,
>> > where should this be queued?  
>> 
>> 
>> This previous patch (s390/vfio-ap: clean up mdev resources when remove callback invoked) is
>> already in master.
>> Can you respin the series with all Acks and RBs?
>> 
>> Alex, can you then take these 2 patches via your tree? Thanks
>> 
>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> for this series.
>
>
> I see some review feedback that seems to suggest a new version would be
> posted:
>
> https://lore.kernel.org/linux-s390/0f03ab0b-2dfd-e1c1-fe43-be2a59030a71@linux.ibm.com/

Yeah, I thought so as well. But it also looks like something that could
be a fixup on top.

>
> I also see in this thread:
>
> https://lore.kernel.org/linux-s390/20210721164550.5402fe1c.pasic@linux.ibm.com/
>
> that Halil's concern's around open/close races are addressed by Jason's
> device_open/close series that's already in my next branch and he
> provided an Ack, but there's still the above question regarding the
> kvm->lock that was looking for a review from... I'm not sure, maybe
> Connie or Paolo.  Christian, is this specifically what you're ack'ing?

I'm also unsure about the kvm->lock thing. Is taking the lock buried
somewhere deep in the code that will ultimately trigger the release?
I would at least like a pointer.

>
> It can ultimately go in through my tree, but not being familiar with
> this code I'd hope for more closure.  Thanks,
>
> Alex


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer
  2021-08-19 13:20     ` Tony Krowiak
@ 2021-08-19 17:54       ` Alex Williamson
  2021-08-19 17:58         ` Jason Gunthorpe
  0 siblings, 1 reply; 42+ messages in thread
From: Alex Williamson @ 2021-08-19 17:54 UTC (permalink / raw)
  To: Tony Krowiak
  Cc: Christian Borntraeger, linux-s390, linux-kernel, cohuck, pasic,
	jjherne, jgg, kwankhede, david

On Thu, 19 Aug 2021 09:20:28 -0400
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> On 8/18/21 1:03 PM, Christian Borntraeger wrote:
> > On 19.07.21 21:35, Tony Krowiak wrote:  
> >> The function pointer to the interception handler for the PQAP 
> >> instruction
> >> can get changed during the interception process. Let's add a
> >> semaphore to struct kvm_s390_crypto to control read/write access to the
> >> function pointer contained therein.
> >>
> >> The semaphore must be locked for write access by the vfio_ap device 
> >> driver
> >> when notified that the KVM pointer has been set or cleared. It must be
> >> locked for read access by the interception framework when the PQAP
> >> instruction is intercepted.
> >>
> >> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> >> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> >> ---
> >>   arch/s390/include/asm/kvm_host.h      |  8 +++-----
> >>   arch/s390/kvm/kvm-s390.c              |  1 +
> >>   arch/s390/kvm/priv.c                  | 10 ++++++----
> >>   drivers/s390/crypto/vfio_ap_ops.c     | 23 +++++++++++++++++------
> >>   drivers/s390/crypto/vfio_ap_private.h |  2 +-
> >>   5 files changed, 28 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/arch/s390/include/asm/kvm_host.h 
> >> b/arch/s390/include/asm/kvm_host.h
> >> index 9b4473f76e56..f18849d259e6 100644
> >> --- a/arch/s390/include/asm/kvm_host.h
> >> +++ b/arch/s390/include/asm/kvm_host.h
> >> @@ -798,14 +798,12 @@ struct kvm_s390_cpu_model {
> >>       unsigned short ibc;
> >>   };
> >>   -struct kvm_s390_module_hook {
> >> -    int (*hook)(struct kvm_vcpu *vcpu);
> >> -    struct module *owner;
> >> -};
> >> +typedef int (*crypto_hook)(struct kvm_vcpu *vcpu);
> >>     struct kvm_s390_crypto {
> >>       struct kvm_s390_crypto_cb *crycb;
> >> -    struct kvm_s390_module_hook *pqap_hook;
> >> +    struct rw_semaphore pqap_hook_rwsem;
> >> +    crypto_hook *pqap_hook;
> >>       __u32 crycbd;
> >>       __u8 aes_kw;
> >>       __u8 dea_kw;
> >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> >> index b655a7d82bf0..a08f242a9f27 100644
> >> --- a/arch/s390/kvm/kvm-s390.c
> >> +++ b/arch/s390/kvm/kvm-s390.c
> >> @@ -2630,6 +2630,7 @@ static void kvm_s390_crypto_init(struct kvm *kvm)
> >>   {
> >>       kvm->arch.crypto.crycb = &kvm->arch.sie_page2->crycb;
> >>       kvm_s390_set_crycb_format(kvm);
> >> +    init_rwsem(&kvm->arch.crypto.pqap_hook_rwsem);
> >>         if (!test_kvm_facility(kvm, 76))
> >>           return;
> >> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
> >> index 9928f785c677..6bed9406c1f3 100644
> >> --- a/arch/s390/kvm/priv.c
> >> +++ b/arch/s390/kvm/priv.c
> >> @@ -610,6 +610,7 @@ static int handle_io_inst(struct kvm_vcpu *vcpu)
> >>   static int handle_pqap(struct kvm_vcpu *vcpu)
> >>   {
> >>       struct ap_queue_status status = {};
> >> +    crypto_hook pqap_hook;
> >>       unsigned long reg0;
> >>       int ret;
> >>       uint8_t fc;
> >> @@ -657,15 +658,16 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
> >>        * Verify that the hook callback is registered, lock the owner
> >>        * and call the hook.
> >>        */
> >> + down_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem);
> >>       if (vcpu->kvm->arch.crypto.pqap_hook) {
> >> -        if (!try_module_get(vcpu->kvm->arch.crypto.pqap_hook->owner))
> >> -            return -EOPNOTSUPP;
> >> -        ret = vcpu->kvm->arch.crypto.pqap_hook->hook(vcpu);
> >> - module_put(vcpu->kvm->arch.crypto.pqap_hook->owner);
> >> +        pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook;  
> >
> > Dont we have to check for NULL here? If not can you add a comment why?  
> 
> Take a look above the removed lines: if (vcpu->kvm->arch.crypto.pqap_hook)
> 
> >
> > Otherwise this looks good.  
> 
> Also, in the cover letter I said this patch was already queued and was
> included here because it pre-reqs the second patch. Is this patch not
> already in Alex's tree?

Nope.  The only requests for merges through my tree that I'm aware of
were [1] and what I understand was the evolution of that here now [2].
Maybe you're thinking of [3], which I do see in mainline where this was
2/2 in that series but afaict only patch 1/2 was committed.  I guess
that explains why there was no respin based on comments for this patch.
Thanks,

Alex

[1]https://lore.kernel.org/linux-s390/9c50fb1b-4574-0cfc-487c-64108d97ed73@de.ibm.com/
[2]https://lore.kernel.org/linux-s390/6d64bd83-1519-6065-a4cd-9356c6be5d1a@de.ibm.com/
[3]https://lore.kernel.org/linux-s390/e809be5b-0b24-34dc-1eae-82b58dc54545@de.ibm.com/


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer
  2021-08-19 17:54       ` Alex Williamson
@ 2021-08-19 17:58         ` Jason Gunthorpe
  2021-08-20 15:59           ` Tony Krowiak
  2021-08-20 22:05           ` Tony Krowiak
  0 siblings, 2 replies; 42+ messages in thread
From: Jason Gunthorpe @ 2021-08-19 17:58 UTC (permalink / raw)
  To: Alex Williamson
  Cc: Tony Krowiak, Christian Borntraeger, linux-s390, linux-kernel,
	cohuck, pasic, jjherne, kwankhede, david

On Thu, Aug 19, 2021 at 11:54:33AM -0600, Alex Williamson wrote:

> Nope.  The only requests for merges through my tree that I'm aware of
> were [1] and what I understand was the evolution of that here now [2].
> Maybe you're thinking of [3], which I do see in mainline where this was
> 2/2 in that series but afaict only patch 1/2 was committed.  I guess
> that explains why there was no respin based on comments for this patch.
> Thanks,

Tony,

If you take Alex's tree from here:

https://github.com/awilliam/linux-vfio/commits/next

And rebase + repost exactly the patches you need applied it would be
helpful.

Jason

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer
  2021-08-19 13:36       ` Tony Krowiak
@ 2021-08-19 21:42         ` Halil Pasic
  2021-08-23 13:08           ` Tony Krowiak
  0 siblings, 1 reply; 42+ messages in thread
From: Halil Pasic @ 2021-08-19 21:42 UTC (permalink / raw)
  To: Tony Krowiak
  Cc: Christian Borntraeger, linux-s390, linux-kernel, cohuck, pasic,
	jjherne, jgg, alex.williamson, kwankhede, david

On Thu, 19 Aug 2021 09:36:34 -0400
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> >>>    static int handle_pqap(struct kvm_vcpu *vcpu)
> >>>    {
> >>>    	struct ap_queue_status status = {};
> >>> +	crypto_hook pqap_hook;
> >>>    	unsigned long reg0;
> >>>    	int ret;
> >>>    	uint8_t fc;
> >>> @@ -657,15 +658,16 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
> >>>    	 * Verify that the hook callback is registered, lock the owner
> >>>    	 * and call the hook.
> >>>    	 */
> >>> +	down_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem);
> >>>    	if (vcpu->kvm->arch.crypto.pqap_hook) {                     <--- HERE
> >>> -		if (!try_module_get(vcpu->kvm->arch.crypto.pqap_hook->owner))
> >>> -			return -EOPNOTSUPP;
> >>> -		ret = vcpu->kvm->arch.crypto.pqap_hook->hook(vcpu);
> >>> -		module_put(vcpu->kvm->arch.crypto.pqap_hook->owner);
> >>> +		pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook;  
> >> Dont we have to check for NULL here? If not can you add a comment why?  
> > I believe we did the necessary check on the line I just marked with
> > "<--- HERE".
> >
> > I find that "*" operator confusing in this context as it doesn't do
> > any good for us. I believe this situation is described in 6.5.3.2.4 of
> > the c11 standard. For convenience I will cite from the corresponding
> > draft:
> > "The unary * operator denotes indirection. If the operand points to a
> > function, the result is a function designator; if it points to an
> > object, the result is an lvalue designating the object. If the operand
> > has type ‘‘pointer to type’’, the result has type ‘‘type’’. If an
> > invalid value has been assigned to the pointer, the behavior of the
> > unary * operator is undefined."
> >
> > Frankly I also fail to see the benefit of introducing the local variable
> > named "pqap_hook", but back then I decided to not complain about style.  
> 
> The vcpu->kvm->arch.crypto.pqap_hook is a pointer to a function
> pointer. The actual function pointer is stored in matrix_mdev->pqap_hook,
> the reason being that the handle_pqap function in vfio_ap_ops.c
> retrieves the matrix_mdev via a container_of macro. The dereferencing
> of the vcpu->kvm->arch.crypto.pqap_hook into a local variable was
> to get the function pointer. There may have been a more stylish
> way of doing this, but the functionality is there.

You are right, and I was wrong. But then we do have to distinct pointer
deferences, and we check for NULL only once.

I still do believe we do not have a potential null pointer dereference
here, but the reason for that is that vfio-ap (the party that manages
these pointers) guarantees that whenever
vcpu->kvm->arch.crypto.pqap_hook != NULL is true, 
*vcpu->kvm->arch.crypto.pqap_hook != NULL is also true (and also that
the function pointer is a valid one). Which is the case, because we
set matrix_mdev->pqap_hook in vfio_ap_mdev_create() and don't touch
it any more.

In my opinion it is worth a comment.


> 
> >
> > Regards,
> > Halil
> >  
> >>  
> >>> +		ret = pqap_hook(vcpu);

BTW the second dereference takes place here.

If we wanted, we could make sure we don't dereference a null pointer
here but I think that would be an overkill.

Regards,
Halil  
> >> [...]  


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 0/2] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification
  2021-08-19 15:30           ` Cornelia Huck
@ 2021-08-20 14:24             ` Tony Krowiak
  0 siblings, 0 replies; 42+ messages in thread
From: Tony Krowiak @ 2021-08-20 14:24 UTC (permalink / raw)
  To: Cornelia Huck, Alex Williamson, Christian Borntraeger
  Cc: Halil Pasic, linux-s390, linux-kernel, pasic, jjherne, jgg,
	kwankhede, david, pbonzini, frankja, imbrenda



On 8/19/21 11:30 AM, Cornelia Huck wrote:
> On Wed, Aug 18 2021, Alex Williamson <alex.williamson@redhat.com> wrote:
>
>> On Wed, 18 Aug 2021 17:59:51 +0200
>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>
>>> On 02.08.21 18:32, Tony Krowiak wrote:
>>>>
>>>> On 8/2/21 9:53 AM, Halil Pasic wrote:
>>>>> On Mon, 2 Aug 2021 09:10:26 -0400
>>>>> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>>>>>   
>>>>>> PING!
>>>>>>
>>>>>> This patch will pre-req version 17 of a patch series I have waiting in
>>>>>> the wings,
>>>>>> so I'd like to get this one merged ASAP. In particular, if a KVM
>>>>>> maintainer can
>>>>>> take a look at the comments concerning the taking of the kvm->lock
>>>>>> before the
>>>>>> matrix_mdev->lock it would be greatly appreciated. Those comments begin with
>>>>>> Message ID <20210727004329.3bcc7d4f.pasic@linux.ibm.com> from Halil Pasic.
>>>>> As far as I'm concerned, we can move forward with this. Was this
>>>>> supposed to go in via Alex's tree?
>>>> I am not certain, Christian queued the previous patches related to
>>>> this on:
>>>>
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git/log/?h=fixes
>>>>
>>>> Jason G., since this will need to be integrated with your other patches,
>>>> where should this be queued?
>>>
>>> This previous patch (s390/vfio-ap: clean up mdev resources when remove callback invoked) is
>>> already in master.
>>> Can you respin the series with all Acks and RBs?
>>>
>>> Alex, can you then take these 2 patches via your tree? Thanks
>>>
>>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>> for this series.
>>
>> I see some review feedback that seems to suggest a new version would be
>> posted:
>>
>> https://lore.kernel.org/linux-s390/0f03ab0b-2dfd-e1c1-fe43-be2a59030a71@linux.ibm.com/
> Yeah, I thought so as well. But it also looks like something that could
> be a fixup on top.

I will post the new patch today. I was waiting for the remainder of
the feedback and frankly forgot to post the patch incorporating
the changes precipitated by the previous comments.

>
>> I also see in this thread:
>>
>> https://lore.kernel.org/linux-s390/20210721164550.5402fe1c.pasic@linux.ibm.com/
>>
>> that Halil's concern's around open/close races are addressed by Jason's
>> device_open/close series that's already in my next branch and he
>> provided an Ack, but there's still the above question regarding the
>> kvm->lock that was looking for a review from... I'm not sure, maybe
>> Connie or Paolo.  Christian, is this specifically what you're ack'ing?
> I'm also unsure about the kvm->lock thing. Is taking the lock buried
> somewhere deep in the code that will ultimately trigger the release?
> I would at least like a pointer.

I'm not quite sure what you're asking here, but if you follow the
thread starting with the link above it may reveal the answer to
what you are asking here.


>
>> It can ultimately go in through my tree, but not being familiar with
>> this code I'd hope for more closure.  Thanks,
>>
>> Alex


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer
  2021-08-19 17:58         ` Jason Gunthorpe
@ 2021-08-20 15:59           ` Tony Krowiak
  2021-08-20 22:05           ` Tony Krowiak
  1 sibling, 0 replies; 42+ messages in thread
From: Tony Krowiak @ 2021-08-20 15:59 UTC (permalink / raw)
  To: Jason Gunthorpe, Alex Williamson
  Cc: Christian Borntraeger, linux-s390, linux-kernel, cohuck, pasic,
	jjherne, kwankhede, david



On 8/19/21 1:58 PM, Jason Gunthorpe wrote:
> On Thu, Aug 19, 2021 at 11:54:33AM -0600, Alex Williamson wrote:
>
>> Nope.  The only requests for merges through my tree that I'm aware of
>> were [1] and what I understand was the evolution of that here now [2].
>> Maybe you're thinking of [3], which I do see in mainline where this was
>> 2/2 in that series but afaict only patch 1/2 was committed.  I guess
>> that explains why there was no respin based on comments for this patch.
>> Thanks,
> Tony,
>
> If you take Alex's tree from here:
>
> https://github.com/awilliam/linux-vfio/commits/next
>
> And rebase + repost exactly the patches you need applied it would be
> helpful.

Will do.

>
> Jason


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer
  2021-08-19 17:58         ` Jason Gunthorpe
  2021-08-20 15:59           ` Tony Krowiak
@ 2021-08-20 22:05           ` Tony Krowiak
  2021-08-20 22:30             ` Jason Gunthorpe
  2021-08-20 22:41             ` Alex Williamson
  1 sibling, 2 replies; 42+ messages in thread
From: Tony Krowiak @ 2021-08-20 22:05 UTC (permalink / raw)
  To: Jason Gunthorpe, Alex Williamson
  Cc: Christian Borntraeger, linux-s390, linux-kernel, cohuck, pasic,
	jjherne, kwankhede, david



On 8/19/21 1:58 PM, Jason Gunthorpe wrote:
> On Thu, Aug 19, 2021 at 11:54:33AM -0600, Alex Williamson wrote:
>
>> Nope.  The only requests for merges through my tree that I'm aware of
>> were [1] and what I understand was the evolution of that here now [2].
>> Maybe you're thinking of [3], which I do see in mainline where this was
>> 2/2 in that series but afaict only patch 1/2 was committed.  I guess
>> that explains why there was no respin based on comments for this patch.
>> Thanks,
> Tony,
>
> If you take Alex's tree from here:
>
> https://github.com/awilliam/linux-vfio/commits/next

I navigated to this URL and clicked the green 'Code'
button. I was given the option to download the zip file or
use git to checkout the code at the URL displayed
'https://github.com/awilliam/linux-vfio.git'. I cloned the
repo at that URL and the code was definitely not in any
way similar to my code base. In particular, the
arch/s390/include/asm/kvm_host.h file did not have any
of the crypto structures.

I then downloaded the zip file and expanded it. The code
looked legitimate, but this was not a git repository, so I
had no way to cherry-pick my patches nor format patches
to post to this mailing list.

Next, I tried cloning from 
'https://github.com/awilliam/linux-vfio-next.git',
but I was prompted for uid/pw.

So, the question is, how to I get the linux-vfio-next repo upon which I
can rebase my patches? I apologize for my ignorance.

>
> And rebase + repost exactly the patches you need applied it would be
> helpful.
>
> Jason


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer
  2021-08-20 22:05           ` Tony Krowiak
@ 2021-08-20 22:30             ` Jason Gunthorpe
  2021-08-23 15:17               ` Tony Krowiak
  2021-08-20 22:41             ` Alex Williamson
  1 sibling, 1 reply; 42+ messages in thread
From: Jason Gunthorpe @ 2021-08-20 22:30 UTC (permalink / raw)
  To: Tony Krowiak
  Cc: Alex Williamson, Christian Borntraeger, linux-s390, linux-kernel,
	cohuck, pasic, jjherne, kwankhede, david

On Fri, Aug 20, 2021 at 06:05:08PM -0400, Tony Krowiak wrote:

> So, the question is, how to I get the linux-vfio-next repo upon which I
> can rebase my patches? I apologize for my ignorance.

Get yourself a kernel git tree somehow, eg by cloning one you already
have

Then something like

$ git fetch https://github.com/awilliam/linux-vfio.git next
$ git reset --hard FETCH_HEAD

Will sort it out, though there are many other varients such as adding
a remote/etc.

When you cloned it from github git checked out the wrong branch for
you - 'git reset --hard origin/next' would fix it too.

Jason

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer
  2021-08-20 22:05           ` Tony Krowiak
  2021-08-20 22:30             ` Jason Gunthorpe
@ 2021-08-20 22:41             ` Alex Williamson
  2021-08-23 20:51               ` Tony Krowiak
  1 sibling, 1 reply; 42+ messages in thread
From: Alex Williamson @ 2021-08-20 22:41 UTC (permalink / raw)
  To: Tony Krowiak
  Cc: Jason Gunthorpe, Christian Borntraeger, linux-s390, linux-kernel,
	cohuck, pasic, jjherne, kwankhede, david

On Fri, 20 Aug 2021 18:05:08 -0400
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> On 8/19/21 1:58 PM, Jason Gunthorpe wrote:
> > On Thu, Aug 19, 2021 at 11:54:33AM -0600, Alex Williamson wrote:
> >  
> >> Nope.  The only requests for merges through my tree that I'm aware of
> >> were [1] and what I understand was the evolution of that here now [2].
> >> Maybe you're thinking of [3], which I do see in mainline where this was
> >> 2/2 in that series but afaict only patch 1/2 was committed.  I guess
> >> that explains why there was no respin based on comments for this patch.
> >> Thanks,  
> > Tony,
> >
> > If you take Alex's tree from here:
> >
> > https://github.com/awilliam/linux-vfio/commits/next  
> 
> I navigated to this URL and clicked the green 'Code'
> button. I was given the option to download the zip file or
> use git to checkout the code at the URL displayed
> 'https://github.com/awilliam/linux-vfio.git'. I cloned the
> repo at that URL and the code was definitely not in any
> way similar to my code base. In particular, the
> arch/s390/include/asm/kvm_host.h file did not have any
> of the crypto structures.
> 
> I then downloaded the zip file and expanded it. The code
> looked legitimate, but this was not a git repository, so I
> had no way to cherry-pick my patches nor format patches
> to post to this mailing list.
> 
> Next, I tried cloning from 
> 'https://github.com/awilliam/linux-vfio-next.git',
> but I was prompted for uid/pw.
> 
> So, the question is, how to I get the linux-vfio-next repo upon which I
> can rebase my patches? I apologize for my ignorance.

You can use git fetch to download the objects, ex:

$ git fetch git://github.com/awilliam/linux-vfio.git next
$ git checkout FETCH_HEAD

Or you could add a remote, ex:

$ git remote add vfio git://github.com/awilliam/linux-vfio.git
$ git remote update vfio
$ git checkout vfio/next

The former might be easier and add a lot less crufty objects to your
local tree if this is a one-off activity.  Thanks,

Alex


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer
  2021-08-19 21:42         ` Halil Pasic
@ 2021-08-23 13:08           ` Tony Krowiak
  0 siblings, 0 replies; 42+ messages in thread
From: Tony Krowiak @ 2021-08-23 13:08 UTC (permalink / raw)
  To: Halil Pasic
  Cc: Christian Borntraeger, linux-s390, linux-kernel, cohuck, pasic,
	jjherne, jgg, alex.williamson, kwankhede, david



On 8/19/21 5:42 PM, Halil Pasic wrote:
> On Thu, 19 Aug 2021 09:36:34 -0400
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>
>>>>>     static int handle_pqap(struct kvm_vcpu *vcpu)
>>>>>     {
>>>>>     	struct ap_queue_status status = {};
>>>>> +	crypto_hook pqap_hook;
>>>>>     	unsigned long reg0;
>>>>>     	int ret;
>>>>>     	uint8_t fc;
>>>>> @@ -657,15 +658,16 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
>>>>>     	 * Verify that the hook callback is registered, lock the owner
>>>>>     	 * and call the hook.
>>>>>     	 */
>>>>> +	down_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem);
>>>>>     	if (vcpu->kvm->arch.crypto.pqap_hook) {                     <--- HERE
>>>>> -		if (!try_module_get(vcpu->kvm->arch.crypto.pqap_hook->owner))
>>>>> -			return -EOPNOTSUPP;
>>>>> -		ret = vcpu->kvm->arch.crypto.pqap_hook->hook(vcpu);
>>>>> -		module_put(vcpu->kvm->arch.crypto.pqap_hook->owner);
>>>>> +		pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook;
>>>> Dont we have to check for NULL here? If not can you add a comment why?
>>> I believe we did the necessary check on the line I just marked with
>>> "<--- HERE".
>>>
>>> I find that "*" operator confusing in this context as it doesn't do
>>> any good for us. I believe this situation is described in 6.5.3.2.4 of
>>> the c11 standard. For convenience I will cite from the corresponding
>>> draft:
>>> "The unary * operator denotes indirection. If the operand points to a
>>> function, the result is a function designator; if it points to an
>>> object, the result is an lvalue designating the object. If the operand
>>> has type ‘‘pointer to type’’, the result has type ‘‘type’’. If an
>>> invalid value has been assigned to the pointer, the behavior of the
>>> unary * operator is undefined."
>>>
>>> Frankly I also fail to see the benefit of introducing the local variable
>>> named "pqap_hook", but back then I decided to not complain about style.
>> The vcpu->kvm->arch.crypto.pqap_hook is a pointer to a function
>> pointer. The actual function pointer is stored in matrix_mdev->pqap_hook,
>> the reason being that the handle_pqap function in vfio_ap_ops.c
>> retrieves the matrix_mdev via a container_of macro. The dereferencing
>> of the vcpu->kvm->arch.crypto.pqap_hook into a local variable was
>> to get the function pointer. There may have been a more stylish
>> way of doing this, but the functionality is there.
> You are right, and I was wrong. But then we do have to distinct pointer
> deferences, and we check for NULL only once.
>
> I still do believe we do not have a potential null pointer dereference
> here, but the reason for that is that vfio-ap (the party that manages
> these pointers) guarantees that whenever
> vcpu->kvm->arch.crypto.pqap_hook != NULL is true,
> *vcpu->kvm->arch.crypto.pqap_hook != NULL is also true (and also that
> the function pointer is a valid one). Which is the case, because we
> set matrix_mdev->pqap_hook in vfio_ap_mdev_create() and don't touch
> it any more.
>
> In my opinion it is worth a comment.

Even I had to look at it again to respond to you, so a comment
is probably called for.

>
>
>>> Regards,
>>> Halil
>>>   
>>>>   
>>>>> +		ret = pqap_hook(vcpu);
> BTW the second dereference takes place here.
>
> If we wanted, we could make sure we don't dereference a null pointer
> here but I think that would be an overkill.

I agree, it is overkill.

>
> Regards,
> Halil
>>>> [...]


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer
  2021-08-20 22:30             ` Jason Gunthorpe
@ 2021-08-23 15:17               ` Tony Krowiak
  0 siblings, 0 replies; 42+ messages in thread
From: Tony Krowiak @ 2021-08-23 15:17 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Alex Williamson, Christian Borntraeger, linux-s390, linux-kernel,
	cohuck, pasic, jjherne, kwankhede, david

Thanks, I think I've got it now.

On 8/20/21 6:30 PM, Jason Gunthorpe wrote:
> git reset --hard origin/next


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer
  2021-08-20 22:41             ` Alex Williamson
@ 2021-08-23 20:51               ` Tony Krowiak
  0 siblings, 0 replies; 42+ messages in thread
From: Tony Krowiak @ 2021-08-23 20:51 UTC (permalink / raw)
  To: Alex Williamson
  Cc: Jason Gunthorpe, Christian Borntraeger, linux-s390, linux-kernel,
	cohuck, pasic, jjherne, kwankhede, david



On 8/20/21 6:41 PM, Alex Williamson wrote:
> On Fri, 20 Aug 2021 18:05:08 -0400
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>
>> On 8/19/21 1:58 PM, Jason Gunthorpe wrote:
>>> On Thu, Aug 19, 2021 at 11:54:33AM -0600, Alex Williamson wrote:
>>>   
>>>> Nope.  The only requests for merges through my tree that I'm aware of
>>>> were [1] and what I understand was the evolution of that here now [2].
>>>> Maybe you're thinking of [3], which I do see in mainline where this was
>>>> 2/2 in that series but afaict only patch 1/2 was committed.  I guess
>>>> that explains why there was no respin based on comments for this patch.
>>>> Thanks,
>>> Tony,
>>>
>>> If you take Alex's tree from here:
>>>
>>> https://github.com/awilliam/linux-vfio/commits/next
>> I navigated to this URL and clicked the green 'Code'
>> button. I was given the option to download the zip file or
>> use git to checkout the code at the URL displayed
>> 'https://github.com/awilliam/linux-vfio.git'. I cloned the
>> repo at that URL and the code was definitely not in any
>> way similar to my code base. In particular, the
>> arch/s390/include/asm/kvm_host.h file did not have any
>> of the crypto structures.
>>
>> I then downloaded the zip file and expanded it. The code
>> looked legitimate, but this was not a git repository, so I
>> had no way to cherry-pick my patches nor format patches
>> to post to this mailing list.
>>
>> Next, I tried cloning from
>> 'https://github.com/awilliam/linux-vfio-next.git',
>> but I was prompted for uid/pw.
>>
>> So, the question is, how to I get the linux-vfio-next repo upon which I
>> can rebase my patches? I apologize for my ignorance.
> You can use git fetch to download the objects, ex:
>
> $ git fetch git://github.com/awilliam/linux-vfio.git next
> $ git checkout FETCH_HEAD
>
> Or you could add a remote, ex:
>
> $ git remote add vfio git://github.com/awilliam/linux-vfio.git
> $ git remote update vfio
> $ git checkout vfio/next
>
> The former might be easier and add a lot less crufty objects to your
> local tree if this is a one-off activity.  Thanks,
>
> Alex

Thanks Alex, I was able to get the repo, cherry-pick my patches and
build and test the kernel. Barring any anomalies, I will be posting the
patches today.

>


^ permalink raw reply	[flat|nested] 42+ messages in thread

end of thread, other threads:[~2021-08-23 20:51 UTC | newest]

Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-19 19:35 [PATCH 0/2] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification Tony Krowiak
2021-07-19 19:35 ` [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer Tony Krowiak
2021-08-18 17:03   ` Christian Borntraeger
2021-08-18 23:25     ` Halil Pasic
2021-08-19  6:56       ` Christian Borntraeger
2021-08-19 13:36       ` Tony Krowiak
2021-08-19 21:42         ` Halil Pasic
2021-08-23 13:08           ` Tony Krowiak
2021-08-19 13:20     ` Tony Krowiak
2021-08-19 17:54       ` Alex Williamson
2021-08-19 17:58         ` Jason Gunthorpe
2021-08-20 15:59           ` Tony Krowiak
2021-08-20 22:05           ` Tony Krowiak
2021-08-20 22:30             ` Jason Gunthorpe
2021-08-23 15:17               ` Tony Krowiak
2021-08-20 22:41             ` Alex Williamson
2021-08-23 20:51               ` Tony Krowiak
2021-07-19 19:35 ` [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification Tony Krowiak
2021-07-21 14:45   ` Halil Pasic
2021-07-22 13:09     ` Tony Krowiak
2021-07-23 14:26       ` Halil Pasic
2021-07-23 21:24         ` Tony Krowiak
2021-07-26 20:36           ` Halil Pasic
2021-07-26 22:03             ` Jason Gunthorpe
2021-07-26 22:43               ` Halil Pasic
2021-07-28 13:43                 ` Tony Krowiak
2021-07-28 19:42                   ` Halil Pasic
2021-07-30 13:33                     ` Tony Krowiak
2021-07-27  6:54               ` Christoph Hellwig
2021-07-21 19:37   ` Jason J. Herne
2021-07-22 13:16     ` Tony Krowiak
2021-08-02 13:10 ` [PATCH 0/2] s390/vfio-ap: do not open code " Tony Krowiak
2021-08-02 13:53   ` Halil Pasic
2021-08-02 16:32     ` Tony Krowiak
2021-08-03 13:08       ` Jason Gunthorpe
2021-08-03 13:34         ` Tony Krowiak
2021-08-18 15:59       ` Christian Borntraeger
2021-08-18 16:39         ` Alex Williamson
2021-08-18 16:50           ` Christian Borntraeger
2021-08-18 22:52             ` Halil Pasic
2021-08-19 15:30           ` Cornelia Huck
2021-08-20 14:24             ` Tony Krowiak

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).