Re: [PATCH v17 08/15] s390/vfio-ap: keep track of active guests

[Tony Krowiak]

On 12/29/21 22:33, Halil Pasic wrote:
> On Thu, 21 Oct 2021 11:23:25 -0400
> Tony Krowiak <akrowiak@xxxxxxxxxxxxx> wrote:
>
> > The vfio_ap device driver registers for notification when the pointer to
> > the KVM object for a guest is set. Let's store the KVM pointer as well as
> > the pointer to the mediated device when the KVM pointer is set.
> [..]
>
>
> > struct ap_matrix_dev {
> >          ...
> >          struct rw_semaphore guests_lock;
> >          struct list_head guests;
> >         ...
> > }
> >
> > The 'guests_lock' field is a r/w semaphore to control access to the
> > 'guests' field. The 'guests' field is a list of ap_guest
> > structures containing the KVM and matrix_mdev pointers for each active
> > guest. An ap_guest structure will be stored into the list whenever the
> > vfio_ap device driver is notified that the KVM pointer has been set and
> > removed when notified that the KVM pointer has been cleared.
> Is this about the field or about the list including all the nodes? This
> reads lie guests_lock only protects the head element, which makes no
> sense to me. Because of how these lists work.

It locks the list, I can rewrite the description.

> The narrowest scope that could make sense is all the list_head stuff
> in the entire list. I.e. one would only need the lock to traverse or
> manipulate the list, while the payload would still be subject to
> the matrix_dev->lock mutex.

The matrix_dev->guests lock is needed whenever the kvm->lock
is needed because the struct ap_guest object is created and the
struct kvm assigned to it when the kvm pointer is set
(vfio_ap_mdev_set_kvm function). So, in order to access the
ap_guest object and retrieve the kvm pointer, we have to ensure
the ap_guest_object is still available. The fact we can get the
kvm pointer from the ap_matrix_mdev object just makes things
more efficient - i.e., we won't have to traverse the list.

Whenever the kvm->lock and matrix_dev->lock mutexes must
be held, the order is:

    matrix_dev->guests_lock
    matrix_dev->guests->kvm->lock
    matrix_dev->lock

There are times where all three locks are not required; for example,
the handle_pqap and vfio_ap_mdev_probe/remove functions only
require the matrix_dev->lock because it does not need to lock kvm.

> [..]
>
> > +struct ap_guest {
> > +	struct kvm *kvm;
> > +	struct list_head node;
> > +};
> > +
> >   /**
> >    * struct ap_matrix_dev - Contains the data for the matrix device.
> >    *
> > @@ -39,6 +44,9 @@
> >    *		single ap_matrix_mdev device. It's quite coarse but we don't
> >    *		expect much contention.
> >    * @vfio_ap_drv: the vfio_ap device driver
> > + * @guests_lock: r/w semaphore for protecting access to @guests
> > + * @guests:	list of guests (struct ap_guest) using AP devices bound to the
> > + *		vfio_ap device driver.
> Please compare the above. Also if it is only about the access to the
> list, then you could drop the lock right after create, and not keep it
> till the very end of vfio_ap_mdev_set_kvm(). Right?

That would be true if it only controlled access to the list, but as I
explained above, that is not its sole purpose.

> In any case I'm skeptical about this whole struct ap_guest business. To
> me, it looks like something that just makes things more obscure and
> complicated without any real benefit.

I'm open to other ideas, but you'll have to come up with a way
to take the kvm->lock before the matrix_mdev->lock in the
vfio_ap_mdev_probe_queue and vfio_ap_mdev_remove_queue
functions where we don't have access to the ap_matrix_mdev
object to which the APQN is assigned and has the pointer to the
kvm object.

In order to retrieve the matrix_mdev, we need the matrix_dev->lock.
In order to hot plug/unplug the queue, we need the kvm->lock.
There's your catch-22 that needs to be solved. This design is my
attempt to solve that.

> Regards,
> Halil
>
> >    */
> >   struct ap_matrix_dev {
> >   	struct device device;
> > @@ -47,6 +55,8 @@ struct ap_matrix_dev {
> >   	struct list_head mdev_list;
> >   	struct mutex lock;
> >   	struct ap_driver  *vfio_ap_drv;
> > +	struct rw_semaphore guests_lock;
> > +	struct list_head guests;
> >   };
> >   
> >   extern struct ap_matrix_dev *matrix_dev;