Re: [PATCH 2/9] vfio/migration: Refactor vfio_devices_all_dirty_tracking() logic

[Joao Martins]

On 16/12/2024 09:46, Avihai Horon wrote:
> During dirty page log sync, vfio_devices_all_dirty_tracking() is used to
> check if dirty tracking has been started in order to avoid errors. The
> current logic checks if migration is in ACTIVE or DEVICE states to
> ensure dirty tracking has been started.
> 
> 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_dirty_tracking() logic so
> it won't use migration_is_active() and migration_is_device(). Instead,
> use internal VFIO dirty tracking flags.
> 
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>

The refactor itself is fine except a pre-existing bug:

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

> ---
>  hw/vfio/common.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index dcef44fe55..a99796403e 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -170,11 +170,30 @@ bool vfio_device_state_is_precopy(VFIODevice *vbasedev)
>             migration->device_state == VFIO_DEVICE_STATE_PRE_COPY_P2P;
>  }
>  
> +static bool vfio_devices_all_device_dirty_tracking_started(
> +    const VFIOContainerBase *bcontainer)
> +{
> +    VFIODevice *vbasedev;
> +
> +    QLIST_FOREACH(vbasedev, &bcontainer->device_list, container_next) {
> +        if (!vbasedev->dirty_tracking) {
> +            return false;
> +        }
> +    }
> +
> +    return true;
> +}
> +
>  static bool vfio_devices_all_dirty_tracking(VFIOContainerBase *bcontainer)
>  {
>      VFIODevice *vbasedev;
>  
> -    if (!migration_is_active() && !migration_is_device()) {
> +    if (!migration_is_running()) {
> +        return false;
> +    }
> +

Tieing to migration status means that non-KVM dirty trackers cannot be toggled
unless somebody starts migration. When really your original intention behind
commit ff180c6bd7 ("vfio/migration: Skip log_sync during migration SETUP state")
was to avoid the setup state when you are indeed during a migration.

Now you can actually start/sync/stop dirty trackers without migration when you
use calc-dirty-rate which is immensely useful to draw out how active a VM prior
to starting migration.

The fix is simple and would be to flex the condition to be something like:

        /* Migration status is 'none' with calc-dirty-rate */
        if (!migration_is_none() && !migration_is_running()) {
            return false;
        }

This is ortoghonal to your series of course, but given you are skimming around
this area, sounded like a good idea to raise this. This patch below is what I
had plan to send when the development window started, but this was before folks
wanted to unexport migration status helpers. What would be the alternative idea
forward?

-------------------->8---------------------

>From ace22f29a0547353e4ed5a0db53292a77f79fa81 Mon Sep 17 00:00:00 2001
From: Joao Martins <joao.m.martins@oracle.com>
Date: Wed, 9 Oct 2024 00:27:46 +0100
Subject: [PATCH] vfio/migration: Allow dirty tracking reports with
 MIGRATION_STATUS_NONE

Invoking calc-dirty-rate HMP/QMP method queries the VM dirty rate
without starting a live migration, which is useful e.g. to understand how
active guests are and even for testing purposes. calc-dirty-rate asks
the dirty rate from the VM and it's not restricted to a particular dirty
tracker.

However commit ff180c6bd7 ("vfio/migration: Skip log_sync during migration SETUP
state")
didn't consider this and currently restricts that VF/IOMMU dirty info when 
migration
is active to allow it to be skipped during SETUP stage.

The vfio dirty tracker is already started, the reports are just skipped
based on migration status. So change vfio_devices_all_dirty_tracking() such
that we include MIGRATION_STATUS_NONE to cover calc-dirty-rate case.

Fixes: ff180c6bd7 ("vfio/migration: Skip log_sync during migration SETUP state")
Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 hw/vfio/common.c         | 4 +++-
 include/migration/misc.h | 1 +
 migration/migration.c    | 7 +++++++
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index dcef44fe55be..0c188a2baac2 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -174,7 +174,9 @@ static bool
vfio_devices_all_dirty_tracking(VFIOContainerBase *bcontainer)
 {
     VFIODevice *vbasedev;

-    if (!migration_is_active() && !migration_is_device()) {
+    /* Migration status is 'none' with calc-dirty-rate */
+    if (!migration_is_none() &&
+        !migration_is_active() && !migration_is_device()) {
         return false;
     }

diff --git a/include/migration/misc.h b/include/migration/misc.h
index 804eb23c0607..857768b51383 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -53,6 +53,7 @@ void dump_vmstate_json_to_file(FILE *out_fp);
 void migration_object_init(void);
 void migration_shutdown(void);

+bool migration_is_none(void);
 bool migration_is_active(void);
 bool migration_is_device(void);
 bool migration_is_running(void);
diff --git a/migration/migration.c b/migration/migration.c
index 8c5bd0a75c85..49d11e1adf04 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1637,6 +1637,13 @@ bool migration_in_bg_snapshot(void)
     return migrate_background_snapshot() && migration_is_running();
 }

+bool migration_is_none(void)
+{
+    MigrationState *s = current_migration;
+
+    return s->state == MIGRATION_STATUS_NONE;
+}
+
 bool migration_is_active(void)
 {
     MigrationState *s = current_migration;
--
2.39.3