[PATCH v2 0/5] s390/vfio-ap: queue_configuration sysfs attribute for mdevctl automation

[PATCH v2 0/5] s390/vfio-ap: queue_configuration sysfs attribute for mdevctl automation

[Jason J. Herne]

Mdevctl requires a way to atomically query and atomically update a vfio-ap
mdev's current state. This patch set creates the queue_configuration sysfs
attribute.  This new attribute allows reading and writing an mdev's entire
state in one go. If a newly written state is invalid for any reason the entire
state is rejected and the target mdev remains unchanged.

Changelog
==========
v2
  - Rebased patched on top of latest master
  - Reworked code to fit changes introduced by f848cba767e59
      s390/vfio-ap: reset queues filtered from the guest's AP config
  - Moved docs changes to separate patch

Jason J. Herne (5):
  s390/ap: Externalize AP bus specific bitmap reading function
  s390/vfio-ap: Add sysfs attr, queue_configuration, to export mdev
    state
  s390/vfio-ap: Ignore duplicate link requests in
    vfio_ap_mdev_link_queue
  s390/vfio-ap: Add write support to sysfs attr ap_config
  s390: doc: Update doc

 Documentation/arch/s390/vfio-ap.rst   |  27 ++++
 drivers/s390/crypto/ap_bus.c          |  13 +-
 drivers/s390/crypto/ap_bus.h          |  22 +++
 drivers/s390/crypto/vfio_ap_ops.c     | 206 ++++++++++++++++++++++++--
 drivers/s390/crypto/vfio_ap_private.h |   6 +-
 5 files changed, 245 insertions(+), 29 deletions(-)

-- 
2.41.0

[PATCH v2 1/5] s390/ap: Externalize AP bus specific bitmap reading function

[Jason J. Herne]

Rename hex2bitmap() to ap_hex2bitmap() and export it for external
use. This function will be used by the implementation of the vfio-ap
queue_configuration sysfs attribute.

Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
Reviewed-by: Tony Krowiak <akrowiak@linux.ibm.com>
Reviewed-by: Harald Freudenberger <freude@linux.ibm.com>
---
 drivers/s390/crypto/ap_bus.c | 13 +++----------
 drivers/s390/crypto/ap_bus.h | 22 ++++++++++++++++++++++
 2 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
index f46dd6abacd7..5d8e379eff44 100644
--- a/drivers/s390/crypto/ap_bus.c
+++ b/drivers/s390/crypto/ap_bus.c
@@ -1035,15 +1035,7 @@ void ap_bus_cfg_chg(void)
 	ap_bus_force_rescan();
 }
 
-/*
- * hex2bitmap() - parse hex mask string and set bitmap.
- * Valid strings are "0x012345678" with at least one valid hex number.
- * Rest of the bitmap to the right is padded with 0. No spaces allowed
- * within the string, the leading 0x may be omitted.
- * Returns the bitmask with exactly the bits set as given by the hex
- * string (both in big endian order).
- */
-static int hex2bitmap(const char *str, unsigned long *bitmap, int bits)
+int ap_hex2bitmap(const char *str, unsigned long *bitmap, int bits)
 {
 	int i, n, b;
 
@@ -1070,6 +1062,7 @@ static int hex2bitmap(const char *str, unsigned long *bitmap, int bits)
 		return -EINVAL;
 	return 0;
 }
