Re: [PATCH v4 11/12] vfio/migration: Don't block migration device dirty tracking is unsupported

[Joao Martins]

On 17/07/2024 17:02, Joao Martins wrote:
> On 17/07/2024 16:35, Joao Martins wrote:
>> On 17/07/2024 10:20, Joao Martins wrote:
>>> On 17/07/2024 03:38, Duan, Zhenzhong wrote:
>>>>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>>>>> index 34d4be2ce1b1..ce3d1b6e9a25 100644
>>>>> --- a/hw/vfio/migration.c
>>>>> +++ b/hw/vfio/migration.c
>>>>> @@ -1036,7 +1036,8 @@ bool vfio_migration_realize(VFIODevice
>>>>> *vbasedev, Error **errp)
>>>>>         return !vfio_block_migration(vbasedev, err, errp);
>>>>>     }
>>>>>
>>>>> -    if (!vbasedev->dirty_pages_supported) {
>>>>> +    if (!vbasedev->dirty_pages_supported &&
>>>>> +        !vbasedev->bcontainer->dirty_pages_supported) {
>>>>>         if (vbasedev->enable_migration == ON_OFF_AUTO_AUTO) {
>>>>>             error_setg(&err,
>>>>>                        "%s: VFIO device doesn't support device dirty 
>>>>> tracking",
>>>>
>>>> I'm not sure if this message needs to be updated, " VFIO device doesn't 
>>>> support device and IOMMU dirty tracking"
>>>>
>>>> Same for the below:
>>>>
>>>> warn_report("%s: VFIO device doesn't support device dirty tracking"
>>>
>>>
>>> Ah yes, good catch. Additionally I think I should check device hwpt rather 
>>> than
>>> container::dirty_pages_supported i.e.
>>>
>>> if (!vbasedev->dirty_pages_supported &&
>>>     (vbasedev->hwpt && !iommufd_hwpt_dirty_tracking(vbasedev->hwpt)))
>>>
>>> This makes sure that migration is blocked with more accuracy
>>
>> I retract this comment as I think it can all be easily detected by not OR-ing
>> the setting of vbasedev->bcontainer->dirty_pages_supported. I should put a
>> warn_report_once() there.
> 
> Something like this below.
> 
> To be clear: this is mostly a safe guard against a theoretic case that we 
> don't
> know it exists. For example on x86, this is homogeneous and I suspect server 
> ARM
> to be the case too. embedded ARM might be different as there's so many
> incantations of it.
> 

Except that it won't work with hotplug :( so the previous snip was actually a
bit better.

> @@ -267,6 +282,13 @@ static bool iommufd_cdev_autodomains_get(VFIODevice 
> *vbasedev,
>      vbasedev->hwpt = hwpt;
>      QLIST_INSERT_HEAD(&hwpt->device_list, vbasedev, hwpt_next);
>      QLIST_INSERT_HEAD(&container->hwpt_list, hwpt, next);
> +
> +    if (container->bcontainer.dirty_pages_supported &&
> +        !iommufd_hwpt_dirty_tracking(hwpt)) {
> +        warn_report("%s: IOMMU dirty tracking not supported\n", 
> vbasedev->name);
> +    }
> +    container->bcontainer.dirty_pages_supported =
> +                              iommufd_hwpt_dirty_tracking(hwpt);
>      return true;
>  }
> 
>