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().
Okay
+ 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?
The VFIO_DEVICE_RESET ioctl expects a return code.
The vfio_ap_mdev_reset_queues() function can return -EIO or
-EBUSY, so I would expect userspace to handle -ENODEV
similarly to -EIO or any other non-zero return code. I also
looked at all of the VFIO_DEVICE_RESET calls from QEMU to see
how the return from the ioctl call is handled:
* ap: reports the reset failed along with the rc
* ccw: doesn't check the rc
* pci: kind of hard to follow without digging deep, but definitely
handles non-zero rc.
I think the caller should be notified whether the queues were
successfully reset or not, and why; in this case, the answer is
there are no devices to reset.
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,
