Re: [PATCH v4 5/7] s390: vfio-ap: allow assignment of unavailable AP resources to mdev device

From: Tony Krowiak
Date: Wed Jun 19 2019 - 09:44:21 EST

On 6/18/19 2:49 AM, Harald Freudenberger wrote:
On 17.06.19 17:07, Tony Krowiak wrote:
On 6/17/19 6:05 AM, Harald Freudenberger wrote:
On 13.06.19 21:39, Tony Krowiak wrote:
The AP architecture does not preclude assignment of AP resources that are
not available. Let's go ahead and implement this facet of the AP
architecture for linux guests.

The current implementation does not allow assignment of AP adapters or
domains to an mdev device if the APQNs resulting from the assignment
reference AP queue devices that are not bound to the vfio_ap device
driver. This patch allows assignment of AP resources to the mdev device as
long as the APQNs resulting from the assignment are not reserved by the AP
BUS for use by the zcrypt device drivers and the APQNs are not assigned to
another mdev device.

Signed-off-by: Tony Krowiak <akrowiak@xxxxxxxxxxxxx>
---
 drivers/s390/crypto/vfio_ap_ops.c | 231 ++++++++------------------------------
 1 file changed, 44 insertions(+), 187 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 60efd3d7896d..9db86c0db52e 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -406,122 +406,6 @@ static struct attribute_group *vfio_ap_mdev_type_groups[] = {
ÂÂÂÂÂ NULL,
 };
 -struct vfio_ap_queue_reserved {
-ÂÂÂ unsigned long *apid;
-ÂÂÂ unsigned long *apqi;
-ÂÂÂ bool reserved;
-};
-
-/**
- * vfio_ap_has_queue
- *
- * @dev: an AP queue device
- * @data: a struct vfio_ap_queue_reserved reference
- *
- * Flags whether the AP queue device (@dev) has a queue ID containing the APQN,
- * apid or apqi specified in @data:
- *
- * - 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.
- */
-static int vfio_ap_has_queue(struct device *dev, void *data)
-{
-ÂÂÂ struct vfio_ap_queue_reserved *qres = data;
-ÂÂÂ struct ap_queue *ap_queue = to_ap_queue(dev);
-ÂÂÂ ap_qid_t qid;
-ÂÂÂ unsigned long id;
-
-ÂÂÂ 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;
-ÂÂÂ }
-
-ÂÂÂ return 0;
-}
-
-/**
- * vfio_ap_verify_queue_reserved
- *
- * @matrix_dev: a mediated matrix device
- * @apid: an AP adapter ID
- * @apqi: an AP queue index
- *
- * Verifies that the AP queue with @apid/@apqi is reserved by the VFIO AP device
- * driver according to the following rules:
- *
- * - 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.
- */
-static int vfio_ap_verify_queue_reserved(unsigned long *apid,
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long *apqi)
-{
-ÂÂÂ int ret;
-ÂÂÂ struct vfio_ap_queue_reserved qres;
-
-ÂÂÂ 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;
-
-ÂÂÂ if (qres.reserved)
-ÂÂÂÂÂÂÂ return 0;
-
-ÂÂÂ return -EADDRNOTAVAIL;
-}
-
-static int
-vfio_ap_mdev_verify_queues_reserved_for_apid(struct ap_matrix_mdev *matrix_mdev,
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long apid)
-{
-ÂÂÂ int ret;
-ÂÂÂ unsigned long apqi;
-ÂÂÂ unsigned long nbits = matrix_mdev->matrix.aqm_max + 1;
-
-ÂÂÂ if (find_first_bit_inv(matrix_mdev->matrix.aqm, nbits) >= nbits)
-ÂÂÂÂÂÂÂ return vfio_ap_verify_queue_reserved(&apid, NULL);
-
-ÂÂÂ for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm, nbits) {
-ÂÂÂÂÂÂÂ ret = vfio_ap_verify_queue_reserved(&apid, &apqi);
-ÂÂÂÂÂÂÂ if (ret)
-ÂÂÂÂÂÂÂÂÂÂÂ return ret;
-ÂÂÂ }
-
-ÂÂÂ return 0;
-}
-
 /**
ÂÂ * vfio_ap_mdev_verify_no_sharing
ÂÂ *
@@ -529,18 +413,26 @@ vfio_ap_mdev_verify_queues_reserved_for_apid(struct ap_matrix_mdev *matrix_mdev,
ÂÂ * and AP queue indexes comprising the AP matrix are not configured for another
ÂÂ * mediated device. AP queue sharing is not allowed.
ÂÂ *
- * @matrix_mdev: the mediated matrix device
+ * @mdev_apm: the mask identifying the adapters assigned to mdev
+ * @mdev_apm: the mask identifying the adapters assigned to mdev
I assume you wanted to write @mdev_aqm ... queues ... at the 2nd line.

You assumed correctly. I also mean to say "domains assigned to mdev".

ÂÂ *
ÂÂ * Returns 0 if the APQNs are not shared, otherwise; returns -EADDRINUSE.
ÂÂ */
-static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
+static int vfio_ap_mdev_verify_no_sharing(unsigned long *mdev_apm,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long *mdev_aqm)
 {
ÂÂÂÂÂ 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)
+ÂÂÂÂÂÂÂ /*
+ÂÂÂÂÂÂÂÂ * If either of the input masks belongs to the mdev to which an
+ÂÂÂÂÂÂÂÂ * AP resource is being assigned, then we don't need to verify
+ÂÂÂÂÂÂÂÂ * that mdev's masks.
+ÂÂÂÂÂÂÂÂ */
+ÂÂÂÂÂÂÂ if ((mdev_apm == lstdev->matrix.apm) ||
+ÂÂÂÂÂÂÂÂÂÂÂ (mdev_aqm == lstdev->matrix.aqm))
ÂÂÂÂÂÂÂÂÂÂÂÂÂ continue;
 Â memset(apm, 0, sizeof(apm));
