On 16/12/2024 15:52, Joao Martins wrote: > On 16/12/2024 14:52, Avihai Horon wrote: >> >> On 16/12/2024 14:32, Joao Martins wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> 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 <avih...@nvidia.com> >>> The refactor itself is fine except a pre-existing bug: >>> >>> Reviewed-by: Joao Martins <joao.m.mart...@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. >> >> It was tied to migration even prior to this commit, as VFIO log syncs were >> restricted to run only during migration (we had "if (! >> migration_is_setup_or_active())" check). >> This commit only narrowed it down further to not run during SETUP. >> > > Ok, good point. > > Btw you are regressing from that behaviour with this change above, because if > migration has state MIGRATION_STATUS_SETUP and migration_is_running() will > return true and so you will log dirty pages. >
Nevermind this comment. Just noticed that it was the point of the whole thread: https://lore.kernel.org/qemu-devel/78729b4b-3747-4408-8146-12d49e70f...@nvidia.com/#r ... where you discuss this.