| Message ID | 20200407192015.19887-3-akrowiak@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390/vfio-ap: dynamic configuration support | expand |
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). > + * > + * @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. > +{ > + 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.
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). I like this idea, how about vfio_ap_link_mdev_to_queues()? > >> + * >> + * @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. How about vfio_ap_link_queue_to_mdevs()? > >> +{ >> + 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. >
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. >
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c index 134860934fe7..00699bd72d2b 100644 --- a/drivers/s390/crypto/vfio_ap_ops.c +++ b/drivers/s390/crypto/vfio_ap_ops.c @@ -148,7 +148,6 @@ 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; } @@ -250,7 +249,6 @@ static int handle_pqap(struct kvm_vcpu *vcpu) struct vfio_ap_queue *q; struct ap_queue_status qstatus = { .response_code = AP_RESPONSE_Q_NOT_AVAIL, }; - struct ap_matrix_mdev *matrix_mdev; /* If we do not use the AIV facility just go to userland */ if (!(vcpu->arch.sie_block->eca & ECA_AIV)) @@ -261,14 +259,11 @@ static int handle_pqap(struct kvm_vcpu *vcpu) if (!vcpu->kvm->arch.crypto.pqap_hook) goto out_unlock; - matrix_mdev = container_of(vcpu->kvm->arch.crypto.pqap_hook, - struct ap_matrix_mdev, pqap_hook); q = vfio_ap_get_queue(apqn); if (!q) goto out_unlock; - q->matrix_mdev = matrix_mdev; status = vcpu->run->s.regs.gprs[1]; /* If IR bit(16) is set we enable the interrupt */ @@ -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 + * + * @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 * @@ -605,6 +625,7 @@ static ssize_t assign_adapter_store(struct device *dev, if (ret) goto share_err; + vfio_ap_mdev_qlinks_for_apid(matrix_mdev, apid); ret = count; goto done; @@ -656,6 +677,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_qlinks_for_apid(NULL, apid); mutex_unlock(&matrix_dev->lock); return count; @@ -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 + * + * @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 * @@ -746,6 +793,7 @@ static ssize_t assign_domain_store(struct device *dev, if (ret) goto share_err; + vfio_ap_mdev_qlinks_for_apqi(matrix_mdev, apqi); ret = count; goto done; @@ -798,6 +846,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_qlinks_for_apqi(NULL, apqi); mutex_unlock(&matrix_dev->lock); return count; @@ -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) +{ + 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);
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(-)