Re: [PATCH v18 08/18] s390/vfio-ap: allow assignment of unavailable AP queues to mdev device

[Tony Krowiak <akrowiak@linux.ibm.com>]

On 3/7/22 12:10, Halil Pasic wrote:
> On Mon, 7 Mar 2022 09:10:29 -0500
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>
>> On 3/7/22 08:27, Halil Pasic wrote:
>>> On Mon, 7 Mar 2022 07:31:21 -0500
>>> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>>>   
>>>> On 3/3/22 10:39, Jason J. Herne wrote:
>>>>> On 2/14/22 19:50, Tony Krowiak wrote:
>>>>>>     /**
>>>>>> - * vfio_ap_mdev_verify_no_sharing - verifies that the AP matrix is
>>>>>> not configured
>>>>>> + * vfio_ap_mdev_verify_no_sharing - verify APQNs are not shared by
>>>>>> matrix mdevs
>>>>>>      *
>>>>>> - * @matrix_mdev: the mediated matrix device
>>>>>> + * @mdev_apm: mask indicating the APIDs of the APQNs to be verified
>>>>>> + * @mdev_aqm: mask indicating the APQIs of the APQNs to be verified
>>>>>>      *
>>>>>> - * Verifies that the APQNs derived from the cross product of the AP
>>>>>> adapter IDs
>>>>>> - * and AP queue indexes comprising the AP matrix are not configured
>>>>>> for another
>>>>>> + * Verifies that each APQN derived from the Cartesian product of a
>>>>>> bitmap of
>>>>>> + * AP adapter IDs and AP queue indexes is not configured for any matrix
>>>>>>      * mediated device. AP queue sharing is not allowed.
>>>>>>      *
>>>>>> - * Return: 0 if the APQNs are not shared; otherwise returns
>>>>>> -EADDRINUSE.
>>>>>> + * Return: 0 if the APQNs are not shared; otherwise return -EADDRINUSE.
>>>>>>      */
>>>>>> -static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev
>>>>>> *matrix_mdev)
>>>>>> +static int vfio_ap_mdev_verify_no_sharing(unsigned long *mdev_apm,
>>>>>> +                      unsigned long *mdev_aqm)
>>>>>>     {
>>>>>> -    struct ap_matrix_mdev *lstdev;
>>>>>> +    struct ap_matrix_mdev *matrix_mdev;
>>>>>>         DECLARE_BITMAP(apm, AP_DEVICES);
>>>>>>         DECLARE_BITMAP(aqm, AP_DOMAINS);
>>>>>>     -    list_for_each_entry(lstdev, &matrix_dev->mdev_list, node) {
>>>>>> -        if (matrix_mdev == lstdev)
>>>>>> +    list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
>>>>>> +        /*
>>>>>> +         * If the input apm and aqm belong to the matrix_mdev's matrix,
>>> How about:
>>>
>>> s/belong to the matrix_mdev's matrix/are fields of the matrix_mdev
>>> object/
>> This is the comment I wrote:
>>
>>           /*
>>            * Comparing an mdev's newly updated apm/aqm with itself would
>>            * result in a false positive when verifying whether any APQNs
>>            * are shared; so, if the input apm and aqm belong to the
>>            * matrix_mdev's matrix, then move on to the next one.
>>            */
>>
>> However, I'd be happy to change it to whatever either of you want.
> What ain't obvious for the comment is that "belong to" actually means
> composition and not association. In other words, there there is no
> pointer/indirection involved, a pointer that would tell us what matrix
> does belong to what matrix_mdev, but rather the matrix is just a part
> of the matrix_mdev object.
>
> I don't like 'false positive' either, and whether the apm/aqm is
> newly updated or not is also redundant and confusing in my opinion. When
> we check because of inuse there is not updated whatever. IMHO the old
> message was better than this one.
>
> Just my opinion, if you two agree, that this is the way to go, I'm fine
> with that.
>
> Regards,
> Halil

Feel free to recommend the verbiage for this comment. I'm not married
to my comments and am open to anything that helps others to
understand what is going on here. It seems obvious to me, but I wrote
the code. Obviously, it is not so obvious based on Jason's comments,
so maybe someone else can compose a better comment.

>
>