Re: [PATCH v3 10/10] vfio/common: Allow disabling device dirty page tracking

[Joao Martins <joao.m.martins@oracle.com>]

On 10/07/2024 11:42, Duan, Zhenzhong wrote:
> 
> 
>> -----Original Message-----
>> From: Joao Martins <joao.m.martins@oracle.com>
>> Subject: [PATCH v3 10/10] vfio/common: Allow disabling device dirty page
>> tracking
>>
>> The property 'x-pre-copy-dirty-page-tracking' allows disabling the whole
>> tracking of VF pre-copy phase of dirty page tracking, though it means
>> that it will only be used at the start of the switchover phase.
>>
>> Add an option that disables the VF dirty page tracking, and fall
>> back into container-based dirty page tracking. This also allows to
>> use IOMMU dirty tracking even on VFs with their own dirty
>> tracker scheme.
>>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> ---
>> include/hw/vfio/vfio-common.h | 1 +
>> hw/vfio/common.c              | 3 +++
>> hw/vfio/migration.c           | 3 ++-
>> hw/vfio/pci.c                 | 3 +++
>> 4 files changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>> common.h
>> index 7ce925cfab19..9db3fd31cfae 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -137,6 +137,7 @@ typedef struct VFIODevice {
>>     VFIOMigration *migration;
>>     Error *migration_blocker;
>>     OnOffAuto pre_copy_dirty_page_tracking;
>> +    OnOffAuto device_dirty_page_tracking;
>>     bool dirty_pages_supported;
>>     bool dirty_tracking;
>>     HostIOMMUDevice *hiod;
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 7cdb969fd396..eaa33d9ee037 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -199,6 +199,9 @@ bool vfio_devices_all_device_dirty_tracking(const
>> VFIOContainerBase *bcontainer)
>>     VFIODevice *vbasedev;
>>
>>     QLIST_FOREACH(vbasedev, &bcontainer->device_list, container_next) {
>> +        if (vbasedev->device_dirty_page_tracking == ON_OFF_AUTO_OFF) {
>> +            return false;
> 
> Maybe we can initialize vbasedev->dirty_pages_supported to false by checking vbasedev->device_dirty_page_tracking == ON_OFF_AUTO_OFF?
> This way we can avoid extra check.
> 

Hmm, I guess it's a matter of style i.e. the device may support it
(vbasedev::dirty_pages_supported), but user disabled
(vbasedev::device_dirty_page_tracking). It's true that it simplifies, but feels
somewhat misleading?

>> +        }
>>         if (!vbasedev->dirty_pages_supported) {
>>             return false;
>>         }
>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>> index 89195928666f..35d67332db20 100644
>> --- a/hw/vfio/migration.c
>> +++ b/hw/vfio/migration.c
>> @@ -1037,7 +1037,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->device_dirty_page_tracking == ON_OFF_AUTO_OFF) &&
>>         (vbasedev->iommufd &&
>>          !hiodc->get_cap(vbasedev->hiod,
>>                          HOST_IOMMU_DEVICE_CAP_DIRTY_TRACKING, NULL))) {
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index e03d9f3ba546..22819b2036b3 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -3362,6 +3362,9 @@ static Property vfio_pci_dev_properties[] = {
>>     DEFINE_PROP_ON_OFF_AUTO("x-pre-copy-dirty-page-tracking",
>> VFIOPCIDevice,
>>                             vbasedev.pre_copy_dirty_page_tracking,
>>                             ON_OFF_AUTO_ON),
>> +    DEFINE_PROP_ON_OFF_AUTO("x-device-dirty-page-tracking",
>> VFIOPCIDevice,
>> +                            vbasedev.device_dirty_page_tracking,
>> +                            ON_OFF_AUTO_ON),
>>     DEFINE_PROP_ON_OFF_AUTO("display", VFIOPCIDevice,
>>                             display, ON_OFF_AUTO_OFF),
>>     DEFINE_PROP_UINT32("xres", VFIOPCIDevice, display_xres, 0),
>> --
>> 2.17.2
>