Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <151241e6-3099-4be2-da54-1f0e5cb3a705@linux.ibm.com>
Date:   Mon, 7 Mar 2022 09:10:29 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.0
Subject: Re: [PATCH v18 08/18] s390/vfio-ap: allow assignment of unavailable
 AP queues to mdev device
Content-Language: en-US
To:     Halil Pasic <pasic@linux.ibm.com>
Cc:     jjherne@linux.ibm.com, linux-s390@vger.kernel.org,
        linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        freude@linux.ibm.com, borntraeger@de.ibm.com, cohuck@redhat.com,
        mjrosato@linux.ibm.com, alex.williamson@redhat.com,
        kwankhede@nvidia.com, fiuczy@linux.ibm.com
References: <20220215005040.52697-1-akrowiak@linux.ibm.com>
 <20220215005040.52697-9-akrowiak@linux.ibm.com>
 <97681738-50a1-976d-9f0f-be326eab7202@linux.ibm.com>
 <9ac3908e-06da-6276-d1df-94898918fc5b@linux.ibm.com>
 <20220307142711.5af33ece.pasic@linux.ibm.com>
From:   Tony Krowiak <akrowiak@linux.ibm.com>
In-Reply-To: <20220307142711.5af33ece.pasic@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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.

>
>
>>>> +         * then move on to the next.
>>>> +         */
>>>> +        if (mdev_apm == matrix_mdev->matrix.apm &&
>>>> +            mdev_aqm == matrix_mdev->matrix.aqm)
>>>>                continue;
>>> We may have a problem here. This check seems like it exists to stop
>>> you from
>>> comparing an mdev's apm/aqm with itself. Obviously comparing an mdev's
>>> newly
>>> updated apm/aqm with itself would cause a false positive sharing
>>> check, right?
>>> If this is the case, I think the comment should be changed to reflect
>>> that.
>> You are correct, this check is performed to prevent comparing an mdev to
>> itself, I'll improve the comment.
>>
>>> Aside from the comment, what stops this particular series of if
>>> statements from
>>> allowing us to configure a second mdev with the exact same apm/aqm
>>> values as an
>>> existing mdev? If we do, then this check's continue will short circuit
>>> the rest
>>> of the function thereby allowing that 2nd mdev even though it should be a
>>> sharing violation.
>> I don't see how this is possible.
> I agree with Tony and his explanation.
>
> Furthermore IMHO is relates to the class identity vs equality problem, in
> a sense that identity always implies equality.
>
> Regards,
> Halil