On Mon, 7 Mar 2022 07:31:21 -0500 Tony Krowiak <akrowiak@xxxxxxxxxxxxx> 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/ > >> + * 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