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;
