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

["Jason J. Herne" <jjherne@linux.ibm.com>]

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.

>   	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

>   {
>   	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.

> +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?

/* 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_ */