+EXPORT_SYMBOL(ap_hex2bitmap);
 
 /*
  * modify_bitmap() - parse bitmask argument and modify an existing
@@ -1135,7 +1128,7 @@ static int ap_parse_bitmap_str(const char *str, unsigned long *bitmap, int bits,
 		rc = modify_bitmap(str, newmap, bits);
 	} else {
 		memset(newmap, 0, size);
-		rc = hex2bitmap(str, newmap, bits);
+		rc = ap_hex2bitmap(str, newmap, bits);
 	}
 	return rc;
 }
diff --git a/drivers/s390/crypto/ap_bus.h b/drivers/s390/crypto/ap_bus.h
index 98814839ef30..954f8d36c2d9 100644
--- a/drivers/s390/crypto/ap_bus.h
+++ b/drivers/s390/crypto/ap_bus.h
@@ -343,6 +343,28 @@ int ap_parse_mask_str(const char *str,
 		      unsigned long *bitmap, int bits,
 		      struct mutex *lock);
 
+/*
+ * ap_hex2bitmap() - Convert a string containing a hexadecimal number (str)
+ * into a bitmap (bitmap) with bits set that correspond to the bits represented
+ * by the hex string. Input and output data is in big endian order.
+ *
+ * str - Input hex string of format "0x1234abcd". The leading "0x" is optional.
+ * At least one digit is required. Must be large enough to hold the number of
+ * bits represented by the bits parameter.
+ *
+ * bitmap - Pointer to a bitmap. Upon successful completion of this function,
+ * this bitmap will have bits set to match the value of str. If bitmap is longer
+ * than str, then the rightmost bits of bitmap are padded with zeros. Must be
+ * large enough to hold the number of bits represented by the bits parameter.
+ *
+ * bits - Length, in bits, of the bitmap represented by str. Must be a multiple
+ * of 8.
+ *
+ * Returns: 0           on success
+ *          -EINVAL     If str format is invalid or bits is not a multiple of 8.
+ */
+int ap_hex2bitmap(const char *str, unsigned long *bitmap, int bits);
+
 /*
  * Interface to wait for the AP bus to have done one initial ap bus
  * scan and all detected APQNs have been bound to device drivers.
-- 
2.41.0

Re: [PATCH v2 1/5] s390/ap: Externalize AP bus specific bitmap reading function

[Matthew Rosato]

On 3/6/24 9:08 AM, Jason J. Herne wrote:
> Rename hex2bitmap() to ap_hex2bitmap() and export it for external
> use. This function will be used by the implementation of the vfio-ap
> queue_configuration sysfs attribute.

Here as well.

s/queue_configuration/ap_config/

[PATCH v2 2/5] s390/vfio-ap: Add sysfs attr, queue_configuration, to export mdev state

[Jason J. Herne]

Add ap_config sysfs attribute. This will provide the means for
setting or displaying the adapters, domains and control domains assigned
to the vfio-ap mediated device in a single operation. This sysfs
attribute is comprised of three masks: One for adapters, one for domains,
and one for control domains.

This attribute is intended to be used by mdevctl to query a vfio-ap
mediated device's state.

Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
Reviewed-by: Tony Krowiak <akrowiak@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 983b3b16196c..b1c1dc0233e1 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -1557,6 +1557,32 @@ static ssize_t guest_matrix_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(guest_matrix);
 
+static ssize_t write_ap_bitmap(unsigned long *bitmap, char *buf, int offset, char sep)
+{
+	return sysfs_emit_at(buf, offset, "0x%016lx%016lx%016lx%016lx%c",
+			 bitmap[0], bitmap[1], bitmap[2], bitmap[3], sep);
+}
+
+static ssize_t ap_config_show(struct device *dev, struct device_attribute *attr,
+			      char *buf)
+{
+	struct ap_matrix_mdev *matrix_mdev = dev_get_drvdata(dev);
+	int idx = 0;
+
+	idx += write_ap_bitmap(matrix_mdev->matrix.apm, buf, idx, ',');
+	idx += write_ap_bitmap(matrix_mdev->matrix.aqm, buf, idx, ',');
+	idx += write_ap_bitmap(matrix_mdev->matrix.adm, buf, idx, '\n');
+
+	return idx;
+}
+
+static ssize_t ap_config_store(struct device *dev, struct device_attribute *attr,
+			       const char *buf, size_t count)
+{
+	return count;
+}
+static DEVICE_ATTR_RW(ap_config);
+
 static struct attribute *vfio_ap_mdev_attrs[] = {
 	&dev_attr_assign_adapter.attr,
 	&dev_attr_unassign_adapter.attr,
@@ -1564,6 +1590,7 @@ static struct attribute *vfio_ap_mdev_attrs[] = {
 	&dev_attr_unassign_domain.attr,
 	&dev_attr_assign_control_domain.attr,
 	&dev_attr_unassign_control_domain.attr,
+	&dev_attr_ap_config.attr,
 	&dev_attr_control_domains.attr,
 	&dev_attr_matrix.attr,
 	&dev_attr_guest_matrix.attr,
-- 
2.41.0

Re: [PATCH v2 2/5] s390/vfio-ap: Add sysfs attr, queue_configuration, to export mdev state

[Matthew Rosato]

On 3/6/24 9:08 AM, Jason J. Herne wrote:
> Add ap_config sysfs attribute. This will provide the means for

Title of patch is wrong due to attribute being renamed.

s/queue_configuration/ap_config/

[PATCH v2 3/5] s390/vfio-ap: Ignore duplicate link requests in vfio_ap_mdev_link_queue

[Jason J. Herne]

vfio_ap_mdev_link_queue is changed to detect if a matrix_mdev has
already linked the given queue. If so, it bails out.

Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
Reviewed-by: Tony Krowiak <akrowiak@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index b1c1dc0233e1..259130347d00 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -781,10 +781,11 @@ static int vfio_ap_mdev_probe(struct mdev_device *mdev)
 static void vfio_ap_mdev_link_queue(struct ap_matrix_mdev *matrix_mdev,
 				    struct vfio_ap_queue *q)
 {
-	if (q) {
-		q->matrix_mdev = matrix_mdev;
-		hash_add(matrix_mdev->qtable.queues, &q->mdev_qnode, q->apqn);
-	}
+	if (!q || vfio_ap_mdev_get_queue(matrix_mdev, q->apqn))
+		return;
+
+	q->matrix_mdev = matrix_mdev;
+	hash_add(matrix_mdev->qtable.queues, &q->mdev_qnode, q->apqn);
 }
 
 static void vfio_ap_mdev_link_apqn(struct ap_matrix_mdev *matrix_mdev, int apqn)
-- 
2.41.0

Re: [PATCH v2 3/5] s390/vfio-ap: Ignore duplicate link requests in vfio_ap_mdev_link_queue

[Anthony Krowiak]

While I don't necessarily object to this change, it is not necessary 
because this function is only called in situations where the link will 
not have been made:

* When an adapter or domain is assigned to the vfio_ap mdev, in which 
case no queue with the APID of the adaper or the APQI of the domain will 
have been linked.

* When a queue device is probed, in which case the vfio_ap_queue object 
is created and linked if the its APQN is assigned to the vfio_ap mdev.

In any case, it certainly doesn't hurt and if a future change is made 
such that this could come into play, the code is already there. So I'll 
leave it up to you if you want to keep this; if so, you already have my r-b.

On 3/6/24 9:08 AM, Jason J. Herne wrote:
> vfio_ap_mdev_link_queue is changed to detect if a matrix_mdev has
> already linked the given queue. If so, it bails out.
>
> Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
> Reviewed-by: Tony Krowiak <akrowiak@linux.ibm.com>
> ---
>   drivers/s390/crypto/vfio_ap_ops.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index b1c1dc0233e1..259130347d00 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -781,10 +781,11 @@ static int vfio_ap_mdev_probe(struct mdev_device *mdev)
>   static void vfio_ap_mdev_link_queue(struct ap_matrix_mdev *matrix_mdev,
>   				    struct vfio_ap_queue *q)
>   {
> -	if (q) {
> -		q->matrix_mdev = matrix_mdev;
> -		hash_add(matrix_mdev->qtable.queues, &q->mdev_qnode, q->apqn);
> -	}
> +	if (!q || vfio_ap_mdev_get_queue(matrix_mdev, q->apqn))
> +		return;
> +
> +	q->matrix_mdev = matrix_mdev;
> +	hash_add(matrix_mdev->qtable.queues, &q->mdev_qnode, q->apqn);
>   }
>   
>   static void vfio_ap_mdev_link_apqn(struct ap_matrix_mdev *matrix_mdev, int apqn)

[PATCH v2 4/5] s390/vfio-ap: Add write support to sysfs attr ap_config

[Jason J. Herne]

Allow writing a complete set of masks to ap_config. Doing so will
cause the vfio-ap driver to replace the vfio-ap mediated device's entire
state with the given set of masks. If the given state cannot be set, then
no changes are made to the vfio-ap mediated device.

The format of the data written to ap_config is as follows:
{amask},{dmask},{cmask}\n

\n is a newline character.

amask, dmask, and cmask are masks identifying which adapters, domains,
and control domains should be assigned to the mediated device.

The format of a mask is as follows:
0xNN..NN

Where NN..NN is 64 hexadecimal characters representing a 256-bit value.
The leftmost (highest order) bit represents adapter/domain 0.

For an example set of masks that represent your mdev's current
configuration, simply cat ap_config.

This attribute is intended to be used by an mdevctl callout script
supporting the mdev type vfio_ap-passthrough to atomically update a
vfio-ap mediated device's state.

Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c     | 172 ++++++++++++++++++++++++--
 drivers/s390/crypto/vfio_ap_private.h |   6 +-
 2 files changed, 162 insertions(+), 16 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 259130347d00..c382e46f620f 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -1106,19 +1106,22 @@ static void vfio_ap_mdev_unlink_adapter(struct ap_matrix_mdev *matrix_mdev,
 	}
 }
 
-static void vfio_ap_mdev_hot_unplug_adapter(struct ap_matrix_mdev *matrix_mdev,
-					    unsigned long apid)
+static void vfio_ap_mdev_hot_unplug_adapters(struct ap_matrix_mdev *matrix_mdev,
+					    unsigned long *apids)
 {
 	struct vfio_ap_queue *q, *tmpq;
 	struct list_head qlist;
+	unsigned long apid;
 
 	INIT_LIST_HEAD(&qlist);
-	vfio_ap_mdev_unlink_adapter(matrix_mdev, apid, &qlist);
 
-	if (test_bit_inv(apid, matrix_mdev->shadow_apcb.apm)) {
-		clear_bit_inv(apid, matrix_mdev->shadow_apcb.apm);
-		vfio_ap_mdev_update_guest_apcb(matrix_mdev);
+	for_each_set_bit_inv(apid, apids, AP_DEVICES) {
+		vfio_ap_mdev_unlink_adapter(matrix_mdev, apid, &qlist);
+
+		if (test_bit_inv(apid, matrix_mdev->shadow_apcb.apm))
+			clear_bit_inv(apid, matrix_mdev->shadow_apcb.apm);
 	}
+	vfio_ap_mdev_update_guest_apcb(matrix_mdev);
 
 	vfio_ap_mdev_reset_qlist(&qlist);
 
@@ -1128,6 +1131,15 @@ static void vfio_ap_mdev_hot_unplug_adapter(struct ap_matrix_mdev *matrix_mdev,
 	}
 }
 
+static void vfio_ap_mdev_hot_unplug_adapter(struct ap_matrix_mdev *matrix_mdev,
+					    unsigned long apid)
+{
+	DECLARE_BITMAP(apids, AP_DEVICES);
+
+	set_bit_inv(apid, apids);
+	vfio_ap_mdev_hot_unplug_adapters(matrix_mdev, apids);
+}
+
 /**
  * unassign_adapter_store - parses the APID from @buf and clears the
  * corresponding bit in the mediated matrix device's APM
@@ -1288,19 +1300,22 @@ static void vfio_ap_mdev_unlink_domain(struct ap_matrix_mdev *matrix_mdev,
 	}
 }
 
-static void vfio_ap_mdev_hot_unplug_domain(struct ap_matrix_mdev *matrix_mdev,
-					   unsigned long apqi)
+static void vfio_ap_mdev_hot_unplug_domains(struct ap_matrix_mdev *matrix_mdev,
+					   unsigned long *apqis)
 {
 	struct vfio_ap_queue *q, *tmpq;
 	struct list_head qlist;
+	unsigned long apqi;
 
 	INIT_LIST_HEAD(&qlist);
-	vfio_ap_mdev_unlink_domain(matrix_mdev, apqi, &qlist);
 
-	if (test_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm)) {
-		clear_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm);
-		vfio_ap_mdev_update_guest_apcb(matrix_mdev);
+	for_each_set_bit_inv(apqi, apqis, AP_DOMAINS) {
+		vfio_ap_mdev_unlink_domain(matrix_mdev, apqi, &qlist);
+
+		if (test_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm))
+			clear_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm);
 	}
+	vfio_ap_mdev_update_guest_apcb(matrix_mdev);
 
 	vfio_ap_mdev_reset_qlist(&qlist);
 
@@ -1310,6 +1325,15 @@ static void vfio_ap_mdev_hot_unplug_domain(struct ap_matrix_mdev *matrix_mdev,
 	}
 }
 
+static void vfio_ap_mdev_hot_unplug_domain(struct ap_matrix_mdev *matrix_mdev,
+					   unsigned long apqi)
+{
+	DECLARE_BITMAP(apqis, AP_DOMAINS);
+
+	set_bit_inv(apqi, apqis);
+	vfio_ap_mdev_hot_unplug_domains(matrix_mdev, apqis);
+}
+
 /**
  * unassign_domain_store - parses the APQI from @buf and clears the
  * corresponding bit in the mediated matrix device's AQM
@@ -1577,10 +1601,132 @@ static ssize_t ap_config_show(struct device *dev, struct device_attribute *attr,
 	return idx;
 }
 
+/* Number of characters needed for a complete hex mask representing the bits in ..  */
+#define AP_DEVICES_STRLEN	(AP_DEVICES/4 + 3)
+#define AP_DOMAINS_STRLEN	(AP_DOMAINS/4 + 3)
+#define AP_CONFIG_STRLEN	(AP_DEVICES_STRLEN + 2 * AP_DOMAINS_STRLEN)
+
+static int parse_bitmap(char **strbufptr, unsigned long *bitmap, int nbits)
+{
+	char *curmask;
+
+	curmask = strsep(strbufptr, ",\n");
+	if (!curmask)
+		return -EINVAL;
+
+	bitmap_clear(bitmap, 0, nbits);
+	return ap_hex2bitmap(curmask, bitmap, nbits);
+}
+
+static int ap_matrix_length_check(struct ap_matrix_mdev *matrix_mdev)
+{
+	unsigned long bit;
+
+	for_each_set_bit_inv(bit, matrix_mdev->matrix.apm, AP_DEVICES) {
+		if (bit > matrix_mdev->matrix.apm_max)
+			return -ENODEV;
+	}
+
+	for_each_set_bit_inv(bit, matrix_mdev->matrix.aqm, AP_DOMAINS) {
+		if (bit > matrix_mdev->matrix.aqm_max)
+			return -ENODEV;
+	}
+
+	for_each_set_bit_inv(bit, matrix_mdev->matrix.adm, AP_DOMAINS) {
+		if (bit > matrix_mdev->matrix.adm_max)
+			return -ENODEV;
+	}
+
+	return 0;
+}
+
+static void ap_matrix_copy(struct ap_matrix *dst, struct ap_matrix *src)
+{
+	bitmap_copy(dst->apm, src->apm, AP_DEVICES);
+	bitmap_copy(dst->aqm, src->aqm, AP_DOMAINS);
+	bitmap_copy(dst->adm, src->adm, AP_DOMAINS);
+}
+
 static ssize_t ap_config_store(struct device *dev, struct device_attribute *attr,
 			       const char *buf, size_t count)
 {
-	return count;
+	struct ap_matrix_mdev *matrix_mdev = dev_get_drvdata(dev);
+	struct ap_matrix m_new, m_old, m_added, m_removed;
+	DECLARE_BITMAP(apm_filtered, AP_DEVICES);
+	unsigned long newbit;
+	char *newbuf, *rest;
+	int rc = count;
+	bool do_update;
+
+	newbuf = rest = kstrndup(buf, AP_CONFIG_STRLEN, GFP_KERNEL);
+	if (!newbuf)
+		return -ENOMEM;
+
+	mutex_lock(&ap_perms_mutex);
+	get_update_locks_for_mdev(matrix_mdev);
+
+	/* Save old state */
+	ap_matrix_copy(&m_old, &matrix_mdev->matrix);
+
+	if (parse_bitmap(&rest, m_new.apm, AP_DEVICES) ||
+	    parse_bitmap(&rest, m_new.aqm, AP_DOMAINS) ||
+	    parse_bitmap(&rest, m_new.adm, AP_DOMAINS)) {
+		rc = -EINVAL;
+		goto out;
+	}
+
+	bitmap_andnot(m_removed.apm, m_old.apm, m_new.apm, AP_DEVICES);
+	bitmap_andnot(m_removed.aqm, m_old.aqm, m_new.aqm, AP_DOMAINS);
+	bitmap_andnot(m_added.apm, m_new.apm, m_old.apm, AP_DEVICES);
+	bitmap_andnot(m_added.aqm, m_new.aqm, m_old.aqm, AP_DOMAINS);
+
+	/* Need new bitmaps in matrix_mdev for validation */
+	ap_matrix_copy(&matrix_mdev->matrix, &m_new);
+
+	/* Ensure new state is valid, else undo new state */
+	rc = vfio_ap_mdev_validate_masks(matrix_mdev);
+	if (rc) {
+		ap_matrix_copy(&matrix_mdev->matrix, &m_old);
+		goto out;
+	}
+	rc = ap_matrix_length_check(matrix_mdev);
+	if (rc) {
+		ap_matrix_copy(&matrix_mdev->matrix, &m_old);
+		goto out;
+	}
+	rc = count;
+
+	/* Need old bitmaps in matrix_mdev for unplug/unlink */
+	ap_matrix_copy(&matrix_mdev->matrix, &m_old);
+
+	/* Unlink removed adapters/domains */
+	vfio_ap_mdev_hot_unplug_adapters(matrix_mdev, m_removed.apm);
+	vfio_ap_mdev_hot_unplug_domains(matrix_mdev, m_removed.aqm);
+
+	/* Need new bitmaps in matrix_mdev for linking new adapters/domains */
+	ap_matrix_copy(&matrix_mdev->matrix, &m_new);
+
+	/* Link newly added adapters */
+	for_each_set_bit_inv(newbit, m_added.apm, AP_DEVICES)
+		vfio_ap_mdev_link_adapter(matrix_mdev, newbit);
+
+	for_each_set_bit_inv(newbit, m_added.aqm, AP_DOMAINS)
+		vfio_ap_mdev_link_domain(matrix_mdev, newbit);
+
+	/* filter resources not bound to vfio-ap */
+	do_update = vfio_ap_mdev_filter_matrix(matrix_mdev, apm_filtered);
+	do_update |= vfio_ap_mdev_filter_cdoms(matrix_mdev);
+
+	/* Apply changes to shadow apbc if things changed */
+	if (do_update) {
+		vfio_ap_mdev_update_guest_apcb(matrix_mdev);
+		reset_queues_for_apids(matrix_mdev, apm_filtered);
+	}
+out:
+	release_update_locks_for_mdev(matrix_mdev);
+	mutex_unlock(&ap_perms_mutex);
+	kfree(newbuf);
+	return rc;
 }
 static DEVICE_ATTR_RW(ap_config);
 
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 98d37aa27044..437a161c8659 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -75,11 +75,11 @@ extern struct ap_matrix_dev *matrix_dev;
  */
 struct ap_matrix {
 	unsigned long apm_max;
-	DECLARE_BITMAP(apm, 256);
+	DECLARE_BITMAP(apm, AP_DEVICES);
 	unsigned long aqm_max;
-	DECLARE_BITMAP(aqm, 256);
+	DECLARE_BITMAP(aqm, AP_DOMAINS);
 	unsigned long adm_max;
-	DECLARE_BITMAP(adm, 256);
+	DECLARE_BITMAP(adm, AP_DOMAINS);
 };
 
 /**
-- 
2.41.0

Re: [PATCH v2 4/5] s390/vfio-ap: Add write support to sysfs attr ap_config

[Anthony Krowiak]

On 3/6/24 9:08 AM, Jason J. Herne wrote:
> Allow writing a complete set of masks to ap_config. Doing so will
> cause the vfio-ap driver to replace the vfio-ap mediated device's entire
> state with the given set of masks. If the given state cannot be set, then


Just a nit, but it will not replace the vfio_ap mdev's entire state; it 
will replace the masks that comprise the matrix and control_domain 
attributes which comprise the guest's AP configuration profile (or the 
masks identifying the adapters, domains and control domains which a 
guest has permission to access). The guest_matrix attribute may or may 
not match the masks written via this sysfs interface.


> no changes are made to the vfio-ap mediated device.
>
> The format of the data written to ap_config is as follows:
> {amask},{dmask},{cmask}\n
>
> \n is a newline character.
>
> amask, dmask, and cmask are masks identifying which adapters, domains,
> and control domains should be assigned to the mediated device.
>
> The format of a mask is as follows:
> 0xNN..NN
>
> Where NN..NN is 64 hexadecimal characters representing a 256-bit value.
> The leftmost (highest order) bit represents adapter/domain 0.


I won't reject giving an r-b for the above, but could be more 
informative; maybe more along the lines of how this is described in all 
documentation:


Where NN..NN is 64 hexadecimal characters comprising a bitmap containing 
256 bits. Each bit, from left

to right, corresponds to a number from 0 to 255. If a bit is set, the

corresponding adapter, domain or control domain is assigned to the 
vfio_ap mdev.

You could also mention that setting an adapter or domain number greater 
than the maximum allowed for

for the system will result in an error.


>
> For an example set of masks that represent your mdev's current
> configuration, simply cat ap_config.
>
> This attribute is intended to be used by an mdevctl callout script
> supporting the mdev type vfio_ap-passthrough to atomically update a
> vfio-ap mediated device's state.
>
> Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
> ---
>   drivers/s390/crypto/vfio_ap_ops.c     | 172 ++++++++++++++++++++++++--
>   drivers/s390/crypto/vfio_ap_private.h |   6 +-
>   2 files changed, 162 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 259130347d00..c382e46f620f 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -1106,19 +1106,22 @@ static void vfio_ap_mdev_unlink_adapter(struct ap_matrix_mdev *matrix_mdev,
>   	}
>   }
>   
> -static void vfio_ap_mdev_hot_unplug_adapter(struct ap_matrix_mdev *matrix_mdev,
> -					    unsigned long apid)
> +static void vfio_ap_mdev_hot_unplug_adapters(struct ap_matrix_mdev *matrix_mdev,
> +					    unsigned long *apids)
>   {
>   	struct vfio_ap_queue *q, *tmpq;
>   	struct list_head qlist;
> +	unsigned long apid;
>   
>   	INIT_LIST_HEAD(&qlist);
> -	vfio_ap_mdev_unlink_adapter(matrix_mdev, apid, &qlist);
>   
> -	if (test_bit_inv(apid, matrix_mdev->shadow_apcb.apm)) {
> -		clear_bit_inv(apid, matrix_mdev->shadow_apcb.apm);
> -		vfio_ap_mdev_update_guest_apcb(matrix_mdev);
> +	for_each_set_bit_inv(apid, apids, AP_DEVICES) {
> +		vfio_ap_mdev_unlink_adapter(matrix_mdev, apid, &qlist);
> +
> +		if (test_bit_inv(apid, matrix_mdev->shadow_apcb.apm))
> +			clear_bit_inv(apid, matrix_mdev->shadow_apcb.apm);
>   	}
> +	vfio_ap_mdev_update_guest_apcb(matrix_mdev);


I wouldn't do the hot plug unless at least one of the APIDs in the apids 
parameter is assigned to matrix_mdev->shadow_apcb. The 
vfio_ap_mdev_update_guest_apcb function calls the 
kvm_arch_crypto_set_masks function which takes the guest's VCPUs out of 
SIE, copies the apm/aqm/adm from matrix_mdev->shadow_apcb to the APCB in 
the SIE state description, then restores the VCPUs to SIE. If no changes 
have been made to matrix_mdev->shadow_apcb, then it doesn't make sense 
to impact the guest in such a manner. So maybe something like this:

if (bitmap_intersects(apids, matrix_mdev->shadow_apcb.apm, AP_DEVICES))

         vfio_ap_mdev_update_guest_apcb(matrix_mdev)



>   
>   	vfio_ap_mdev_reset_qlist(&qlist);
>   
> @@ -1128,6 +1131,15 @@ static void vfio_ap_mdev_hot_unplug_adapter(struct ap_matrix_mdev *matrix_mdev,
>   	}
>   }
>   
> +static void vfio_ap_mdev_hot_unplug_adapter(struct ap_matrix_mdev *matrix_mdev,
> +					    unsigned long apid)
> +{
> +	DECLARE_BITMAP(apids, AP_DEVICES);


I'm not sure about this, but should the apids bitmap be zeroed out?

memset(apids, 0, sizeof(apids));


> +
> +	set_bit_inv(apid, apids);
> +	vfio_ap_mdev_hot_unplug_adapters(matrix_mdev, apids);
> +}
> +
>   /**
>    * unassign_adapter_store - parses the APID from @buf and clears the
>    * corresponding bit in the mediated matrix device's APM
> @@ -1288,19 +1300,22 @@ static void vfio_ap_mdev_unlink_domain(struct ap_matrix_mdev *matrix_mdev,
>   	}
>   }
>   
> -static void vfio_ap_mdev_hot_unplug_domain(struct ap_matrix_mdev *matrix_mdev,
> -					   unsigned long apqi)
> +static void vfio_ap_mdev_hot_unplug_domains(struct ap_matrix_mdev *matrix_mdev,
> +					   unsigned long *apqis)
>   {
>   	struct vfio_ap_queue *q, *tmpq;
>   	struct list_head qlist;
> +	unsigned long apqi;
>   
>   	INIT_LIST_HEAD(&qlist);
> -	vfio_ap_mdev_unlink_domain(matrix_mdev, apqi, &qlist);
>   
> -	if (test_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm)) {
> -		clear_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm);
> -		vfio_ap_mdev_update_guest_apcb(matrix_mdev);
> +	for_each_set_bit_inv(apqi, apqis, AP_DOMAINS) {
> +		vfio_ap_mdev_unlink_domain(matrix_mdev, apqi, &qlist);
> +
> +		if (test_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm))
> +			clear_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm);
>   	}
> +	vfio_ap_mdev_update_guest_apcb(matrix_mdev);


Same comment here as for vfio_ap_mdev_hot_unplug_adapters function.


>   
>   	vfio_ap_mdev_reset_qlist(&qlist);
>   
> @@ -1310,6 +1325,15 @@ static void vfio_ap_mdev_hot_unplug_domain(struct ap_matrix_mdev *matrix_mdev,
>   	}
>   }
>   
> +static void vfio_ap_mdev_hot_unplug_domain(struct ap_matrix_mdev *matrix_mdev,
> +					   unsigned long apqi)
> +{
> +	DECLARE_BITMAP(apqis, AP_DOMAINS);


See comment/question in vfio_ap_mdev_hot_unplug_adapter function.


> +
> +	set_bit_inv(apqi, apqis);
> +	vfio_ap_mdev_hot_unplug_domains(matrix_mdev, apqis);
> +}
> +
>   /**
>    * unassign_domain_store - parses the APQI from @buf and clears the
>    * corresponding bit in the mediated matrix device's AQM
> @@ -1577,10 +1601,132 @@ static ssize_t ap_config_show(struct device *dev, struct device_attribute *attr,
>   	return idx;
>   }
>   
> +/* Number of characters needed for a complete hex mask representing the bits in ..  */
> +#define AP_DEVICES_STRLEN	(AP_DEVICES/4 + 3)
> +#define AP_DOMAINS_STRLEN	(AP_DOMAINS/4 + 3)
> +#define AP_CONFIG_STRLEN	(AP_DEVICES_STRLEN + 2 * AP_DOMAINS_STRLEN)
> +
> +static int parse_bitmap(char **strbufptr, unsigned long *bitmap, int nbits)
> +{
> +	char *curmask;
> +
> +	curmask = strsep(strbufptr, ",\n");
> +	if (!curmask)
> +		return -EINVAL;
> +
> +	bitmap_clear(bitmap, 0, nbits);
> +	return ap_hex2bitmap(curmask, bitmap, nbits);
> +}
> +
> +static int ap_matrix_length_check(struct ap_matrix_mdev *matrix_mdev)


