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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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);
> };
>
> /**
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 <[email protected]>
>> ---
>> 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.