| Message ID | 20220404221039.1272245-9-akrowiak@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390/vfio-ap: dynamic configuration support | expand |
On 4/4/22 18:10, Tony Krowiak wrote: > The vfio_ap device driver registers for notification when the pointer to > the KVM object for a guest is set. Recall that the KVM lock (kvm->lock) > mutex must be taken outside of the matrix_dev->lock mutex to prevent the > reporting by lockdep of a circular locking dependency (a.k.a., a lockdep > splat): > > * see commit 0cc00c8d4050 ("Fix circular lockdep when setting/clearing > crypto masks") > > * see commit 86956e70761b ("replace open coded locks for > VFIO_GROUP_NOTIFY_SET_KVM notification") > > With the introduction of support for hot plugging/unplugging AP devices > passed through to a KVM guest, a new guests_lock mutex is introduced to > ensure the proper locking order is maintained: > > struct ap_matrix_dev { > ... > struct mutex guests_lock; > ... > } > > The matrix_dev->guests_lock controls access to the matrix_mdev instances > that hold the state for AP devices that have been passed through to a > KVM guest. This lock must be held to control access to the KVM pointer > (matrix_mdev->kvm) while the vfio_ap device driver is using it to > plug/unplug AP devices passed through to the KVM guest. > > Keep in mind, the proper locking order must be maintained whenever > dynamically updating a KVM guest's APCB to plug/unplug adapters, domains > and control domains: > > 1. matrix_dev->guests_lock: required to use the KVM pointer - stored in > a struct ap_matrix_mdev instance - to update a KVM guest's APCB > > 2. matrix_mdev->kvm->lock: required to update a guest's APCB > > 3. matrix_dev->mdevs_lock: required to access data stored in a > struct ap_matrix_mdev instance. > > Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> > --- > drivers/s390/crypto/vfio_ap_drv.c | 1 + > drivers/s390/crypto/vfio_ap_private.h | 6 ++++++ > 2 files changed, 7 insertions(+) > > diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c > index 0a5acd151a9b..c258e5f7fdfc 100644 > --- a/drivers/s390/crypto/vfio_ap_drv.c > +++ b/drivers/s390/crypto/vfio_ap_drv.c > @@ -161,6 +161,7 @@ static int vfio_ap_matrix_dev_create(void) > > mutex_init(&matrix_dev->mdevs_lock); > INIT_LIST_HEAD(&matrix_dev->mdev_list); > + mutex_init(&matrix_dev->guests_lock); > > dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME); > matrix_dev->device.parent = root_device; > diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h > index 5262e02192a4..ec926f2f2930 100644 > --- a/drivers/s390/crypto/vfio_ap_private.h > +++ b/drivers/s390/crypto/vfio_ap_private.h > @@ -39,6 +39,11 @@ > * single ap_matrix_mdev device. It's quite coarse but we don't > * expect much contention. > * @vfio_ap_drv: the vfio_ap device driver > + * @guests_lock: mutex for controlling access to a guest that is using AP > + * devices passed through by the vfio_ap device driver. This lock > + * will be taken when the AP devices are plugged into or unplugged > + * from a guest, and when an ap_matrix_mdev device is added to or > + * removed from @mdev_list or the list is iterated. > */ > struct ap_matrix_dev { > struct device device; > @@ -47,6 +52,7 @@ struct ap_matrix_dev { > struct list_head mdev_list; > struct mutex mdevs_lock; > struct ap_driver *vfio_ap_drv; > + struct mutex guests_lock; > }; > > extern struct ap_matrix_dev *matrix_dev; In isolation... Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c index 0a5acd151a9b..c258e5f7fdfc 100644 --- a/drivers/s390/crypto/vfio_ap_drv.c +++ b/drivers/s390/crypto/vfio_ap_drv.c @@ -161,6 +161,7 @@ static int vfio_ap_matrix_dev_create(void) mutex_init(&matrix_dev->mdevs_lock); INIT_LIST_HEAD(&matrix_dev->mdev_list); + mutex_init(&matrix_dev->guests_lock); dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME); matrix_dev->device.parent = root_device; diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h index 5262e02192a4..ec926f2f2930 100644 --- a/drivers/s390/crypto/vfio_ap_private.h +++ b/drivers/s390/crypto/vfio_ap_private.h @@ -39,6 +39,11 @@ * single ap_matrix_mdev device. It's quite coarse but we don't * expect much contention. * @vfio_ap_drv: the vfio_ap device driver + * @guests_lock: mutex for controlling access to a guest that is using AP + * devices passed through by the vfio_ap device driver. This lock + * will be taken when the AP devices are plugged into or unplugged + * from a guest, and when an ap_matrix_mdev device is added to or + * removed from @mdev_list or the list is iterated. */ struct ap_matrix_dev { struct device device; @@ -47,6 +52,7 @@ struct ap_matrix_dev { struct list_head mdev_list; struct mutex mdevs_lock; struct ap_driver *vfio_ap_drv; + struct mutex guests_lock; }; extern struct ap_matrix_dev *matrix_dev;
The vfio_ap device driver registers for notification when the pointer to the KVM object for a guest is set. Recall that the KVM lock (kvm->lock) mutex must be taken outside of the matrix_dev->lock mutex to prevent the reporting by lockdep of a circular locking dependency (a.k.a., a lockdep splat): * see commit 0cc00c8d4050 ("Fix circular lockdep when setting/clearing crypto masks") * see commit 86956e70761b ("replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification") With the introduction of support for hot plugging/unplugging AP devices passed through to a KVM guest, a new guests_lock mutex is introduced to ensure the proper locking order is maintained: struct ap_matrix_dev { ... struct mutex guests_lock; ... } The matrix_dev->guests_lock controls access to the matrix_mdev instances that hold the state for AP devices that have been passed through to a KVM guest. This lock must be held to control access to the KVM pointer (matrix_mdev->kvm) while the vfio_ap device driver is using it to plug/unplug AP devices passed through to the KVM guest. Keep in mind, the proper locking order must be maintained whenever dynamically updating a KVM guest's APCB to plug/unplug adapters, domains and control domains: 1. matrix_dev->guests_lock: required to use the KVM pointer - stored in a struct ap_matrix_mdev instance - to update a KVM guest's APCB 2. matrix_mdev->kvm->lock: required to update a guest's APCB 3. matrix_dev->mdevs_lock: required to access data stored in a struct ap_matrix_mdev instance. Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> --- drivers/s390/crypto/vfio_ap_drv.c | 1 + drivers/s390/crypto/vfio_ap_private.h | 6 ++++++ 2 files changed, 7 insertions(+)