Re: [PATCH v6 2/7] s390: ap: new vfio_ap_queue structure

[Harald Freudenberger]

On 26.03.19 21:45, Tony Krowiak wrote:
> On 3/22/19 10:43 AM, Pierre Morel wrote:
>> The AP interruptions are assigned on a queue basis and
>> the GISA structure is handled on a VM basis, so that
>> we need to add a structure we can retrieve from both side
>
> s/side/sides/
>
>> holding the information we need to handle PQAP/AQIC interception
>> and setup the GISA.
>
> s/setup/set up/
>
>>
>> Since we can not add more information to the ap_device
>> we add a new structure vfio_ap_queue, to hold per queue
>> information useful to handle interruptions and set it as
>> driver's data of the standard ap_queue device.
>>
>> Usually, the device and the mediated device are linked together
>> but in the vfio_ap driver design we have a bunch of "sub" devices
>> (the ap_queue devices) belonging to the mediated device.
>>
>> Linking these structure to the mediated device it is assigned to,
>> with the help of the vfio_ap_queue structure will help us to
>> retrieve the AP devices associated with the mediated devices
>> during the mediated device operations.
>>
>> ------------ÂÂÂ -------------
>> | AP queue |--> | AP_vfio_q |<----
>> ------------ÂÂÂ ------^------ÂÂÂ |ÂÂÂ ---------------
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |ÂÂÂÂÂÂÂÂÂ <--->| matrix_mdev |
>> ------------ÂÂÂ ------v------ÂÂÂ |ÂÂÂ ---------------
>> | AP queue |--> | AP_vfio_q |-----
>> ------------ÂÂÂ -------------
>>
>> The vfio_ap_queue device will hold the following entries:
>> - apqn: AP queue number (defined here)
>> - isc : Interrupt subclass (defined later)
>> - nib : notification information byte (defined later)
>> - list: a list_head entry allowing to link this structure to a
>> ÂÂÂÂmatrix mediated device it is assigned to.
>>
>> The vfio_ap_queue structure is allocated when the vfio_ap_driver
>> is probed and added as driver data to the ap_queue device.
>> It is free on remove.
>>
>> The structure is linked to the matrix_dev host device at the
>> probe of the device building some kind of free list for the
>> matrix mediated devices.
>>
>> When the vfio_queue is associated to a matrix mediated device,
>> during assign_adapter or assign_domain,
>> the vfio_ap_queue device is linked to this matrix mediated device
>> and unlinked when dissociated.
>>
>> Queuing the devices on a list of free devices and testing the
>> matrix_mdev pointer to the associated matrix allow us to know
>> if the queue is associated to the matrix device and associated
>> or not to a mediated device.
>>
>> All the operation on the free_list must be protected by the
>> VFIO AP matrix_dev lock.
>>
>> Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxx>
>> ---
>> Â drivers/s390/crypto/vfio_ap_drv.cÂÂÂÂ |Â 31 ++-
>> Â drivers/s390/crypto/vfio_ap_ops.cÂÂÂÂ | 423 ++++++++++++++++++----------------
>> Â drivers/s390/crypto/vfio_ap_private.h |ÂÂ 7 +
>> Â 3 files changed, 266 insertions(+), 195 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
>> index e9824c3..df6f21a 100644
>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>> @@ -40,14 +40,42 @@ static struct ap_device_id ap_queue_ids[] = {
>> Â Â MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
>> Â +/**
>> + * vfio_ap_queue_dev_probe:
>> + *
>> + * Allocate a vfio_ap_queue structure and associate it
>> + * with the device as driver_data.
>> + */
>> Â static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
>> Â {
>> +ÂÂÂ struct vfio_ap_queue *q;
>> +
>> +ÂÂÂ q = kzalloc(sizeof(*q), GFP_KERNEL);
>> +ÂÂÂ if (!q)
>> +ÂÂÂÂÂÂÂ return -ENOMEM;
>> +ÂÂÂ dev_set_drvdata(&apdev->device, q);
>> +ÂÂÂ q->apqn = to_ap_queue(&apdev->device)->qid;
>> +ÂÂÂ INIT_LIST_HEAD(&q->list);
>> +ÂÂÂ mutex_lock(&matrix_dev->lock);
>> +ÂÂÂ list_add(&q->list, &matrix_dev->free_list);
>> +ÂÂÂ mutex_unlock(&matrix_dev->lock);
>> ÂÂÂÂÂ return 0;
>> Â }
>> Â +/**
>> + * vfio_ap_queue_dev_remove:
>> + *
>> + * Free the associated vfio_ap_queue structure
>> + */
>> Â static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
>> Â {
>> -ÂÂÂ /* Nothing to do yet */
>> +ÂÂÂ struct vfio_ap_queue *q;
>> +
>> +ÂÂÂ q = dev_get_drvdata(&apdev->device);
>> +ÂÂÂ mutex_lock(&matrix_dev->lock);
>> +ÂÂÂ list_del(&q->list);
>> +ÂÂÂ mutex_unlock(&matrix_dev->lock);
>> +ÂÂÂ kfree(q);
>> Â }
>> Â Â static void vfio_ap_matrix_dev_release(struct device *dev)
>> @@ -107,6 +135,7 @@ static int vfio_ap_matrix_dev_create(void)
>> ÂÂÂÂÂ matrix_dev->device.bus = &matrix_bus;
>> ÂÂÂÂÂ matrix_dev->device.release = vfio_ap_matrix_dev_release;
>> ÂÂÂÂÂ matrix_dev->vfio_ap_drv = &vfio_ap_drv;
>> +ÂÂÂ INIT_LIST_HEAD(&matrix_dev->free_list);
>> Â ÂÂÂÂÂ ret = device_register(&matrix_dev->device);
>> ÂÂÂÂÂ if (ret)
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 900b9cf..77f7bac 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -24,6 +24,68 @@
>> Â #define VFIO_AP_MDEV_TYPE_HWVIRT "passthrough"
>> Â #define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device"
>> Â +/**
>> + * vfio_ap_get_queue: Retrieve a queue with a specific APQN from a list
>> + * @apqn: The queue APQN
>> + *
>> + * Retrieve a queue with a specific APQN from the list of the
>> + * devices associated with a list.
>> + *
>> + * Returns the pointer to the associated vfio_ap_queue
>> + */
>> +struct vfio_ap_queue *vfio_ap_get_queue(int apqn, struct list_head *l)
>> +{
>> +ÂÂÂ struct vfio_ap_queue *q;
>> +
>> +ÂÂÂ list_for_each_entry(q, l, list)
>> +ÂÂÂÂÂÂÂ if (q->apqn == apqn)
>> +ÂÂÂÂÂÂÂÂÂÂÂ return q;
>> +ÂÂÂ return NULL;
>> +}
>> +
>> +static int vfio_ap_find_any_card(int apid)
>> +{
>> +ÂÂÂ struct vfio_ap_queue *q;
>> +
>> +ÂÂÂ list_for_each_entry(q, &matrix_dev->free_list, list)
>> +ÂÂÂÂÂÂÂ if (AP_QID_CARD(q->apqn) == apid)
>> +ÂÂÂÂÂÂÂÂÂÂÂ return 1;
>> +ÂÂÂ return 0;
>> +}
>> +
>> +static int vfio_ap_find_any_domain(int apqi)
>> +{
>> +ÂÂÂ struct vfio_ap_queue *q;
>> +
>> +ÂÂÂ list_for_each_entry(q, &matrix_dev->free_list, list)
>> +ÂÂÂÂÂÂÂ if (AP_QID_QUEUE(q->apqn) == apqi)
>> +ÂÂÂÂÂÂÂÂÂÂÂ return 1;
>> +ÂÂÂ return 0;
>> +}
>> +
>> +static int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q)
>> +{
>> +ÂÂÂ struct ap_queue_status status;
>> +ÂÂÂ int retry = 1;
>> +
>> +ÂÂÂ do {
>> +ÂÂÂÂÂÂÂ status = ap_zapq(q->apqn);
>> +ÂÂÂÂÂÂÂ switch (status.response_code) {
>> +ÂÂÂÂÂÂÂ case AP_RESPONSE_NORMAL:
>> +ÂÂÂÂÂÂÂÂÂÂÂ return 0;
>> +ÂÂÂÂÂÂÂ case AP_RESPONSE_RESET_IN_PROGRESS:
>> +ÂÂÂÂÂÂÂ case AP_RESPONSE_BUSY:
>> +ÂÂÂÂÂÂÂÂÂÂÂ msleep(20);
>> +ÂÂÂÂÂÂÂÂÂÂÂ break;
>> +ÂÂÂÂÂÂÂ default:
>> +ÂÂÂÂÂÂÂÂÂÂÂ /* things are really broken, give up */
>
> I'm not sure things are necessarily broken. We could end up here if
> the AP is removed from the configuration via the SE or SCLP Deconfigure
> Adjunct Processor command.
Yes, that's right. The default is also reached when the APQN
goes away from the configuration e. g. when an admin
drives the card "offline" on the SE. So maybe correct the comment.
>
>> +ÂÂÂÂÂÂÂÂÂÂÂ return -EIO;
>> +ÂÂÂÂÂÂÂ }
>> +ÂÂÂ } while (retry--);
>> +
>> +ÂÂÂ return -EBUSY;
>> +}
>> +
>> Â static void vfio_ap_matrix_init(struct ap_config_info *info,
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct ap_matrix *matrix)
>> Â {
>> @@ -45,6 +107,7 @@ static int vfio_ap_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
>> ÂÂÂÂÂÂÂÂÂ return -ENOMEM;
>> ÂÂÂÂÂ }
>> Â +ÂÂÂ INIT_LIST_HEAD(&matrix_mdev->qlist);
>> ÂÂÂÂÂ vfio_ap_matrix_init(&matrix_dev->info, &matrix_mdev->matrix);
>> ÂÂÂÂÂ mdev_set_drvdata(mdev, matrix_mdev);
>> ÂÂÂÂÂ mutex_lock(&matrix_dev->lock);
>> @@ -113,162 +176,189 @@ static struct attribute_group *vfio_ap_mdev_type_groups[] = {
>> ÂÂÂÂÂ NULL,
>> Â };
>> Â -struct vfio_ap_queue_reserved {
>> -ÂÂÂ unsigned long *apid;
>> -ÂÂÂ unsigned long *apqi;
>> -ÂÂÂ bool reserved;
>> -};
>> +static void vfio_ap_free_queue(int apqn, struct ap_matrix_mdev *matrix_mdev)
>> +{
>> +ÂÂÂ struct vfio_ap_queue *q;
>> +
>> +ÂÂÂ q = vfio_ap_get_queue(apqn, &matrix_mdev->qlist);
>> +ÂÂÂ if (!q)
>> +ÂÂÂÂÂÂÂ return;
>> +ÂÂÂ q->matrix_mdev = NULL;
>> +ÂÂÂ vfio_ap_mdev_reset_queue(q);
>
> I'm wondering if it's necessary to reset the queue here. The only time
> a queue is used is when a guest using the mdev device is started. When
> that guest is terminated, the fd for the mdev device is closed and the
> mdev device's release callback is invoked. The release callback resets
> the queues assigned to the mdev device. Is it really necessary to
> reset the queue again when it is unassigned even if there would have
> been no subsequent activity?
When I understand this here right this code is called when a queue goes
away from the guest but is still reserved for use by the vfio dd. So it is
possible to assign the queue now to another guest. But then it makes
sense to clear all the entries in the millicode queue because a pending
reply could be "received" by the wrong guest.

If this function is just called on remove of a queue device where the
device goes back to the AP bus, a reset is not needed.
>
>> +ÂÂÂ list_move(&q->list, &matrix_dev->free_list);
>> +}
>> Â Â /**
>> - * vfio_ap_has_queue
>> - *
>> - * @dev: an AP queue device
>> - * @data: a struct vfio_ap_queue_reserved reference
>> + * vfio_ap_put_all_domains:
>> ÂÂ *
>> - * Flags whether the AP queue device (@dev) has a queue ID containing the APQN,
>> - * apid or apqi specified in @data:
>> + * @matrix_mdev: the matrix mediated device for which we want to associate
>> + *ÂÂÂÂÂÂÂÂ all available queues with a given apqi.
>> + * @apid:ÂÂÂÂ The apid which associated with all defined APQI of the
>> + *ÂÂÂÂÂÂÂÂ mediated device will define a AP queue.
>> ÂÂ *
>> - * - If @data contains both an apid and apqi value, then @data will be flagged
>> - *ÂÂ as reserved if the APID and APQI fields for the AP queue device matches
>> - *
>> - * - If @data contains only an apid value, @data will be flagged as
>> - *ÂÂ reserved if the APID field in the AP queue device matches
>> - *
>> - * - If @data contains only an apqi value, @data will be flagged as
>> - *ÂÂ reserved if the APQI field in the AP queue device matches
>> - *
>> - * Returns 0 to indicate the input to function succeeded. Returns -EINVAL if
>> - * @data does not contain either an apid or apqi.
>> + * We remove the queue from the list of queues associated with the
>> + * mediated device and put them back to the free list of the matrix
>> + * device and clear the matrix_mdev pointer.
>> ÂÂ */
>> -static int vfio_ap_has_queue(struct device *dev, void *data)
>> +static void vfio_ap_put_all_domains(struct ap_matrix_mdev *matrix_mdev,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ int apid)
>> Â {
>> -ÂÂÂ struct vfio_ap_queue_reserved *qres = data;
>> -ÂÂÂ struct ap_queue *ap_queue = to_ap_queue(dev);
>> -ÂÂÂ ap_qid_t qid;
>> -ÂÂÂ unsigned long id;
>> +ÂÂÂ int apqi, apqn;
>> Â -ÂÂÂ if (qres->apid && qres->apqi) {
>> -ÂÂÂÂÂÂÂ qid = AP_MKQID(*qres->apid, *qres->apqi);
>> -ÂÂÂÂÂÂÂ if (qid == ap_queue->qid)
>> -ÂÂÂÂÂÂÂÂÂÂÂ qres->reserved = true;
>> -ÂÂÂ } else if (qres->apid && !qres->apqi) {
>> -ÂÂÂÂÂÂÂ id = AP_QID_CARD(ap_queue->qid);
>> -ÂÂÂÂÂÂÂ if (id == *qres->apid)
>> -ÂÂÂÂÂÂÂÂÂÂÂ qres->reserved = true;
>> -ÂÂÂ } else if (!qres->apid && qres->apqi) {
>> -ÂÂÂÂÂÂÂ id = AP_QID_QUEUE(ap_queue->qid);
>> -ÂÂÂÂÂÂÂ if (id == *qres->apqi)
>> -ÂÂÂÂÂÂÂÂÂÂÂ qres->reserved = true;
>> -ÂÂÂ } else {
>> -ÂÂÂÂÂÂÂ return -EINVAL;
>> +ÂÂÂ for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm, AP_DOMAINS) {
>> +ÂÂÂÂÂÂÂ apqn = AP_MKQID(apid, apqi);
>> +ÂÂÂÂÂÂÂ vfio_ap_free_queue(apqn, matrix_mdev);
>> ÂÂÂÂÂ }
>> -
>> -ÂÂÂ return 0;
>> Â }
>> Â Â /**
>> - * vfio_ap_verify_queue_reserved
>> - *
>> - * @matrix_dev: a mediated matrix device
>> - * @apid: an AP adapter ID
>> - * @apqi: an AP queue index
>> + * vfio_ap_put_all_cards:
>> ÂÂ *
>> - * Verifies that the AP queue with @apid/@apqi is reserved by the VFIO AP device
>> - * driver according to the following rules:
>> + * @matrix_mdev: the matrix mediated device for which we want to associate
>> + *ÂÂÂÂÂÂÂÂ all available queues with a given apqi.
>> + * @apqi:ÂÂÂÂ The apqi which associated with all defined APID of the
>> + *ÂÂÂÂÂÂÂÂ mediated device will define a AP queue.
>> ÂÂ *
>> - * - If both @apid and @apqi are not NULL, then there must be an AP queue
>> - *ÂÂ device bound to the vfio_ap driver with the APQN identified by @apid and
>> - *ÂÂ @apqi
>> - *
>> - * - If only @apid is not NULL, then there must be an AP queue device bound
>> - *ÂÂ to the vfio_ap driver with an APQN containing @apid
>> - *
>> - * - If only @apqi is not NULL, then there must be an AP queue device bound
>> - *ÂÂ to the vfio_ap driver with an APQN containing @apqi
>> - *
>> - * Returns 0 if the AP queue is reserved; otherwise, returns -EADDRNOTAVAIL.
>> + * We remove the queue from the list of queues associated with the
>> + * mediated device and put them back to the free list of the matrix
>> + * device and clear the matrix_mdev pointer.
>> ÂÂ */
>> -static int vfio_ap_verify_queue_reserved(unsigned long *apid,
>> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long *apqi)
>> +static void vfio_ap_put_all_cards(struct ap_matrix_mdev *matrix_mdev, int apqi)
>> Â {
>> -ÂÂÂ int ret;
>> -ÂÂÂ struct vfio_ap_queue_reserved qres;
>> +ÂÂÂ int apid, apqn;
>> Â -ÂÂÂ qres.apid = apid;
>> -ÂÂÂ qres.apqi = apqi;
>> -ÂÂÂ qres.reserved = false;
>> -
>> -ÂÂÂ ret = driver_for_each_device(&matrix_dev->vfio_ap_drv->driver, NULL,
>> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &qres, vfio_ap_has_queue);
>> -ÂÂÂ if (ret)
>> -ÂÂÂÂÂÂÂ return ret;
>> +ÂÂÂ for_each_set_bit_inv(apid, matrix_mdev->matrix.apm, AP_DEVICES) {
>> +ÂÂÂÂÂÂÂ apqn = AP_MKQID(apid, apqi);
>> +ÂÂÂÂÂÂÂ vfio_ap_free_queue(apqn, matrix_mdev);
>> +ÂÂÂ }
>> +}
>> Â -ÂÂÂ if (qres.reserved)
>> -ÂÂÂÂÂÂÂ return 0;
>> +static void move_and_set(struct list_head *src, struct list_head *dst,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂ struct ap_matrix_mdev *matrix_mdev)
>> +{
>> +ÂÂÂ struct vfio_ap_queue *q, *qtmp;
>> Â -ÂÂÂ return -EADDRNOTAVAIL;
>> +ÂÂÂ list_for_each_entry_safe(q, qtmp, src, list) {
>> +ÂÂÂÂÂÂÂ list_move(&q->list, dst);
>> +ÂÂÂÂÂÂÂ q->matrix_mdev = matrix_mdev;
>> +ÂÂÂ }
>> Â }
>> Â -static int
>> -vfio_ap_mdev_verify_queues_reserved_for_apid(struct ap_matrix_mdev *matrix_mdev,
>> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long apid)
>> +static int vfio_ap_queue_match(struct device *dev, void *data)
>> Â {
>> -ÂÂÂ int ret;
>> -ÂÂÂ unsigned long apqi;
>> -ÂÂÂ unsigned long nbits = matrix_mdev->matrix.aqm_max + 1;
>> +ÂÂÂ struct ap_queue *ap;
>> Â -ÂÂÂ if (find_first_bit_inv(matrix_mdev->matrix.aqm, nbits) >= nbits)
>> -ÂÂÂÂÂÂÂ return vfio_ap_verify_queue_reserved(&apid, NULL);
>> +ÂÂÂ ap = to_ap_queue(dev);
>> +ÂÂÂ return ap->qid == *(int *)data;
>> +}
>> Â -ÂÂÂ for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm, nbits) {
>> -ÂÂÂÂÂÂÂ ret = vfio_ap_verify_queue_reserved(&apid, &apqi);
>> -ÂÂÂÂÂÂÂ if (ret)
>> -ÂÂÂÂÂÂÂÂÂÂÂ return ret;
>> -ÂÂÂ }
>> +static struct vfio_ap_queue *vfio_ap_find_queue(int apqn)
>> +{
>> +ÂÂÂ struct device *dev;
>> +ÂÂÂ struct vfio_ap_queue *q;
>> +
>> +ÂÂÂ dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &apqn, vfio_ap_queue_match);
>> +ÂÂÂ if (!dev)
>> +ÂÂÂÂÂÂÂ return NULL;
>> +ÂÂÂ q = dev_get_drvdata(dev);
>> +ÂÂÂ put_device(dev);
>> +ÂÂÂ return q;
>> +}
>> Â +/**
>> + * vfio_ap_get_all_domains:
>> + *
>> + * @matrix_mdev: the matrix mediated device for which we want to associate
>> + *ÂÂÂÂÂÂÂÂ all available queues with a given apqi.
>> + * @apqi:ÂÂÂÂ The apqi which associated with all defined APID of the
>> + *ÂÂÂÂÂÂÂÂ mediated device will define a AP queue.
>> + *
>> + * We define a local list to put all queues we find on the matrix driver
>> + * device list when associating the apqi with all already defined apid for
>> + * this matrix mediated device.
>> + *
>> + * If we can get all the devices we roll them to the mediated device list
>> + * If we get errors we unroll them to the free list.
>> + */
>> +static int vfio_ap_get_all_domains(struct ap_matrix_mdev *matrix_mdev, int apid)
>> +{
>> +ÂÂÂ int apqi, apqn;
>> +ÂÂÂ int ret = 0;
>> +ÂÂÂ struct vfio_ap_queue *q;
>> +ÂÂÂ struct list_head q_list;
>> +
>> +ÂÂÂ if (!vfio_ap_find_any_card(apid))
>> +ÂÂÂÂÂÂÂ return -EADDRNOTAVAIL;
>> +
>> +ÂÂÂ INIT_LIST_HEAD(&q_list);
>> +
>> +ÂÂÂ for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm, AP_DOMAINS) {
>> +ÂÂÂÂÂÂÂ apqn = AP_MKQID(apid, apqi);
>> +ÂÂÂÂÂÂÂ q = vfio_ap_find_queue(apqn);
>> +ÂÂÂÂÂÂÂ if (!q) {
>> +ÂÂÂÂÂÂÂÂÂÂÂ ret = -EADDRNOTAVAIL;
>> +ÂÂÂÂÂÂÂÂÂÂÂ goto rewind;
>> +ÂÂÂÂÂÂÂ }
>> +ÂÂÂÂÂÂÂ if (q->matrix_mdev) {
>
> If somebody assigns the same adapter a second time, the assignment will
> fail because the matrix_mdev will already have been associated with the
> queue. I don't think it is appropriate to fail the assignment if the
> q->matrix_mdev is the same as the input matrix_mdev. This should be
> changed to:
>
> ÂÂÂÂif (q->matrix_mdev != matrix_mdev)
>
>> +ÂÂÂÂÂÂÂÂÂÂÂ ret = -EADDRINUSE;
>> +ÂÂÂÂÂÂÂÂÂÂÂ goto rewind;
>> +ÂÂÂÂÂÂÂ }
>> +ÂÂÂÂÂÂÂ list_move(&q->list, &q_list);
>> +ÂÂÂ }
>> +ÂÂÂ move_and_set(&q_list, &matrix_mdev->qlist, matrix_mdev);
>> ÂÂÂÂÂ return 0;
>> +rewind:
>> +ÂÂÂ move_and_set(&q_list, &matrix_dev->free_list, NULL);
>> +ÂÂÂ return ret;
>> Â }
>> -
>> Â /**
>> - * vfio_ap_mdev_verify_no_sharing
>> + * vfio_ap_get_all_cards:
>> ÂÂ *
>> - * Verifies that the APQNs derived from the cross product of the AP adapter IDs
>> - * and AP queue indexes comprising the AP matrix are not configured for another
>> - * mediated device. AP queue sharing is not allowed.
>> + * @matrix_mdev: the matrix mediated device for which we want to associate
>> + *ÂÂÂÂÂÂÂÂ all available queues with a given apqi.
>> + * @apqi:ÂÂÂÂ The apqi which associated with all defined APID of the
>> + *ÂÂÂÂÂÂÂÂ mediated device will define a AP queue.
>> ÂÂ *
>> - * @matrix_mdev: the mediated matrix device
>> + * We define a local list to put all queues we find on the matrix device
>> + * free list when associating the apqi with all already defined apid for
>> + * this matrix mediated device.
>> ÂÂ *
>> - * Returns 0 if the APQNs are not shared, otherwise; returns -EADDRINUSE.
>> + * If we can get all the devices we roll them to the mediated device list
>> + * If we get errors we unroll them to the free list.
>> ÂÂ */
>> -static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
>> +static int vfio_ap_get_all_cards(struct ap_matrix_mdev *matrix_mdev, int apqi)
>> Â {
>> -ÂÂÂ struct ap_matrix_mdev *lstdev;
>> -ÂÂÂ DECLARE_BITMAP(apm, AP_DEVICES);
>> -ÂÂÂ DECLARE_BITMAP(aqm, AP_DOMAINS);
>> -
>> -ÂÂÂ list_for_each_entry(lstdev, &matrix_dev->mdev_list, node) {
>> -ÂÂÂÂÂÂÂ if (matrix_mdev == lstdev)
>> -ÂÂÂÂÂÂÂÂÂÂÂ continue;
>> -
>> -ÂÂÂÂÂÂÂ memset(apm, 0, sizeof(apm));
>> -ÂÂÂÂÂÂÂ memset(aqm, 0, sizeof(aqm));
>> -
>> -ÂÂÂÂÂÂÂ /*
>> -ÂÂÂÂÂÂÂÂ * We work on full longs, as we can only exclude the leftover
>> -ÂÂÂÂÂÂÂÂ * bits in non-inverse order. The leftover is all zeros.
>> -ÂÂÂÂÂÂÂÂ */
>> -ÂÂÂÂÂÂÂ if (!bitmap_and(apm, matrix_mdev->matrix.apm,
>> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ lstdev->matrix.apm, AP_DEVICES))
>> -ÂÂÂÂÂÂÂÂÂÂÂ continue;
>> -
>> -ÂÂÂÂÂÂÂ if (!bitmap_and(aqm, matrix_mdev->matrix.aqm,
>> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ lstdev->matrix.aqm, AP_DOMAINS))
>> -ÂÂÂÂÂÂÂÂÂÂÂ continue;
>> -
>> -ÂÂÂÂÂÂÂ return -EADDRINUSE;
>> +ÂÂÂ int apid, apqn;
>> +ÂÂÂ int ret = 0;
>> +ÂÂÂ struct vfio_ap_queue *q;
>> +ÂÂÂ struct list_head q_list;
>> +ÂÂÂ struct ap_matrix_mdev *tmp = NULL;
>> +
>> +ÂÂÂ if (!vfio_ap_find_any_domain(apqi))
>> +ÂÂÂÂÂÂÂ return -EADDRNOTAVAIL;
>> +
>> +ÂÂÂ INIT_LIST_HEAD(&q_list);
>> +
>> +ÂÂÂ for_each_set_bit_inv(apid, matrix_mdev->matrix.apm, AP_DEVICES) {
>> +ÂÂÂÂÂÂÂ apqn = AP_MKQID(apid, apqi);
>> +ÂÂÂÂÂÂÂ q = vfio_ap_find_queue(apqn);
>> +ÂÂÂÂÂÂÂ if (!q) {
>> +ÂÂÂÂÂÂÂÂÂÂÂ ret = -EADDRNOTAVAIL;
>> +ÂÂÂÂÂÂÂÂÂÂÂ goto rewind;
>> +ÂÂÂÂÂÂÂ }
>> +ÂÂÂÂÂÂÂ if (q->matrix_mdev) {
>
> If somebody assigns the same domain a second time, the assignment will
> fail because the matrix_mdev will already have been associated with the
> queue. I don't think it is appropriate to fail the assignment if the
> q->matrix_mdev is the same as the input matrix_mdev. This should be
> changed to:
>
> ÂÂÂÂif (q->matrix_mdev != matrix_mdev)
>
>> +ÂÂÂÂÂÂÂÂÂÂÂ ret = -EADDRINUSE;
>> +ÂÂÂÂÂÂÂÂÂÂÂ goto rewind;
>> +ÂÂÂÂÂÂÂ }
>> +ÂÂÂÂÂÂÂ list_move(&q->list, &q_list);
>> ÂÂÂÂÂ }
>> -
>> +ÂÂÂ tmp = matrix_mdev;
>> +ÂÂÂ move_and_set(&q_list, &matrix_mdev->qlist, matrix_mdev);
>> ÂÂÂÂÂ return 0;
>> +rewind:
>> +ÂÂÂ move_and_set(&q_list, &matrix_dev->free_list, NULL);
>> +ÂÂÂ return ret;
>> Â }
>> Â Â /**
>> @@ -330,21 +420,15 @@ static ssize_t assign_adapter_store(struct device *dev,
>> ÂÂÂÂÂÂ */
>> ÂÂÂÂÂ mutex_lock(&matrix_dev->lock);
>> Â -ÂÂÂ ret = vfio_ap_mdev_verify_queues_reserved_for_apid(matrix_mdev, apid);
>> +ÂÂÂ ret = vfio_ap_get_all_domains(matrix_mdev, apid);
>> ÂÂÂÂÂ if (ret)
>> ÂÂÂÂÂÂÂÂÂ goto done;
>> Â ÂÂÂÂÂ set_bit_inv(apid, matrix_mdev->matrix.apm);
>> Â -ÂÂÂ ret = vfio_ap_mdev_verify_no_sharing(matrix_mdev);
>> -ÂÂÂ if (ret)
>> -ÂÂÂÂÂÂÂ goto share_err;
>> -
>> ÂÂÂÂÂ ret = count;
>> ÂÂÂÂÂ goto done;
>> Â -share_err:
>> -ÂÂÂ clear_bit_inv(apid, matrix_mdev->matrix.apm);
>> Â done:
>> ÂÂÂÂÂ mutex_unlock(&matrix_dev->lock);
>> Â @@ -391,32 +475,13 @@ 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_put_all_domains(matrix_mdev, apid);
>> ÂÂÂÂÂ mutex_unlock(&matrix_dev->lock);
>> Â ÂÂÂÂÂ return count;
>> Â }
>> Â static DEVICE_ATTR_WO(unassign_adapter);
>> Â -static int
>> -vfio_ap_mdev_verify_queues_reserved_for_apqi(struct ap_matrix_mdev *matrix_mdev,
>> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long apqi)
>> -{
>> -ÂÂÂ int ret;
>> -ÂÂÂ unsigned long apid;
>> -ÂÂÂ unsigned long nbits = matrix_mdev->matrix.apm_max + 1;
>> -
>> -ÂÂÂ if (find_first_bit_inv(matrix_mdev->matrix.apm, nbits) >= nbits)
>> -ÂÂÂÂÂÂÂ return vfio_ap_verify_queue_reserved(NULL, &apqi);
>> -
>> -ÂÂÂ for_each_set_bit_inv(apid, matrix_mdev->matrix.apm, nbits) {
>> -ÂÂÂÂÂÂÂ ret = vfio_ap_verify_queue_reserved(&apid, &apqi);
>> -ÂÂÂÂÂÂÂ if (ret)
>> -ÂÂÂÂÂÂÂÂÂÂÂ return ret;
>> -ÂÂÂ }
>> -
>> -ÂÂÂ return 0;
>> -}
>> -
>> Â /**
>> ÂÂ * assign_domain_store
>> ÂÂ *
>> @@ -471,21 +536,15 @@ static ssize_t assign_domain_store(struct device *dev,
>> Â ÂÂÂÂÂ mutex_lock(&matrix_dev->lock);
>> Â -ÂÂÂ ret = vfio_ap_mdev_verify_queues_reserved_for_apqi(matrix_mdev, apqi);
>> +ÂÂÂ ret = vfio_ap_get_all_cards(matrix_mdev, apqi);
>> ÂÂÂÂÂ if (ret)
>> ÂÂÂÂÂÂÂÂÂ goto done;
>> Â ÂÂÂÂÂ set_bit_inv(apqi, matrix_mdev->matrix.aqm);
>> Â -ÂÂÂ ret = vfio_ap_mdev_verify_no_sharing(matrix_mdev);
>> -ÂÂÂ if (ret)
>> -ÂÂÂÂÂÂÂ goto share_err;
>> -
>> ÂÂÂÂÂ ret = count;
>> ÂÂÂÂÂ goto done;
>> Â -share_err:
>> -ÂÂÂ clear_bit_inv(apqi, matrix_mdev->matrix.aqm);
>> Â done:
>> ÂÂÂÂÂ mutex_unlock(&matrix_dev->lock);
>> Â @@ -533,6 +592,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_put_all_cards(matrix_mdev, apqi);
>> ÂÂÂÂÂ mutex_unlock(&matrix_dev->lock);
>> Â ÂÂÂÂÂ return count;
>> @@ -790,49 +850,22 @@ static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
>> ÂÂÂÂÂ return NOTIFY_OK;
>> Â }
>> Â -static int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
>> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned int retry)
>> -{
>> -ÂÂÂ struct ap_queue_status status;
>> -
>> -ÂÂÂ do {
>> -ÂÂÂÂÂÂÂ status = ap_zapq(AP_MKQID(apid, apqi));
>> -ÂÂÂÂÂÂÂ switch (status.response_code) {
>> -ÂÂÂÂÂÂÂ case AP_RESPONSE_NORMAL:
>> -ÂÂÂÂÂÂÂÂÂÂÂ return 0;
>> -ÂÂÂÂÂÂÂ case AP_RESPONSE_RESET_IN_PROGRESS:
>> -ÂÂÂÂÂÂÂ case AP_RESPONSE_BUSY:
>> -ÂÂÂÂÂÂÂÂÂÂÂ msleep(20);
>> -ÂÂÂÂÂÂÂÂÂÂÂ break;
>> -ÂÂÂÂÂÂÂ default:
>> -ÂÂÂÂÂÂÂÂÂÂÂ /* things are really broken, give up */
>> -ÂÂÂÂÂÂÂÂÂÂÂ return -EIO;
>> -ÂÂÂÂÂÂÂ }
>> -ÂÂÂ } while (retry--);
>> -
>> -ÂÂÂ return -EBUSY;
>> -}
>> -
>> Â static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev)
>> Â {
>> ÂÂÂÂÂ int ret;
>> ÂÂÂÂÂ int rc = 0;
>> -ÂÂÂ unsigned long apid, apqi;
>> ÂÂÂÂÂ struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
>> +ÂÂÂ struct vfio_ap_queue *q;
>> Â -ÂÂÂ for_each_set_bit_inv(apid, matrix_mdev->matrix.apm,
>> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ matrix_mdev->matrix.apm_max + 1) {
>> -ÂÂÂÂÂÂÂ for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm,
>> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ matrix_mdev->matrix.aqm_max + 1) {
>> -ÂÂÂÂÂÂÂÂÂÂÂ ret = vfio_ap_mdev_reset_queue(apid, apqi, 1);
>> -ÂÂÂÂÂÂÂÂÂÂÂ /*
>> -ÂÂÂÂÂÂÂÂÂÂÂÂ * Regardless whether a queue turns out to be busy, or
>> -ÂÂÂÂÂÂÂÂÂÂÂÂ * is not operational, we need to continue resetting
>> -ÂÂÂÂÂÂÂÂÂÂÂÂ * the remaining queues.
>> -ÂÂÂÂÂÂÂÂÂÂÂÂ */
>> -ÂÂÂÂÂÂÂÂÂÂÂ if (ret)
>> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ rc = ret;
>> -ÂÂÂÂÂÂÂ }
>> +ÂÂÂ list_for_each_entry(q, &matrix_mdev->qlist, list) {
>> +ÂÂÂÂÂÂÂ ret = vfio_ap_mdev_reset_queue(q);
>> +ÂÂÂÂÂÂÂ /*
>> +ÂÂÂÂÂÂÂÂ * Regardless whether a queue turns out to be busy, or
>> +ÂÂÂÂÂÂÂÂ * is not operational, we need to continue resetting
>> +ÂÂÂÂÂÂÂÂ * the remaining queues but notice the last error code.
>> +ÂÂÂÂÂÂÂÂ */
>> +ÂÂÂÂÂÂÂ if (ret)
>> +ÂÂÂÂÂÂÂÂÂÂÂ rc = ret;
>> ÂÂÂÂÂ }
>> Â ÂÂÂÂÂ return rc;
>> @@ -868,10 +901,10 @@ static void vfio_ap_mdev_release(struct mdev_device *mdev)
>> ÂÂÂÂÂ if (matrix_mdev->kvm)
>> ÂÂÂÂÂÂÂÂÂ kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
>> Â +ÂÂÂ matrix_mdev->kvm = NULL;
>> ÂÂÂÂÂ vfio_ap_mdev_reset_queues(mdev);
>> ÂÂÂÂÂ vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &matrix_mdev->group_notifier);
>> -ÂÂÂ matrix_mdev->kvm = NULL;
>> ÂÂÂÂÂ module_put(THIS_MODULE);
>> Â }
>> Â @@ -905,7 +938,9 @@ static ssize_t vfio_ap_mdev_ioctl(struct mdev_device *mdev,
>> ÂÂÂÂÂÂÂÂÂ ret = vfio_ap_mdev_get_device_info(arg);
>> ÂÂÂÂÂÂÂÂÂ break;
>> ÂÂÂÂÂ case VFIO_DEVICE_RESET:
>> +ÂÂÂÂÂÂÂ mutex_lock(&matrix_dev->lock);
>> ÂÂÂÂÂÂÂÂÂ ret = vfio_ap_mdev_reset_queues(mdev);
>> +ÂÂÂÂÂÂÂ mutex_unlock(&matrix_dev->lock);
>> ÂÂÂÂÂÂÂÂÂ break;
>> ÂÂÂÂÂ default:
>> ÂÂÂÂÂÂÂÂÂ ret = -EOPNOTSUPP;
>> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
>> index a910be1..3e6940c 100644
>> --- a/drivers/s390/crypto/vfio_ap_private.h
>> +++ b/drivers/s390/crypto/vfio_ap_private.h
>> @@ -40,6 +40,7 @@ struct ap_matrix_dev {
>> ÂÂÂÂÂ atomic_t available_instances;
>> ÂÂÂÂÂ struct ap_config_info info;
>> ÂÂÂÂÂ struct list_head mdev_list;
>> +ÂÂÂ struct list_head free_list;
>> ÂÂÂÂÂ struct mutex lock;
>>  struct ap_driver *vfio_ap_drv;
>> Â };
>> @@ -83,9 +84,15 @@ struct ap_matrix_mdev {
>> ÂÂÂÂÂ struct notifier_block group_notifier;
>> ÂÂÂÂÂ struct kvm *kvm;
>> ÂÂÂÂÂ struct kvm_s390_module_hook pqap_hook;
>> +ÂÂÂ struct list_head qlist;
>> Â };
>> Â Â extern int vfio_ap_mdev_register(void);
>> Â extern void vfio_ap_mdev_unregister(void);
>> Â +struct vfio_ap_queue {
>> +ÂÂÂ struct list_head list;
>> +ÂÂÂ struct ap_matrix_mdev *matrix_mdev;
>> +ÂÂÂ intÂÂÂ apqn;
>> +};
>> Â #endif /* _VFIO_AP_PRIVATE_H_ */
>>
>