| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <37d91598-0198-49e1-1008-6028ad78c1e5@linux.ibm.com>
Date: Wed, 13 Mar 2024 14:00:20 -0400
From: "Jason J. Herne" <jjherne@...ux.ibm.com>
To: Anthony Krowiak <akrowiak@...ux.ibm.com>, linux-s390@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, pasic@...ux.ibm.com, borntraeger@...ibm.com,
agordeev@...ux.ibm.com, gor@...ux.ibm.com
Subject: Re: [PATCH v2 4/5] s390/vfio-ap: Add write support to sysfs attr
ap_config
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@...ux.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.
Powered by blists - more mailing lists