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

[Joao Martins]

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 <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.
>>
>> 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-12d49e70fed1@nvidia.com/#r

... where you discuss this.