lkml.org 
[lkml]   [2022]   [May]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    SubjectRe: [PATCH v19 11/20] s390/vfio-ap: prepare for dynamic update of guest's APCB on queue probe/remove
    From


    On 5/27/22 9:50 AM, Jason J. Herne wrote:
    > On 4/4/22 18:10, Tony Krowiak wrote:
    >> The callback functions for probing and removing a queue device must take
    >> and release the locks required to perform a dynamic update of a guest's
    >> APCB in the proper order.
    >>
    >> The proper order for taking the locks is:
    >>
    >>          matrix_dev->guests_lock => kvm->lock => matrix_dev->mdevs_lock
    >>
    >> The proper order for releasing the locks is:
    >>
    >>          matrix_dev->mdevs_lock => kvm->lock => matrix_dev->guests_lock
    >>
    >> A new helper function is introduced to be used by the probe callback to
    >> acquire the required locks. Since the probe callback only has
    >> access to a queue device when it is called, the helper function will
    >> find
    >> the ap_matrix_mdev object to which the queue device's APQN is
    >> assigned and
    >> return it so the KVM guest to which the mdev is attached can be
    >> dynamically
    >> updated.
    >>
    >> Note that in order to find the ap_matrix_mdev (matrix_mdev) object,
    >> it is
    >> necessary to search the matrix_dev->mdev_list. This presents a
    >> locking order dilemma because the matrix_dev->mdevs_lock can't be
    >> taken to
    >> protect against changes to the list while searching for the
    >> matrix_mdev to
    >> which a queue device's APQN is assigned. This is due to the fact that
    >> the
    >> proper locking order requires that the matrix_dev->mdevs_lock be taken
    >> after both the matrix_mdev->kvm->lock and the matrix_dev->mdevs_lock.
    >> Consequently, the matrix_dev->guests_lock will be used to protect
    >> against
    >> removal of a matrix_mdev object from the list while a queue device is
    >> being probed. This necessitates changes to the mdev probe/remove
    >> callback functions to take the matrix_dev->guests_lock prior to removing
    >> a matrix_mdev object from the list.
    >>
    >> A new macro is also introduced to acquire the locks required to
    >> dynamically
    >> update the guest's APCB in the proper order when a queue device is
    >> removed.
    >>
    >> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
    >> ---
    >>   drivers/s390/crypto/vfio_ap_ops.c | 126 +++++++++++++++++++++---------
    >>   1 file changed, 88 insertions(+), 38 deletions(-)
    >>
    >> diff --git a/drivers/s390/crypto/vfio_ap_ops.c
    >> b/drivers/s390/crypto/vfio_ap_ops.c
    >> index 2219b1069ceb..080a733f7cd2 100644
    >> --- a/drivers/s390/crypto/vfio_ap_ops.c
    >> +++ b/drivers/s390/crypto/vfio_ap_ops.c
    >> @@ -116,6 +116,74 @@ static const struct vfio_device_ops
    >> vfio_ap_matrix_dev_ops;
    >>       mutex_unlock(&matrix_dev->guests_lock);        \
    >>   })
    >>   +/**
    >> + * vfio_ap_mdev_get_update_locks_for_apqn: retrieve the matrix mdev
    >> to which an
    >> + *                       APQN is assigned and acquire the
    >> + *                       locks required to update the APCB of
    >> + *                       the KVM guest to which the mdev is
    >> + *                       attached.
    >> + *
    >> + * @apqn: the APQN of a queue device.
    >> + *
    >> + * The proper locking order is:
    >> + * 1. matrix_dev->guests_lock: required to use the KVM pointer 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
    >> matrix_mdev
    >> + *
    >> + * Note: If @apqn is not assigned to a matrix_mdev, the
    >> matrix_mdev->kvm->lock
    >> + *     will not be taken.
    >> + *
    >> + * Return: the ap_matrix_mdev object to which @apqn is assigned or
    >> NULL if @apqn
    >> + *       is not assigned to an ap_matrix_mdev.
    >> + */
    >> +static struct ap_matrix_mdev
    >> *vfio_ap_mdev_get_update_locks_for_apqn(int apqn)
    >> +{
    >> +    struct ap_matrix_mdev *matrix_mdev;
    >> +
    >> +    mutex_lock(&matrix_dev->guests_lock);
    >> +
    >> +    list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
    >> +        if (test_bit_inv(AP_QID_CARD(apqn), matrix_mdev->matrix.apm) &&
    >> +            test_bit_inv(AP_QID_QUEUE(apqn),
    >> matrix_mdev->matrix.aqm)) {
    >> +            if (matrix_mdev->kvm)
    >> +                mutex_lock(&matrix_mdev->kvm->lock);
    >> +
    >> +            mutex_lock(&matrix_dev->mdevs_lock);
    >> +
    >> +            return matrix_mdev;
    >> +        }
    >> +    }
    >> +
    >> +    mutex_lock(&matrix_dev->mdevs_lock);
    >> +
    >> +    return NULL;
    >> +}
    >> +
    >> +/**
    >> + * get_update_locks_for_queue: get the locks required to update the
    >> APCB of the
    >> + *                   KVM guest to which the matrix mdev linked to a
    >> + *                   vfio_ap_queue object is attached.
    >> + *
    >> + * @queue: a pointer to a vfio_ap_queue object.
    >> + *
    >> + * The proper locking order is:
    >> + * 1. matrix_dev->guests_lock: required to use the KVM pointer to
    >> update a KVM
    >> + *                guest's APCB.
    >> + * 2. queue->matrix_mdev->kvm->lock: required to update a guest's APCB
    >> + * 3. matrix_dev->mdevs_lock:    required to access data stored in a
    >> matrix_mdev
    >> + *
    >> + * Note: if @queue is not linked to an ap_matrix_mdev object, the
    >> KVM lock
    >> + *      will not be taken.
    >> + */
    >> +#define get_update_locks_for_queue(queue) ({            \
    >> +    struct ap_matrix_mdev *matrix_mdev = q->matrix_mdev; \
    >> +    mutex_lock(&matrix_dev->guests_lock);            \
    >> +    if (matrix_mdev && matrix_mdev->kvm) \
    >> +        mutex_lock(&matrix_mdev->kvm->lock);        \
    >> +    mutex_lock(&matrix_dev->mdevs_lock);            \
    >> +})
    >> +
    >
    >
    > One more comment I forgot to include before:
    > This macro is far too similar to existing macro,
    > get_update_locks_for_mdev. And it is only called in one place. Let's
    > remove this and replace the single invocation with:
    >
    > get_update_locks_for_mdev(q->matrix_mdev);

    We can't do that, but your comment does point out a flaw in this macro;
    namely, we must take the matrix_dev->guests_lock before attempting to
    access q->matrix_mdev.

    An ap_matrix_mdev can be unlinked from a vfio_ap_queue (and vice versa)
    when the queue is removed, but it also can be unlinked when an adapter
    or domain is unassigned from an ap_matrix_mdev. In order to ensure that
    the q->matrix_mdev is not in the process of being nullified (unlinked),
    we must be holding the guests_lock which is also held when an adapter or
    domain is unassigned.

    >
    >
    >>   /**
    >>    * vfio_ap_mdev_get_queue - retrieve a queue with a specific APQN
    >> from a
    >>    *                hash table of queues assigned to a matrix mdev
    >> @@ -615,21 +683,18 @@ static int vfio_ap_mdev_probe(struct
    >> mdev_device *mdev)
    >>       matrix_mdev->pqap_hook = handle_pqap;
    >>       vfio_ap_matrix_init(&matrix_dev->info, &matrix_mdev->shadow_apcb);
    >>       hash_init(matrix_mdev->qtable.queues);
    >> -    mdev_set_drvdata(mdev, matrix_mdev);
    >> -    mutex_lock(&matrix_dev->mdevs_lock);
    >> -    list_add(&matrix_mdev->node, &matrix_dev->mdev_list);
    >> -    mutex_unlock(&matrix_dev->mdevs_lock);
    >>         ret = vfio_register_emulated_iommu_dev(&matrix_mdev->vdev);
    >>       if (ret)
    >>           goto err_list;
    >> +    mdev_set_drvdata(mdev, matrix_mdev);
    >> +    mutex_lock(&matrix_dev->mdevs_lock);
    >> +    list_add(&matrix_mdev->node, &matrix_dev->mdev_list);
    >> +    mutex_unlock(&matrix_dev->mdevs_lock);
    >>       dev_set_drvdata(&mdev->dev, matrix_mdev);
    >>       return 0;
    >>     err_list:
    >> -    mutex_lock(&matrix_dev->mdevs_lock);
    >> -    list_del(&matrix_mdev->node);
    >> -    mutex_unlock(&matrix_dev->mdevs_lock);
    >>       vfio_uninit_group_dev(&matrix_mdev->vdev);
    >>       kfree(matrix_mdev);
    >>   err_dec_available:
    >> @@ -692,11 +757,13 @@ static void vfio_ap_mdev_remove(struct
    >> mdev_device *mdev)
    >>         vfio_unregister_group_dev(&matrix_mdev->vdev);
    >>   +    mutex_lock(&matrix_dev->guests_lock);
    >>       mutex_lock(&matrix_dev->mdevs_lock);
    >>       vfio_ap_mdev_reset_queues(matrix_mdev);
    >>       vfio_ap_mdev_unlink_fr_queues(matrix_mdev);
    >>       list_del(&matrix_mdev->node);
    >>       mutex_unlock(&matrix_dev->mdevs_lock);
    >> +    mutex_unlock(&matrix_dev->guests_lock);
    >>       vfio_uninit_group_dev(&matrix_mdev->vdev);
    >>       kfree(matrix_mdev);
    >>       atomic_inc(&matrix_dev->available_instances);
    >> @@ -1665,49 +1732,30 @@ void vfio_ap_mdev_unregister(void)
    >>       mdev_unregister_driver(&vfio_ap_matrix_driver);
    >>   }
    >>   -/*
    >> - * vfio_ap_queue_link_mdev
    >> - *
    >> - * @q: The queue to link with the matrix mdev.
    >> - *
    >> - * Links @q with the matrix mdev to which the queue's APQN is assigned.
    >> - */
    >> -static void vfio_ap_queue_link_mdev(struct vfio_ap_queue *q)
    >> -{
    >> -    unsigned long apid = AP_QID_CARD(q->apqn);
    >> -    unsigned long apqi = AP_QID_QUEUE(q->apqn);
    >> -    struct ap_matrix_mdev *matrix_mdev;
    >> -
    >> -    list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
    >> -        if (test_bit_inv(apid, matrix_mdev->matrix.apm) &&
    >> -            test_bit_inv(apqi, matrix_mdev->matrix.aqm)) {
    >> -            vfio_ap_mdev_link_queue(matrix_mdev, q);
    >> -            break;
    >> -        }
    >> -    }
    >> -}
    >> -
    >>   int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
    >>   {
    >>       struct vfio_ap_queue *q;
    >> +    struct ap_matrix_mdev *matrix_mdev;
    >>       DECLARE_BITMAP(apm_delta, AP_DEVICES);
    >>         q = kzalloc(sizeof(*q), GFP_KERNEL);
    >>       if (!q)
    >>           return -ENOMEM;
    >> -    mutex_lock(&matrix_dev->mdevs_lock);
    >>       q->apqn = to_ap_queue(&apdev->device)->qid;
    >>       q->saved_isc = VFIO_AP_ISC_INVALID;
    >> -    vfio_ap_queue_link_mdev(q);
    >> -    if (q->matrix_mdev) {
    >> +
    >> +    matrix_mdev = vfio_ap_mdev_get_update_locks_for_apqn(q->apqn);
    >> +
    >> +    if (matrix_mdev) {
    >> +        vfio_ap_mdev_link_queue(matrix_mdev, q);
    >>           memset(apm_delta, 0, sizeof(apm_delta));
    >>           set_bit_inv(AP_QID_CARD(q->apqn), apm_delta);
    >>           vfio_ap_mdev_filter_matrix(apm_delta,
    >> -                       q->matrix_mdev->matrix.aqm,
    >> -                       q->matrix_mdev);
    >> +                       matrix_mdev->matrix.aqm,
    >> +                       matrix_mdev);
    >>       }
    >>       dev_set_drvdata(&apdev->device, q);
    >> -    mutex_unlock(&matrix_dev->mdevs_lock);
    >> +    release_update_locks_for_mdev(matrix_mdev);
    >>         return 0;
    >>   }
    >> @@ -1716,11 +1764,13 @@ void vfio_ap_mdev_remove_queue(struct
    >> ap_device *apdev)
    >>   {
    >>       unsigned long apid;
    >>       struct vfio_ap_queue *q;
    >> +    struct ap_matrix_mdev *matrix_mdev;
    >>   -    mutex_lock(&matrix_dev->mdevs_lock);
    >>       q = dev_get_drvdata(&apdev->device);
    >> +    get_update_locks_for_queue(q);
    >> +    matrix_mdev = q->matrix_mdev;
    >>   -    if (q->matrix_mdev) {
    >> +    if (matrix_mdev) {
    >>           vfio_ap_unlink_queue_fr_mdev(q);
    >>             apid = AP_QID_CARD(q->apqn);
    >> @@ -1731,5 +1781,5 @@ void vfio_ap_mdev_remove_queue(struct ap_device
    >> *apdev)
    >>       vfio_ap_mdev_reset_queue(q, 1);
    >>       dev_set_drvdata(&apdev->device, NULL);
    >>       kfree(q);
    >> -    mutex_unlock(&matrix_dev->mdevs_lock);
    >> +    release_update_locks_for_mdev(matrix_mdev);
    >>   }
    >
    >

    \
     
     \ /
      Last update: 2022-05-31 13:58    [W:4.194 / U:0.116 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site