We're not really checking the matrix length here, we're checking whether 
any set bits exceed that maximum value, so maybe something like:

ap_matrix_max_bitnum_check(struct ap_matrix_mdev *matrix_mdev)?

Not critical though.


> +{
> +	unsigned long bit;
> +
> +	for_each_set_bit_inv(bit, matrix_mdev->matrix.apm, AP_DEVICES) {
> +		if (bit > matrix_mdev->matrix.apm_max)
> +			return -ENODEV;
> +	}
> +
> +	for_each_set_bit_inv(bit, matrix_mdev->matrix.aqm, AP_DOMAINS) {
> +		if (bit > matrix_mdev->matrix.aqm_max)
> +			return -ENODEV;
> +	}
> +
> +	for_each_set_bit_inv(bit, matrix_mdev->matrix.adm, AP_DOMAINS) {
> +		if (bit > matrix_mdev->matrix.adm_max)
> +			return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +static void ap_matrix_copy(struct ap_matrix *dst, struct ap_matrix *src)
> +{
> +	bitmap_copy(dst->apm, src->apm, AP_DEVICES);
> +	bitmap_copy(dst->aqm, src->aqm, AP_DOMAINS);
> +	bitmap_copy(dst->adm, src->adm, AP_DOMAINS);
> +}
> +
>   static ssize_t ap_config_store(struct device *dev, struct device_attribute *attr,
>   			       const char *buf, size_t count)
>   {
> -	return count;
> +	struct ap_matrix_mdev *matrix_mdev = dev_get_drvdata(dev);
> +	struct ap_matrix m_new, m_old, m_added, m_removed;
> +	DECLARE_BITMAP(apm_filtered, AP_DEVICES);
> +	unsigned long newbit;
> +	char *newbuf, *rest;
> +	int rc = count;
> +	bool do_update;
> +
> +	newbuf = rest = kstrndup(buf, AP_CONFIG_STRLEN, GFP_KERNEL);
> +	if (!newbuf)
> +		return -ENOMEM;
> +
> +	mutex_lock(&ap_perms_mutex);
> +	get_update_locks_for_mdev(matrix_mdev);
> +
> +	/* Save old state */
> +	ap_matrix_copy(&m_old, &matrix_mdev->matrix);
> +
> +	if (parse_bitmap(&rest, m_new.apm, AP_DEVICES) ||
> +	    parse_bitmap(&rest, m_new.aqm, AP_DOMAINS) ||
> +	    parse_bitmap(&rest, m_new.adm, AP_DOMAINS)) {
> +		rc = -EINVAL;
> +		goto out;
> +	}
> +
> +	bitmap_andnot(m_removed.apm, m_old.apm, m_new.apm, AP_DEVICES);
> +	bitmap_andnot(m_removed.aqm, m_old.aqm, m_new.aqm, AP_DOMAINS);
> +	bitmap_andnot(m_added.apm, m_new.apm, m_old.apm, AP_DEVICES);
> +	bitmap_andnot(m_added.aqm, m_new.aqm, m_old.aqm, AP_DOMAINS);
> +
> +	/* Need new bitmaps in matrix_mdev for validation */
> +	ap_matrix_copy(&matrix_mdev->matrix, &m_new);
> +
> +	/* Ensure new state is valid, else undo new state */
> +	rc = vfio_ap_mdev_validate_masks(matrix_mdev);
> +	if (rc) {
> +		ap_matrix_copy(&matrix_mdev->matrix, &m_old);
> +		goto out;
> +	}
> +	rc = ap_matrix_length_check(matrix_mdev);
> +	if (rc) {
> +		ap_matrix_copy(&matrix_mdev->matrix, &m_old);
> +		goto out;
> +	}
> +	rc = count;
> +
> +	/* Need old bitmaps in matrix_mdev for unplug/unlink */
> +	ap_matrix_copy(&matrix_mdev->matrix, &m_old);
> +
> +	/* Unlink removed adapters/domains */
> +	vfio_ap_mdev_hot_unplug_adapters(matrix_mdev, m_removed.apm);
> +	vfio_ap_mdev_hot_unplug_domains(matrix_mdev, m_removed.aqm);
> +
> +	/* Need new bitmaps in matrix_mdev for linking new adapters/domains */
> +	ap_matrix_copy(&matrix_mdev->matrix, &m_new);
> +
> +	/* Link newly added adapters */
> +	for_each_set_bit_inv(newbit, m_added.apm, AP_DEVICES)
> +		vfio_ap_mdev_link_adapter(matrix_mdev, newbit);
> +
> +	for_each_set_bit_inv(newbit, m_added.aqm, AP_DOMAINS)
> +		vfio_ap_mdev_link_domain(matrix_mdev, newbit);
> +
> +	/* filter resources not bound to vfio-ap */
> +	do_update = vfio_ap_mdev_filter_matrix(matrix_mdev, apm_filtered);
> +	do_update |= vfio_ap_mdev_filter_cdoms(matrix_mdev);
> +
> +	/* Apply changes to shadow apbc if things changed */
> +	if (do_update) {
> +		vfio_ap_mdev_update_guest_apcb(matrix_mdev);
> +		reset_queues_for_apids(matrix_mdev, apm_filtered);
> +	}
> +out:
> +	release_update_locks_for_mdev(matrix_mdev);
> +	mutex_unlock(&ap_perms_mutex);
> +	kfree(newbuf);
> +	return rc;
>   }
>   static DEVICE_ATTR_RW(ap_config);
>   
> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> index 98d37aa27044..437a161c8659 100644
> --- a/drivers/s390/crypto/vfio_ap_private.h
> +++ b/drivers/s390/crypto/vfio_ap_private.h
> @@ -75,11 +75,11 @@ extern struct ap_matrix_dev *matrix_dev;
>    */
>   struct ap_matrix {
>   	unsigned long apm_max;
> -	DECLARE_BITMAP(apm, 256);
> +	DECLARE_BITMAP(apm, AP_DEVICES);
>   	unsigned long aqm_max;
> -	DECLARE_BITMAP(aqm, 256);
> +	DECLARE_BITMAP(aqm, AP_DOMAINS);
>   	unsigned long adm_max;
> -	DECLARE_BITMAP(adm, 256);
> +	DECLARE_BITMAP(adm, AP_DOMAINS);
>   };
>   
>   /**

Re: [PATCH v2 4/5] s390/vfio-ap: Add write support to sysfs attr ap_config

[Jason J. Herne]

On 3/11/24 2:15 PM, Anthony Krowiak wrote:
> 
> On 3/6/24 9:08 AM, Jason J. Herne wrote:
>> Allow writing a complete set of masks to ap_config. Doing so will
>> cause the vfio-ap driver to replace the vfio-ap mediated device's entire
>> state with the given set of masks. If the given state cannot be set, then
> 
> 
> Just a nit, but it will not replace the vfio_ap mdev's entire state; it 
> will replace the masks that comprise the matrix and control_domain 
> attributes which comprise the guest's AP configuration profile (or the 
> masks identifying the adapters, domains and control domains which a 
> guest has permission to access). The guest_matrix attribute may or may 
> not match the masks written via this sysfs interface.
> 
> 

Fixed in v3.

>> no changes are made to the vfio-ap mediated device.
>>
>> The format of the data written to ap_config is as follows:
>> {amask},{dmask},{cmask}\n
>>
>> \n is a newline character.
>>
>> amask, dmask, and cmask are masks identifying which adapters, domains,
>> and control domains should be assigned to the mediated device.
>>
>> The format of a mask is as follows:
>> 0xNN..NN
>>
>> Where NN..NN is 64 hexadecimal characters representing a 256-bit value.
>> The leftmost (highest order) bit represents adapter/domain 0.
> 
> 
> I won't reject giving an r-b for the above, but could be more 
> informative; maybe more along the lines of how this is described in all 
> documentation:
> 

Leaving as is. It gets the point across.

> Where NN..NN is 64 hexadecimal characters comprising a bitmap containing 
> 256 bits. Each bit, from left
> 
> to right, corresponds to a number from 0 to 255. If a bit is set, the
> 
> corresponding adapter, domain or control domain is assigned to the 
> vfio_ap mdev.
> 
> You could also mention that setting an adapter or domain number greater 
> than the maximum allowed for
> 
> for the system will result in an error.
> 
> 

I feel like the description is long enough for a commit message. Maybe 
this would be more at home in the doc patch.

>>
>> For an example set of masks that represent your mdev's current
>> configuration, simply cat ap_config.
>>
>> This attribute is intended to be used by an mdevctl callout script
>> supporting the mdev type vfio_ap-passthrough to atomically update a
>> vfio-ap mediated device's state.
>>
>> Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
>> ---
>>   drivers/s390/crypto/vfio_ap_ops.c     | 172 ++++++++++++++++++++++++--
>>   drivers/s390/crypto/vfio_ap_private.h |   6 +-
>>   2 files changed, 162 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c 
>> b/drivers/s390/crypto/vfio_ap_ops.c
>> index 259130347d00..c382e46f620f 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -1106,19 +1106,22 @@ static void vfio_ap_mdev_unlink_adapter(struct 
>> ap_matrix_mdev *matrix_mdev,
>>       }
>>   }
>> -static void vfio_ap_mdev_hot_unplug_adapter(struct ap_matrix_mdev 
>> *matrix_mdev,
>> -                        unsigned long apid)
>> +static void vfio_ap_mdev_hot_unplug_adapters(struct ap_matrix_mdev 
>> *matrix_mdev,
>> +                        unsigned long *apids)
>>   {
>>       struct vfio_ap_queue *q, *tmpq;
>>       struct list_head qlist;
>> +    unsigned long apid;
>>       INIT_LIST_HEAD(&qlist);
>> -    vfio_ap_mdev_unlink_adapter(matrix_mdev, apid, &qlist);
>> -    if (test_bit_inv(apid, matrix_mdev->shadow_apcb.apm)) {
>> -        clear_bit_inv(apid, matrix_mdev->shadow_apcb.apm);
>> -        vfio_ap_mdev_update_guest_apcb(matrix_mdev);
>> +    for_each_set_bit_inv(apid, apids, AP_DEVICES) {
>> +        vfio_ap_mdev_unlink_adapter(matrix_mdev, apid, &qlist);
>> +
>> +        if (test_bit_inv(apid, matrix_mdev->shadow_apcb.apm))
>> +            clear_bit_inv(apid, matrix_mdev->shadow_apcb.apm);
>>       }
>> +    vfio_ap_mdev_update_guest_apcb(matrix_mdev);
> 
> 
> I wouldn't do the hot plug unless at least one of the APIDs in the apids 
> parameter is assigned to matrix_mdev->shadow_apcb. The 
> vfio_ap_mdev_update_guest_apcb function calls the 
> kvm_arch_crypto_set_masks function which takes the guest's VCPUs out of 
> SIE, copies the apm/aqm/adm from matrix_mdev->shadow_apcb to the APCB in 
> the SIE state description, then restores the VCPUs to SIE. If no changes 
> have been made to matrix_mdev->shadow_apcb, then it doesn't make sense 
> to impact the guest in such a manner. So maybe something like this:
> 
> if (bitmap_intersects(apids, matrix_mdev->shadow_apcb.apm, AP_DEVICES))
> 
>          vfio_ap_mdev_update_guest_apcb(matrix_mdev)
> 

Fixed in v3.

> 
>>       vfio_ap_mdev_reset_qlist(&qlist);
>> @@ -1128,6 +1131,15 @@ static void 
>> vfio_ap_mdev_hot_unplug_adapter(struct ap_matrix_mdev *matrix_mdev,
>>       }
>>   }
>> +static void vfio_ap_mdev_hot_unplug_adapter(struct ap_matrix_mdev 
>> *matrix_mdev,
>> +                        unsigned long apid)
>> +{
>> +    DECLARE_BITMAP(apids, AP_DEVICES);
> 
> 
> I'm not sure about this, but should the apids bitmap be zeroed out?
> 
> memset(apids, 0, sizeof(apids));
> 

I would think it is not needed, but I do see precent in other code so it 
is better to be safe here I think. I'll add this for v3. I'll use 
bitmap_zero, however, instead of memeset.

>> +
>> +    set_bit_inv(apid, apids);
>> +    vfio_ap_mdev_hot_unplug_adapters(matrix_mdev, apids);
>> +}
>> +
>>   /**
>>    * unassign_adapter_store - parses the APID from @buf and clears the
>>    * corresponding bit in the mediated matrix device's APM
>> @@ -1288,19 +1300,22 @@ static void vfio_ap_mdev_unlink_domain(struct 
>> ap_matrix_mdev *matrix_mdev,
>>       }
>>   }
>> -static void vfio_ap_mdev_hot_unplug_domain(struct ap_matrix_mdev 
>> *matrix_mdev,
>> -                       unsigned long apqi)
>> +static void vfio_ap_mdev_hot_unplug_domains(struct ap_matrix_mdev 
>> *matrix_mdev,
>> +                       unsigned long *apqis)
>>   {
>>       struct vfio_ap_queue *q, *tmpq;
>>       struct list_head qlist;
>> +    unsigned long apqi;
>>       INIT_LIST_HEAD(&qlist);
>> -    vfio_ap_mdev_unlink_domain(matrix_mdev, apqi, &qlist);
>> -    if (test_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm)) {
>> -        clear_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm);
>> -        vfio_ap_mdev_update_guest_apcb(matrix_mdev);
>> +    for_each_set_bit_inv(apqi, apqis, AP_DOMAINS) {
>> +        vfio_ap_mdev_unlink_domain(matrix_mdev, apqi, &qlist);
>> +
>> +        if (test_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm))
>> +            clear_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm);
>>       }
>> +    vfio_ap_mdev_update_guest_apcb(matrix_mdev);
> 
> 
> Same comment here as for vfio_ap_mdev_hot_unplug_adapters function.
> 
> 

Fixed in v3.

>>       vfio_ap_mdev_reset_qlist(&qlist);
>> @@ -1310,6 +1325,15 @@ static void 
>> vfio_ap_mdev_hot_unplug_domain(struct ap_matrix_mdev *matrix_mdev,
>>       }
>>   }
>> +static void vfio_ap_mdev_hot_unplug_domain(struct ap_matrix_mdev 
>> *matrix_mdev,
>> +                       unsigned long apqi)
>> +{
>> +    DECLARE_BITMAP(apqis, AP_DOMAINS);
> 
> 
> See comment/question in vfio_ap_mdev_hot_unplug_adapter function.
> 
> 
Fixed in v3.
>> +
>> +    set_bit_inv(apqi, apqis);
>> +    vfio_ap_mdev_hot_unplug_domains(matrix_mdev, apqis);
>> +}
>> +
>>   /**
>>    * unassign_domain_store - parses the APQI from @buf and clears the
>>    * corresponding bit in the mediated matrix device's AQM
>> @@ -1577,10 +1601,132 @@ static ssize_t ap_config_show(struct device 
>> *dev, struct device_attribute *attr,
>>       return idx;
>>   }
>> +/* Number of characters needed for a complete hex mask representing 
>> the bits in ..  */
>> +#define AP_DEVICES_STRLEN    (AP_DEVICES/4 + 3)
>> +#define AP_DOMAINS_STRLEN    (AP_DOMAINS/4 + 3)
>> +#define AP_CONFIG_STRLEN    (AP_DEVICES_STRLEN + 2 * AP_DOMAINS_STRLEN)
>> +
>> +static int parse_bitmap(char **strbufptr, unsigned long *bitmap, int 
>> nbits)
>> +{
>> +    char *curmask;
>> +
>> +    curmask = strsep(strbufptr, ",\n");
>> +    if (!curmask)
>> +        return -EINVAL;
>> +
>> +    bitmap_clear(bitmap, 0, nbits);
>> +    return ap_hex2bitmap(curmask, bitmap, nbits);
>> +}
>> +
>> +static int ap_matrix_length_check(struct ap_matrix_mdev *matrix_mdev)
> 
> 
> We're not really checking the matrix length here, we're checking whether 
> any set bits exceed that maximum value, so maybe something like:
> 
> ap_matrix_max_bitnum_check(struct ap_matrix_mdev *matrix_mdev)?
> 
> Not critical though.
> 
> 

Renaming to ap_matrix_overflow_check for v3. That name is more concise 
in my opinion.

[PATCH v2 5/5] s390: doc: Update doc

[Jason J. Herne]

fix me

Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
---
 Documentation/arch/s390/vfio-ap.rst | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/Documentation/arch/s390/vfio-ap.rst b/Documentation/arch/s390/vfio-ap.rst
index 929ee1c1c940..af5ef60355a2 100644
--- a/Documentation/arch/s390/vfio-ap.rst
+++ b/Documentation/arch/s390/vfio-ap.rst
@@ -380,6 +380,33 @@ matrix device.
     control_domains:
       A read-only file for displaying the control domain numbers assigned to the
       vfio_ap mediated device.
+    ap_config:
+        A read/write file that, when written to, allows the entire vfio_ap mediated
+        device's ap configuration to be replaced in one shot. Three masks are given,
+        one for adapters, one for domains, and one for control domains. If the
+        given state cannot be set, then no changes are made to the vfio-ap
+        mediated device.
+
+        The format of the data written to ap_config is as follows:
+        {amask},{dmask},{cmask}\n
+
+        \n is a newline character.
+
+        amask, dmask, and cmask are masks identifying which adapters, domains,
+        and control domains should be assigned to the mediated device.
+
+        The format of a mask is as follows:
+        0xNN..NN
+
+        Where NN..NN is 64 hexadecimal characters representing a 256-bit value.
+        The leftmost (highest order) bit represents adapter/domain 0.
+
+        For an example set of masks that represent your mdev's current
+        configuration, simply cat ap_config.
+
+        This attribute is intended to be used by automation. End users would be
+        better served using the respective assign/unassign attributes for
+        adapters, domains, and control domains.
 
 * functions:
 
-- 
2.41.0

Re: [PATCH v2 5/5] s390: doc: Update doc

[Matthew Rosato]

On 3/6/24 9:08 AM, Jason J. Herne wrote:
> fix me

^^ Oops, just needs a small message here. 

Re: [PATCH v2 5/5] s390: doc: Update doc

[Jason J. Herne]

On 3/6/24 9:38 AM, Matthew Rosato wrote:
> On 3/6/24 9:08 AM, Jason J. Herne wrote:
>> fix me
> 
> ^^ Oops, just needs a small message here.
> 
> 

NACK, looks good to me :)

This is what I get for thinking "nahh, no need to double check the docs 
patch... It's fine."  I'll fix for v3 or reply with an updated patch if 
there's no other rework.

Re: [PATCH v2 5/5] s390: doc: Update doc

[Anthony Krowiak]

On 3/6/24 9:08 AM, Jason J. Herne wrote:
> fix me
>
> Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
> ---
>   Documentation/arch/s390/vfio-ap.rst | 27 +++++++++++++++++++++++++++
>   1 file changed, 27 insertions(+)
>
> diff --git a/Documentation/arch/s390/vfio-ap.rst b/Documentation/arch/s390/vfio-ap.rst
> index 929ee1c1c940..af5ef60355a2 100644
> --- a/Documentation/arch/s390/vfio-ap.rst
> +++ b/Documentation/arch/s390/vfio-ap.rst
> @@ -380,6 +380,33 @@ matrix device.
>       control_domains:
>         A read-only file for displaying the control domain numbers assigned to the
>         vfio_ap mediated device.
> +    ap_config:
> +        A read/write file that, when written to, allows the entire vfio_ap mediated
> +        device's ap configuration to be replaced in one shot. Three masks are given,
> +        one for adapters, one for domains, and one for control domains. If the
> +        given state cannot be set, then no changes are made to the vfio-ap
> +        mediated device.
> +
> +        The format of the data written to ap_config is as follows:
> +        {amask},{dmask},{cmask}\n
> +
> +        \n is a newline character.
> +
> +        amask, dmask, and cmask are masks identifying which adapters, domains,
> +        and control domains should be assigned to the mediated device.
> +
> +        The format of a mask is as follows:
> +        0xNN..NN
> +
> +        Where NN..NN is 64 hexadecimal characters representing a 256-bit value.
> +        The leftmost (highest order) bit represents adapter/domain 0.


Same comment I made in patch 4/5:

I won't reject giving an r-b for the above, but could be more 
informative; maybe more along the lines of how this is described in all 
documentation:


Where NN..NN is 64 hexadecimal characters comprising a bitmap containing 
256 bits. Each bit, from left

to right, corresponds to a number from 0 to 255. If a bit is set, the

corresponding adapter, domain or control domain is assigned to the 
vfio_ap mdev.

You could also mention that setting an adapter or domain number greater 
than the maximum allowed for

for the system will result in an error.


> +
> +        For an example set of masks that represent your mdev's current
> +        configuration, simply cat ap_config.
> +
> +        This attribute is intended to be used by automation. End users would be
> +        better served using the respective assign/unassign attributes for
> +        adapters, domains, and control domains.
>   
>   * functions:
>   

Re: [PATCH v2 0/5] s390/vfio-ap: queue_configuration sysfs attribute for mdevctl automation

[Matthew Rosato]

On 3/6/24 9:08 AM, Jason J. Herne wrote:
> Mdevctl requires a way to atomically query and atomically update a vfio-ap
> mdev's current state. This patch set creates the queue_configuration sysfs

Less important than the patches themselves, but s/queue_configuration/ap_config/ here as well as title of cover letter.

Re: [PATCH v2 0/5] s390/vfio-ap: queue_configuration sysfs attribute for mdevctl automation

[Matthew Rosato]

On 3/6/24 9:08 AM, Jason J. Herne wrote:
> Mdevctl requires a way to atomically query and atomically update a vfio-ap
> mdev's current state. This patch set creates the queue_configuration sysfs
> attribute.  This new attribute allows reading and writing an mdev's entire
> state in one go. If a newly written state is invalid for any reason the entire
> state is rejected and the target mdev remains unchanged.
> 
> Changelog
> ==========
> v2
>   - Rebased patched on top of latest master
>   - Reworked code to fit changes introduced by f848cba767e59
>       s390/vfio-ap: reset queues filtered from the guest's AP config
>   - Moved docs changes to separate patch

Tested exploitation of the new sysfs interface using proposed s390-tools code + mdevctl.  Besides the other minor changes mentioned separately, feel free to include

Tested-by: Matthew Rosato <mjrosato@linux.ibm.com>

for the code patches.