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

Jason J. Herne
2024-03-13T18:00:28Z

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.