Re: [PATCH v6 9/9] vfio/common: Allow disabling device dirty page tracking

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

On 23/07/2024 09:31, Eric Auger wrote:
> 
> 
> On 7/22/24 23:13, Joao Martins wrote:
>> 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.
> 
> Forced ON value isn't really meaningful though. I mean only valid values
> are AUTO and OFF, correct?
> 

It's more like 'on' if supported.

You could argue that AUTO_AUTO is the right value considering that device may or
may not support. But really both AUTO/ON are the same as the property that
outlines device dirty tracking is ::dirty_tracking for the VF which is
eventually seeded from VFIO pci feature. So this is mostly to override the
default and hence AUTO_OFF is the only thing that it's tested against in the
various places.

Me picking AUTO_ON just mirrored how it's already done for
x-pre-copy-dirty-page-tracking (and I see used e.g. kernel_irqchip and others)

> Eric
>>
>> 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           | 4 +++-
>>  hw/vfio/pci.c                 | 3 +++
>>  4 files changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>> index 1e02c98b09ba..fed499b199f0 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -138,6 +138,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;
>>      bool iommu_dirty_tracking;
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index da12cbd56408..36d0cf6585b2 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;
>> +        }
>>          if (!vbasedev->dirty_pages_supported) {
>>              return false;
>>          }
>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>> index cbfaef7afffe..262d42a46e58 100644
>> --- a/hw/vfio/migration.c
>> +++ b/hw/vfio/migration.c
>> @@ -1036,7 +1036,9 @@ bool vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
>>          return !vfio_block_migration(vbasedev, err, errp);
>>      }
>>  
>> -    if (!vbasedev->dirty_pages_supported && !vbasedev->iommu_dirty_tracking) {
>> +    if ((!vbasedev->dirty_pages_supported ||
>> +         vbasedev->device_dirty_page_tracking == ON_OFF_AUTO_OFF) &&
>> +        !vbasedev->iommu_dirty_tracking) {
>>          if (vbasedev->enable_migration == ON_OFF_AUTO_AUTO) {
>>              error_setg(&err,
>>                         "%s: VFIO device doesn't support device and "
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 8c0f212a163e..a0767de54b8d 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -3364,6 +3364,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),
>