| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 18 Mar 2022 13:54:27 -0400
From: Tony Krowiak <akrowiak@...ux.ibm.com>
To: jjherne@...ux.ibm.com, linux-s390@...r.kernel.org,
linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Cc: freude@...ux.ibm.com, borntraeger@...ibm.com, cohuck@...hat.com,
mjrosato@...ux.ibm.com, pasic@...ux.ibm.com,
alex.williamson@...hat.com, kwankhede@...dia.com,
fiuczy@...ux.ibm.com
Subject: Re: [PATCH v18 12/18] s390/vfio-ap: reset queues after adapter/domain
unassignment
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@...ux.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;
>> }
> ...
>
Powered by blists - more mailing lists