Re: [PATCH v7 10/15] s390/vfio-ap: File ops called to resume the vfio device migration

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

On 8/7/26 6:18 PM, Anthony Krowiak wrote:
> Implements the 'write' callback function that was added to the
> 'file_operations' structure for the file stream created to restore the
> state of the vfio-ap device on the destination system when the migration
> state transitioned from STOP to RESUMING
> 
> The write callback retrieves the vfio device migration state saved to the
> file stream created when the vfio device state was transitioned from
> STOP to STOP_COPY. The saved state contains the source guest's AP
> configuration information. This data is copied from the userspace buffer
> passed to the 'write' callback and stored in the vfio_ap_config structure
> used to set the state of the vfio-ap device on the destination host. If the
> source guest's AP configuration is compatible with the AP configuration on
> the destination host, it will be hot plugged into the destination guest.
> 
> In order for the source guest's and destination host's AP configurations
> to be considered compatible:
> 
> * Each APQN in the source guest's AP configuration must also be in the
>    destination host's AP configuration
> 
> * Each matching APQN in the destination host's AP configuration must be
>    bound to the vfio_ap device driver
> 
> * Each matching APQN in the destination host's AP configuration must
>    reference a queue device with compatible hardware:
> 
>    - The source and destination queues must have the same facilities
>      installed:
>      ~ APSC facility
>      ~ APQKM facility
>      ~ AP4KC facility
> 
>    - The source and destination queues must have the same mode:
>      ~ Coprocessor-mode
>      ~ Accelerator-mode
>      ~ XCP-mode
> 
>    - The source and destination queues must have the same APXA facility
>      setting
>      ~ If the APXA facility is installed on source queue, it must also
>        be installed on the destination queue and vice versa
> 
>    - The source and destination queues must have a compatible
>      classification setting. If the source queue has full native card
>      function, then the destination queue must also have full native
>      card function. If the source queue has stateless functions, then
>      the destination queue can have stateless functions or full native card
>      function because the latter includes the stateless functions.
> 
>    - The binding and associated state for both the source and destination
>      queues must indicate that the queue is usable for all messages
>      (i.e., BS bits equal to 00).
> 
>    - The AP type of the destination queue must be the same as or newer than
>      the source queue (backward compatibility)
> 
> Note: The get_hardware_info_for_queue function that was created in
>        a previous patch was modified to take a mediated device name rather
>        than an ap_matrix_mdev object because that is what is needed for
>        this patch so the function can be executed without holding the
>        matrix_dev->mdevs_lock.
> 
> Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
> ---
>   drivers/s390/crypto/vfio_ap_migration.c | 1009 ++++++++++++++++++++++-
>   1 file changed, 1001 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index e2e7ae8515e5..4dd7373c3d9d 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c
> ...
>   static ssize_t vfio_ap_resuming_write(struct file *filp, const char __user *buf,
>   				      size_t len, loff_t *pos)
>   {
> -	/* TODO */
> -	return -EOPNOTSUPP;
> +	struct vfio_ap_config_buffer *resuming_config_buf;
> +	struct ap_matrix_mdev *matrix_mdev;
> +	struct vfio_ap_config *ap_config;
> +	bool new_allocation = false;
> +	ssize_t ret, cfg_sz;
> +	loff_t write_pos;
> +	size_t write_len;
> +
> +	/*
> +	 * This file was opened with stream_open(), so pos should be NULL for
> +	 * sequential write() calls; a non-NULL pointer will be passed only
> +	 * for positional pwrite() calls in which case we return an error
> +	 * indicating broken pipe/illegal seek on a non-seekable file
> +	 */
> +	if (pos)
> +		return -ESPIPE;
> +
> +	mutex_lock(&matrix_dev->mdevs_lock);
> +	pos = &filp->f_pos;
> +
> +	ret = validate_resuming_write_parms(filp, len, pos);
> +	if (ret) {
> +		mutex_unlock(&matrix_dev->mdevs_lock);
> +		return ret;
> +	}
> +
> +	matrix_mdev = filp->private_data;
> +	matrix_mdev->mig_data->write_in_progress = true;
> +	resuming_config_buf = &matrix_mdev->mig_data->resuming_config_buf;
> +
> +	/*
> +	 * If we have not yet filled the vfio_ap_config_buffer, then we need
> +	 * to continue filling it with data sent from userspace.
> +	 */
> +	if (!resuming_config_buf->filled) {
> +		ret = fill_resuming_config_buffer(resuming_config_buf, buf, len,
> +						  *pos);
> +		if (ret) {
> +			matrix_mdev->mig_data->write_in_progress = false;
> +			mutex_unlock(&matrix_dev->mdevs_lock);
> +			return ret;
> +		}
> +
> +		/*
> +		 * If the vfio_ap_config_buffer is not yet filled, we don't
> +		 * yet have enough data to allocate the vfio_ap_config instance;
> +		 * otherwise, go ahead and allocate it.
> +		 */
> +		if (!resuming_config_buf->filled) {
> +			*pos += len;
> +			matrix_mdev->mig_data->write_in_progress = false;
> +			mutex_unlock(&matrix_dev->mdevs_lock);
> +			return len;
> +		}
> +
> +		ret = allocate_ap_config(resuming_config_buf, &ap_config);
> +		if (ret < 0) {
> +			matrix_mdev->mig_data->write_in_progress = false;
> +			mutex_unlock(&matrix_dev->mdevs_lock);
> +			return ret;
> +		}
> +
> +		new_allocation = true;
> +		cfg_sz = ret;
> +	}
> +
> +	/*
> +	 * If this is not a new allocation of the vfio_ap_config object,
> +	 * then create a copy of it so we can continue filling it in via
> +	 * the copy_from_user() while the mdevs_lock is dropped.
> +	 */
> +	if (!new_allocation) {
> +		cfg_sz = matrix_mdev->mig_data->resuming_mig_file.config_sz;
> +		ap_config = kvzalloc(cfg_sz, GFP_KERNEL_ACCOUNT);
> +
> +		if (!ap_config) {
> +			matrix_mdev->mig_data->write_in_progress = false;
> +			mutex_unlock(&matrix_dev->mdevs_lock);
> +			return -ENOMEM;
> +		}
> +
> +		memcpy(ap_config,
> +		       matrix_mdev->mig_data->resuming_mig_file.ap_config, cfg_sz);
> +	}
> +
> +	/*
> +	 * If ap_config is a new allocation, then the contents of the
> +	 * 'magic', 'version' and 'num_queues' fields will already have
> +	 * been copied in; so the write_pos must be set to the location
> +	 * following the 'num_queues' field and the length to be written must be
> +	 * adjusted accordingly.
> +	 */
> +	if (new_allocation) {
> +		size_t nbytes_already_copied = VFIO_AP_CONFIG_BUF_SIZE - *pos;
> +
> +		write_pos = VFIO_AP_CONFIG_BUF_SIZE;
> +		write_len = len - nbytes_already_copied;
> +		buf += nbytes_already_copied;
> +	} else {
> +		write_pos = *pos;
> +		write_len = len;
> +	}
> +
> +	*pos += len;
> +
> +	mutex_unlock(&matrix_dev->mdevs_lock);
> +
> +	if (copy_from_user((char *)ap_config + write_pos, buf, write_len)) {
> +		if (new_allocation)
> +			kvfree(ap_config);
> +		ret = -EFAULT;
> +		goto out_clear_write_in_progress;
> +	}
> +
> +	/* Check if we've completed writing the entire configuration */
> +	if (write_pos + write_len == cfg_sz) {
> +		ret = do_post_copy_processing(matrix_mdev, ap_config);
> +
> +		if (ret) {
> +			kvfree(ap_config);
> +			goto out_clear_write_in_progress;
> +		}
> +	}
> +
> +	ret = set_new_ap_configuration(matrix_mdev, ap_config, cfg_sz);
> +	if (ret) {
> +		kvfree(ap_config);
> +		goto out_clear_write_in_progress;
> +	}
> +
> +	/*
> +	 * If this is not a new allocation of vfio_ap_config, then the contents
> +	 * of ap_config would have been copied into the existing object, so we
> +	 * can free it so we don't leak the storage.
> +	 */
> +	if (!new_allocation)
> +		kvfree(ap_config);
> +
> +	ret = len;
> +
> +out_clear_write_in_progress:
> +	mutex_lock(&matrix_dev->mdevs_lock);
> +	if (matrix_mdev->mig_data)
> +		matrix_mdev->mig_data->write_in_progress = false;
> +	mutex_unlock(&matrix_dev->mdevs_lock);
> +
> +	return ret;
>   }
>   
>   static const struct file_operations vfio_ap_resume_fops = {

Why can't vfio_ap_resuming_write() just copy the data it is given into a 
buffer, remembering the next position to write and then exit if the 
buffer is not full? If the buffer is full then it can then process that 
buffer. Whatever is happening here seems very convoluted.

The AP config for a guest is pretty small. At most it will be 16 bytes * 
64k queues = 1 MB. Allocating a max buffer up front would be a 
reasonable thing to do, in my opinion. Even if we don't,  I feel like 
this code could be simplified quite a bit.

I'm imagining an algorithm similar to the this:

read_data();
if (not have_all_data):
     return;
post_process_data();