From: Tony Krowiak <akrowiak@linux.ibm.com>
To: jjherne@linux.ibm.com, linux-s390@vger.kernel.org,
linux-kernel@vger.kernel.org, kvm@vger.kernel.org
Cc: freude@linux.ibm.com, borntraeger@de.ibm.com, cohuck@redhat.com,
mjrosato@linux.ibm.com, pasic@linux.ibm.com,
alex.williamson@redhat.com, kwankhede@nvidia.com,
fiuczy@linux.ibm.com
Subject: Re: [PATCH v19 11/20] s390/vfio-ap: prepare for dynamic update of guest's APCB on queue probe/remove
Date: Tue, 31 May 2022 08:02:41 -0400 [thread overview]
Message-ID: <7e561de8-be73-4b62-ea7c-36e659d87788@linux.ibm.com> (raw)
In-Reply-To: <4d05a8f4-d2e9-bc54-3e9b-6becc3281f0f@linux.ibm.com>
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);
Yikes, I see another flaw in this macro! Either the input parameter
needs to be renamed to 'q' or the q->matrix_mdev needs to be changed to
queue->matrix_mdev. I think I'll go with the former since vfio_ap_queue
is referred to as 'q' everywhere else.
>
>
>> /**
>> * 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);
>> }
>
>
next prev parent reply other threads:[~2022-05-31 12:02 UTC|newest]
Thread overview: 55+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-04 22:10 [PATCH v19 00/20] s390/vfio-ap: dynamic configuration support Tony Krowiak
2022-04-04 22:10 ` [PATCH v19 01/20] s390/vfio-ap: use new AP bus interface to search for queue devices Tony Krowiak
2022-04-04 22:10 ` [PATCH v19 02/20] s390/vfio-ap: move probe and remove callbacks to vfio_ap_ops.c Tony Krowiak
2022-05-24 14:49 ` Jason J. Herne
2022-05-24 17:41 ` Tony Krowiak
2022-04-04 22:10 ` [PATCH v19 03/20] s390/vfio-ap: manage link between queue struct and matrix mdev Tony Krowiak
2022-04-04 22:10 ` [PATCH v19 04/20] s390/vfio-ap: introduce shadow APCB Tony Krowiak
2022-04-04 22:10 ` [PATCH v19 05/20] s390/vfio-ap: refresh guest's APCB by filtering AP resources assigned to mdev Tony Krowiak
2022-05-16 16:36 ` Jason J. Herne
2022-05-16 17:13 ` Tony Krowiak
2022-05-16 17:50 ` Jason J. Herne
2022-05-16 18:06 ` Tony Krowiak
2022-04-04 22:10 ` [PATCH v19 06/20] s390/vfio-ap: allow assignment of unavailable AP queues to mdev device Tony Krowiak
2022-04-04 22:10 ` [PATCH v19 07/20] s390/vfio-ap: rename matrix_dev->lock mutex to matrix_dev->mdevs_lock Tony Krowiak
2022-05-17 14:02 ` Jason J. Herne
2022-05-17 18:36 ` Tony Krowiak
2022-04-04 22:10 ` [PATCH v19 08/20] s390/vfio-ap: introduce new mutex to control access to the KVM pointer Tony Krowiak
2022-05-27 12:40 ` Jason J. Herne
2022-04-04 22:10 ` [PATCH v19 09/20] s390/vfio-ap: use proper locking order when setting/clearing " Tony Krowiak
2022-05-27 12:41 ` Jason J. Herne
2022-04-04 22:10 ` [PATCH v19 10/20] s390/vfio-ap: prepare for dynamic update of guest's APCB on assign/unassign Tony Krowiak
2022-05-27 13:18 ` Jason J. Herne
2022-05-31 10:32 ` Tony Krowiak
2022-04-04 22:10 ` [PATCH v19 11/20] s390/vfio-ap: prepare for dynamic update of guest's APCB on queue probe/remove Tony Krowiak
2022-05-27 13:36 ` Jason J. Herne
2022-05-31 10:44 ` Tony Krowiak
2022-06-07 12:05 ` Halil Pasic
2022-06-08 13:31 ` Tony Krowiak
2022-05-27 13:50 ` Jason J. Herne
2022-05-31 11:57 ` Tony Krowiak
2022-05-31 12:02 ` Tony Krowiak [this message]
2022-04-04 22:10 ` [PATCH v19 12/20] s390/vfio-ap: allow hot plug/unplug of AP devices when assigned/unassigned Tony Krowiak
2022-06-01 18:54 ` Jason J. Herne
2022-04-04 22:10 ` [PATCH v19 13/20] s390/vfio-ap: hot plug/unplug of AP devices when probed/removed Tony Krowiak
2022-06-01 18:55 ` Jason J. Herne
2022-04-04 22:10 ` [PATCH v19 14/20] s390/vfio-ap: reset queues after adapter/domain unassignment Tony Krowiak
2022-06-02 15:00 ` Jason J. Herne
2022-04-04 22:10 ` [PATCH v19 15/20] s390/vfio-ap: implement in-use callback for vfio_ap driver Tony Krowiak
2022-06-02 18:16 ` Jason J. Herne
2022-06-02 19:19 ` Tony Krowiak
2022-06-02 20:21 ` Jason J. Herne
2022-04-04 22:10 ` [PATCH v19 16/20] s390/vfio-ap: sysfs attribute to display the guest's matrix Tony Krowiak
2022-04-04 22:10 ` [PATCH v19 17/20] s390/vfio-ap: handle config changed and scan complete notification Tony Krowiak
2022-06-06 17:50 ` Jason J. Herne
2022-04-04 22:10 ` [PATCH v19 18/20] s390/vfio-ap: update docs to include dynamic config support Tony Krowiak
2022-05-31 13:22 ` Jason J. Herne
2022-04-04 22:10 ` [PATCH v19 19/20] s390/Docs: new doc describing lock usage by the vfio_ap device driver Tony Krowiak
2022-05-31 19:23 ` Jason J. Herne
2022-06-02 16:11 ` Tony Krowiak
2022-04-04 22:10 ` [PATCH v19 20/20] MAINTAINERS: pick up all vfio_ap docs for VFIO AP maintainers Tony Krowiak
2022-05-31 18:26 ` Matthew Rosato
2022-06-02 16:19 ` Tony Krowiak
2022-04-29 19:57 ` [PATCH v19 00/20] s390/vfio-ap: dynamic configuration support Tony Krowiak
2022-05-03 17:39 ` Tony Krowiak
2022-05-09 14:34 ` Tony Krowiak
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7e561de8-be73-4b62-ea7c-36e659d87788@linux.ibm.com \
--to=akrowiak@linux.ibm.com \
--cc=alex.williamson@redhat.com \
--cc=borntraeger@de.ibm.com \
--cc=cohuck@redhat.com \
--cc=fiuczy@linux.ibm.com \
--cc=freude@linux.ibm.com \
--cc=jjherne@linux.ibm.com \
--cc=kvm@vger.kernel.org \
--cc=kwankhede@nvidia.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-s390@vger.kernel.org \
--cc=mjrosato@linux.ibm.com \
--cc=pasic@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).