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

[Tony Krowiak]

On 1/11/22 16:58, Tony Krowiak wrote:
> 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.

Ignore this response and read the answers to your comments below.

> > 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;