Re: [PATCH v2 3/7] vfio/migration: Refactor vfio_devices_all_running_and_mig_active() logic

[Joao Martins]

On 18/12/2024 13:40, Avihai Horon wrote:
> During DMA unmap with vIOMMU, vfio_devices_all_running_and_mig_active()
> is used to check whether a dirty page log sync of the unmapped pages is
> required. Such log sync is needed during migration pre-copy phase, and
> the current logic detects it by checking if migration is active and if
> the VFIO devices are running.
> 
> However, recently there has been an effort to simplify the migration
> status API and reduce it to a single migration_is_running() function.
> 
> To accommodate this, refactor vfio_devices_all_running_and_mig_active()
> logic so it won't use migration_is_active(). Do it by simply checking if
> dirty tracking has been started using internal VFIO flags.
> 
> This should be equivalent to the previous logic as during migration
> dirty tracking is active and when the guest is stopped there shouldn't
> be DMA unmaps coming from it.
> 
> As a side effect, now that migration status is no longer used, DMA unmap
> log syncs are untied from migration. This will make calc-dirty-rate more
> accurate as now it will also include VFIO dirty pages that were DMA
> unmapped.
> 
> Also rename the function to properly reflect its new logic and extract
> common code from vfio_devices_all_dirty_tracking().
> 
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>

Reviewed-by: Joao Martins <joao.m.martins@oracle.com>

> ---
>  include/hw/vfio/vfio-common.h |  4 ++--
>  hw/vfio/common.c              | 40 +++++++----------------------------
>  hw/vfio/container.c           |  2 +-
>  3 files changed, 11 insertions(+), 35 deletions(-)
> 
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 6c999be398..d5f542beab 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -296,8 +296,8 @@ bool vfio_migration_realize(VFIODevice *vbasedev, Error 
> **errp);
>  void vfio_migration_exit(VFIODevice *vbasedev);
>  
>  int vfio_bitmap_alloc(VFIOBitmap *vbmap, hwaddr size);
> -bool
> -vfio_devices_all_running_and_mig_active(const VFIOContainerBase *bcontainer);
> +bool vfio_devices_all_dirty_tracking_started(
> +    const VFIOContainerBase *bcontainer);
>  bool
>  vfio_devices_all_device_dirty_tracking(const VFIOContainerBase *bcontainer);
>  int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index e032ce1b6f..2831f674ff 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -184,12 +184,18 @@ static bool 
> vfio_devices_all_device_dirty_tracking_started(
>      return true;
>  }
>  
> +bool vfio_devices_all_dirty_tracking_started(
> +    const VFIOContainerBase *bcontainer)
> +{
> +    return vfio_devices_all_device_dirty_tracking_started(bcontainer) ||
> +           bcontainer->dirty_pages_started;
> +}
> +
>  static bool vfio_devices_all_dirty_tracking(VFIOContainerBase *bcontainer)
>  {
>      VFIODevice *vbasedev;
>  
> -    if (!(vfio_devices_all_device_dirty_tracking_started(bcontainer) ||
> -          bcontainer->dirty_pages_started)) {
> +    if (!vfio_devices_all_dirty_tracking_started(bcontainer)) {
>          return false;
>      }
>  
> @@ -225,36 +231,6 @@ bool vfio_devices_all_device_dirty_tracking(const 
> VFIOContainerBase *bcontainer)
>      return true;
>  }
>  
> -/*
> - * Check if all VFIO devices are running and migration is active, which is
> - * essentially equivalent to the migration being in pre-copy phase.
> - */
> -bool
> -vfio_devices_all_running_and_mig_active(const VFIOContainerBase *bcontainer)
> -{
> -    VFIODevice *vbasedev;
> -
> -    if (!migration_is_active()) {
> -        return false;
> -    }
> -
> -    QLIST_FOREACH(vbasedev, &bcontainer->device_list, container_next) {
> -        VFIOMigration *migration = vbasedev->migration;
> -
> -        if (!migration) {
> -            return false;
> -        }
> -
> -        if (vfio_device_state_is_running(vbasedev) ||
> -            vfio_device_state_is_precopy(vbasedev)) {
> -            continue;
> -        } else {
> -            return false;
> -        }
> -    }
> -    return true;
> -}
> -
>  static bool vfio_listener_skipped_section(MemoryRegionSection *section)
>  {
>      return (!memory_region_is_ram(section->mr) &&
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index 9ccdb639ac..15deffe3e4 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -131,7 +131,7 @@ static int vfio_legacy_dma_unmap(const VFIOContainerBase 
> *bcontainer,
>      int ret;
>      Error *local_err = NULL;
>  
> -    if (iotlb && vfio_devices_all_running_and_mig_active(bcontainer)) {
> +    if (iotlb && vfio_devices_all_dirty_tracking_started(bcontainer)) {
>          if (!vfio_devices_all_device_dirty_tracking(bcontainer) &&
>              bcontainer->dirty_pages_supported) {
>              return vfio_dma_unmap_bitmap(container, iova, size, iotlb);