@@ -550,12 +442,10 @@ static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
ÂÂÂÂÂÂÂÂÂÂ * 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))
+ÂÂÂÂÂÂÂ if (!bitmap_and(apm, mdev_apm, lstdev->matrix.apm, AP_DEVICES))
ÂÂÂÂÂÂÂÂÂÂÂÂÂ continue;
 - if (!bitmap_and(aqm, matrix_mdev->matrix.aqm,
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ lstdev->matrix.aqm, AP_DOMAINS))
+ÂÂÂÂÂÂÂ if (!bitmap_and(aqm, mdev_aqm, lstdev->matrix.aqm, AP_DOMAINS))
ÂÂÂÂÂÂÂÂÂÂÂÂÂ continue;
 Â return -EADDRINUSE;
@@ -564,6 +454,17 @@ static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
ÂÂÂÂÂ return 0;
 }
 +static int vfio_ap_mdev_validate_masks(unsigned long *apm, unsigned long *aqm)
+{
+ÂÂÂ int ret;
+
+ÂÂÂ ret = ap_apqn_in_matrix_owned_by_def_drv(apm, aqm);
+ÂÂÂ if (ret)
+ÂÂÂÂÂÂÂ return (ret == 1) ? -EADDRNOTAVAIL : ret;
+
+ÂÂÂ return vfio_ap_mdev_verify_no_sharing(apm, aqm);
+}
+
 /**
ÂÂ * assign_adapter_store
ÂÂ *
@@ -602,6 +503,7 @@ static ssize_t assign_adapter_store(struct device *dev,
 {
ÂÂÂÂÂ int ret;
ÂÂÂÂÂ unsigned long apid;
+ÂÂÂ DECLARE_BITMAP(apm, AP_DEVICES);
ÂÂÂÂÂ struct mdev_device *mdev = mdev_from_dev(dev);
ÂÂÂÂÂ struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
 @@ -616,32 +518,19 @@ static ssize_t assign_adapter_store(struct device *dev,
ÂÂÂÂÂ if (apid > matrix_mdev->matrix.apm_max)
ÂÂÂÂÂÂÂÂÂ return -ENODEV;
 - /*
-ÂÂÂÂ * Set the bit in the AP mask (APM) corresponding to the AP adapter
-ÂÂÂÂ * number (APID). The bits in the mask, from most significant to least
-ÂÂÂÂ * significant bit, correspond to APIDs 0-255.
-ÂÂÂÂ */
-ÂÂÂ mutex_lock(&matrix_dev->lock);
-
-ÂÂÂ ret = vfio_ap_mdev_verify_queues_reserved_for_apid(matrix_mdev, apid);
-ÂÂÂ if (ret)
-ÂÂÂÂÂÂÂ goto done;
+ÂÂÂ memset(apm, 0, ARRAY_SIZE(apm) * sizeof(*apm));
What about just memset(apm, 0, sizeof(apm));

