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: <9ac3908e-06da-6276-d1df-94898918fc5b@linux.ibm.com>
Date:   Mon, 7 Mar 2022 07:31:21 -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:     jjherne@linux.ibm.com, linux-s390@vger.kernel.org,
        linux-kernel@vger.kernel.org, kvm@vger.kernel.org
Cc:     freude@linux.ibm.com, borntraeger@de.ibm.com, cohuck@redhat.com,
        mjrosato@linux.ibm.com, pasic@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>
From:   Tony Krowiak <akrowiak@linux.ibm.com>
In-Reply-To: <97681738-50a1-976d-9f0f-be326eab7202@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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,
>> +         * 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.

The function above is called from two places: the 
vfio_ap_mdev_validate_masks()
function which is invoked when an adapter or domain is assigned to the 
mdev; and the
vfio_ap_mdev_resource_in_use() function which is a callback registered 
with the
AP bus and is called by the bus when the apmask/aqmask are changed.

In the former case, the addresses passed in are from the apm/aqm fields 
within
the ap_matrix structure. Each ap_matrix structure is a field contained 
within an
ap_matrix_mdev structure, it is not a pointer to storage allocated 
external to
the matrix_mdev, so the apm/aqm addresses passed in from the
vfio_ap_mdev_validate_masks() function will be unique to each 
ap_matrix_mdev
structure.

In the latter case, the addresses are passed in by the AP bus and are 
allocated by the
bus and would definitely not be contained within an ap_matrix_mdev since 
the AP bus
doesn't even have access to that structure.

>
>