| Message ID | 20220215005040.52697-13-akrowiak@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390/vfio-ap: dynamic configuration support | expand |
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); } } } > +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? > + 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? > > 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. > 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; > } ...
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; >> } > ... >
snip ... > >> >> >>> + 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. The 'struct kvm *kvm' parameter was added to the signature of the vfio_ap_mdev_unset_kvm function with the following commit: 86956e70761b (s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification) I also noticed the the kernel doc for the vfio_ap_mdev_set_kvm and vfio_ap_mdev_unset_kvm functions still contained a comment that is no longer valid by the following commit: 0cc00c8d4050 (s390/vfio-ap: fix circular lockdep when setting/clearing crypto masks) I pushed a patch to our devel branch that removes the invalid comment from the two functions and removes the 'struct kvm *kvm' parameter from the vfio_ap_mdev_unset_kvm function. That patch will prereq this series. > >> >> >>> 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;
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c index f0fdfbbe2d29..e9f7ec6fc6a5 100644 --- a/drivers/s390/crypto/vfio_ap_ops.c +++ b/drivers/s390/crypto/vfio_ap_ops.c @@ -24,9 +24,10 @@ #define VFIO_AP_MDEV_TYPE_HWVIRT "passthrough" #define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device" -static int vfio_ap_mdev_reset_queues(struct ap_matrix_mdev *matrix_mdev); +static int vfio_ap_mdev_reset_queues(struct ap_queue_table *qtable); static struct vfio_ap_queue *vfio_ap_find_queue(int apqn); static const struct vfio_device_ops vfio_ap_matrix_dev_ops; +static int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q, unsigned int retry); /** * vfio_ap_mdev_get_queue - retrieve a queue with a specific APQN from a @@ -356,6 +357,7 @@ static bool vfio_ap_mdev_filter_matrix(unsigned long *apm, unsigned long *aqm, unsigned long apid, apqi, apqn; DECLARE_BITMAP(shadow_apm, AP_DEVICES); DECLARE_BITMAP(shadow_aqm, AP_DOMAINS); + struct vfio_ap_queue *q; ret = ap_qci(&matrix_dev->info); if (ret) @@ -386,8 +388,8 @@ static bool vfio_ap_mdev_filter_matrix(unsigned long *apm, unsigned long *aqm, * hardware device. */ apqn = AP_MKQID(apid, apqi); - - if (!vfio_ap_mdev_get_queue(matrix_mdev, apqn)) { + q = vfio_ap_mdev_get_queue(matrix_mdev, apqn); + if (!q || q->reset_rc) { clear_bit_inv(apid, matrix_mdev->shadow_apcb.apm); break; @@ -471,12 +473,6 @@ static void vfio_ap_unlink_mdev_fr_queue(struct vfio_ap_queue *q) q->matrix_mdev = NULL; } -static void vfio_ap_mdev_unlink_queue(struct vfio_ap_queue *q) -{ - vfio_ap_unlink_queue_fr_mdev(q); - vfio_ap_unlink_mdev_fr_queue(q); -} - static void vfio_ap_mdev_unlink_fr_queues(struct ap_matrix_mdev *matrix_mdev) { struct vfio_ap_queue *q; @@ -501,7 +497,7 @@ static void vfio_ap_mdev_remove(struct mdev_device *mdev) mutex_lock(&matrix_dev->guests_lock); mutex_lock(&matrix_dev->mdevs_lock); - vfio_ap_mdev_reset_queues(matrix_mdev); + vfio_ap_mdev_reset_queues(&matrix_mdev->qtable); vfio_ap_mdev_unlink_fr_queues(matrix_mdev); list_del(&matrix_mdev->node); mutex_unlock(&matrix_dev->mdevs_lock); @@ -760,17 +756,61 @@ static ssize_t assign_adapter_store(struct device *dev, } static DEVICE_ATTR_WO(assign_adapter); +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); +} + +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) { + vfio_ap_unlink_mdev_fr_queue(q); + hash_del(&q->mdev_qnode); } } @@ -809,13 +849,7 @@ static ssize_t unassign_adapter_store(struct device *dev, } clear_bit_inv((unsigned long)apid, matrix_mdev->matrix.apm); - vfio_ap_mdev_unlink_adapter(matrix_mdev, apid); - - 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); - } - + vfio_ap_mdev_hot_unplug_adapter(matrix_mdev, apid); ret = count; done: vfio_ap_mdev_put_locks(matrix_mdev); @@ -909,16 +943,35 @@ static ssize_t assign_domain_store(struct device *dev, static DEVICE_ATTR_WO(assign_domain); static void vfio_ap_mdev_unlink_domain(struct ap_matrix_mdev *matrix_mdev, - unsigned long apqi) + unsigned long apqi, + struct ap_queue_table *qtable) { unsigned long apid; + + for_each_set_bit_inv(apid, matrix_mdev->matrix.apm, AP_DEVICES) + vfio_ap_unlink_apqn_fr_mdev(matrix_mdev, apid, apqi, qtable); +} + +static void vfio_ap_mdev_hot_unplug_domain(struct ap_matrix_mdev *matrix_mdev, + unsigned long apqi) +{ + int bkt; struct vfio_ap_queue *q; + struct ap_queue_table qtable; - for_each_set_bit_inv(apid, matrix_mdev->matrix.apm, AP_DEVICES) { - q = vfio_ap_mdev_get_queue(matrix_mdev, AP_MKQID(apid, apqi)); + hash_init(qtable.queues); + vfio_ap_mdev_unlink_domain(matrix_mdev, apqi, &qtable); - if (q) - vfio_ap_mdev_unlink_queue(q); + if (test_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm)) { + clear_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm); + vfio_ap_mdev_hotplug_apcb(matrix_mdev); + } + + vfio_ap_mdev_reset_queues(&qtable); + + hash_for_each(qtable.queues, bkt, q, mdev_qnode) { + vfio_ap_unlink_mdev_fr_queue(q); + hash_del(&q->mdev_qnode); } } @@ -957,13 +1010,7 @@ static ssize_t unassign_domain_store(struct device *dev, } clear_bit_inv((unsigned long)apqi, matrix_mdev->matrix.aqm); - vfio_ap_mdev_unlink_domain(matrix_mdev, apqi); - - if (test_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm)) { - clear_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm); - vfio_ap_mdev_hotplug_apcb(matrix_mdev); - } - + vfio_ap_mdev_hot_unplug_domain(matrix_mdev, apqi); ret = count; done: vfio_ap_mdev_put_locks(matrix_mdev); @@ -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; 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; 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; } @@ -1362,7 +1419,8 @@ static int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q, unsigned int retry) msleep(20); status = ap_tapq(q->apqn, NULL); } - WARN_ON_ONCE(retry2 <= 0); + WARN_ONCE(retry2 <= 0, "unable to verify reset of queue %02x.%04x", + AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn)); free_resources: vfio_ap_free_aqic_resources(q); @@ -1370,12 +1428,12 @@ static int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q, unsigned int retry) return ret; } -static int vfio_ap_mdev_reset_queues(struct ap_matrix_mdev *matrix_mdev) +static int vfio_ap_mdev_reset_queues(struct ap_queue_table *qtable) { - int ret, bkt, rc = 0; + int rc = 0, ret, bkt; struct vfio_ap_queue *q; - hash_for_each(matrix_mdev->qtable.queues, bkt, q, mdev_qnode) { + hash_for_each(qtable->queues, bkt, q, mdev_qnode) { ret = vfio_ap_mdev_reset_queue(q, 1); /* * Regardless whether a queue turns out to be busy, or @@ -1463,7 +1521,7 @@ static ssize_t vfio_ap_mdev_ioctl(struct vfio_device *vdev, ret = vfio_ap_mdev_get_device_info(arg); break; case VFIO_DEVICE_RESET: - ret = vfio_ap_mdev_reset_queues(matrix_mdev); + ret = vfio_ap_mdev_reset_queues(&matrix_mdev->qtable); break; default: ret = -EOPNOTSUPP; diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h index 7e82a72d2083..a3811cd9852d 100644 --- a/drivers/s390/crypto/vfio_ap_private.h +++ b/drivers/s390/crypto/vfio_ap_private.h @@ -128,6 +128,7 @@ struct ap_matrix_mdev { * @apqn: the APQN of the AP queue device * @saved_isc: the guest ISC registered with the GIB interface * @mdev_qnode: allows the vfio_ap_queue struct to be added to a hashtable + * @reset_rc: the status response code from the last reset of the queue */ struct vfio_ap_queue { struct ap_matrix_mdev *matrix_mdev; @@ -136,6 +137,7 @@ struct vfio_ap_queue { #define VFIO_AP_ISC_INVALID 0xff unsigned char saved_isc; struct hlist_node mdev_qnode; + unsigned int reset_rc; }; int vfio_ap_mdev_register(void);
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> --- drivers/s390/crypto/vfio_ap_ops.c | 146 ++++++++++++++++++-------- drivers/s390/crypto/vfio_ap_private.h | 2 + 2 files changed, 104 insertions(+), 44 deletions(-)