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@xxxxxxxxxxxxx>
---
...
+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;
}
...
--
-- Jason J. Herne (jjherne@xxxxxxxxxxxxx)
