Re: [PATCH v11 08/14] s390/vfio-ap: hot plug/unplug queues on bind/unbind of queue device

[Tony Krowiak <akrowiak@linux.ibm.com>]

On 11/4/20 7:52 AM, Halil Pasic wrote:
> On Tue, 3 Nov 2020 17:49:21 -0500
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>
>>>>    
>>>> +void vfio_ap_mdev_hot_unplug_queue(struct vfio_ap_queue *q)
>>>> +{
>>>> +	unsigned long apid = AP_QID_CARD(q->apqn);
>>>> +
>>>> +	if ((q->matrix_mdev == NULL) || !vfio_ap_mdev_has_crycb(q->matrix_mdev))
>>>> +		return;
>>>> +
>>>> +	/*
>>>> +	 * If the APID is assigned to the guest, then let's
>>>> +	 * go ahead and unplug the adapter since the
>>>> +	 * architecture does not provide a means to unplug
>>>> +	 * an individual queue.
>>>> +	 */
>>>> +	if (test_bit_inv(apid, q->matrix_mdev->shadow_apcb.apm)) {
>>>> +		clear_bit_inv(apid, q->matrix_mdev->shadow_apcb.apm);
>>> Shouldn't we check aqm as well? I mean it may be clear at this point
>>> bacause of info->aqm. If the bit is clear, we don't have to remove
>>> the apm bit.
>> The rule we agreed upon is that if a queue is removed, we unplug
>> the card because we can't unplug an individual queue, so this code
>> is consistent with the stated rule.
> All I'm asking for is to verify that the queue is actually plugged. The
> queue is actually plugged iff
> test_bit_inv(apid, q->matrix_mdev->shadow_apcb.apm) && test_bit_inv(apqi,
> q->matrix_mdev->shadow_apcb.aqm).
>
> There is no point in unplugging the whole card, if the queue removed is
> unplugged in the first place.

No problem, I can make that change.

>
>> Typically, a queue is unplugged
>> because the adapter has been deconfigured or is broken which means
>> that all queues for that adapter will be removed in succession. On the
>> other hand, that situation would be handled when the last queue is
>> removed if we check the AQM, so I'm not adverse to making that
>> check if you insist.
> I don't agree. Let's detail your scenario. We have a nicely
> operating card which is as a whole passed trough to our guest. It
> goes broken, and the ap bus decides to deconstruct the queues.
> Already the first queue removed would unplug the the card, because
> both the apm and the aqm bits are set at this point. Subsequent removals
> then see that the apm bit is removed. Actually IMHO everything works
> like without the extra check on aqm (in this scenario).
>
> Would make reasoning about the code much easier to me, so sorry I do
> insist.

As you said, it works as-is in the scenario you pointed out:)
Whether it makes it any easier to understand the code is in
the eyes of the beholder (for example, I disagree),
but I'm willing to make the change, it's not a big deal.

>
>> Of course, if the queue is manually unbound from
>> the vfio driver, what you are asking for makes sense I suppose. I'll have
>> to think about this one some more, but feel free to respond to this.
> I'm not sure the situation where the queues ->mdev_matrix pointer is set
> but the apqi is not in the shadow_apcb can actually happen (races not
> considered).

Of course it can, for example:

1. No queues bound to vfio driver

2. APQN 04.0004 assigned to matrix mdev

3. Guest started:
     a. No bits set in shadow_apcb because no queues are bound to vfio

4. queue device 04.0004 is bound to the driver
     a. q->matrix_mdev is set because 04.0004 is assigned to matrix mdev
     b. apqi 0004 is not in shadow_apcb (see 3a.)


> But I'm sure the code is suggesting it can, because
> vfio_ap_mdev_filter_guest_matrix() has a third parameter called filter_apid,
> which governs whether the apm or the aqm bit should be removed. And
> vfio_ap_mdev_filter_guest_matrix() does get called with filter_apid=false in
> assign_domain_store() and I don't see subsequent unlink operations that would
> severe q->mdev_matrix.

I think you may be conflating two different things. The q in q->matrix_mdev
represents a queue device bound to the driver. The link to matrix_mdev
indicates the APQN of the queue device is assigned to the matrix_mdev.
When a new domain is assigned to matrix_mdev, we know that
all APQNS currently assigned to the shadow_apcb  are bound to the vfio 
driver
because of previous filtering, so we are only concerned with those APQNs
with the APQI of the new domain being assigned.

1. Queues bound to vfio_ap:
     04.0004
     04.0047
2. APQNs assigned to matrix_mdev:
     04.0004
     04.0047
3. shadow_apcb:
     04.0004
     04.0047
4. Assign domain 0054 to matrix_mdev
5. APQI 0054 gets filtered because 04.0054 not bound to vfio_ap
6. no change to shadow_apcb:
     04.0004
     04.0047

Or:

1. Queues bound to vfio_ap:
     04.0004
     04.0047
     04.0054
2. APQNs assigned to matrix_mdev:
     04.0004
     04.0047
3. shadow_apcb:
     04.0004
     04.0047
4. Assign domain 0054 to matrix_mdev
5. APQNs assigned to matrix_mdev
     04.0004
     04.0047
     04.0054
5. APQI 0054 does not get filtered because 04.0054 is bound to vfio_ap
6. shadow_apcb after filtering:
     04.0004
     04.0047
     04.0054

I'm not sure why you are bringing up unlinking in the context of assigning
a new domain. Unlinking only occurs when an APID or APQI is unassigned.

>
> Another case where the aqm may get filtered in
> vfio_ap_mdev_filter_guest_matrix() is the info->aqm bit not set, as I've
> mentioned in my previous mail. If that can not happen, we should turn
> that into an assert.

In an earlier email of yours, you brought up the scenario whereby
a queue is probed not because of a change in the QCI info,
but because an unbound queue is bound; for instance manually.
I made a change to account for that so consider the following
scenario:

1. APQI 0004 removed from info->aqm
2. AP bus notifies vfio_ap that AP configuration has changed
3. vfio_ap removes APQI 0004 from shadow_apcb
4. Userspace binds queue 04.0004 to vfio_ap
5. Filtering code filters 0004 because it has been removed
     from info->aqm
6. AP bus notifies vfio_ap scan is over

>
> Actually if you are convinced that apqi bit is always set in the
> q->matrix_mdev->shadow_apcb.aqm, I would agree to turning that into an
> assertion instead of condition. Then if not completely convinced, I
> could at least try to trigger the assert :).
>
> Regards,
> Halil