Re: [PATCH v6 2/7] s390: ap: new vfio_ap_queue structure
From: Tony Krowiak
Date: Thu Mar 28 2019 - 11:32:47 EST
On 3/28/19 9:06 AM, Pierre Morel wrote:
On 26/03/2019 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/
OK
holding the information we need to handle PQAP/AQIC interception
and setup the GISA.
s/setup/set up/
OK
...snip...
+
+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.
OK, but note that it is your original comment I just moved the function
here ;)
Yes, it is. I'm smarter now;)
+ÂÂÂÂÂÂÂÂÂÂÂ 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 /* Bits 41-47 must all be zeros */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?
Yes, it is necessary, the queue can be re-assigned to another guest later.
Release will only be called when unbinding the queue from the driver.
That is true, but if the queue is never used, there is nothing to reset.
+ÂÂÂ list_move(&q->list, &matrix_dev->free_list);
+}
...snip...
+ÂÂÂ 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
It is usual to report a failure in the case the operation requested has
already be done.
But we can do as you want. Any other opinion?
q->matrix_mdev is the same as the input matrix_mdev. This should be
changed to:
ÂÂÂÂÂif (q->matrix_mdev != matrix_mdev)
You surely want to say: add this, not change to this. ;)
Yes
Thanks for commenting,
Regards,
Pierre