Messages in this thread | | | Date | Wed, 13 Mar 2024 14:00:20 -0400 | Subject | Re: [PATCH v2 4/5] s390/vfio-ap: Add write support to sysfs attr ap_config | From | "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.
| |