On Wed, 3 Mar 2021 12:10:11 -0500 Tony Krowiak akrowiak@linux.ibm.com wrote:
On 3/3/21 10:23 AM, Halil Pasic wrote:
On Tue, 2 Mar 2021 15:43:22 -0500 Tony Krowiak akrowiak@linux.ibm.com 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);elseret = -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()).
My point is if !->kvm the other required conditions are not met. But yes we can go back to unconditional vfio_ap_mdev_reset_queues(mdev), and think about the necessity of performing a vfio_ap_mdev_reset_queues() if !->kvm later as I proposed in the other mail.
Regards, Halil