Re: [PATCH v18 12/18] s390/vfio-ap: reset queues after adapter/domain unassignment

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

On 3/15/22 10:13, Jason J. Herne wrote:
> On 2/14/22 19:50, Tony Krowiak wrote:
>> When an adapter or domain is unassigned from an mdev providing the AP
>> configuration to a running KVM guest, one or more of the guest's 
>> queues may
>> get dynamically removed. Since the removed queues could get 
>> re-assigned to
>> another mdev, they need to be reset. So, when an adapter or domain is
>> unassigned from the mdev, the queues that are removed from the guest's
>> AP configuration will be reset.
>>
>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>> ---
> ...
>>   +static void vfio_ap_unlink_apqn_fr_mdev(struct ap_matrix_mdev 
>> *matrix_mdev,
>> +                    unsigned long apid, unsigned long apqi,
>> +                    struct ap_queue_table *qtable)
>> +{
>> +    struct vfio_ap_queue *q;
>> +
>> +    q = vfio_ap_mdev_get_queue(matrix_mdev, AP_MKQID(apid, apqi));
>> +    /* If the queue is assigned to the matrix mdev, unlink it. */
>> +    if (q)
>> +        vfio_ap_unlink_queue_fr_mdev(q);
>> +
>> +    /* If the queue is assigned to the APCB, store it in @qtable. */
>> +    if (test_bit_inv(apid, matrix_mdev->shadow_apcb.apm) &&
>> +        test_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm))
>> +        hash_add(qtable->queues, &q->mdev_qnode, q->apqn);
>> +}
>> +
>> +/**
>> + * vfio_ap_mdev_unlink_adapter - unlink all queues associated with 
>> unassigned
>> + *                 adapter from the matrix mdev to which the
>> + *                 adapter was assigned.
>> + * @matrix_mdev: the matrix mediated device to which the adapter was 
>> assigned.
>> + * @apid: the APID of the unassigned adapter.
>> + * @qtable: table for storing queues associated with unassigned 
>> adapter.
>> + */
>>   static void vfio_ap_mdev_unlink_adapter(struct ap_matrix_mdev 
>> *matrix_mdev,
>> -                    unsigned long apid)
>> +                    unsigned long apid,
>> +                    struct ap_queue_table *qtable)
>>   {
>>       unsigned long apqi;
>> +
>> +    for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm, AP_DOMAINS)
>> +        vfio_ap_unlink_apqn_fr_mdev(matrix_mdev, apid, apqi, qtable);
>> +}
>
> Here is an alternate version of the above two functions that stops the
> profileration of the qtables variable into vfio_ap_unlink_apqn_fr_mdev.
> It may seem like a change with no benefit, but it simplifies things a
> bit and avoids the reader from having to sink three functions deep to
> find out where qtables is used. This is 100% untested.
>
>
> static bool vfio_ap_unlink_apqn_fr_mdev(struct ap_matrix_mdev 
> *matrix_mdev,
>                     unsigned long apid, unsigned long apqi)
> {
>     struct vfio_ap_queue *q;
>
>     q = vfio_ap_mdev_get_queue(matrix_mdev, AP_MKQID(apid, apqi));
>     /* If the queue is assigned to the matrix mdev, unlink it. */
>     if (q) {
>         vfio_ap_unlink_queue_fr_mdev(q);
>         return true;
>     }
>     return false;
> }
>
> static void vfio_ap_mdev_unlink_adapter(struct ap_matrix_mdev 
> *matrix_mdev,
>                     unsigned long apid,
>                     struct ap_queue_table *qtable)
> {
>     unsigned long apqi;
>     bool unlinked;
>
>     for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm, AP_DOMAINS) {
>         unlinked = vfio_ap_unlink_apqn_fr_mdev(matrix_mdev, apid, 
> apqi, qtable);
>
>         if (unlinked && qtable) {
>             if (test_bit_inv(apid, matrix_mdev->shadow_apcb.apm) &&
>                 test_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm))
>                 hash_add(qtable->queues, &q->mdev_qnode,
>                      q->apqn);
>         }
>     }
> }

This code didn't compile because in the function immediately above,
the variable q is not defined nor is it initialized with a value. What I did
to fix that is return  the vfio_ap_queue pointer from the
vfio_ap_unlink_apqn_fr_mdev function and checked the return value
for NULL instead of the boolean:

vfio_ap_queue *q;
...
q = vfio_ap_unlink_apqn_fr_mdev(matrix_mdev, apid, apqi, qtable);
...
if (q && qtable)
...

