Re: [PATCH v7 09/15] s390/vfio-ap: Add method to set a new guest AP configuration
From: Anthony Krowiak
Date: Wed Oct 07 2026 - 09:50:38 EST
On 8/19/26 9:25 AM, Jason J. Herne wrote:
On 8/7/26 6:18 PM, Anthony Krowiak wrote:
Adds a new vfio_ap_set_new_config function to set a guest's AP
configuration. This is needed in order to set the state of the mdev when
it is migrated from a remote host system during the RESUMING phase.
Key changes:
* Refactored code from the ap_config_store function - handles changes to
the sysfs ap_config attribute - into a new, non-static function which
is callable from the ap_config_store function as well as the live guest
migration code.
Signed-off-by: Anthony Krowiak <akrowiak@xxxxxxxxxxxxx>
---
drivers/s390/crypto/vfio_ap_ops.c | 221 ++++++++++++++------------
drivers/s390/crypto/vfio_ap_private.h | 61 +++++++
2 files changed, 184 insertions(+), 98 deletions(-)
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index d05372b50d2f..0f33f5189153 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -81,53 +81,6 @@ static inline void release_update_locks_for_kvm(struct kvm *kvm)
mutex_unlock(&matrix_dev->guests_lock);
}
-/**
- * get_update_locks_for_mdev: Acquire the locks required to dynamically update a
- * KVM guest's APCB in the proper order.
- *
- * @matrix_mdev: a pointer to a struct ap_matrix_mdev object containing the AP
- * configuration data to use to update a KVM guest's APCB.
- *
- * The proper locking order is:
- * 1. matrix_dev->guests_lock: required to use the KVM pointer to update a KVM
- * guest's APCB.
- * 2. matrix_mdev->kvm->lock: required to update a guest's APCB
- * 3. matrix_dev->mdevs_lock: required to access data stored in a matrix_mdev
- *
- * Note: If @matrix_mdev is NULL or is not attached to a KVM guest, the KVM
- * lock will not be taken.
- */
-static inline void get_update_locks_for_mdev(struct ap_matrix_mdev *matrix_mdev)
-{
- mutex_lock(&matrix_dev->guests_lock);
- if (matrix_mdev && matrix_mdev->kvm)
- mutex_lock(&matrix_mdev->kvm->lock);
- mutex_lock(&matrix_dev->mdevs_lock);
-}
-
-/**
- * release_update_locks_for_mdev: Release the locks used to dynamically update a
- * KVM guest's APCB in the proper order.
- *
- * @matrix_mdev: a pointer to a struct ap_matrix_mdev object containing the AP
- * configuration data to use to update a KVM guest's APCB.
- *
- * The proper unlocking order is:
- * 1. matrix_dev->mdevs_lock
- * 2. matrix_mdev->kvm->lock
- * 3. matrix_dev->guests_lock
- *
- * Note: If @matrix_mdev is NULL or is not attached to a KVM guest, the KVM
- * lock will not be released.
- */
-static inline void release_update_locks_for_mdev(struct ap_matrix_mdev *matrix_mdev)
-{
- mutex_unlock(&matrix_dev->mdevs_lock);
- if (matrix_mdev && matrix_mdev->kvm)
- mutex_unlock(&matrix_mdev->kvm->lock);
- mutex_unlock(&matrix_dev->guests_lock);
-}
-
/**
* get_update_locks_by_apqn: Find the mdev to which an APQN is assigned and
* acquire the locks required to update the APCB of
@@ -642,8 +595,7 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
return 0;
}
-static void vfio_ap_matrix_init(struct ap_config_info *info,
- struct ap_matrix *matrix)
+void vfio_ap_matrix_init(struct ap_config_info *info, struct ap_matrix *matrix)
{
This is unrelated cleaup work. Good change :) But it should be a separate patch. Same for several other changes in this patch.
This was made non-static because it is called in patch 10/15; in other words you are right, it doesn't
belong here. This and the other unrelated patches will be done via a new patch which will
precede this one. In fact, all of the changes making static functions non-static in this patch will
be done in the new patch.
matrix->apm_max = info->apxa ? info->na : 63;
matrix->aqm_max = info->apxa ? info->nd : 15;
@@ -1018,13 +970,12 @@ static void vfio_ap_mdev_link_adapter(struct ap_matrix_mdev *matrix_mdev,
unsigned long apqi;
for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm, AP_DOMAINS)
- vfio_ap_mdev_link_apqn(matrix_mdev,
- AP_MKQID(apid, apqi));
+ vfio_ap_mdev_link_apqn(matrix_mdev, AP_MKQID(apid, apqi));
}
-static void collect_queues_to_reset(struct ap_matrix_mdev *matrix_mdev,
- unsigned long apid,
- struct list_head *qlist)
+static void collect_queues_by_apid(struct ap_matrix_mdev *matrix_mdev,
+ unsigned long apid,
+ struct list_head *qlist)
unrelated
This rename is not only unrelated, it is unnecessary. The original name will be
restored.
{
struct vfio_ap_queue *q;
unsigned long apqi;
@@ -1042,7 +993,7 @@ static void reset_queues_for_apid(struct ap_matrix_mdev *matrix_mdev,
struct list_head qlist;
INIT_LIST_HEAD(&qlist);
- collect_queues_to_reset(matrix_mdev, apid, &qlist);
+ collect_queues_by_apid(matrix_mdev, apid, &qlist);
vfio_ap_mdev_reset_qlist(&qlist);
}
@@ -1058,7 +1009,7 @@ static int reset_queues_for_apids(struct ap_matrix_mdev *matrix_mdev,
INIT_LIST_HEAD(&qlist);
for_each_set_bit_inv(apid, apm_reset, AP_DEVICES)
- collect_queues_to_reset(matrix_mdev, apid, &qlist);
+ collect_queues_by_apid(matrix_mdev, apid, &qlist);
unrelated
return vfio_ap_mdev_reset_qlist(&qlist);
}
@@ -1729,54 +1680,100 @@ static void ap_matrix_copy(struct ap_matrix *dst, struct ap_matrix *src)
bitmap_copy(dst->adm, src->adm, AP_DOMAINS);
}
-static ssize_t ap_config_store(struct device *dev, struct device_attribute *attr,
- const char *buf, size_t count)
+static void get_removed_matrixes(struct ap_matrix *m_removed,
+ struct ap_matrix *m_old,
+ struct ap_matrix *m_new)
{
- struct ap_matrix_mdev *matrix_mdev = dev_get_drvdata(dev);
- struct ap_matrix m_new, m_old, m_added, m_removed;
+ bitmap_andnot(m_removed->apm, m_old->apm, m_new->apm, AP_DEVICES);
+ bitmap_andnot(m_removed->aqm, m_old->aqm, m_new->aqm, AP_DOMAINS);
+ bitmap_andnot(m_removed->adm, m_old->adm, m_new->adm, AP_DOMAINS);
+}
+
+static void get_added_matrixes(struct ap_matrix *m_added,
+ struct ap_matrix *m_old,
+ struct ap_matrix *m_new)
+{
+ bitmap_andnot(m_added->apm, m_new->apm, m_old->apm, AP_DEVICES);
+ bitmap_andnot(m_added->aqm, m_new->aqm, m_old->aqm, AP_DOMAINS);
+ bitmap_andnot(m_added->adm, m_new->adm, m_old->adm, AP_DOMAINS);
+}
Unless you plan on reusing these functions elsewhere, I'd argue they're cleaner and easier to read when left in their calling function.
This is a stylistic preference, not a correctness concern; my preference is to keep it.
The primary reason I created these functions
is because I really don't like overly long functions that I have to page through.
vfio_ap_set_new_guest_config() is quite a verbose function, so I shortened it with
these functions. I don't think the code is any easier to read or understand if
left in-line. In my opinion, since the function names are self-documenting, it
makes the code more understandable at the point they are called. Looking
at bitmap_andnot and trying to figure out what that does is not cleaner
and clearer in my opinion. Of course, one could argue a doc block prior to
those three lines would do the same thing, but I think if one is reading the
code, they can always go to these functions to see how the sausage is made.
I'm sorry for the long dissertation, but I thought I'd state my reasoning for
keeping them.
+static int validate_new_state(struct ap_matrix_mdev *matrix_mdev)
+{
+ int rc;
+
+ /* Ensure new state is valid, else undo new state */
+ rc = vfio_ap_mdev_validate_masks(matrix_mdev);
+ if (rc)
+ return rc;
+
+ rc = ap_matrix_overflow_check(matrix_mdev);
+ if (rc)
+ return rc;
+
+ return 0;
+}
+
+static void link_new_queues(struct ap_matrix_mdev *matrix_mdev,
+ struct ap_matrix *m_added)
+{
+ unsigned long apid, apqi;
+
+ for_each_set_bit_inv(apid, m_added->apm, AP_DEVICES)
+ vfio_ap_mdev_link_adapter(matrix_mdev, apid);
+
+ for_each_set_bit_inv(apqi, m_added->aqm, AP_DOMAINS)
+ vfio_ap_mdev_link_domain(matrix_mdev, apqi);
+}
+
+/**
+ * vfio_ap_set_new_guest_config:
+ *
+ * Set a new AP configuration for a guest.
+ *
+ * @matrix_mdev: Object used to maintain the AP configuration for a guest
+ * @m_new: Object used to set the new AP configuration
+ *
+ * Returns: zero (0) if the new AP configuration is successfully set; otherwise,
+ * returns an error:
+ *
+ * ~ EADDRNOTAVAIL One or more APQNs are reserved for host use
+ * ~ EADDRINUSE One or more APQNs are assigned to another mdev
+ * ~ ENODEV An adapter, domain or control domain in the new
+ * AP configuration exceeds the max architected value
+ */
+int vfio_ap_set_new_guest_config(struct ap_matrix_mdev *matrix_mdev,
+ struct ap_matrix *m_new)
+{
+ struct ap_matrix m_old, m_old_shadow, m_added, m_removed;
DECLARE_BITMAP(apm_filtered, AP_DEVICES);
- unsigned long newbit;
- char *newbuf, *rest;
- int rc = count;
bool do_update;
+ int rc;
- newbuf = kstrndup(buf, AP_CONFIG_STRLEN, GFP_KERNEL);
- if (!newbuf)
- return -ENOMEM;
- rest = newbuf;
+ lockdep_assert_held(&ap_attr_mutex);
+ assert_has_update_locks_for_mdev(matrix_mdev);
- mutex_lock(&ap_attr_mutex);
- get_update_locks_for_mdev(matrix_mdev);
-
- /* Save old state */
+ /* Save the old state */
ap_matrix_copy(&m_old, &matrix_mdev->matrix);
- if (parse_bitmap(&rest, m_new.apm, AP_DEVICES) ||
- parse_bitmap(&rest, m_new.aqm, AP_DOMAINS) ||
- parse_bitmap(&rest, m_new.adm, AP_DOMAINS)) {
- rc = -EINVAL;
- goto out;
- }
+ ap_matrix_copy(&m_old_shadow, &matrix_mdev->shadow_apcb);
- bitmap_andnot(m_removed.apm, m_old.apm, m_new.apm, AP_DEVICES);
- bitmap_andnot(m_removed.aqm, m_old.aqm, m_new.aqm, AP_DOMAINS);
- bitmap_andnot(m_added.apm, m_new.apm, m_old.apm, AP_DEVICES);
- bitmap_andnot(m_added.aqm, m_new.aqm, m_old.aqm, AP_DOMAINS);
+ /*
+ * Get the adapters, domains and control domains added and/or removed
+ * from the existing configuration
+ */
+ get_removed_matrixes(&m_removed, &m_old, m_new);
+ get_added_matrixes(&m_added, &m_old, m_new);
/* Need new bitmaps in matrix_mdev for validation */
- ap_matrix_copy(&matrix_mdev->matrix, &m_new);
+ ap_matrix_copy(&matrix_mdev->matrix, m_new);
/* Ensure new state is valid, else undo new state */
- rc = vfio_ap_mdev_validate_masks(matrix_mdev);
- if (rc) {
- ap_matrix_copy(&matrix_mdev->matrix, &m_old);
- goto out;
- }
- rc = ap_matrix_overflow_check(matrix_mdev);
+ rc = validate_new_state(matrix_mdev);
if (rc) {
ap_matrix_copy(&matrix_mdev->matrix, &m_old);
- goto out;
+ ap_matrix_copy(&matrix_mdev->shadow_apcb, &m_old_shadow);
You seem to be keeping shadow_apcb in lockstep with matrix_mdev->matrix. But we still have the original code below that does the official shadow_apcb update after validation has been performed. Why did you decide to change how/when shadow_apcb was being updated? And if this was intentional, why are we still performing a separate update of shadow_apcb at the end?
This is a valid point, there is no need for m_old_shadow
/* Apply changes to shadow apbc if things changed */
if (do_update) {
vfio_ap_mdev_update_guest_apcb(matrix_mdev);
reset_queues_for_apids(matrix_mdev, apm_filtered);
}
+ return rc;
}
- rc = count;
/* Need old bitmaps in matrix_mdev for unplug/unlink */
ap_matrix_copy(&matrix_mdev->matrix, &m_old);
@@ -1786,14 +1783,10 @@ static ssize_t ap_config_store(struct device *dev, struct device_attribute *attr
vfio_ap_mdev_hot_unplug_domains(matrix_mdev, m_removed.aqm);
/* Need new bitmaps in matrix_mdev for linking new adapters/domains */
- ap_matrix_copy(&matrix_mdev->matrix, &m_new);
-
- /* Link newly added adapters */
- for_each_set_bit_inv(newbit, m_added.apm, AP_DEVICES)
- vfio_ap_mdev_link_adapter(matrix_mdev, newbit);
+ ap_matrix_copy(&matrix_mdev->matrix, m_new);
- for_each_set_bit_inv(newbit, m_added.aqm, AP_DOMAINS)
- vfio_ap_mdev_link_domain(matrix_mdev, newbit);
+ /* Link queues associated with the newly added adapters and domains */
+ link_new_queues(matrix_mdev, &m_added);
/* filter resources not bound to vfio-ap */
do_update = vfio_ap_mdev_filter_matrix(matrix_mdev, apm_filtered);
@@ -1804,7 +1797,39 @@ static ssize_t ap_config_store(struct device *dev, struct device_attribute *attr
vfio_ap_mdev_update_guest_apcb(matrix_mdev);
reset_queues_for_apids(matrix_mdev, apm_filtered);
}
-out:
+
+ return 0;
+}
+
+static ssize_t ap_config_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct ap_matrix_mdev *matrix_mdev = dev_get_drvdata(dev);
+ struct ap_matrix m_new;
+ char *newbuf, *rest;
+ ssize_t rc;
+
+ newbuf = kstrndup(buf, AP_CONFIG_STRLEN, GFP_KERNEL);
+ if (!newbuf)
+ return -ENOMEM;
+ rest = newbuf;
+
+ mutex_lock(&ap_attr_mutex);
+ get_update_locks_for_mdev(matrix_mdev);
+
+ if (parse_bitmap(&rest, m_new.apm, AP_DEVICES) ||
+ parse_bitmap(&rest, m_new.aqm, AP_DOMAINS) ||
+ parse_bitmap(&rest, m_new.adm, AP_DOMAINS)) {
+ kfree(newbuf);
+ release_update_locks_for_mdev(matrix_mdev);
+ mutex_unlock(&ap_attr_mutex);
+ return -EINVAL;
+ }
+
+ rc = vfio_ap_set_new_guest_config(matrix_mdev, &m_new);
+ if (!rc)
+ rc = count;
+
release_update_locks_for_mdev(matrix_mdev);
mutex_unlock(&ap_attr_mutex);
kfree(newbuf);
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 1fbdfcce5a11..150dfce8a674 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -157,6 +157,62 @@ struct vfio_ap_queue {
struct work_struct reset_work;
};
+/**
+ * get_update_locks_for_mdev: Acquire the locks required to dynamically update a
+ * KVM guest's APCB in the proper order.
+ *
+ * @matrix_mdev: a pointer to a struct ap_matrix_mdev object containing the AP
+ * configuration data to use to update a KVM guest's APCB.
+ *
+ * The proper locking order is:
+ * 1. matrix_dev->guests_lock: required to use the KVM pointer to update a KVM
+ * guest's APCB.
+ * 2. matrix_mdev->kvm->lock: required to update a guest's APCB
+ * 3. matrix_dev->mdevs_lock: required to access data stored in a matrix_mdev
+ *
+ * Note: If @matrix_mdev is NULL or is not attached to a KVM guest, the KVM
+ * lock will not be taken.
+ */
+static inline void get_update_locks_for_mdev(struct ap_matrix_mdev *matrix_mdev)
+{
+ mutex_lock(&matrix_dev->guests_lock);
+ if (matrix_mdev && matrix_mdev->kvm)
+ mutex_lock(&matrix_mdev->kvm->lock);
+ mutex_lock(&matrix_dev->mdevs_lock);
+}
+
+/**
+ * release_update_locks_for_mdev: Release the locks used to dynamically update a
+ * KVM guest's APCB in the proper order.
+ *
+ * @matrix_mdev: a pointer to a struct ap_matrix_mdev object containing the AP
+ * configuration data to use to update a KVM guest's APCB.
+ *
+ * The proper unlocking order is:
+ * 1. matrix_dev->mdevs_lock
+ * 2. matrix_mdev->kvm->lock
+ * 3. matrix_dev->guests_lock
+ *
+ * Note: If @matrix_mdev is NULL or is not attached to a KVM guest, the KVM
+ * lock will not be released.
+ */
+static inline void release_update_locks_for_mdev(struct ap_matrix_mdev *matrix_mdev)
+{
+ mutex_unlock(&matrix_dev->mdevs_lock);
+ if (matrix_mdev && matrix_mdev->kvm)
+ mutex_unlock(&matrix_mdev->kvm->lock);
+ mutex_unlock(&matrix_dev->guests_lock);
+}
+
+static inline void
+assert_has_update_locks_for_mdev(struct ap_matrix_mdev *matrix_mdev)
+{
+ lockdep_assert_held(&matrix_dev->guests_lock);
+ if (matrix_mdev && matrix_mdev->kvm)
+ lockdep_assert_held(&matrix_mdev->kvm->lock);
+ lockdep_assert_held(&matrix_dev->mdevs_lock);
+}
+
int vfio_ap_mdev_get_num_queues(struct ap_matrix *ap_matrix);
int vfio_ap_mdev_register(void);
@@ -172,9 +228,14 @@ void vfio_ap_on_cfg_changed(struct ap_config_info *new_config_info,
void vfio_ap_on_scan_complete(struct ap_config_info *new_config_info,
struct ap_config_info *old_config_info);
+void vfio_ap_matrix_init(struct ap_config_info *info, struct ap_matrix *matrix);
+
void vfio_ap_init_migration_capabilities(struct ap_matrix_mdev *matrix_mdev);
int vfio_ap_init_migration_data(struct ap_matrix_mdev *matrix_mdev);
void vfio_ap_release_migration_data(struct ap_matrix_mdev *matrix_mdev);
void vfio_ap_reset_migration_state(struct ap_matrix_mdev *matrix_mdev);
+int vfio_ap_set_new_guest_config(struct ap_matrix_mdev *matrix_mdev,
+ struct ap_matrix *m_new);
+
#endif /* _VFIO_AP_PRIVATE_H_ */