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

[Tony Krowiak]

On 3/7/22 08:27, Halil Pasic wrote:
> 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/

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