Messages in this thread | | | Date | Mon, 7 Mar 2022 07:31:21 -0500 | Subject | Re: [PATCH v18 08/18] s390/vfio-ap: allow assignment of unavailable AP queues to mdev device | From | Tony Krowiak <> |
| |
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.
> >
| |