Re: [PATCH v13 06/15] s390/vfio-ap: allow assignment of unavailable AP queues to mdev device

[Tony Krowiak]

On 1/14/21 8:44 PM, Halil Pasic wrote:
> On Thu, 14 Jan 2021 12:54:39 -0500
> Tony Krowiak <akrowiak@xxxxxxxxxxxxx> wrote:
>
> > > >    /**
> > > >     * vfio_ap_mdev_verify_no_sharing
> > > >     *
> > > > - * 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
> > > > - * mediated device. AP queue sharing is not allowed.
> > > > + * Verifies that each APQN derived from the Cartesian product of the AP adapter
> > > > + * IDs and AP queue indexes comprising the AP matrix are not configured for
> > > > + * another mediated device. AP queue sharing is not allowed.
> > > >     *
> > > > - * @matrix_mdev: the mediated matrix device
> > > > + * @matrix_mdev: the mediated matrix device to which the APQNs being verified
> > > > + *		 are assigned.
> > > > + * @mdev_apm: mask indicating the APIDs of the APQNs to be verified
> > > > + * @mdev_aqm: mask indicating the APQIs of the APQNs to be verified
> > > >     *
> > > > - * Returns 0 if the APQNs are not shared, otherwise; returns -EADDRINUSE.
> > > > + * Returns 0 if the APQNs are not shared, otherwise; returns -EBUSY.
> > > >     */
> > > > -static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
> > > > +static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev,
> > > > +					  unsigned long *mdev_apm,
> > > > +					  unsigned long *mdev_aqm)
> > > >    {
> > > >    	struct ap_matrix_mdev *lstdev;
> > > >    	DECLARE_BITMAP(apm, AP_DEVICES);
> > > > @@ -523,20 +426,31 @@ static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
> > > >    		 * We work on full longs, as we can only exclude the leftover
> > > >    		 * bits in non-inverse order. The leftover is all zeros.
> > > >    		 */
> > > > -		if (!bitmap_and(apm, matrix_mdev->matrix.apm,
> > > > -				lstdev->matrix.apm, AP_DEVICES))
> > > > +		if (!bitmap_and(apm, mdev_apm, lstdev->matrix.apm, AP_DEVICES))
> > > >    			continue;
> > > >    
> > > > -		if (!bitmap_and(aqm, matrix_mdev->matrix.aqm,
> > > > -				lstdev->matrix.aqm, AP_DOMAINS))
> > > > +		if (!bitmap_and(aqm, mdev_aqm, lstdev->matrix.aqm, AP_DOMAINS))
> > > >    			continue;
> > > >    
> > > > -		return -EADDRINUSE;
> > > > +		vfio_ap_mdev_log_sharing_err(dev_name(mdev_dev(lstdev->mdev)),
> > > > +					     apm, aqm);
> > > > +
> > > > +		return -EBUSY;
> > > Why do we change -EADDRINUSE to -EBUSY? This gets bubbled up to
> > > userspace, or? So a tool that checks for the other mdev has it
> > > condition by checking for -EADDRINUSE, would be confused...
> > Back in v8 of the series, Christian suggested the occurrences
> > of -EADDRINUSE should be replaced by the more appropriate
> > -EBUSY (Message ID <d7954c15-b14f-d6e5-0193-aadca61883a8@xxxxxxxxxx>),
> > so I changed it here. It does get bubbled up to userspace, so you make a
> > valid point. I will
> > change it back. I will, however, set the value returned from the
> > __verify_card_reservations() function in ap_bus.c to -EBUSY as
> > suggested by Christian.
> As long as the error code for an ephemeral failure due to can't take a
> lock right now, and the error code for a failure due to a sharing
> conflict are (which most likely requires admin action to be resolved)
> I'm fine.
>
> Choosing EBUSY for sharing conflict, and something else for can't take
> lock for the bus attributes, while choosing EADDRINUSE for sharing
> conflict, and EBUSY for can't take lock in the case of the mdev
> attributes (assign_*; unassign_*) sounds confusing to me, but is still
> better than collating the two conditions. Maybe we can choose EAGAIN
> or EWOULDBLOCK for the can't take the lock right now. I don't know.

I was in the process of creating the change log for v14 of
this patch series and realized I never addressed this.
I think EAGAIN would be a better return code for the
mutex_trylock failures in the mdev assign/unassign
operations.

> I'm open to suggestions. And if Christian wants to change this for
> the already released interfaces, I will have to live with that. But it
> has to be a conscious decision at least.
>
> What I consider tricky about EBUSY, is that according to my intuition,
> in pseudocode, object.operation(argument) returns -EBUSY probably tells
> me that object is busy (i.e. is in the middle of something incompatible
> with performing operation). In our case, it is not the object that is
> busy, but the resource denoted by the argument.
>
> Regards,
> Halil