Received-SPF: pass (google.com: domain of linux-kernel+bounces-102325-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <37d91598-0198-49e1-1008-6028ad78c1e5@linux.ibm.com>
Date: Wed, 13 Mar 2024 14:00:20 -0400
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH v2 4/5] s390/vfio-ap: Add write support to sysfs attr
 ap_config
To: Anthony Krowiak <akrowiak@linux.ibm.com>, linux-s390@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, pasic@linux.ibm.com, borntraeger@de.ibm.com,
        agordeev@linux.ibm.com, gor@linux.ibm.com
References: <20240306140843.10782-1-jjherne@linux.ibm.com>
 <20240306140843.10782-5-jjherne@linux.ibm.com>
 <0389f521-e9e2-43d4-8d78-87695853a536@linux.ibm.com>
Content-Language: en-US
From: "Jason J. Herne" <jjherne@linux.ibm.com>
In-Reply-To: <0389f521-e9e2-43d4-8d78-87695853a536@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

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.