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

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

On 8/19/26 1:49 PM, Jason J. Herne wrote:
>
>
> 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();

This is a good suggestion, I'll simplify the logic.