Re: [PATCH V2 vfio 02/14] vfio: Extend the device migration protocol with PRE_COPY

Yishai Hadas
2022-12-05T13:55:04Z

On 02/12/2022 11:38, Shameerali Kolothum Thodi wrote:
>
>> -----Original Message-----
>> From: Yishai Hadas [mailto:yishaih@nvidia.com]
>> Sent: 01 December 2022 15:29
>> To: alex.williamson@redhat.com; jgg@nvidia.com
>> Cc: kvm@vger.kernel.org; kevin.tian@intel.com; joao.m.martins@oracle.com;
>> leonro@nvidia.com; shayd@nvidia.com; yishaih@nvidia.com;
>> maorg@nvidia.com; avihaih@nvidia.com; cohuck@redhat.com
>> Subject: [PATCH V2 vfio 02/14] vfio: Extend the device migration protocol
>> with PRE_COPY
> [...]
>
>> +/**
>> + * VFIO_MIG_GET_PRECOPY_INFO - _IO(VFIO_TYPE, VFIO_BASE + 21)
>> + *
>> + * This ioctl is used on the migration data FD in the precopy phase of the
>> + * migration data transfer. It returns an estimate of the current data sizes
>> + * remaining to be transferred. It allows the user to judge when it is
>> + * appropriate to leave PRE_COPY for STOP_COPY.
>> + *
>> + * This ioctl is valid only in PRE_COPY states and kernel driver should
>> + * return -EINVAL from any other migration state.
>> + *
>> + * The vfio_precopy_info data structure returned by this ioctl provides
>> + * estimates of data available from the device during the PRE_COPY states.
>> + * This estimate is split into two categories, initial_bytes and
>> + * dirty_bytes.
>> + *
>> + * The initial_bytes field indicates the amount of initial precopy
>> + * data available from the device. This field should have a non-zero initial
>> + * value and decrease as migration data is read from the device.
>> + * It is recommended to leave PRE_COPY for STOP_COPY only after this field
>> + * reaches zero. Leaving PRE_COPY earlier might make things slower.
>> + *
>> + * The dirty_bytes field tracks device state changes relative to data
>> + * previously retrieved. This field starts at zero and may increase as
>> + * the internal device state is modified or decrease as that modified
>> + * state is read from the device.
>> + *
>> + * Userspace may use the combination of these fields to estimate the
>> + * potential data size available during the PRE_COPY phases, as well as
>> + * trends relative to the rate the device is dirtying its internal
>> + * state, but these fields are not required to have any bearing relative
>> + * to the data size available during the STOP_COPY phase.
>> + *
>> + * Drivers have a lot of flexibility in when and what they transfer during the
>> + * PRE_COPY phase, and how they report this from
>> VFIO_MIG_GET_PRECOPY_INFO.
>> + *
>> + * During pre-copy the migration data FD has a temporary "end of stream"
>> that is
>> + * reached when both initial_bytes and dirty_byte are zero. For instance,
>> this
>> + * may indicate that the device is idle and not currently dirtying any internal
>> + * state. When read() is done on this temporary end of stream the kernel
>> driver
>> + * should return ENOMSG from read(). Userspace can wait for more data
>> (which may
>> + * never come) by using poll.
>> + *
>> + * Once in STOP_COPY the migration data FD has a permanent end of
>> stream
>> + * signaled in the usual way by read() always returning 0 and poll always
>> + * returning readable. ENOMSG may not be returned in STOP_COPY.
>> Support
>> + * for this ioctl is optional.
> Isn't mandatory if the driver claims support for VFIO_MIGRATION_PRE_COPY?

It seems reasonable to let it be mandatory once the driver claims to
support PRE_COPY.

This will also simplify things from QEMU point of view which can expect
the IOCTL to be supported.

Will add a note here as part of V3.

>
> Other than that looks fine to me.
>
> Reviewed-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>
Thanks, will add your Reviewed-by as part of V3.

Yishai