The current implementation does not allow assignment of an AP adapter or
domain to an mdev device if each APQN resulting from the assignment
does not reference an AP queue device that is bound to the vfio_ap device
driver. This patch allows assignment of AP resources to the matrix mdev as
long as the APQNs resulting from the assignment:
1. Are not reserved by the AP BUS for use by the zcrypt device drivers.
2. Are not assigned to another matrix mdev.
The rationale behind this is that the AP architecture does not preclude
assignment of APQNs to an AP configuration profile that are not available
to the system.
Signed-off-by: Tony Krowiak <[email protected]>
Reviewed-by: Halil Pasic <[email protected]>
---
drivers/s390/crypto/vfio_ap_ops.c | 224 +++++++-----------------------
1 file changed, 53 insertions(+), 171 deletions(-)
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index b67b2f0faeea..f2b98f347f9f 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -525,141 +525,48 @@ static struct attribute_group *vfio_ap_mdev_type_groups[] = {
NULL,
};
-struct vfio_ap_queue_reserved {
- unsigned long *apid;
- unsigned long *apqi;
- bool reserved;
-};
+#define MDEV_SHARING_ERR "Userspace may not re-assign queue %02lx.%04lx " \
+ "already assigned to %s"
-/**
- * vfio_ap_has_queue - determines if the AP queue containing the target in @data
- *
- * @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
- *
- * Return: 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)
+static void vfio_ap_mdev_log_sharing_err(struct ap_matrix_mdev *matrix_mdev,
+ unsigned long *apm,
+ unsigned long *aqm)
{
- 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 - verifies that the AP queue containing
- * @apid or @aqpi is reserved
- *
- * @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
- *
- * Return: 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;
- }
+ unsigned long apid, apqi;
+ const struct device *dev = mdev_dev(matrix_mdev->mdev);
+ const char *mdev_name = dev_name(dev);
- return 0;
+ for_each_set_bit_inv(apid, apm, AP_DEVICES)
+ for_each_set_bit_inv(apqi, aqm, AP_DOMAINS)
+ dev_warn(dev, MDEV_SHARING_ERR, apid, apqi, mdev_name);
}
/**
- * vfio_ap_mdev_verify_no_sharing - verifies that the AP matrix is not configured
+ * vfio_ap_mdev_verify_no_sharing - verify APQNs are not shared by matrix mdevs
*
- * @matrix_mdev: the mediated matrix device
+ * @mdev_apm: mask indicating the APIDs of the APQNs to be verified
+ * @mdev_aqm: mask indicating the APQIs of the APQNs to be verified
*
- * Verifies that the APQNs derived from the cross product of the AP adapter IDs
- * and AP queue indexes comprising the AP matrix are not configured for another
+ * Verifies that each APQN derived from the Cartesian product of a bitmap of
+ * AP adapter IDs and AP queue indexes is not configured for any matrix
* mediated device. AP queue sharing is not allowed.
*
- * Return: 0 if the APQNs are not shared; otherwise returns -EADDRINUSE.
+ * Return: 0 if the APQNs are not shared; otherwise return -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;
+ struct ap_matrix_mdev *matrix_mdev;
DECLARE_BITMAP(apm, AP_DEVICES);
DECLARE_BITMAP(aqm, AP_DOMAINS);
- list_for_each_entry(lstdev, &matrix_dev->mdev_list, node) {
- if (matrix_mdev == lstdev)
+ list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
+ /*
+ * If the input apm and aqm belong to the matrix_mdev's matrix,
+ * then move on to the next.
+ */
+ if (mdev_apm == matrix_mdev->matrix.apm &&
+ mdev_aqm == matrix_mdev->matrix.aqm)
continue;
memset(apm, 0, sizeof(apm));
@@ -669,20 +576,32 @@ 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, matrix_mdev->matrix.apm,
+ AP_DEVICES))
continue;
- if (!bitmap_and(aqm, matrix_mdev->matrix.aqm,
- lstdev->matrix.aqm, AP_DOMAINS))
+ if (!bitmap_and(aqm, mdev_aqm, matrix_mdev->matrix.aqm,
+ AP_DOMAINS))
continue;
+ vfio_ap_mdev_log_sharing_err(matrix_mdev, apm, aqm);
+
return -EADDRINUSE;
}
return 0;
}
+static int vfio_ap_mdev_validate_masks(struct ap_matrix_mdev *matrix_mdev)
+{
+ if (ap_apqn_in_matrix_owned_by_def_drv(matrix_mdev->matrix.apm,
+ matrix_mdev->matrix.aqm))
+ return -EADDRNOTAVAIL;
+
+ return vfio_ap_mdev_verify_no_sharing(matrix_mdev->matrix.apm,
+ matrix_mdev->matrix.aqm);
+}
+
static void vfio_ap_mdev_link_adapter(struct ap_matrix_mdev *matrix_mdev,
unsigned long apid)
{
@@ -750,30 +669,19 @@ static ssize_t assign_adapter_store(struct device *dev,
goto done;
}
- /*
- * 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.
- */
- ret = vfio_ap_mdev_verify_queues_reserved_for_apid(matrix_mdev, apid);
- if (ret)
- goto done;
-
set_bit_inv(apid, matrix_mdev->matrix.apm);
memset(apm, 0, sizeof(apm));
set_bit_inv(apid, apm);
- ret = vfio_ap_mdev_verify_no_sharing(matrix_mdev);
- if (ret)
- goto share_err;
+ ret = vfio_ap_mdev_validate_masks(matrix_mdev);
+ if (ret) {
+ clear_bit_inv(apid, matrix_mdev->matrix.apm);
+ goto done;
+ }
vfio_ap_mdev_link_adapter(matrix_mdev, apid);
vfio_ap_mdev_filter_matrix(apm, matrix_mdev->matrix.aqm, matrix_mdev);
ret = count;
- goto done;
-
-share_err:
- clear_bit_inv(apid, matrix_mdev->matrix.apm);
done:
mutex_unlock(&matrix_dev->lock);
@@ -848,26 +756,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;
-}
-
static void vfio_ap_mdev_link_domain(struct ap_matrix_mdev *matrix_mdev,
unsigned long apqi)
{
@@ -934,25 +822,19 @@ static ssize_t assign_domain_store(struct device *dev,
goto done;
}
- ret = vfio_ap_mdev_verify_queues_reserved_for_apqi(matrix_mdev, apqi);
- if (ret)
- goto done;
-
set_bit_inv(apqi, matrix_mdev->matrix.aqm);
memset(aqm, 0, sizeof(aqm));
set_bit_inv(apqi, aqm);
- ret = vfio_ap_mdev_verify_no_sharing(matrix_mdev);
- if (ret)
- goto share_err;
+ ret = vfio_ap_mdev_validate_masks(matrix_mdev);
+ if (ret) {
+ clear_bit_inv(apqi, matrix_mdev->matrix.aqm);
+ goto done;
+ }
vfio_ap_mdev_link_domain(matrix_mdev, apqi);
vfio_ap_mdev_filter_matrix(matrix_mdev->matrix.apm, aqm, matrix_mdev);
ret = count;
- goto done;
-
-share_err:
- clear_bit_inv(apqi, matrix_mdev->matrix.aqm);
done:
mutex_unlock(&matrix_dev->lock);
--
2.31.1
On 2/14/22 19:50, Tony Krowiak wrote:
> /**
> - * vfio_ap_mdev_verify_no_sharing - verifies that the AP matrix is not configured
> + * vfio_ap_mdev_verify_no_sharing - verify APQNs are not shared by matrix mdevs
> *
> - * @matrix_mdev: the mediated matrix device
> + * @mdev_apm: mask indicating the APIDs of the APQNs to be verified
> + * @mdev_aqm: mask indicating the APQIs of the APQNs to be verified
> *
> - * Verifies that the APQNs derived from the cross product of the AP adapter IDs
> - * and AP queue indexes comprising the AP matrix are not configured for another
> + * Verifies that each APQN derived from the Cartesian product of a bitmap of
> + * AP adapter IDs and AP queue indexes is not configured for any matrix
> * mediated device. AP queue sharing is not allowed.
> *
> - * Return: 0 if the APQNs are not shared; otherwise returns -EADDRINUSE.
> + * Return: 0 if the APQNs are not shared; otherwise return -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;
> + struct ap_matrix_mdev *matrix_mdev;
> DECLARE_BITMAP(apm, AP_DEVICES);
> DECLARE_BITMAP(aqm, AP_DOMAINS);
>
> - list_for_each_entry(lstdev, &matrix_dev->mdev_list, node) {
> - if (matrix_mdev == lstdev)
> + list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
> + /*
> + * If the input apm and aqm belong to the matrix_mdev's matrix,
> + * then move on to the next.
> + */
> + if (mdev_apm == matrix_mdev->matrix.apm &&
> + mdev_aqm == matrix_mdev->matrix.aqm)
> continue;
We may have a problem here. This check seems like it exists to stop you from
comparing an mdev's apm/aqm with itself. Obviously comparing an mdev's newly
updated apm/aqm with itself would cause a false positive sharing check, right?
If this is the case, I think the comment should be changed to reflect that.
Aside from the comment, what stops this particular series of if statements from
allowing us to configure a second mdev with the exact same apm/aqm values as an
existing mdev? If we do, then this check's continue will short circuit the rest
of the function thereby allowing that 2nd mdev even though it should be a
sharing violation.
--
-- Jason J. Herne ([email protected])
On 3/3/22 10:39, Jason J. Herne wrote:
> On 2/14/22 19:50, Tony Krowiak wrote:
>> /**
>> - * vfio_ap_mdev_verify_no_sharing - verifies that the AP matrix is
>> not configured
>> + * vfio_ap_mdev_verify_no_sharing - verify APQNs are not shared by
>> matrix mdevs
>> *
>> - * @matrix_mdev: the mediated matrix device
>> + * @mdev_apm: mask indicating the APIDs of the APQNs to be verified
>> + * @mdev_aqm: mask indicating the APQIs of the APQNs to be verified
>> *
>> - * Verifies that the APQNs derived from the cross product of the AP
>> adapter IDs
>> - * and AP queue indexes comprising the AP matrix are not configured
>> for another
>> + * Verifies that each APQN derived from the Cartesian product of a
>> bitmap of
>> + * AP adapter IDs and AP queue indexes is not configured for any matrix
>> * mediated device. AP queue sharing is not allowed.
>> *
>> - * Return: 0 if the APQNs are not shared; otherwise returns
>> -EADDRINUSE.
>> + * Return: 0 if the APQNs are not shared; otherwise return -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;
>> + struct ap_matrix_mdev *matrix_mdev;
>> DECLARE_BITMAP(apm, AP_DEVICES);
>> DECLARE_BITMAP(aqm, AP_DOMAINS);
>> - list_for_each_entry(lstdev, &matrix_dev->mdev_list, node) {
>> - if (matrix_mdev == lstdev)
>> + list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
>> + /*
>> + * If the input apm and aqm belong to the matrix_mdev's matrix,
>> + * then move on to the next.
>> + */
>> + if (mdev_apm == matrix_mdev->matrix.apm &&
>> + mdev_aqm == matrix_mdev->matrix.aqm)
>> continue;
>
> We may have a problem here. This check seems like it exists to stop
> you from
> comparing an mdev's apm/aqm with itself. Obviously comparing an mdev's
> newly
> updated apm/aqm with itself would cause a false positive sharing
> check, right?
> If this is the case, I think the comment should be changed to reflect
> that.
You are correct, this check is performed to prevent comparing an mdev to
itself, I'll improve the comment.
>
> Aside from the comment, what stops this particular series of if
> statements from
> allowing us to configure a second mdev with the exact same apm/aqm
> values as an
> existing mdev? If we do, then this check's continue will short circuit
> the rest
> of the function thereby allowing that 2nd mdev even though it should be a
> sharing violation.
I don't see how this is possible.
The function above is called from two places: the
vfio_ap_mdev_validate_masks()
function which is invoked when an adapter or domain is assigned to the
mdev; and the
vfio_ap_mdev_resource_in_use() function which is a callback registered
with the
AP bus and is called by the bus when the apmask/aqmask are changed.
In the former case, the addresses passed in are from the apm/aqm fields
within
the ap_matrix structure. Each ap_matrix structure is a field contained
within an
ap_matrix_mdev structure, it is not a pointer to storage allocated
external to
the matrix_mdev, so the apm/aqm addresses passed in from the
vfio_ap_mdev_validate_masks() function will be unique to each
ap_matrix_mdev
structure.
In the latter case, the addresses are passed in by the AP bus and are
allocated by the
bus and would definitely not be contained within an ap_matrix_mdev since
the AP bus
doesn't even have access to that structure.
>
>
On 3/7/22 08:27, Halil Pasic wrote:
> On Mon, 7 Mar 2022 07:31:21 -0500
> Tony Krowiak <[email protected]> wrote:
>
>> On 3/3/22 10:39, Jason J. Herne wrote:
>>> On 2/14/22 19:50, Tony Krowiak wrote:
>>>> /**
>>>> - * vfio_ap_mdev_verify_no_sharing - verifies that the AP matrix is
>>>> not configured
>>>> + * vfio_ap_mdev_verify_no_sharing - verify APQNs are not shared by
>>>> matrix mdevs
>>>> *
>>>> - * @matrix_mdev: the mediated matrix device
>>>> + * @mdev_apm: mask indicating the APIDs of the APQNs to be verified
>>>> + * @mdev_aqm: mask indicating the APQIs of the APQNs to be verified
>>>> *
>>>> - * Verifies that the APQNs derived from the cross product of the AP
>>>> adapter IDs
>>>> - * and AP queue indexes comprising the AP matrix are not configured
>>>> for another
>>>> + * Verifies that each APQN derived from the Cartesian product of a
>>>> bitmap of
>>>> + * AP adapter IDs and AP queue indexes is not configured for any matrix
>>>> * mediated device. AP queue sharing is not allowed.
>>>> *
>>>> - * Return: 0 if the APQNs are not shared; otherwise returns
>>>> -EADDRINUSE.
>>>> + * Return: 0 if the APQNs are not shared; otherwise return -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;
>>>> + struct ap_matrix_mdev *matrix_mdev;
>>>> DECLARE_BITMAP(apm, AP_DEVICES);
>>>> DECLARE_BITMAP(aqm, AP_DOMAINS);
>>>> - list_for_each_entry(lstdev, &matrix_dev->mdev_list, node) {
>>>> - if (matrix_mdev == lstdev)
>>>> + list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
>>>> + /*
>>>> + * If the input apm and aqm belong to the matrix_mdev's matrix,
> How about:
>
> s/belong to the matrix_mdev's matrix/are fields of the matrix_mdev
> object/
This is the comment I wrote:
/*
* Comparing an mdev's newly updated apm/aqm with itself would
* result in a false positive when verifying whether any APQNs
* are shared; so, if the input apm and aqm belong to the
* matrix_mdev's matrix, then move on to the next one.
*/
However, I'd be happy to change it to whatever either of you want.
>
>
>>>> + * then move on to the next.
>>>> + */
>>>> + if (mdev_apm == matrix_mdev->matrix.apm &&
>>>> + mdev_aqm == matrix_mdev->matrix.aqm)
>>>> continue;
>>> We may have a problem here. This check seems like it exists to stop
>>> you from
>>> comparing an mdev's apm/aqm with itself. Obviously comparing an mdev's
>>> newly
>>> updated apm/aqm with itself would cause a false positive sharing
>>> check, right?
>>> If this is the case, I think the comment should be changed to reflect
>>> that.
>> You are correct, this check is performed to prevent comparing an mdev to
>> itself, I'll improve the comment.
>>
>>> Aside from the comment, what stops this particular series of if
>>> statements from
>>> allowing us to configure a second mdev with the exact same apm/aqm
>>> values as an
>>> existing mdev? If we do, then this check's continue will short circuit
>>> the rest
>>> of the function thereby allowing that 2nd mdev even though it should be a
>>> sharing violation.
>> I don't see how this is possible.
> I agree with Tony and his explanation.
>
> Furthermore IMHO is relates to the class identity vs equality problem, in
> a sense that identity always implies equality.
>
> Regards,
> Halil
On Mon, 7 Mar 2022 09:10:29 -0500
Tony Krowiak <[email protected]> wrote:
> On 3/7/22 08:27, Halil Pasic wrote:
> > On Mon, 7 Mar 2022 07:31:21 -0500
> > Tony Krowiak <[email protected]> wrote:
> >
> >> On 3/3/22 10:39, Jason J. Herne wrote:
> >>> On 2/14/22 19:50, Tony Krowiak wrote:
> >>>> /**
> >>>> - * vfio_ap_mdev_verify_no_sharing - verifies that the AP matrix is
> >>>> not configured
> >>>> + * vfio_ap_mdev_verify_no_sharing - verify APQNs are not shared by
> >>>> matrix mdevs
> >>>> *
> >>>> - * @matrix_mdev: the mediated matrix device
> >>>> + * @mdev_apm: mask indicating the APIDs of the APQNs to be verified
> >>>> + * @mdev_aqm: mask indicating the APQIs of the APQNs to be verified
> >>>> *
> >>>> - * Verifies that the APQNs derived from the cross product of the AP
> >>>> adapter IDs
> >>>> - * and AP queue indexes comprising the AP matrix are not configured
> >>>> for another
> >>>> + * Verifies that each APQN derived from the Cartesian product of a
> >>>> bitmap of
> >>>> + * AP adapter IDs and AP queue indexes is not configured for any matrix
> >>>> * mediated device. AP queue sharing is not allowed.
> >>>> *
> >>>> - * Return: 0 if the APQNs are not shared; otherwise returns
> >>>> -EADDRINUSE.
> >>>> + * Return: 0 if the APQNs are not shared; otherwise return -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;
> >>>> + struct ap_matrix_mdev *matrix_mdev;
> >>>> DECLARE_BITMAP(apm, AP_DEVICES);
> >>>> DECLARE_BITMAP(aqm, AP_DOMAINS);
> >>>> - list_for_each_entry(lstdev, &matrix_dev->mdev_list, node) {
> >>>> - if (matrix_mdev == lstdev)
> >>>> + list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
> >>>> + /*
> >>>> + * If the input apm and aqm belong to the matrix_mdev's matrix,
> > How about:
> >
> > s/belong to the matrix_mdev's matrix/are fields of the matrix_mdev
> > object/
>
> This is the comment I wrote:
>
> /*
> * Comparing an mdev's newly updated apm/aqm with itself would
> * result in a false positive when verifying whether any APQNs
> * are shared; so, if the input apm and aqm belong to the
> * matrix_mdev's matrix, then move on to the next one.
> */
>
> However, I'd be happy to change it to whatever either of you want.
What ain't obvious for the comment is that "belong to" actually means
composition and not association. In other words, there there is no
pointer/indirection involved, a pointer that would tell us what matrix
does belong to what matrix_mdev, but rather the matrix is just a part
of the matrix_mdev object.
I don't like 'false positive' either, and whether the apm/aqm is
newly updated or not is also redundant and confusing in my opinion. When
we check because of inuse there is not updated whatever. IMHO the old
message was better than this one.
Just my opinion, if you two agree, that this is the way to go, I'm fine
with that.
Regards,
Halil
On 3/7/22 07:31, Tony Krowiak wrote:
>>> + * If the input apm and aqm belong to the matrix_mdev's matrix,
>>> + * then move on to the next.
>>> + */
>>> + if (mdev_apm == matrix_mdev->matrix.apm &&
>>> + mdev_aqm == matrix_mdev->matrix.aqm)
>>> continue;
>>
>> We may have a problem here. This check seems like it exists to stop you from
>> comparing an mdev's apm/aqm with itself. Obviously comparing an mdev's newly
>> updated apm/aqm with itself would cause a false positive sharing check, right?
>> If this is the case, I think the comment should be changed to reflect that.
>
> You are correct, this check is performed to prevent comparing an mdev to
> itself, I'll improve the comment.
>
>>
>> Aside from the comment, what stops this particular series of if statements from
>> allowing us to configure a second mdev with the exact same apm/aqm values as an
>> existing mdev? If we do, then this check's continue will short circuit the rest
>> of the function thereby allowing that 2nd mdev even though it should be a
>> sharing violation.
>
> I don't see how this is possible.
You are correct. I missed the fact that you are comparing pointers here, and not
values. Apologies. Now that I understand the code, I agree that this is fine as is.
--
-- Jason J. Herne ([email protected])
On 3/8/22 05:06, Halil Pasic wrote:
> On Mon, 7 Mar 2022 18:45:45 -0500
> Tony Krowiak <[email protected]> wrote:
>
> [..]
>>>>> s/belong to the matrix_mdev's matrix/are fields of the matrix_mdev
>>>>> object/
>>>> This is the comment I wrote:
>>>>
>>>> /*
>>>> * Comparing an mdev's newly updated apm/aqm with itself would
>>>> * result in a false positive when verifying whether any APQNs
>>>> * are shared; so, if the input apm and aqm belong to the
>>>> * matrix_mdev's matrix, then move on to the next one.
>>>> */
>>>>
>>>> However, I'd be happy to change it to whatever either of you want.
>>> What ain't obvious for the comment is that "belong to" actually means
>>> composition and not association. In other words, there there is no
>>> pointer/indirection involved, a pointer that would tell us what matrix
>>> does belong to what matrix_mdev, but rather the matrix is just a part
>>> of the matrix_mdev object.
>>>
>>> I don't like 'false positive' either, and whether the apm/aqm is
>>> newly updated or not is also redundant and confusing in my opinion. When
>>> we check because of inuse there is not updated whatever. IMHO the old
>>> message was better than this one.
>>>
>>> Just my opinion, if you two agree, that this is the way to go, I'm fine
>>> with that.
>>>
>>> Regards,
>>> Halil
>> Feel free to recommend the verbiage for this comment. I'm not married
>> to my comments and am open to anything that helps others to
>> understand what is going on here. It seems obvious to me, but I wrote
>> the code. Obviously, it is not so obvious based on Jason's comments,
>> so maybe someone else can compose a better comment.
> /*
> * If the input apm and aqm are fields of the matrix_mdev object,
> * then move on to the next matrix_mdev.
> */
Perfect, you write better English than me!
>
> Regards,
> Halil
On Mon, 7 Mar 2022 18:45:45 -0500
Tony Krowiak <[email protected]> wrote:
[..]
> >>> s/belong to the matrix_mdev's matrix/are fields of the matrix_mdev
> >>> object/
> >> This is the comment I wrote:
> >>
> >> /*
> >> * Comparing an mdev's newly updated apm/aqm with itself would
> >> * result in a false positive when verifying whether any APQNs
> >> * are shared; so, if the input apm and aqm belong to the
> >> * matrix_mdev's matrix, then move on to the next one.
> >> */
> >>
> >> However, I'd be happy to change it to whatever either of you want.
> > What ain't obvious for the comment is that "belong to" actually means
> > composition and not association. In other words, there there is no
> > pointer/indirection involved, a pointer that would tell us what matrix
> > does belong to what matrix_mdev, but rather the matrix is just a part
> > of the matrix_mdev object.
> >
> > I don't like 'false positive' either, and whether the apm/aqm is
> > newly updated or not is also redundant and confusing in my opinion. When
> > we check because of inuse there is not updated whatever. IMHO the old
> > message was better than this one.
> >
> > Just my opinion, if you two agree, that this is the way to go, I'm fine
> > with that.
> >
> > Regards,
> > Halil
>
> Feel free to recommend the verbiage for this comment. I'm not married
> to my comments and am open to anything that helps others to
> understand what is going on here. It seems obvious to me, but I wrote
> the code. Obviously, it is not so obvious based on Jason's comments,
> so maybe someone else can compose a better comment.
/*
* If the input apm and aqm are fields of the matrix_mdev object,
* then move on to the next matrix_mdev.
*/
Regards,
Halil
On 3/7/22 12:10, Halil Pasic wrote:
> On Mon, 7 Mar 2022 09:10:29 -0500
> Tony Krowiak <[email protected]> wrote:
>
>> On 3/7/22 08:27, Halil Pasic wrote:
>>> On Mon, 7 Mar 2022 07:31:21 -0500
>>> Tony Krowiak <[email protected]> wrote:
>>>
>>>> On 3/3/22 10:39, Jason J. Herne wrote:
>>>>> On 2/14/22 19:50, Tony Krowiak wrote:
>>>>>> /**
>>>>>> - * vfio_ap_mdev_verify_no_sharing - verifies that the AP matrix is
>>>>>> not configured
>>>>>> + * vfio_ap_mdev_verify_no_sharing - verify APQNs are not shared by
>>>>>> matrix mdevs
>>>>>> *
>>>>>> - * @matrix_mdev: the mediated matrix device
>>>>>> + * @mdev_apm: mask indicating the APIDs of the APQNs to be verified
>>>>>> + * @mdev_aqm: mask indicating the APQIs of the APQNs to be verified
>>>>>> *
>>>>>> - * Verifies that the APQNs derived from the cross product of the AP
>>>>>> adapter IDs
>>>>>> - * and AP queue indexes comprising the AP matrix are not configured
>>>>>> for another
>>>>>> + * Verifies that each APQN derived from the Cartesian product of a
>>>>>> bitmap of
>>>>>> + * AP adapter IDs and AP queue indexes is not configured for any matrix
>>>>>> * mediated device. AP queue sharing is not allowed.
>>>>>> *
>>>>>> - * Return: 0 if the APQNs are not shared; otherwise returns
>>>>>> -EADDRINUSE.
>>>>>> + * Return: 0 if the APQNs are not shared; otherwise return -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;
>>>>>> + struct ap_matrix_mdev *matrix_mdev;
>>>>>> DECLARE_BITMAP(apm, AP_DEVICES);
>>>>>> DECLARE_BITMAP(aqm, AP_DOMAINS);
>>>>>> - list_for_each_entry(lstdev, &matrix_dev->mdev_list, node) {
>>>>>> - if (matrix_mdev == lstdev)
>>>>>> + list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
>>>>>> + /*
>>>>>> + * If the input apm and aqm belong to the matrix_mdev's matrix,
>>> How about:
>>>
>>> s/belong to the matrix_mdev's matrix/are fields of the matrix_mdev
>>> object/
>> This is the comment I wrote:
>>
>> /*
>> * Comparing an mdev's newly updated apm/aqm with itself would
>> * result in a false positive when verifying whether any APQNs
>> * are shared; so, if the input apm and aqm belong to the
>> * matrix_mdev's matrix, then move on to the next one.
>> */
>>
>> However, I'd be happy to change it to whatever either of you want.
> What ain't obvious for the comment is that "belong to" actually means
> composition and not association. In other words, there there is no
> pointer/indirection involved, a pointer that would tell us what matrix
> does belong to what matrix_mdev, but rather the matrix is just a part
> of the matrix_mdev object.
>
> I don't like 'false positive' either, and whether the apm/aqm is
> newly updated or not is also redundant and confusing in my opinion. When
> we check because of inuse there is not updated whatever. IMHO the old
> message was better than this one.
>
> Just my opinion, if you two agree, that this is the way to go, I'm fine
> with that.
>
> Regards,
> Halil
Feel free to recommend the verbiage for this comment. I'm not married
to my comments and am open to anything that helps others to
understand what is going on here. It seems obvious to me, but I wrote
the code. Obviously, it is not so obvious based on Jason's comments,
so maybe someone else can compose a better comment.
>
>
On Mon, 7 Mar 2022 07:31:21 -0500
Tony Krowiak <[email protected]> wrote:
> On 3/3/22 10:39, Jason J. Herne wrote:
> > On 2/14/22 19:50, Tony Krowiak wrote:
> >> /**
> >> - * vfio_ap_mdev_verify_no_sharing - verifies that the AP matrix is
> >> not configured
> >> + * vfio_ap_mdev_verify_no_sharing - verify APQNs are not shared by
> >> matrix mdevs
> >> *
> >> - * @matrix_mdev: the mediated matrix device
> >> + * @mdev_apm: mask indicating the APIDs of the APQNs to be verified
> >> + * @mdev_aqm: mask indicating the APQIs of the APQNs to be verified
> >> *
> >> - * Verifies that the APQNs derived from the cross product of the AP
> >> adapter IDs
> >> - * and AP queue indexes comprising the AP matrix are not configured
> >> for another
> >> + * Verifies that each APQN derived from the Cartesian product of a
> >> bitmap of
> >> + * AP adapter IDs and AP queue indexes is not configured for any matrix
> >> * mediated device. AP queue sharing is not allowed.
> >> *
> >> - * Return: 0 if the APQNs are not shared; otherwise returns
> >> -EADDRINUSE.
> >> + * Return: 0 if the APQNs are not shared; otherwise return -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;
> >> + struct ap_matrix_mdev *matrix_mdev;
> >> DECLARE_BITMAP(apm, AP_DEVICES);
> >> DECLARE_BITMAP(aqm, AP_DOMAINS);
> >> - list_for_each_entry(lstdev, &matrix_dev->mdev_list, node) {
> >> - if (matrix_mdev == lstdev)
> >> + list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
> >> + /*
> >> + * If the input apm and aqm belong to the matrix_mdev's matrix,
How about:
s/belong to the matrix_mdev's matrix/are fields of the matrix_mdev
object/
> >> + * then move on to the next.
> >> + */
> >> + if (mdev_apm == matrix_mdev->matrix.apm &&
> >> + mdev_aqm == matrix_mdev->matrix.aqm)
> >> continue;
> >
> > We may have a problem here. This check seems like it exists to stop
> > you from
> > comparing an mdev's apm/aqm with itself. Obviously comparing an mdev's
> > newly
> > updated apm/aqm with itself would cause a false positive sharing
> > check, right?
> > If this is the case, I think the comment should be changed to reflect
> > that.
>
> You are correct, this check is performed to prevent comparing an mdev to
> itself, I'll improve the comment.
>
> >
> > Aside from the comment, what stops this particular series of if
> > statements from
> > allowing us to configure a second mdev with the exact same apm/aqm
> > values as an
> > existing mdev? If we do, then this check's continue will short circuit
> > the rest
> > of the function thereby allowing that 2nd mdev even though it should be a
> > sharing violation.
>
> I don't see how this is possible.
I agree with Tony and his explanation.
Furthermore IMHO is relates to the class identity vs equality problem, in
a sense that identity always implies equality.
Regards,
Halil
On Tue, 8 Mar 2022 10:39:09 -0500
"Jason J. Herne" <[email protected]> wrote:
> On 3/7/22 07:31, Tony Krowiak wrote:
> >>> + * If the input apm and aqm belong to the matrix_mdev's matrix,
> >>> + * then move on to the next.
> >>> + */
> >>> + if (mdev_apm == matrix_mdev->matrix.apm &&
> >>> + mdev_aqm == matrix_mdev->matrix.aqm)
> >>> continue;
> >>
> >> We may have a problem here. This check seems like it exists to stop you from
> >> comparing an mdev's apm/aqm with itself. Obviously comparing an mdev's newly
> >> updated apm/aqm with itself would cause a false positive sharing check, right?
> >> If this is the case, I think the comment should be changed to reflect that.
> >
> > You are correct, this check is performed to prevent comparing an mdev to
> > itself, I'll improve the comment.
> >
> >>
> >> Aside from the comment, what stops this particular series of if statements from
> >> allowing us to configure a second mdev with the exact same apm/aqm values as an
> >> existing mdev? If we do, then this check's continue will short circuit the rest
> >> of the function thereby allowing that 2nd mdev even though it should be a
> >> sharing violation.
> >
> > I don't see how this is possible.
>
> You are correct. I missed the fact that you are comparing pointers here, and not
> values. Apologies. Now that I understand the code, I agree that this is fine as is.
>
I believe clarifying the 'belongs to' vs 'is a part of' stuff is still
worthwhile, because 'belongs to' does beg the question you asked. Thus
IMHO it is good that you raised the question.
Regards,
Halil