apm is a pointer to an array of unsigned long, so sizeof(apm) will
yield the number of bytes in the pointer (8), not the number of bytes in
the array (32); or am I wrong about that?
The
 DECLARE_BITMAP(apm, AP_DEVICES);
with the macro definition in types.h:
 #define DECLARE_BITMAP(name,bits) \
ÂÂÂ unsigned long name[BITS_TO_LONGS(bits)]
gives this expanded code:
 unsigned long apm[BITS_TO_LONGS(AP_DEVICES)];
which is an ordinary unsigned long array.
So sizeof(apm) gives the size of the array in bytes:
 sizeof(apm) = 32
I know this is weird, but it is common practice.

It must be the compiler figures out that apm points to
an array. Although probably not necessary, I will make
the change.


Funnily look at this code:
 int blubber[10];
 int *ptr = blubber;
sizeof(blubber) gives 10*sizeof(int) but
sizeof(ptr) gives the size of the pointer pointing to
the blubber array.


+ÂÂÂ set_bit_inv(apid, apm);
 + mutex_lock(&matrix_dev->lock);
+ÂÂÂ ret = vfio_ap_mdev_validate_masks(apm, matrix_mdev->matrix.aqm);
+ÂÂÂ if (ret) {
+ÂÂÂÂÂÂÂ mutex_unlock(&matrix_dev->lock);
+ÂÂÂÂÂÂÂ return ret;
+ÂÂÂ }
ÂÂÂÂÂ 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);
 - return ret;
+ÂÂÂ return count;
 }
 static DEVICE_ATTR_WO(assign_adapter);
 @@ -690,26 +579,6 @@ static ssize_t unassign_adapter_store(struct device *dev,
 }
 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
ÂÂ *
@@ -748,6 +617,7 @@ static ssize_t assign_domain_store(struct device *dev,
 {
ÂÂÂÂÂ int ret;
ÂÂÂÂÂ unsigned long apqi;
+ÂÂÂ DECLARE_BITMAP(aqm, AP_DEVICES);
AP_DEVICES -> AP_DOMAINS

Copy and paste error, it should be AP_DOMAINS.

ÂÂÂÂÂ struct mdev_device *mdev = mdev_from_dev(dev);
ÂÂÂÂÂ struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
ÂÂÂÂÂ unsigned long max_apqi = matrix_mdev->matrix.aqm_max;
@@ -762,27 +632,19 @@ static ssize_t assign_domain_store(struct device *dev,
ÂÂÂÂÂ if (apqi > max_apqi)
ÂÂÂÂÂÂÂÂÂ return -ENODEV;
 - mutex_lock(&matrix_dev->lock);
-
-ÂÂÂ ret = vfio_ap_mdev_verify_queues_reserved_for_apqi(matrix_mdev, apqi);
-ÂÂÂ if (ret)
-ÂÂÂÂÂÂÂ goto done;
+ÂÂÂ memset(aqm, 0, ARRAY_SIZE(aqm) * sizeof(*aqm));
memset(aqm, 0, sizeof(aqm));

See response above.

+ÂÂÂ set_bit_inv(apqi, aqm);
 + mutex_lock(&matrix_dev->lock);
+ÂÂÂ ret = vfio_ap_mdev_validate_masks(matrix_mdev->matrix.apm, aqm);
+ÂÂÂ if (ret) {
+ÂÂÂÂÂÂÂ mutex_unlock(&matrix_dev->lock);
+ÂÂÂÂÂÂÂ return ret;
+ÂÂÂ }
ÂÂÂÂÂ 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);
 - return ret;
+ÂÂÂ return count;
 }
 static DEVICE_ATTR_WO(assign_domain);
 @@ -868,11 +730,6 @@ static ssize_t assign_control_domain_store(struct device *dev,
ÂÂÂÂÂ if (id > matrix_mdev->matrix.adm_max)
ÂÂÂÂÂÂÂÂÂ return -ENODEV;
 - /* Set the bit in the ADM (bitmask) corresponding to the AP control
-ÂÂÂÂ * domain number (id). The bits in the mask, from most significant to
-ÂÂÂÂ * least significant, correspond to IDs 0 up to the one less than the
-ÂÂÂÂ * number of control domains that can be assigned.
-ÂÂÂÂ */
ÂÂÂÂÂ mutex_lock(&matrix_dev->lock);
ÂÂÂÂÂ set_bit_inv(id, matrix_mdev->matrix.adm);
ÂÂÂÂÂ mutex_unlock(&matrix_dev->lock);