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

[Joao Martins]

On 22/07/2024 15:53, Cédric Le Goater wrote:
> On 7/19/24 19:26, Joao Martins wrote:
>> On 19/07/2024 15:24, Joao Martins wrote:
>>> On 19/07/2024 15:17, Cédric Le Goater wrote:
>>>> On 7/19/24 14:05, Joao Martins wrote:
>>>>> By default VFIO migration is set to auto, which will support live
>>>>> migration if the migration capability is set *and* also dirty page
>>>>> tracking is supported.
>>>>>
>>>>> For testing purposes one can force enable without dirty page tracking
>>>>> via enable-migration=on, but that option is generally left for testing
>>>>> purposes.
>>>>>
>>>>> So starting with IOMMU dirty tracking it can use to accomodate the lack of
>>>>> VF dirty page tracking allowing us to minimize the VF requirements for
>>>>> migration and thus enabling migration by default for those too.
>>>>>
>>>>> While at it change the error messages to mention IOMMU dirty tracking as
>>>>> well.
>>>>>
>>>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>>>>> ---
>>>>>    include/hw/vfio/vfio-common.h |  1 +
>>>>>    hw/vfio/iommufd.c             |  2 +-
>>>>>    hw/vfio/migration.c           | 11 ++++++-----
>>>>>    3 files changed, 8 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>>>>> index 7e530c7869dc..00b9e933449e 100644
>>>>> --- a/include/hw/vfio/vfio-common.h
>>>>> +++ b/include/hw/vfio/vfio-common.h
>>>>> @@ -299,6 +299,7 @@ int vfio_devices_query_dirty_bitmap(const
>>>>> VFIOContainerBase *bcontainer,
>>>>>                    VFIOBitmap *vbmap, hwaddr iova, hwaddr size, Error 
>>>>> **errp);
>>>>>    int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t
>>>>> iova,
>>>>>                              uint64_t size, ram_addr_t ram_addr, Error
>>>>> **errp);
>>>>> +bool iommufd_hwpt_dirty_tracking(VFIOIOASHwpt *hwpt);
>>>>>      /* Returns 0 on success, or a negative errno. */
>>>>>    bool vfio_device_get_name(VFIODevice *vbasedev, Error **errp);
>>>>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>>>>> index 7dd5d43ce06a..a998e8578552 100644
>>>>> --- a/hw/vfio/iommufd.c
>>>>> +++ b/hw/vfio/iommufd.c
>>>>> @@ -111,7 +111,7 @@ static void 
>>>>> iommufd_cdev_unbind_and_disconnect(VFIODevice
>>>>> *vbasedev)
>>>>>        iommufd_backend_disconnect(vbasedev->iommufd);
>>>>>    }
>>>>>    -static bool iommufd_hwpt_dirty_tracking(VFIOIOASHwpt *hwpt)
>>>>> +bool iommufd_hwpt_dirty_tracking(VFIOIOASHwpt *hwpt)
>>>>>    {
>>>>>        return hwpt && hwpt->hwpt_flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
>>>>>    }
>>>>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>>>>> index 34d4be2ce1b1..63ffa46c9652 100644
>>>>> --- a/hw/vfio/migration.c
>>>>> +++ b/hw/vfio/migration.c
>>>>> @@ -1036,16 +1036,17 @@ 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 &&
>>>>> +        !iommufd_hwpt_dirty_tracking(vbasedev->hwpt)) {
>>>>
>>>>
>>>> Some platforms do not have IOMMUFD support and this call will need
>>>> some kind of abstract wrapper to reflect dirty tracking support in
>>>> the IOMMU backend.
>>>>
>>>
>>> This was actually on purpose because only IOMMUFD presents a view of 
>>> hardware
>>> whereas type1 supporting dirty page tracking is not used as means to 
>>> 'migration
>>> is supported'.
>>>
>>> The hwpt is nil in type1 and the helper checks that, so it should return 
>>> false.
>>>
>>
>> Oh wait, maybe you're talking about CONFIG_IOMMUFD=n which I totally didn't
>> consider. Maybe this would be a elegant way to address it? Looks to pass my
>> build with CONFIG_IOMMUFD=n
>>
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>> index 61dd48e79b71..422ad4a5bdd1 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -300,7 +300,14 @@ int vfio_devices_query_dirty_bitmap(const 
>> VFIOContainerBase
>> *bcontainer,
>>                   VFIOBitmap *vbmap, hwaddr iova, hwaddr size, Error **errp);
>>   int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t 
>> iova,
>>                             uint64_t size, ram_addr_t ram_addr, Error 
>> **errp);
>> +#ifdef CONFIG_IOMMUFD
>>   bool iommufd_hwpt_dirty_tracking(VFIOIOASHwpt *hwpt);
>> +#else
>> +static inline bool iommufd_hwpt_dirty_tracking(VFIOIOASHwpt *hwpt)
>> +{
>> +    return false;
>> +}
>> +#endif
>>
>>   /* Returns 0 on success, or a negative errno. */
>>   bool vfio_device_get_name(VFIODevice *vbasedev, Error **errp);
>>
> 
> hmm, no. You will need to introduce a new Host IOMMU device capability,
> something like :
> 
>    HOST_IOMMU_DEVICE_CAP_DIRTY_TRACKING,
> 
> Then, introduce an helper routine to check the capability  :
> 
>    return hiodc->get_cap( ... HOST_IOMMU_DEVICE_CAP_DIRTY_TRACKING...)
>  
> and replace the iommufd_hwpt_dirty_tracking call with it.
> 
> Yeah I know, it's cumbersome but it's cleaner !
> 

Funny you mention it, because that's what I did in v3:

20240708143420.16953-9-joao.m.martins@oracle.com/">https://lore.kernel.org/qemu-devel/20240708143420.16953-9-joao.m.martins@oracle.com/

But it was suggested to drop (I am assuming to avoid complexity)

> That's not a major problem in the series. I can address it at the end
> to avoid a resend. First, let's get a R-b on all other patches.
> 
> Thanks,
> 
> C.
> 
>