Re: [PATCH v11 03/14] s390/vfio-ap: manage link between queue struct and matrix mdev
On Thu, 22 Oct 2020 13:11:58 -0400
Tony Krowiak wrote:
> Let's create links between each queue device bound to the vfio_ap device
> driver and the matrix mdev to which the queue 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.
>
I would prefer if the changes to the q->matrix_mdev link were restricted
to this patch. Patches 1 and 2 do some of that stuff as well. See my
comments at the code.
> Signed-off-by: Tony Krowiak
> ---
> drivers/s390/crypto/vfio_ap_ops.c | 146 +++---
> drivers/s390/crypto/vfio_ap_private.h | 3 +
> 2 files changed, 135 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c
> b/drivers/s390/crypto/vfio_ap_ops.c
> index 049b97d7444c..1357f8f8b7e4 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -28,7 +28,6 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device
> *mdev);
>
> /**
> * vfio_ap_get_queue: Retrieve a queue with a specific APQN.
> - * @matrix_mdev: the associated mediated matrix
> * @apqn: The queue APQN
> *
> * Retrieve a queue with a specific APQN from the AP queue devices attached
> to
> @@ -36,18 +35,11 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device
> *mdev);
> *
> * Returns the pointer to the vfio_ap_queue with the specified APQN, or NULL.
> */
> -static struct vfio_ap_queue *vfio_ap_get_queue(
> - struct ap_matrix_mdev *matrix_mdev,
> - unsigned long apqn)
> +static struct vfio_ap_queue *vfio_ap_get_queue(unsigned long apqn)
> {
> struct ap_queue *queue;
> 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;
> -
> queue = ap_get_qdev(apqn);
> if (!queue)
> return NULL;
Patch 2 removed
q->matrix_mdev = matrix_mdev;
because patch 1 make it redundant. But patch 1 should not have made it
redundant in the first place.
It should be removed in this patch.
> @@ -60,6 +52,19 @@ static struct vfio_ap_queue *vfio_ap_get_queue(
> return q;
> }
>
> +static struct vfio_ap_queue *
> +vfio_ap_mdev_get_queue(struct ap_matrix_mdev *matrix_mdev, unsigned long
> apqn)
> +{
> + struct vfio_ap_queue *q;
> +
> + hash_for_each_possible(matrix_mdev->qtable, q, mdev_qnode, apqn) {
> + if (q && (q->apqn == apqn))
> + return q;
> + }
> +
> + return NULL;
> +}
> +
> /**
> * vfio_ap_wait_for_irqclear
> * @apqn: The AP Queue number
> @@ -171,7 +176,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;
> }
>
> @@ -284,14 +288,14 @@ 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(matrix_mdev, apqn);
> + q = vfio_ap_mdev_get_queue(matrix_mdev, apqn);
> if (!q)
> goto out_unlock;
>
> - q->matrix_mdev = matrix_mdev;
This was unnecessarily added in patch 1, now it's removed.
> status = vcpu->run->s.regs.gprs[1];
>
> /* If IR bit(16) is set we enable the interrupt */
> @@ -331,6 +335,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->qt
[PATCH v11 03/14] s390/vfio-ap: manage link between queue struct and matrix mdev
Let's create links between each queue device bound to the vfio_ap device
driver and the matrix mdev to which the queue 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
---
drivers/s390/crypto/vfio_ap_ops.c | 146 +++---
drivers/s390/crypto/vfio_ap_private.h | 3 +
2 files changed, 135 insertions(+), 14 deletions(-)
diff --git a/drivers/s390/crypto/vfio_ap_ops.c
b/drivers/s390/crypto/vfio_ap_ops.c
index 049b97d7444c..1357f8f8b7e4 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -28,7 +28,6 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device
*mdev);
/**
* vfio_ap_get_queue: Retrieve a queue with a specific APQN.
- * @matrix_mdev: the associated mediated matrix
* @apqn: The queue APQN
*
* Retrieve a queue with a specific APQN from the AP queue devices attached to
@@ -36,18 +35,11 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device
*mdev);
*
* Returns the pointer to the vfio_ap_queue with the specified APQN, or NULL.
*/
-static struct vfio_ap_queue *vfio_ap_get_queue(
- struct ap_matrix_mdev *matrix_mdev,
- unsigned long apqn)
+static struct vfio_ap_queue *vfio_ap_get_queue(unsigned long apqn)
{
struct ap_queue *queue;
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;
-
queue = ap_get_qdev(apqn);
if (!queue)
return NULL;
@@ -60,6 +52,19 @@ static struct vfio_ap_queue *vfio_ap_get_queue(
return q;
}
+static struct vfio_ap_queue *
+vfio_ap_mdev_get_queue(struct ap_matrix_mdev *matrix_mdev, unsigned long apqn)
+{
+ struct vfio_ap_queue *q;
+
+ hash_for_each_possible(matrix_mdev->qtable, q, mdev_qnode, apqn) {
+ if (q && (q->apqn == apqn))
+ return q;
+ }
+
+ return NULL;
+}
+
/**
* vfio_ap_wait_for_irqclear
* @apqn: The AP Queue number
@@ -171,7 +176,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;
}
@@ -284,14 +288,14 @@ 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(matrix_mdev, apqn);
+ q = vfio_ap_mdev_get_queue(matrix_mdev, 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 */
@@ -331,6 +335,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;
@@ -559,6 +564,87 @@ static int vfio_ap_mdev_verify_no_sharing(struct
ap_matrix_mdev *matrix_mdev)
return 0;
}
+enum qlink_type {
+ LINK_APID,
+ LINK_APQI,
+ UNLINK_APID,
+ UNLINK_APQI,
+};
+
+static void vfio_ap_mdev_link_queue(struct ap_matrix_mdev *matrix_mdev,
+ unsigned long apid, unsigned long apqi)
+{
+ struct vfio_ap_queue *q;
+
+ q = vfio_ap_get_queue(AP_MKQID(apid, apqi));
+ if (q) {
+
