Re: [PATCH v3 1/1] docs/devel: Add VFIO device migration documentation

[Cornelia Huck]

On Fri, 26 Mar 2021 18:48:50 +0530
Tarun Gupta <targupta@nvidia.com> wrote:

> Document interfaces used for VFIO device migration. Added flow of state 
> changes
> during live migration with VFIO device. Tested by building docs with the new
> vfio-migration.rst file.

I don't think you want to include the test state in the patch
description; that should go into a --- section that is stripped off by
git am.

> 
> v3:
> - Add introductory line about VM migration in general.
> - Remove occurcences of vfio_pin_pages() to describe pinning.
> - Incorporated comments from v2
> 
> v2:
> - Included the new vfio-migration.rst file in index.rst
> - Updated dirty page tracking section, also added details about
>   'pre-copy-dirty-page-tracking' opt-out option.
> - Incorporated comments around wording of doc.

Same for the changelog; this is interesting for review, but not for the
final git log.

> 
> Signed-off-by: Tarun Gupta <targupta@nvidia.com>
> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>

This S-o-b chain does not look correct. Your address should be the last
one in the chain, signing off on all of the previous ones. (Maybe Kirti
also needs to be listed in a Co-developed-by: statement?)

> ---
>  MAINTAINERS                   |   1 +
>  docs/devel/index.rst          |   1 +
>  docs/devel/vfio-migration.rst | 143 ++++++++++++++++++++++++++++++++++
>  3 files changed, 145 insertions(+)
>  create mode 100644 docs/devel/vfio-migration.rst

> diff --git a/docs/devel/vfio-migration.rst b/docs/devel/vfio-migration.rst
> new file mode 100644
> index 0000000000..24cb55991a
> --- /dev/null
> +++ b/docs/devel/vfio-migration.rst

(...)

> +VFIO device hooks for iterative approach:

"VFIO implements the device hooks for the iterative approach as
follows:"

?

> +
> +* A ``save_setup`` function that sets up the migration region, sets _SAVING
> +  flag in the VFIO device state and informs the VFIO IOMMU module to start
> +  dirty page tracking.
> +
> +* A ``load_setup`` function that sets up the migration region on the
> +  destination and sets _RESUMING flag in the VFIO device state.
> +
> +* A ``save_live_pending`` function that reads pending_bytes from the vendor
> +  driver, which indicates the amount of data that the vendor driver has yet 
> to
> +  save for the VFIO device.
> +
> +* A ``save_live_iterate`` function that reads the VFIO device's data from the
> +  vendor driver through the migration region during iterative phase.
> +
> +* A ``save_live_complete_precopy`` function that resets _RUNNING flag from 
> the
> +  VFIO device state, saves the device config space, if any, and iteratively
> +  copies the remaining data for the VFIO device until the vendor driver
> +  indicates that no data remains (pending bytes is zero).
> +
> +* A ``load_state`` function that loads the config section and the data
> +  sections that are generated by the save functions above
> +
> +* ``cleanup`` functions for both save and load that perform any migration
> +  related cleanup, including unmapping the migration region
> +
> +A VM state change handler is registered to change the VFIO device state when
> +the VM state changes.

This sentence is not very informative. What about:

"The VFIO migration code uses a VM state change handler to change the
VFIO device state when the VM state changes from running to
not-running, and vice versa."

> +
> +Similarly, a migration state change notifier is registered to get a
> +notification on migration state change. These states are translated to the
> +corresponding VFIO device state and conveyed to the vendor driver.

"Similarly, a migration state change handler is used to transition the
VFIO device state back to _RUNNING in case a migration failed or was
canceled."


> +
> +System memory dirty pages tracking
> +----------------------------------
> +
> +A ``log_sync`` memory listener callback marks those system memory pages
> +as dirty which are used for DMA by the VFIO device. The dirty pages bitmap is
> +queried per container. All pages pinned by the vendor driver through external
> +APIs have to be marked as dirty during migration. When there are CPU writes,
> +CPU dirty page tracking can identify dirtied pages, but any page pinned by 
> the
> +vendor driver can also be written by device. There is currently no device or

s/by/by the/

> +IOMMU support for dirty page tracking in hardware.
> +
> +By default, dirty pages are tracked when the device is in pre-copy as well as
> +stop-and-copy phase. So, a page pinned by vendor driver will be copied to

s/by/by the/
s/to/to the/

> +destination in both the phases. Copying dirty pages in pre-copy phase helps

s/both the/both/ ?

> +QEMU to predict if it can achieve its downtime tolerances. If QEMU during
> +pre-copy phase keeps finding dirty pages continuously, then it understands
> +that even in stop-and-copy phase, it is likely to find dirty pages and can
> +predict the downtime accordingly
> +
> +QEMU also provides per device opt-out option ``pre-copy-dirty-page-tracking``

s/provides/provides a/

> +which disables querying dirty bitmap during pre-copy phase. If it is set to

s/querying/querying the/

> +off, all dirty pages will be copied to destination in stop-and-copy phase 
> only

s/to/to the/

(...)