Messages in this thread | | | Subject | Re: [PATCH v7 02/15] s390/vfio-ap: manage link between queue struct and matrix mdev | From | Tony Krowiak <> | Date | Fri, 10 Apr 2020 11:41:22 -0400 |
| |
On 4/9/20 11:06 AM, Cornelia Huck wrote: > On Tue, 7 Apr 2020 15:20:02 -0400 > Tony Krowiak <akrowiak@linux.ibm.com> wrote: > >> A vfio_ap_queue structure is created for each queue device probed. To >> ensure that the matrix mdev to which a queue's APQN is assigned is linked >> to the queue structure as long as the queue device is bound to the vfio_ap >> device driver, let's go ahead and manage these links when the queue device >> is probed and removed as well as whenever an adapter or domain is assigned >> to or unassigned from the matrix mdev. >> >> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> >> --- >> drivers/s390/crypto/vfio_ap_ops.c | 75 ++++++++++++++++++++++++++++--- >> 1 file changed, 70 insertions(+), 5 deletions(-) > (...) > >> @@ -536,6 +531,31 @@ static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev) >> return 0; >> } >> >> +/** >> + * vfio_ap_mdev_qlinks_for_apid > Hm... maybe the function name should express that there's some actual > (un)linking going on? > > vfio_ap_mdev_link_by_apid? > > Or make this vfio_ap_mdev_link_queues() and pass in an indicator whether > the passed value is an apid or an aqid? Both function names look so > very similar to be easily confused (at least to me).
Forget my last response, I like your function names better.
> >> + * >> + * @matrix_mdev: a matrix mediated device >> + * @apqi: the APID of one or more APQNs assigned to @matrix_mdev >> + * >> + * Set the link to @matrix_mdev for each queue device bound to the vfio_ap >> + * device driver with an APQN assigned to @matrix_mdev with the specified @apid. >> + * >> + * Note: If @matrix_mdev is NULL, the link to @matrix_mdev will be severed. >> + */ >> +static void vfio_ap_mdev_qlinks_for_apid(struct ap_matrix_mdev *matrix_mdev, >> + unsigned long apid) >> +{ >> + unsigned long apqi; >> + struct vfio_ap_queue *q; >> + >> + for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm, >> + matrix_mdev->matrix.aqm_max + 1) { >> + q = vfio_ap_get_queue(AP_MKQID(apid, apqi)); >> + if (q) >> + q->matrix_mdev = matrix_mdev; >> + } >> +} >> + >> /** >> * assign_adapter_store >> * > (...) > >> @@ -682,6 +704,31 @@ vfio_ap_mdev_verify_queues_reserved_for_apqi(struct ap_matrix_mdev *matrix_mdev, >> return 0; >> } >> >> +/** >> + * vfio_ap_mdev_qlinks_for_apqi > See my comment above. > >> + * >> + * @matrix_mdev: a matrix mediated device >> + * @apqi: the APQI of one or more APQNs assigned to @matrix_mdev >> + * >> + * Set the link to @matrix_mdev for each queue device bound to the vfio_ap >> + * device driver with an APQN assigned to @matrix_mdev with the specified @apqi. >> + * >> + * Note: If @matrix_mdev is NULL, the link to @matrix_mdev will be severed. >> + */ >> +static void vfio_ap_mdev_qlinks_for_apqi(struct ap_matrix_mdev *matrix_mdev, >> + unsigned long apqi) >> +{ >> + unsigned long apid; >> + struct vfio_ap_queue *q; >> + >> + for_each_set_bit_inv(apid, matrix_mdev->matrix.apm, >> + matrix_mdev->matrix.apm_max + 1) { >> + q = vfio_ap_get_queue(AP_MKQID(apid, apqi)); >> + if (q) >> + q->matrix_mdev = matrix_mdev; >> + } >> +} >> + >> /** >> * assign_domain_store >> * > (...) > >> @@ -1270,6 +1319,21 @@ void vfio_ap_mdev_unregister(void) >> mdev_unregister_device(&matrix_dev->device); >> } >> >> +static void vfio_ap_mdev_for_queue(struct vfio_ap_queue *q) > vfio_ap_queue_link_mdev()? It is the other direction from the linking > above.
See my comment above.
> >> +{ >> + 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)) { >> + q->matrix_mdev = matrix_mdev; >> + break; >> + } >> + } >> +} >> + >> int vfio_ap_mdev_probe_queue(struct ap_queue *queue) >> { >> struct vfio_ap_queue *q; >> @@ -1282,6 +1346,7 @@ int vfio_ap_mdev_probe_queue(struct ap_queue *queue) >> dev_set_drvdata(&queue->ap_dev.device, q); >> q->apqn = queue->qid; >> q->saved_isc = VFIO_AP_ISC_INVALID; >> + vfio_ap_mdev_for_queue(q); >> hash_add(matrix_dev->qtable, &q->qnode, q->apqn); >> mutex_unlock(&matrix_dev->lock); >> > In general, looks sane. >
| |