Re: [PATCH v7 09/15] s390/vfio-ap: Add method to set a new guest AP configuration

[Anthony Krowiak <akrowiak@linux.ibm.com>]

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@linux.ibm.com>
>> ---
>>   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_ */
>