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@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);
}
}
}
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;
}
...
