Avihai Horon <avih...@nvidia.com> wrote:
Reviewed-by: Juan Quintela <quint...@redhat.com>.
1st comment is a bug, but as you just remove it.
Not that it matters a lot in the end (you are removing v1 on the
following patch).
> @@ -438,7 +445,13 @@ static bool
> vfio_devices_all_running_and_mig_active(VFIOContainer *container)
> return false;
> }
>
> - if (migration->device_state_v1 & VFIO_DEVICE_STATE_V1_RUNNING) {
> + if (!migration->v2 &&
> + migration->device_state_v1 & VFIO_DEVICE_STATE_V1_RUNNING) {
> + continue;
> + }
Are you missing here:
} else {
return false;
}
Or I am missunderstanding the code.
> + if (migration->v2 &&
> + migration->device_state == VFIO_DEVICE_STATE_RUNNING) {
> continue;
> } else {
> return false;
[...]
> +/*
> + * This is an arbitrary size based on migration of mlx5 devices, where
> typically
> + * total device migration size is on the order of 100s of MB. Testing with
> + * larger values, e.g. 128MB and 1GB, did not show a performance improvement.
> + */
> +#define VFIO_MIG_DEFAULT_DATA_BUFFER_SIZE (1 * MiB)
Just in case that it makes a difference. You are using here a 1MB
buffer.
But qemu_file_get_to_fd() uses a 32KB buffer.
> }
> }
>
> +static int vfio_query_stop_copy_size(VFIODevice *vbasedev,
> + uint64_t *stop_copy_size)
> +{
> + uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature) +
> + sizeof(struct
> vfio_device_feature_mig_data_size),
> + sizeof(uint64_t))] = {};
> + struct vfio_device_feature *feature = (struct vfio_device_feature *)buf;
suggestion:
size_t size = DIV_ROUND_UP(sizeof(struct vfio_device_feature) +
sizeof(struct
vfio_device_feature_mig_data_size),
sizeof(uint64_t));
g_autofree struct vfio_device_feature *feature = g_malloc0(size *
sizeof(uint64_t));
I have to reread several times to see what was going on there.
And call it a day?
Later, Juan.
