On Wed, 13 Jan 2021 16:41:27 -0500
Tony Krowiak <akrowiak@xxxxxxxxxxxxx> wrote:
On 1/11/21 2:17 PM, Halil Pasic wrote:[..]
On Tue, 22 Dec 2020 20:15:56 -0500
Tony Krowiak <akrowiak@xxxxxxxxxxxxx> wrote:
Let's create links between each queue device bound to the vfio_ap deviceReviewed-by: Halil Pasic <pasic@xxxxxxxxxxxxx>
driver and the matrix mdev to which the queue's APQN is assigned. The idea
is to facilitate efficient retrieval of the objects representing the queue
devices and matrix mdevs as well as to verify that a queue assigned to
a matrix mdev is bound to the driver.
The links will be created as follows:
* When the queue device is probed, if its APQN is assigned to a matrix
mdev, the structures representing the queue device and the matrix mdev
will be linked.
* When an adapter or domain is assigned to a matrix mdev, for each new
APQN assigned that references a queue device bound to the vfio_ap
device driver, the structures representing the queue device and the
matrix mdev will be linked.
The links will be removed as follows:
* When the queue device is removed, if its APQN is assigned to a matrix
mdev, the structures representing the queue device and the matrix mdev
will be unlinked.
* When an adapter or domain is unassigned from a matrix mdev, for each
APQN unassigned that references a queue device bound to the vfio_ap
device driver, the structures representing the queue device and the
matrix mdev will be unlinked.
Signed-off-by: Tony Krowiak <akrowiak@xxxxxxxxxxxxx>
I was asking out of curiosity, not because I want it changed. I wasWe did not need the critical section before this patch because+Does the critical section have to include more than just
int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
{
struct vfio_ap_queue *q;
@@ -1324,9 +1404,13 @@ int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
q = kzalloc(sizeof(*q), GFP_KERNEL);
if (!q)
return -ENOMEM;
+ mutex_lock(&matrix_dev->lock);
dev_set_drvdata(&apdev->device, q);
q->apqn = to_ap_queue(&apdev->device)->qid;
q->saved_isc = VFIO_AP_ISC_INVALID;
+ vfio_ap_queue_link_mdev(q);
+ mutex_unlock(&matrix_dev->lock);
+
vfio_ap_queue_link_mdev()? Did we need the critical section
before this patch?
the only function that retrieved the vfio_ap_queue via the queue
device's drvdata was the remove callback. I included the initialization
of the vfio_ap_queue object under lock because the
vfio_ap_find_queue() function retrieves the vfio_ap_queue object from
the queue device's drvdata so it might be advantageous to initialize
it under the mdev lock. On the other hand, I can't come up with a good
argument to change this.
also wondering if somebody could see a partially initialized device:
we even first call dev_set_drvdata() and only then finish the
initialization. Before 's390/vfio-ap: use new AP bus interface to search
for queue devices', which is the previous patch, we had the klist code
in between, which uses spinlocks, which I think ensure, that all
effects of probe are seen when we get the queue from
vfio_ap_find_queue(). But with patch 4 in place that is not the case any
more. Or am I wrong?
Regards,
Halil