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
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 <[email protected]>
---
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
On Mon, 19 Jul 2021 15:35:03 -0400
Tony Krowiak <[email protected]> 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
[..]
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 <[email protected]>
> ---
> 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 ([email protected])
On 7/21/21 10:45 AM, Halil Pasic wrote:
> On Mon, 19 Jul 2021 15:35:03 -0400
> Tony Krowiak <[email protected]> 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
>
> [..]
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 <[email protected]>
>> ---
>> 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?
>
>
>
On Thu, 22 Jul 2021 09:09:26 -0400
Tony Krowiak <[email protected]> wrote:
> On 7/21/21 10:45 AM, Halil Pasic wrote:
> > On Mon, 19 Jul 2021 15:35:03 -0400
> > Tony Krowiak <[email protected]> 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
On 7/23/21 10:26 AM, Halil Pasic wrote:
> On Thu, 22 Jul 2021 09:09:26 -0400
> Tony Krowiak <[email protected]> wrote:
>
>> On 7/21/21 10:45 AM, Halil Pasic wrote:
>>> On Mon, 19 Jul 2021 15:35:03 -0400
>>> Tony Krowiak <[email protected]> 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
On Fri, 23 Jul 2021 17:24:16 -0400
Tony Krowiak <[email protected]> wrote:
> On 7/23/21 10:26 AM, Halil Pasic wrote:
> > On Thu, 22 Jul 2021 09:09:26 -0400
> > Tony Krowiak <[email protected]> wrote:
> >
> >> On 7/21/21 10:45 AM, Halil Pasic wrote:
> >>> On Mon, 19 Jul 2021 15:35:03 -0400
> >>> Tony Krowiak <[email protected]> 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
>
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
On Mon, 26 Jul 2021 19:03:17 -0300
Jason Gunthorpe <[email protected]> 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 <[email protected]>
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
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?
On 7/26/21 6:43 PM, Halil Pasic wrote:
> On Mon, 26 Jul 2021 19:03:17 -0300
> Jason Gunthorpe <[email protected]> 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 <[email protected]>
>
> 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
On Wed, 28 Jul 2021 09:43:03 -0400
Tony Krowiak <[email protected]> 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
On 7/28/21 3:42 PM, Halil Pasic wrote:
> On Wed, 28 Jul 2021 09:43:03 -0400
> Tony Krowiak <[email protected]> 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
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 <[email protected]> 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(-)
>
On Mon, 2 Aug 2021 09:10:26 -0400
Tony Krowiak <[email protected]> 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 <[email protected]> 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(-)
> >
>
On 8/2/21 9:53 AM, Halil Pasic wrote:
> On Mon, 2 Aug 2021 09:10:26 -0400
> Tony Krowiak <[email protected]> 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 <[email protected]> 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(-)
>>>
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 <[email protected]> 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 <[email protected]> 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
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 <[email protected]> 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 <[email protected]> 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
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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
for this series.
On Wed, 18 Aug 2021 17:59:51 +0200
Christian Borntraeger <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> for this series.
I see some review feedback that seems to suggest a new version would be
posted:
https://lore.kernel.org/linux-s390/[email protected]/
I also see in this thread:
https://lore.kernel.org/linux-s390/[email protected]/
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
On 18.08.21 18:39, Alex Williamson wrote:
> On Wed, 18 Aug 2021 17:59:51 +0200
> Christian Borntraeger <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
>> for this series.
>
>
> I see some review feedback that seems to suggest a new version would be
> posted:
>
> https://lore.kernel.org/linux-s390/[email protected]/
>
> I also see in this thread:
>
> https://lore.kernel.org/linux-s390/[email protected]/
>
> 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
>
On Wed, 18 Aug 2021 18:50:47 +0200
Christian Borntraeger <[email protected]> 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
On Wed, Aug 18 2021, Alex Williamson <[email protected]> wrote:
> On Wed, 18 Aug 2021 17:59:51 +0200
> Christian Borntraeger <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
>> for this series.
>
>
> I see some review feedback that seems to suggest a new version would be
> posted:
>
> https://lore.kernel.org/linux-s390/[email protected]/
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/[email protected]/
>
> 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
On 8/19/21 11:30 AM, Cornelia Huck wrote:
> On Wed, Aug 18 2021, Alex Williamson <[email protected]> wrote:
>
>> On Wed, 18 Aug 2021 17:59:51 +0200
>> Christian Borntraeger <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>> for this series.
>>
>> I see some review feedback that seems to suggest a new version would be
>> posted:
>>
>> https://lore.kernel.org/linux-s390/[email protected]/
> 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/[email protected]/
>>
>> 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