>
>
>> +static void vfio_ap_mdev_hot_unplug_adapter(struct ap_matrix_mdev 
>> *matrix_mdev,
>> +                        unsigned long apid)
>> +{
>> +    int bkt;
>>       struct vfio_ap_queue *q;
>> +    struct ap_queue_table qtable;
>> +    hash_init(qtable.queues);
>> +    vfio_ap_mdev_unlink_adapter(matrix_mdev, apid, &qtable);
>> +
>> +    if (test_bit_inv(apid, matrix_mdev->shadow_apcb.apm)) {
>> +        clear_bit_inv(apid, matrix_mdev->shadow_apcb.apm);
>> +        vfio_ap_mdev_hotplug_apcb(matrix_mdev);
>> +    }
>>   -    for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm, AP_DOMAINS) {
>> -        q = vfio_ap_mdev_get_queue(matrix_mdev, AP_MKQID(apid, apqi));
>> +    vfio_ap_mdev_reset_queues(&qtable);
>>   -        if (q)
>> -            vfio_ap_mdev_unlink_queue(q);
>> +    hash_for_each(qtable.queues, bkt, q, mdev_qnode) {
>
> This comment applies to all instances of btk: What is btk? Can we come
> up with a more descriptive name?

If you look at the hash_for_each macro, you will see that I used the exact
same variable name as the macro does to indicate it is a bucket loop
cursor. Since that is a long name I'll go with loop_cursor.

>
>
>> +        vfio_ap_unlink_mdev_fr_queue(q);
>> +        hash_del(&q->mdev_qnode);
>>       }
>>   }
> ...
>> @@ -1273,9 +1320,9 @@ static void vfio_ap_mdev_unset_kvm(struct 
>> ap_matrix_mdev *matrix_mdev,
>>           mutex_lock(&kvm->lock);
>>           mutex_lock(&matrix_dev->mdevs_lock);
>>   -        kvm_arch_crypto_clear_masks(kvm);
>> -        vfio_ap_mdev_reset_queues(matrix_mdev);
>> -        kvm_put_kvm(kvm);
>> +        kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
>> +        vfio_ap_mdev_reset_queues(&matrix_mdev->qtable);
>> +        kvm_put_kvm(matrix_mdev->kvm);
>>           matrix_mdev->kvm = NULL;
>
> I understand changing the call to vfio_ap_mdev_reset_queues, but why 
> are we changing the
> kvm pointer on the surrounding lines?

In reality, both pointers are one in the same given the two callers pass
matrix_mdev->kvm into the function. I'm not sure why that is the case,
it is probably a remnant from the commits that fixed the lockdep splat;
however, there is no reason other than I've gotten used to retrieving the
KVM pointer from the ap_matrix_mdev structure. In reality, there is no
reason to pass a 'struct kvm *kvm' into this function, so I'm going to
look into that and adjust accordingly.

>
>
>> mutex_unlock(&matrix_dev->mdevs_lock);
>> @@ -1328,14 +1375,17 @@ static int vfio_ap_mdev_reset_queue(struct 
>> vfio_ap_queue *q, unsigned int retry)
>>         if (!q)
>>           return 0;
>> +    q->reset_rc = 0;
>
> This line seems unnecessary. You set q->reset_rc in every single case 
> below, so this 0
> will always get overwritten.

Right you are. It is also unnecessary to set q->reset_rc every case when
it can be set once right after the call to ap_zapq.

>
>
>>   retry_zapq:
>>       status = ap_zapq(q->apqn);
>>       switch (status.response_code) {
>>       case AP_RESPONSE_NORMAL:
>>           ret = 0;
>> +        q->reset_rc = status.response_code;
>>           break;
>>       case AP_RESPONSE_RESET_IN_PROGRESS:
>> +        q->reset_rc = status.response_code;
>>           if (retry--) {
>>               msleep(20);
>>               goto retry_zapq;
>> @@ -1345,13 +1395,20 @@ static int vfio_ap_mdev_reset_queue(struct 
>> vfio_ap_queue *q, unsigned int retry)
>>       case AP_RESPONSE_Q_NOT_AVAIL:
>>       case AP_RESPONSE_DECONFIGURED:
>>       case AP_RESPONSE_CHECKSTOPPED:
>> -        WARN_ON_ONCE(status.irq_enabled);
>> +        WARN_ONCE(status.irq_enabled,
>> +              "PQAP/ZAPQ for %02x.%04x failed with rc=%u while IRQ 
>> enabled",
>> +              AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
>> +              status.response_code);
>> +        q->reset_rc = status.response_code;
>>           ret = -EBUSY;
>>           goto free_resources;
>>       default:
>>           /* things are really broken, give up */
>> -        WARN(true, "PQAP/ZAPQ completed with invalid rc (%x)\n",
>> +        WARN(true,
>> +             "PQAP/ZAPQ for %02x.%04x failed with invalid rc=%u\n",
>> +             AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
>>                status.response_code);
>> +        q->reset_rc = status.response_code;
>>           return -EIO;
>>       }
> ...
>