From: Tony Krowiak <akrowiak@linux.ibm.com>
To: Halil Pasic <pasic@linux.ibm.com>
Cc: linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org,
kvm@vger.kernel.org, freude@linux.ibm.com,
borntraeger@de.ibm.com, cohuck@redhat.com,
mjrosato@linux.ibm.com, alex.williamson@redhat.com,
kwankhede@nvidia.com, fiuczy@linux.ibm.com,
frankja@linux.ibm.com, david@redhat.com, hca@linux.ibm.com,
gor@linux.ibm.com
Subject: Re: [PATCH v13 05/15] s390/vfio-ap: manage link between queue struct and matrix mdev
Date: Wed, 13 Jan 2021 16:41:27 -0500 [thread overview]
Message-ID: <8c701a5c-39f8-0c22-c936-aebbc3c8f60e@linux.ibm.com> (raw)
In-Reply-To: <20210111201752.21a41db4.pasic@linux.ibm.com>
On 1/11/21 2:17 PM, Halil Pasic wrote:
> On Tue, 22 Dec 2020 20:15:56 -0500
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>
>> Let's create links between each queue device bound to the vfio_ap device
>> driver and the matrix mdev to which the queue's APQN is assigned. The idea
>> is to facilitate efficient retrieval of the objects representing the queue
>> devices and matrix mdevs as well as to verify that a queue assigned to
>> a matrix mdev is bound to the driver.
>>
>> The links will be created as follows:
>>
>> * When the queue device is probed, if its APQN is assigned to a matrix
>> mdev, the structures representing the queue device and the matrix mdev
>> will be linked.
>>
>> * When an adapter or domain is assigned to a matrix mdev, for each new
>> APQN assigned that references a queue device bound to the vfio_ap
>> device driver, the structures representing the queue device and the
>> matrix mdev will be linked.
>>
>> The links will be removed as follows:
>>
>> * When the queue device is removed, if its APQN is assigned to a matrix
>> mdev, the structures representing the queue device and the matrix mdev
>> will be unlinked.
>>
>> * When an adapter or domain is unassigned from a matrix mdev, for each
>> APQN unassigned that references a queue device bound to the vfio_ap
>> device driver, the structures representing the queue device and the
>> matrix mdev will be unlinked.
>>
>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
>
>> ---
>> drivers/s390/crypto/vfio_ap_ops.c | 140 +++++++++++++++++++++-----
>> drivers/s390/crypto/vfio_ap_private.h | 3 +
>> 2 files changed, 117 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 835c963ae16d..cdcc6378b4a5 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -27,33 +27,17 @@
>> static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev);
>> static struct vfio_ap_queue *vfio_ap_find_queue(int apqn);
>>
>> -/**
>> - * vfio_ap_get_queue: Retrieve a queue with a specific APQN from a list
>> - * @matrix_mdev: the associated mediated matrix
>> - * @apqn: The queue APQN
>> - *
>> - * Retrieve a queue with a specific APQN from the list of the
>> - * devices of the vfio_ap_drv.
>> - * Verify that the APID and the APQI are set in the matrix.
>> - *
>> - * Returns the pointer to the associated vfio_ap_queue
>> - */
>> -static struct vfio_ap_queue *vfio_ap_get_queue(
>> - struct ap_matrix_mdev *matrix_mdev,
>> - int apqn)
>> +static struct vfio_ap_queue *
>> +vfio_ap_mdev_get_queue(struct ap_matrix_mdev *matrix_mdev, unsigned long apqn)
>> {
>> - struct vfio_ap_queue *q = NULL;
>> -
>> - if (!test_bit_inv(AP_QID_CARD(apqn), matrix_mdev->matrix.apm))
>> - return NULL;
>> - if (!test_bit_inv(AP_QID_QUEUE(apqn), matrix_mdev->matrix.aqm))
>> - return NULL;
>> + struct vfio_ap_queue *q;
>>
>> - q = vfio_ap_find_queue(apqn);
>> - if (q)
>> - q->matrix_mdev = matrix_mdev;
>> + hash_for_each_possible(matrix_mdev->qtable, q, mdev_qnode, apqn) {
>> + if (q && (q->apqn == apqn))
>> + return q;
>> + }
>>
>> - return q;
>> + return NULL;
>> }
>>
>> /**
>> @@ -166,7 +150,6 @@ static struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q)
>> status.response_code);
>> end_free:
>> vfio_ap_free_aqic_resources(q);
>> - q->matrix_mdev = NULL;
>> return status;
>> }
>>
>> @@ -282,7 +265,7 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
>> matrix_mdev = container_of(vcpu->kvm->arch.crypto.pqap_hook,
>> struct ap_matrix_mdev, pqap_hook);
>>
>> - q = vfio_ap_get_queue(matrix_mdev, apqn);
>> + q = vfio_ap_mdev_get_queue(matrix_mdev, apqn);
>> if (!q)
>> goto out_unlock;
>>
>> @@ -325,6 +308,7 @@ static int vfio_ap_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
>>
>> matrix_mdev->mdev = mdev;
>> vfio_ap_matrix_init(&matrix_dev->info, &matrix_mdev->matrix);
>> + hash_init(matrix_mdev->qtable);
>> mdev_set_drvdata(mdev, matrix_mdev);
>> matrix_mdev->pqap_hook.hook = handle_pqap;
>> matrix_mdev->pqap_hook.owner = THIS_MODULE;
>> @@ -553,6 +537,50 @@ static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
>> return 0;
>> }
>>
>> +static void vfio_ap_mdev_link_queue(struct ap_matrix_mdev *matrix_mdev,
>> + struct vfio_ap_queue *q)
>> +{
>> + if (q) {
>> + q->matrix_mdev = matrix_mdev;
>> + hash_add(matrix_mdev->qtable,
>> + &q->mdev_qnode, q->apqn);
>> + }
>> +}
>> +
>> +static void vfio_ap_mdev_link_apqn(struct ap_matrix_mdev *matrix_mdev, int apqn)
>> +{
>> + struct vfio_ap_queue *q;
>> +
>> + q = vfio_ap_find_queue(apqn);
>> + vfio_ap_mdev_link_queue(matrix_mdev, q);
>> +}
>> +
>> +static void vfio_ap_mdev_unlink_queue(struct vfio_ap_queue *q)
>> +{
>> + if (q) {
>> + q->matrix_mdev = NULL;
>> + hash_del(&q->mdev_qnode);
>> + }
>> +}
>> +
>> +static void vfio_ap_mdev_unlink_apqn(int apqn)
>> +{
>> + struct vfio_ap_queue *q;
>> +
>> + q = vfio_ap_find_queue(apqn);
>> + vfio_ap_mdev_unlink_queue(q);
>> +}
>> +
>> +static void vfio_ap_mdev_link_adapter(struct ap_matrix_mdev *matrix_mdev,
>> + unsigned long apid)
>> +{
>> + unsigned long apqi;
>> +
>> + for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm, AP_DOMAINS)
>> + vfio_ap_mdev_link_apqn(matrix_mdev,
>> + AP_MKQID(apid, apqi));
>> +}
>> +
>> /**
>> * assign_adapter_store
>> *
>> @@ -622,6 +650,7 @@ static ssize_t assign_adapter_store(struct device *dev,
>> if (ret)
>> goto share_err;
>>
>> + vfio_ap_mdev_link_adapter(matrix_mdev, apid);
>> ret = count;
>> goto done;
>>
>> @@ -634,6 +663,15 @@ static ssize_t assign_adapter_store(struct device *dev,
>> }
>> static DEVICE_ATTR_WO(assign_adapter);
>>
>> +static void vfio_ap_mdev_unlink_adapter(struct ap_matrix_mdev *matrix_mdev,
>> + unsigned long apid)
>> +{
>> + unsigned long apqi;
>> +
>> + for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm, AP_DOMAINS)
>> + vfio_ap_mdev_unlink_apqn(AP_MKQID(apid, apqi));
>> +}
>> +
>> /**
>> * unassign_adapter_store
>> *
>> @@ -673,6 +711,7 @@ static ssize_t unassign_adapter_store(struct device *dev,
>>
>> mutex_lock(&matrix_dev->lock);
>> clear_bit_inv((unsigned long)apid, matrix_mdev->matrix.apm);
>> + vfio_ap_mdev_unlink_adapter(matrix_mdev, apid);
>> mutex_unlock(&matrix_dev->lock);
>>
>> return count;
>> @@ -699,6 +738,15 @@ vfio_ap_mdev_verify_queues_reserved_for_apqi(struct ap_matrix_mdev *matrix_mdev,
>> return 0;
>> }
>>
>> +static void vfio_ap_mdev_link_domain(struct ap_matrix_mdev *matrix_mdev,
>> + unsigned long apqi)
>> +{
>> + unsigned long apid;
>> +
>> + for_each_set_bit_inv(apid, matrix_mdev->matrix.apm, AP_DEVICES)
>> + vfio_ap_mdev_link_apqn(matrix_mdev, AP_MKQID(apid, apqi));
>> +}
>> +
>> /**
>> * assign_domain_store
>> *
>> @@ -763,6 +811,7 @@ static ssize_t assign_domain_store(struct device *dev,
>> if (ret)
>> goto share_err;
>>
>> + vfio_ap_mdev_link_domain(matrix_mdev, apqi);
>> ret = count;
>> goto done;
>>
>> @@ -775,6 +824,14 @@ static ssize_t assign_domain_store(struct device *dev,
>> }
>> static DEVICE_ATTR_WO(assign_domain);
>>
>> +static void vfio_ap_mdev_unlink_domain(struct ap_matrix_mdev *matrix_mdev,
>> + unsigned long apqi)
>> +{
>> + unsigned long apid;
>> +
>> + for_each_set_bit_inv(apid, matrix_mdev->matrix.apm, AP_DEVICES)
>> + vfio_ap_mdev_unlink_apqn(AP_MKQID(apid, apqi));
>> +}
>>
>> /**
>> * unassign_domain_store
>> @@ -815,6 +872,7 @@ static ssize_t unassign_domain_store(struct device *dev,
>>
>> mutex_lock(&matrix_dev->lock);
>> clear_bit_inv((unsigned long)apqi, matrix_mdev->matrix.aqm);
>> + vfio_ap_mdev_unlink_domain(matrix_mdev, apqi);
>> mutex_unlock(&matrix_dev->lock);
>>
>> return count;
>> @@ -1317,6 +1375,28 @@ void vfio_ap_mdev_unregister(void)
>> mdev_unregister_device(&matrix_dev->device);
>> }
>>
>> +/*
>> + * 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;
>> @@ -1324,9 +1404,13 @@ int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
>> q = kzalloc(sizeof(*q), GFP_KERNEL);
>> if (!q)
>> return -ENOMEM;
>> + mutex_lock(&matrix_dev->lock);
>> dev_set_drvdata(&apdev->device, q);
>> q->apqn = to_ap_queue(&apdev->device)->qid;
>> q->saved_isc = VFIO_AP_ISC_INVALID;
>> + vfio_ap_queue_link_mdev(q);
>> + mutex_unlock(&matrix_dev->lock);
>> +
> Does the critical section have to include more than just
> vfio_ap_queue_link_mdev()? Did we need the critical section
> before this patch?
We did not need the critical section before this patch because
the only function that retrieved the vfio_ap_queue via the queue
device's drvdata was the remove callback. I included the initialization
of the vfio_ap_queue object under lock because the
vfio_ap_find_queue() function retrieves the vfio_ap_queue object from
the queue device's drvdata so it might be advantageous to initialize
it under the mdev lock. On the other hand, I can't come up with a good
argument to change this.
>
>> return 0;
>> }
>>
>> @@ -1341,6 +1425,10 @@ void vfio_ap_mdev_remove_queue(struct ap_device *apdev)
>> apid = AP_QID_CARD(q->apqn);
>> apqi = AP_QID_QUEUE(q->apqn);
>> vfio_ap_mdev_reset_queue(apid, apqi, 1);
>> +
>> + if (q->matrix_mdev)
>> + vfio_ap_mdev_unlink_queue(q);
>> +
>> kfree(q);
>> mutex_unlock(&matrix_dev->lock);
>> }
>> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
>> index d9003de4fbad..4e5cc72fc0db 100644
>> --- a/drivers/s390/crypto/vfio_ap_private.h
>> +++ b/drivers/s390/crypto/vfio_ap_private.h
>> @@ -18,6 +18,7 @@
>> #include <linux/delay.h>
>> #include <linux/mutex.h>
>> #include <linux/kvm_host.h>
>> +#include <linux/hashtable.h>
>>
>> #include "ap_bus.h"
>>
>> @@ -86,6 +87,7 @@ struct ap_matrix_mdev {
>> struct kvm *kvm;
>> struct kvm_s390_module_hook pqap_hook;
>> struct mdev_device *mdev;
>> + DECLARE_HASHTABLE(qtable, 8);
>> };
>>
>> extern int vfio_ap_mdev_register(void);
>> @@ -97,6 +99,7 @@ struct vfio_ap_queue {
>> int apqn;
>> #define VFIO_AP_ISC_INVALID 0xff
>> unsigned char saved_isc;
>> + struct hlist_node mdev_qnode;
>> };
>>
>> int vfio_ap_mdev_probe_queue(struct ap_device *queue);
next prev parent reply other threads:[~2021-01-14 2:13 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-23 1:15 [PATCH v13 00/15] s390/vfio-ap: dynamic configuration support Tony Krowiak
2020-12-23 1:15 ` [PATCH v13 01/15] s390/vfio-ap: clean up vfio_ap resources when KVM pointer invalidated Tony Krowiak
2020-12-23 1:15 ` [PATCH v13 02/15] s390/vfio-ap: No need to disable IRQ after queue reset Tony Krowiak
2021-01-11 16:32 ` Halil Pasic
2021-01-13 17:06 ` Tony Krowiak
2021-01-13 21:21 ` Halil Pasic
2021-01-14 0:46 ` Tony Krowiak
2021-01-14 3:13 ` Halil Pasic
2020-12-23 1:15 ` [PATCH v13 03/15] s390/vfio-ap: move probe and remove callbacks to vfio_ap_ops.c Tony Krowiak
2020-12-23 1:15 ` [PATCH v13 04/15] s390/vfio-ap: use new AP bus interface to search for queue devices Tony Krowiak
2020-12-23 1:15 ` [PATCH v13 05/15] s390/vfio-ap: manage link between queue struct and matrix mdev Tony Krowiak
2021-01-11 19:17 ` Halil Pasic
2021-01-13 21:41 ` Tony Krowiak [this message]
2021-01-14 2:50 ` Halil Pasic
2021-01-14 21:10 ` Tony Krowiak
2020-12-23 1:15 ` [PATCH v13 06/15] s390/vfio-ap: allow assignment of unavailable AP queues to mdev device Tony Krowiak
2021-01-11 20:40 ` Halil Pasic
2021-01-14 17:54 ` Tony Krowiak
2021-01-15 1:08 ` Halil Pasic
2021-01-15 1:44 ` Halil Pasic
2021-03-31 14:36 ` Tony Krowiak
2020-12-23 1:15 ` [PATCH v13 07/15] s390/vfio-ap: introduce shadow APCB Tony Krowiak
2021-01-11 22:50 ` Halil Pasic
2021-01-14 21:35 ` Tony Krowiak
2020-12-23 1:15 ` [PATCH v13 08/15] s390/vfio-ap: sysfs attribute to display the guest's matrix Tony Krowiak
2021-01-11 22:58 ` Halil Pasic
2021-01-28 21:29 ` Tony Krowiak
2020-12-23 1:16 ` [PATCH v13 09/15] s390/vfio-ap: allow hot plug/unplug of AP resources using mdev device Tony Krowiak
2021-01-12 1:12 ` Halil Pasic
2021-01-12 17:55 ` Halil Pasic
2021-02-01 14:41 ` Tony Krowiak
2021-02-03 23:13 ` Tony Krowiak
2021-02-04 0:21 ` Halil Pasic
2020-12-23 1:16 ` [PATCH v13 10/15] s390/zcrypt: driver callback to indicate resource in use Tony Krowiak
2021-01-12 16:50 ` Halil Pasic
2020-12-23 1:16 ` [PATCH v13 11/15] s390/vfio-ap: implement in-use callback for vfio_ap driver Tony Krowiak
2021-01-12 1:20 ` Halil Pasic
2021-01-12 14:14 ` Matthew Rosato
2021-01-12 16:49 ` Halil Pasic
2020-12-23 1:16 ` [PATCH v13 12/15] s390/zcrypt: Notify driver on config changed and scan complete callbacks Tony Krowiak
2021-01-12 16:58 ` Halil Pasic
2020-12-23 1:16 ` [PATCH v13 13/15] s390/vfio-ap: handle host AP config change notification Tony Krowiak
2021-01-12 18:39 ` Halil Pasic
2020-12-23 1:16 ` [PATCH v13 14/15] s390/vfio-ap: handle AP bus scan completed notification Tony Krowiak
2021-01-12 18:44 ` Halil Pasic
2020-12-23 1:16 ` [PATCH v13 15/15] s390/vfio-ap: update docs to include dynamic config support Tony Krowiak
2021-01-06 15:16 ` [PATCH v13 00/15] s390/vfio-ap: dynamic configuration support Tony Krowiak
2021-01-07 14:41 ` Halil Pasic
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=8c701a5c-39f8-0c22-c936-aebbc3c8f60e@linux.ibm.com \
--to=akrowiak@linux.ibm.com \
--cc=alex.williamson@redhat.com \
--cc=borntraeger@de.ibm.com \
--cc=cohuck@redhat.com \
--cc=david@redhat.com \
--cc=fiuczy@linux.ibm.com \
--cc=frankja@linux.ibm.com \
--cc=freude@linux.ibm.com \
--cc=gor@linux.ibm.com \
--cc=hca@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).