On 3/3/21 10:23 AM, Halil Pasic wrote:
On Tue, 2 Mar 2021 15:43:22 -0500
Tony Krowiak <akrowiak@xxxxxxxxxxxxx> wrote:
This patch fixes a lockdep splat introduced by commit f21916ec4826
("s390/vfio-ap: clean up vfio_ap resources when KVM pointer invalidated").
The lockdep splat only occurs when starting a Secure Execution guest.
Crypto virtualization (vfio_ap) is not yet supported for SE guests;
however, in order to avoid this problem when support becomes available,
this fix is being provided.
[..]
@@ -1038,14 +1116,28 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
{
struct ap_matrix_mdev *m;
- list_for_each_entry(m, &matrix_dev->mdev_list, node) {
- if ((m != matrix_mdev) && (m->kvm == kvm))
- return -EPERM;
- }
+ if (kvm->arch.crypto.crycbd) {
+ matrix_mdev->kvm_busy = true;
- matrix_mdev->kvm = kvm;
- kvm_get_kvm(kvm);
- kvm->arch.crypto.pqap_hook = &matrix_mdev->pqap_hook;
+ list_for_each_entry(m, &matrix_dev->mdev_list, node) {
+ if ((m != matrix_mdev) && (m->kvm == kvm)) {
+ wake_up_all(&matrix_mdev->wait_for_kvm);
This ain't no good. kvm_busy will remain true if we take this exit. The
wake_up_all() is not needed, because we hold the lock, so nobody can
observe it if we don't forget kvm_busy set.
I suggest moving matrix_mdev->kvm_busy = true; after this loop, maybe right
before the unlock, and removing the wake_up_all().
+ return -EPERM;
+ }
+ }
+
+ kvm_get_kvm(kvm);
+ mutex_unlock(&matrix_dev->lock);
+ 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);
+ }
return 0;
}
[..]
@@ -1300,7 +1406,21 @@ static ssize_t vfio_ap_mdev_ioctl(struct mdev_device *mdev,
ret = vfio_ap_mdev_get_device_info(arg);
break;
case VFIO_DEVICE_RESET:
- ret = vfio_ap_mdev_reset_queues(mdev);
+ matrix_mdev = mdev_get_drvdata(mdev);
+
+ /*
+ * 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 == false,
+ mutex_unlock(&matrix_dev->lock),
+ mutex_lock(&matrix_dev->lock));
+
+ if (matrix_mdev->kvm)
+ ret = vfio_ap_mdev_reset_queues(mdev);
+ else
+ ret = -ENODEV;
I don't think rejecting the reset is a good idea. I have you a more detailed
explanation of the list, where we initially discussed this question.
How do you exect userspace to react to this -ENODEV?
After reading your more detailed explanation, I have come to the
conclusion that the test for matrix_mdev->kvm should not be
performed here and the the vfio_ap_mdev_reset_queues() function
should be called regardless. Each queue assigned to the mdev
that is also bound to the vfio_ap driver will get reset and its
IRQ resources cleaned up if they haven't already been and the
other required conditions are met (i.e., see
vfio_ap_mdev_free_irq_resources()).
Otherwise looks good to me!
I've tested your branch from yesterday (which looks to me like this patch
without the above check on ->kvm and reset) for the lockdep splat, but I
didn't do any comprehensive testing -- which would ensure that we didn't
break something else in the process. With the two issues fixed, and your
word that the patch was properly tested (except for the lockdep splat
which I tested myself), I feel comfortable with moving forward with this.
Regards,
