* [PATCH v3 0/5] s390/vfio-ap: ap_config sysfs attribute for mdevctl automation
@ 2024-03-13 20:58 Jason J. Herne
2024-03-13 20:58 ` [PATCH v3 1/5] s390/ap: Externalize AP bus specific bitmap reading function Jason J. Herne
` (5 more replies)
0 siblings, 6 replies; 12+ messages in thread
From: Jason J. Herne @ 2024-03-13 20:58 UTC (permalink / raw)
To: linux-s390; +Cc: linux-kernel, pasic, akrowiak, borntraeger, agordeev, gor
Mdevctl requires a way to atomically query and atomically update a vfio-ap
mdev's current state. This patch set creates the ap_config 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
==========
v3
- Optimization: hot unplug functions skip apcb update if nothing was actually
unplugged.
- Hot unplug functions modified to zero bitmaps before use.
- Rename ap_matrix_length_check to ap_matrix_overflow_check
- Fixed omissions and errors in several commit messages and the docs.
- Added Tested-by tags.
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, ap_config, 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
docs: Update s390 vfio-ap doc for ap_config sysfs attribute
Documentation/arch/s390/vfio-ap.rst | 30 ++++
drivers/s390/crypto/ap_bus.c | 13 +-
drivers/s390/crypto/ap_bus.h | 22 +++
drivers/s390/crypto/vfio_ap_ops.c | 220 ++++++++++++++++++++++++--
drivers/s390/crypto/vfio_ap_private.h | 6 +-
5 files changed, 262 insertions(+), 29 deletions(-)
--
2.41.0
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 1/5] s390/ap: Externalize AP bus specific bitmap reading function
2024-03-13 20:58 [PATCH v3 0/5] s390/vfio-ap: ap_config sysfs attribute for mdevctl automation Jason J. Herne
@ 2024-03-13 20:58 ` Jason J. Herne
2024-03-13 20:58 ` [PATCH v3 2/5] s390/vfio-ap: Add sysfs attr, ap_config, to export mdev state Jason J. Herne
` (4 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Jason J. Herne @ 2024-03-13 20:58 UTC (permalink / raw)
To: linux-s390; +Cc: linux-kernel, pasic, akrowiak, borntraeger, agordeev, gor
Rename hex2bitmap() to ap_hex2bitmap() and export it for external
use. This function will be used by the implementation of the vfio-ap
ap_config 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 cce0bafd4c92..1be07197eda3 100644
--- a/drivers/s390/crypto/ap_bus.c
+++ b/drivers/s390/crypto/ap_bus.c
@@ -1068,15 +1068,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;
@@ -1103,6 +1095,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
@@ -1168,7 +1161,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 59c7ed49aa02..1db9597509da 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 2/5] s390/vfio-ap: Add sysfs attr, ap_config, to export mdev state
2024-03-13 20:58 [PATCH v3 0/5] s390/vfio-ap: ap_config sysfs attribute for mdevctl automation Jason J. Herne
2024-03-13 20:58 ` [PATCH v3 1/5] s390/ap: Externalize AP bus specific bitmap reading function Jason J. Herne
@ 2024-03-13 20:58 ` Jason J. Herne
2024-03-13 20:58 ` [PATCH v3 3/5] s390/vfio-ap: Ignore duplicate link requests in vfio_ap_mdev_link_queue Jason J. Herne
` (3 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Jason J. Herne @ 2024-03-13 20:58 UTC (permalink / raw)
To: linux-s390; +Cc: linux-kernel, pasic, akrowiak, borntraeger, agordeev, gor
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>
Tested-by: Matthew Rosato <mjrosato@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 fc169bc61593..e01f53a3c5b7 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -1570,6 +1570,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,
@@ -1577,6 +1603,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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 3/5] s390/vfio-ap: Ignore duplicate link requests in vfio_ap_mdev_link_queue
2024-03-13 20:58 [PATCH v3 0/5] s390/vfio-ap: ap_config sysfs attribute for mdevctl automation Jason J. Herne
2024-03-13 20:58 ` [PATCH v3 1/5] s390/ap: Externalize AP bus specific bitmap reading function Jason J. Herne
2024-03-13 20:58 ` [PATCH v3 2/5] s390/vfio-ap: Add sysfs attr, ap_config, to export mdev state Jason J. Herne
@ 2024-03-13 20:58 ` Jason J. Herne
2024-03-13 20:58 ` [PATCH v3 4/5] s390/vfio-ap: Add write support to sysfs attr ap_config Jason J. Herne
` (2 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Jason J. Herne @ 2024-03-13 20:58 UTC (permalink / raw)
To: linux-s390; +Cc: linux-kernel, pasic, akrowiak, borntraeger, agordeev, gor
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 e01f53a3c5b7..1499c2181122 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -794,10 +794,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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 4/5] s390/vfio-ap: Add write support to sysfs attr ap_config
2024-03-13 20:58 [PATCH v3 0/5] s390/vfio-ap: ap_config sysfs attribute for mdevctl automation Jason J. Herne
` (2 preceding siblings ...)
2024-03-13 20:58 ` [PATCH v3 3/5] s390/vfio-ap: Ignore duplicate link requests in vfio_ap_mdev_link_queue Jason J. Herne
@ 2024-03-13 20:58 ` Jason J. Herne
2024-03-15 14:45 ` Anthony Krowiak
2024-03-13 20:58 ` [PATCH v3 5/5] docs: Update s390 vfio-ap doc for ap_config sysfs attribute Jason J. Herne
2024-03-15 14:48 ` [PATCH v3 0/5] s390/vfio-ap: ap_config sysfs attribute for mdevctl automation Anthony Krowiak
5 siblings, 1 reply; 12+ messages in thread
From: Jason J. Herne @ 2024-03-13 20:58 UTC (permalink / raw)
To: linux-s390; +Cc: linux-kernel, pasic, akrowiak, borntraeger, agordeev, gor
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
matrix masks 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>
Tested-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
drivers/s390/crypto/vfio_ap_ops.c | 186 ++++++++++++++++++++++++--
drivers/s390/crypto/vfio_ap_private.h | 6 +-
2 files changed, 176 insertions(+), 16 deletions(-)
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 1499c2181122..05b1d311b31f 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -1119,20 +1119,29 @@ 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;
+ bool apcb_update = false;
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);
+ apcb_update = true;
+ }
}
+ /* Only update apcb if needed to avoid impacting guest */
+ if (apcb_update)
+ vfio_ap_mdev_update_guest_apcb(matrix_mdev);
+
vfio_ap_mdev_reset_qlist(&qlist);
list_for_each_entry_safe(q, tmpq, &qlist, reset_qnode) {
@@ -1141,6 +1150,16 @@ 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);
+
+ bitmap_zero(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
@@ -1301,20 +1320,29 @@ 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;
+ bool apcb_update = false;
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);
+ apcb_update = true;
+ }
}
+ /* Only update apcb if needed to avoid impacting guest */
+ if (apcb_update)
+ vfio_ap_mdev_update_guest_apcb(matrix_mdev);
+
vfio_ap_mdev_reset_qlist(&qlist);
list_for_each_entry_safe(q, tmpq, &qlist, reset_qnode) {
@@ -1323,6 +1351,16 @@ 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);
+
+ bitmap_zero(apqis, AP_DEVICES);
+ 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
@@ -1590,10 +1628,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_overflow_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_overflow_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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 5/5] docs: Update s390 vfio-ap doc for ap_config sysfs attribute
2024-03-13 20:58 [PATCH v3 0/5] s390/vfio-ap: ap_config sysfs attribute for mdevctl automation Jason J. Herne
` (3 preceding siblings ...)
2024-03-13 20:58 ` [PATCH v3 4/5] s390/vfio-ap: Add write support to sysfs attr ap_config Jason J. Herne
@ 2024-03-13 20:58 ` Jason J. Herne
2024-03-15 14:46 ` Anthony Krowiak
2024-03-15 14:48 ` [PATCH v3 0/5] s390/vfio-ap: ap_config sysfs attribute for mdevctl automation Anthony Krowiak
5 siblings, 1 reply; 12+ messages in thread
From: Jason J. Herne @ 2024-03-13 20:58 UTC (permalink / raw)
To: linux-s390; +Cc: linux-kernel, pasic, akrowiak, borntraeger, agordeev, gor
A new sysfs attribute, ap_config, for the vfio_ap driver is
documented.
Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
---
Documentation/arch/s390/vfio-ap.rst | 30 +++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/Documentation/arch/s390/vfio-ap.rst b/Documentation/arch/s390/vfio-ap.rst
index 929ee1c1c940..6056a50ee841 100644
--- a/Documentation/arch/s390/vfio-ap.rst
+++ b/Documentation/arch/s390/vfio-ap.rst
@@ -380,6 +380,36 @@ 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 all three of the
+ vfio_ap mediated device's ap matrix masks 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.
+
+ Setting an adapter or domain number greater than the maximum allowed for
+ the system will result in an error.
+
+ 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3 4/5] s390/vfio-ap: Add write support to sysfs attr ap_config
2024-03-13 20:58 ` [PATCH v3 4/5] s390/vfio-ap: Add write support to sysfs attr ap_config Jason J. Herne
@ 2024-03-15 14:45 ` Anthony Krowiak
0 siblings, 0 replies; 12+ messages in thread
From: Anthony Krowiak @ 2024-03-15 14:45 UTC (permalink / raw)
To: Jason J. Herne, linux-s390
Cc: linux-kernel, pasic, borntraeger, agordeev, gor
Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>
On 3/13/24 4:58 PM, 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
> matrix masks 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>
> Tested-by: Matthew Rosato <mjrosato@linux.ibm.com>
> ---
> drivers/s390/crypto/vfio_ap_ops.c | 186 ++++++++++++++++++++++++--
> drivers/s390/crypto/vfio_ap_private.h | 6 +-
> 2 files changed, 176 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 1499c2181122..05b1d311b31f 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -1119,20 +1119,29 @@ 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;
> + bool apcb_update = false;
>
> 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);
> + apcb_update = true;
> + }
> }
>
> + /* Only update apcb if needed to avoid impacting guest */
> + if (apcb_update)
> + vfio_ap_mdev_update_guest_apcb(matrix_mdev);
> +
> vfio_ap_mdev_reset_qlist(&qlist);
>
> list_for_each_entry_safe(q, tmpq, &qlist, reset_qnode) {
> @@ -1141,6 +1150,16 @@ 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);
> +
> + bitmap_zero(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
> @@ -1301,20 +1320,29 @@ 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;
> + bool apcb_update = false;
>
> 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);
> + apcb_update = true;
> + }
> }
>
> + /* Only update apcb if needed to avoid impacting guest */
> + if (apcb_update)
> + vfio_ap_mdev_update_guest_apcb(matrix_mdev);
> +
> vfio_ap_mdev_reset_qlist(&qlist);
>
> list_for_each_entry_safe(q, tmpq, &qlist, reset_qnode) {
> @@ -1323,6 +1351,16 @@ 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);
> +
> + bitmap_zero(apqis, AP_DEVICES);
> + 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
> @@ -1590,10 +1628,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_overflow_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_overflow_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);
> };
>
> /**
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 5/5] docs: Update s390 vfio-ap doc for ap_config sysfs attribute
2024-03-13 20:58 ` [PATCH v3 5/5] docs: Update s390 vfio-ap doc for ap_config sysfs attribute Jason J. Herne
@ 2024-03-15 14:46 ` Anthony Krowiak
0 siblings, 0 replies; 12+ messages in thread
From: Anthony Krowiak @ 2024-03-15 14:46 UTC (permalink / raw)
To: Jason J. Herne, linux-s390
Cc: linux-kernel, pasic, borntraeger, agordeev, gor
Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>
On 3/13/24 4:58 PM, Jason J. Herne wrote:
> A new sysfs attribute, ap_config, for the vfio_ap driver is
> documented.
>
> Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
> ---
> Documentation/arch/s390/vfio-ap.rst | 30 +++++++++++++++++++++++++++++
> 1 file changed, 30 insertions(+)
>
> diff --git a/Documentation/arch/s390/vfio-ap.rst b/Documentation/arch/s390/vfio-ap.rst
> index 929ee1c1c940..6056a50ee841 100644
> --- a/Documentation/arch/s390/vfio-ap.rst
> +++ b/Documentation/arch/s390/vfio-ap.rst
> @@ -380,6 +380,36 @@ 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 all three of the
> + vfio_ap mediated device's ap matrix masks 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.
> +
> + Setting an adapter or domain number greater than the maximum allowed for
> + the system will result in an error.
> +
> + 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:
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 0/5] s390/vfio-ap: ap_config sysfs attribute for mdevctl automation
2024-03-13 20:58 [PATCH v3 0/5] s390/vfio-ap: ap_config sysfs attribute for mdevctl automation Jason J. Herne
` (4 preceding siblings ...)
2024-03-13 20:58 ` [PATCH v3 5/5] docs: Update s390 vfio-ap doc for ap_config sysfs attribute Jason J. Herne
@ 2024-03-15 14:48 ` Anthony Krowiak
2024-03-19 11:12 ` Heiko Carstens
5 siblings, 1 reply; 12+ messages in thread
From: Anthony Krowiak @ 2024-03-15 14:48 UTC (permalink / raw)
To: Jason J. Herne, linux-s390
Cc: linux-kernel, pasic, borntraeger, agordeev, gor
Unless someone else chooses to review these, my opinion is that they are
good to go.
On 3/13/24 4:58 PM, 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 ap_config 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
> ==========
> v3
> - Optimization: hot unplug functions skip apcb update if nothing was actually
> unplugged.
> - Hot unplug functions modified to zero bitmaps before use.
> - Rename ap_matrix_length_check to ap_matrix_overflow_check
> - Fixed omissions and errors in several commit messages and the docs.
> - Added Tested-by tags.
> 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, ap_config, 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
> docs: Update s390 vfio-ap doc for ap_config sysfs attribute
>
> Documentation/arch/s390/vfio-ap.rst | 30 ++++
> drivers/s390/crypto/ap_bus.c | 13 +-
> drivers/s390/crypto/ap_bus.h | 22 +++
> drivers/s390/crypto/vfio_ap_ops.c | 220 ++++++++++++++++++++++++--
> drivers/s390/crypto/vfio_ap_private.h | 6 +-
> 5 files changed, 262 insertions(+), 29 deletions(-)
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 0/5] s390/vfio-ap: ap_config sysfs attribute for mdevctl automation
2024-03-15 14:48 ` [PATCH v3 0/5] s390/vfio-ap: ap_config sysfs attribute for mdevctl automation Anthony Krowiak
@ 2024-03-19 11:12 ` Heiko Carstens
2024-03-21 14:14 ` Jason J. Herne
0 siblings, 1 reply; 12+ messages in thread
From: Heiko Carstens @ 2024-03-19 11:12 UTC (permalink / raw)
To: Anthony Krowiak
Cc: Jason J. Herne, linux-s390, linux-kernel, pasic, borntraeger,
agordeev, gor
On Fri, Mar 15, 2024 at 10:48:44AM -0400, Anthony Krowiak wrote:
> Unless someone else chooses to review these, my opinion is that they are
> good to go.
Please don't top-post.
> On 3/13/24 4:58 PM, 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 ap_config 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.
...
> > Jason J. Herne (5):
> > s390/ap: Externalize AP bus specific bitmap reading function
> > s390/vfio-ap: Add sysfs attr, ap_config, 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
> > docs: Update s390 vfio-ap doc for ap_config sysfs attribute
> >
> > Documentation/arch/s390/vfio-ap.rst | 30 ++++
> > drivers/s390/crypto/ap_bus.c | 13 +-
> > drivers/s390/crypto/ap_bus.h | 22 +++
> > drivers/s390/crypto/vfio_ap_ops.c | 220 ++++++++++++++++++++++++--
> > drivers/s390/crypto/vfio_ap_private.h | 6 +-
> > 5 files changed, 262 insertions(+), 29 deletions(-)
With gcc gcc 13.2.0 / binutils 2.40.90.20230730 I get this (defconfig):
CC [M] drivers/s390/crypto/vfio_ap_ops.o
In file included from ./include/linux/cpumask.h:13,
from ./include/linux/smp.h:13,
from ./include/linux/lockdep.h:14,
from ./include/linux/spinlock.h:63,
from ./include/linux/mmzone.h:8,
from ./include/linux/gfp.h:7,
from ./include/linux/mm.h:7,
from ./include/linux/scatterlist.h:8,
from ./include/linux/iommu.h:10,
from ./include/linux/vfio.h:12,
from drivers/s390/crypto/vfio_ap_ops.c:12:
In function ‘bitmap_copy’,
inlined from ‘ap_matrix_copy’ at drivers/s390/crypto/vfio_ap_ops.c:1672:2,
inlined from ‘ap_config_store’ at drivers/s390/crypto/vfio_ap_ops.c:1696:2:
./include/linux/bitmap.h:253:17: warning: ‘memcpy’ reading 32 bytes from a region of size 0 [-Wstringop-overread]
253 | memcpy(dst, src, len);
| ^~~~~~~~~~~~~~~~~~~~~
In function ‘ap_config_store’:
cc1: note: source object is likely at address zero
In function ‘bitmap_copy’,
inlined from ‘ap_matrix_copy’ at drivers/s390/crypto/vfio_ap_ops.c:1673:2,
inlined from ‘ap_config_store’ at drivers/s390/crypto/vfio_ap_ops.c:1696:2:
./include/linux/bitmap.h:253:17: warning: ‘memcpy’ reading 32 bytes from a region of size 0 [-Wstringop-overread]
253 | memcpy(dst, src, len);
| ^~~~~~~~~~~~~~~~~~~~~
In function ‘ap_config_store’:
cc1: note: source object is likely at address zero
In function ‘bitmap_copy’,
inlined from ‘ap_matrix_copy’ at drivers/s390/crypto/vfio_ap_ops.c:1674:2,
inlined from ‘ap_config_store’ at drivers/s390/crypto/vfio_ap_ops.c:1696:2:
./include/linux/bitmap.h:253:17: warning: ‘memcpy’ reading 32 bytes from a region of size 0 [-Wstringop-overread]
253 | memcpy(dst, src, len);
| ^~~~~~~~~~~~~~~~~~~~~
In function ‘ap_config_store’:
cc1: note: source object is likely at address zero
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 0/5] s390/vfio-ap: ap_config sysfs attribute for mdevctl automation
2024-03-19 11:12 ` Heiko Carstens
@ 2024-03-21 14:14 ` Jason J. Herne
2024-03-21 14:41 ` Heiko Carstens
0 siblings, 1 reply; 12+ messages in thread
From: Jason J. Herne @ 2024-03-21 14:14 UTC (permalink / raw)
To: Heiko Carstens, Anthony Krowiak
Cc: linux-s390, linux-kernel, pasic, borntraeger, agordeev, gor
On 3/19/24 7:12 AM, Heiko Carstens wrote:
> With gcc gcc 13.2.0 / binutils 2.40.90.20230730 I get this (defconfig):
>
> CC [M] drivers/s390/crypto/vfio_ap_ops.o
> In file included from ./include/linux/cpumask.h:13,
> from ./include/linux/smp.h:13,
> from ./include/linux/lockdep.h:14,
> from ./include/linux/spinlock.h:63,
> from ./include/linux/mmzone.h:8,
> from ./include/linux/gfp.h:7,
> from ./include/linux/mm.h:7,
> from ./include/linux/scatterlist.h:8,
> from ./include/linux/iommu.h:10,
> from ./include/linux/vfio.h:12,
> from drivers/s390/crypto/vfio_ap_ops.c:12:
> In function ‘bitmap_copy’,
> inlined from ‘ap_matrix_copy’ at drivers/s390/crypto/vfio_ap_ops.c:1672:2,
> inlined from ‘ap_config_store’ at drivers/s390/crypto/vfio_ap_ops.c:1696:2:
> ./include/linux/bitmap.h:253:17: warning: ‘memcpy’ reading 32 bytes from a region of size 0 [-Wstringop-overread]
> 253 | memcpy(dst, src, len);
> | ^~~~~~~~~~~~~~~~~~~~~
> In function ‘ap_config_store’:
> cc1: note: source object is likely at address zero
> In function ‘bitmap_copy’,
> inlined from ‘ap_matrix_copy’ at drivers/s390/crypto/vfio_ap_ops.c:1673:2,
> inlined from ‘ap_config_store’ at drivers/s390/crypto/vfio_ap_ops.c:1696:2:
> ./include/linux/bitmap.h:253:17: warning: ‘memcpy’ reading 32 bytes from a region of size 0 [-Wstringop-overread]
> 253 | memcpy(dst, src, len);
> | ^~~~~~~~~~~~~~~~~~~~~
> In function ‘ap_config_store’:
> cc1: note: source object is likely at address zero
> In function ‘bitmap_copy’,
> inlined from ‘ap_matrix_copy’ at drivers/s390/crypto/vfio_ap_ops.c:1674:2,
> inlined from ‘ap_config_store’ at drivers/s390/crypto/vfio_ap_ops.c:1696:2:
> ./include/linux/bitmap.h:253:17: warning: ‘memcpy’ reading 32 bytes from a region of size 0 [-Wstringop-overread]
> 253 | memcpy(dst, src, len);
> | ^~~~~~~~~~~~~~~~~~~~~
> In function ‘ap_config_store’:
> cc1: note: source object is likely at address zero
I believe that this is a bogus compiler warning. I cannot reproduce it,
fwiw.
gcc: gcc (GCC) 13.2.1 20231205 (Red Hat 13.2.1-6)
binutls binutils-2.40-14.fc39
make W=1 modules
Here is the supposedly offending code.
drivers/s390/crypto/vfio_ap_ops.c:
1670 static void ap_matrix_copy(struct ap_matrix *dst, struct ap_matrix
*src)
1671 {
1672 bitmap_copy(dst->apm, src->apm, AP_DEVICES);
1673 bitmap_copy(dst->aqm, src->aqm, AP_DOMAINS);
1674 bitmap_copy(dst->adm, src->adm, AP_DOMAINS);
1675 }
called from drivers/s390/crypto/vfio_ap_ops.c:
1695 /* Save old state */
1696 ap_matrix_copy(&m_old, &matrix_mdev->matrix);
Definition of struct in drivers/s390/crypto/vfio_ap_private.h:
113 struct ap_matrix_mdev {
114 struct vfio_device vdev;
115 struct list_head node;
116 struct ap_matrix matrix;
117 struct ap_matrix shadow_apcb;
118 struct kvm *kvm;
119 crypto_hook pqap_hook;
120 struct mdev_device *mdev;
121 struct ap_queue_table qtable;
122 struct eventfd_ctx *req_trigger;
123 DECLARE_BITMAP(apm_add, AP_DEVICES);
124 DECLARE_BITMAP(aqm_add, AP_DOMAINS);
125 DECLARE_BITMAP(adm_add, AP_DOMAINS);
126 };
drivers/s390/crypto/vfio_ap_private.h:
76 struct ap_matrix {
77 unsigned long apm_max;
78 DECLARE_BITMAP(apm, AP_DEVICES);
79 unsigned long aqm_max;
80 DECLARE_BITMAP(aqm, AP_DOMAINS);
81 unsigned long adm_max;
82 DECLARE_BITMAP(adm, AP_DOMAINS);
83 };
drivers/s390/crypto/ap_bus.h:
22 #define AP_DEVICES 256 /* Number of AP devices. */
23 #define AP_DOMAINS 256 /* Number of AP domains. */
The source object seems to have a well defined size.
A quick web search seems to indicate gcc throws quite a few
Wstringop-overread warnings for valid code. I suspect this is
another example of that.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 0/5] s390/vfio-ap: ap_config sysfs attribute for mdevctl automation
2024-03-21 14:14 ` Jason J. Herne
@ 2024-03-21 14:41 ` Heiko Carstens
0 siblings, 0 replies; 12+ messages in thread
From: Heiko Carstens @ 2024-03-21 14:41 UTC (permalink / raw)
To: Jason J. Herne
Cc: Anthony Krowiak, linux-s390, linux-kernel, pasic, borntraeger,
agordeev, gor
Hi Jason,
> > In function ‘bitmap_copy’,
> > inlined from ‘ap_matrix_copy’ at drivers/s390/crypto/vfio_ap_ops.c:1674:2,
> > inlined from ‘ap_config_store’ at drivers/s390/crypto/vfio_ap_ops.c:1696:2:
> > ./include/linux/bitmap.h:253:17: warning: ‘memcpy’ reading 32 bytes from a region of size 0 [-Wstringop-overread]
> > 253 | memcpy(dst, src, len);
> > | ^~~~~~~~~~~~~~~~~~~~~
> > In function ‘ap_config_store’:
> > cc1: note: source object is likely at address zero
>
> I believe that this is a bogus compiler warning. I cannot reproduce it,
> fwiw.
>
> gcc: gcc (GCC) 13.2.1 20231205 (Red Hat 13.2.1-6)
> binutls binutils-2.40-14.fc39
...
> A quick web search seems to indicate gcc throws quite a few
> Wstringop-overread warnings for valid code. I suspect this is
> another example of that.
This might be the case, however the code has to compile without
warnings also with plain gcc 13.2.0 (built from source).
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2024-03-21 14:41 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-13 20:58 [PATCH v3 0/5] s390/vfio-ap: ap_config sysfs attribute for mdevctl automation Jason J. Herne
2024-03-13 20:58 ` [PATCH v3 1/5] s390/ap: Externalize AP bus specific bitmap reading function Jason J. Herne
2024-03-13 20:58 ` [PATCH v3 2/5] s390/vfio-ap: Add sysfs attr, ap_config, to export mdev state Jason J. Herne
2024-03-13 20:58 ` [PATCH v3 3/5] s390/vfio-ap: Ignore duplicate link requests in vfio_ap_mdev_link_queue Jason J. Herne
2024-03-13 20:58 ` [PATCH v3 4/5] s390/vfio-ap: Add write support to sysfs attr ap_config Jason J. Herne
2024-03-15 14:45 ` Anthony Krowiak
2024-03-13 20:58 ` [PATCH v3 5/5] docs: Update s390 vfio-ap doc for ap_config sysfs attribute Jason J. Herne
2024-03-15 14:46 ` Anthony Krowiak
2024-03-15 14:48 ` [PATCH v3 0/5] s390/vfio-ap: ap_config sysfs attribute for mdevctl automation Anthony Krowiak
2024-03-19 11:12 ` Heiko Carstens
2024-03-21 14:14 ` Jason J. Herne
2024-03-21 14:41 ` Heiko Carstens
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.