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@xxxxxxxxxxxxx>
---
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.
