Re: [PATCH 5/9] vfio/migration: Drop vfio_dma_unmap_dirty_sync_needed()

[Avihai Horon]

On 16/12/2024 14:57, Joao Martins wrote:
> External email: Use caution opening links or attachments
>
>
> On 16/12/2024 09:46, Avihai Horon wrote:
> > There is no need for vfio_dma_unmap_dirty_sync_needed(), as it simply
> > calls vfio_devices_all_dirty_tracking_started().
> >
> > Drop vfio_dma_unmap_dirty_sync_needed(), export
> > vfio_devices_all_dirty_tracking_started() and use it instead.
> >
> > Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> > ---
> >   include/hw/vfio/vfio-common.h | 3 ++-
> >   hw/vfio/common.c              | 9 ++-------
> >   hw/vfio/container.c           | 2 +-
> >   3 files changed, 5 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> > index c23ca34871..c5aa606890 100644
> > --- a/include/hw/vfio/vfio-common.h
> > +++ b/include/hw/vfio/vfio-common.h
> > @@ -296,7 +296,8 @@ bool vfio_migration_realize(VFIODevice *vbasedev, Error 
> > **errp);
> >   void vfio_migration_exit(VFIODevice *vbasedev);
> >
> >   int vfio_bitmap_alloc(VFIOBitmap *vbmap, hwaddr size);
> > -bool vfio_dma_unmap_dirty_sync_needed(const VFIOContainerBase *bcontainer);
> > +bool vfio_devices_all_dirty_tracking_started(
> > +    const VFIOContainerBase *bcontainer);
> >   bool
> >   vfio_devices_all_device_dirty_tracking(const VFIOContainerBase *bcontainer);
> >   int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
> > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > index 45783982c9..6e4654218a 100644
> > --- a/hw/vfio/common.c
> > +++ b/hw/vfio/common.c
> > @@ -184,8 +184,8 @@ static bool vfio_devices_all_device_dirty_tracking_started(
> >       return true;
> >   }
> >
> > -static bool
> > -vfio_devices_all_dirty_tracking_started(const VFIOContainerBase *bcontainer)
> > +bool vfio_devices_all_dirty_tracking_started(
> > +    const VFIOContainerBase *bcontainer)
> >   {
> >       if (!migration_is_running()) {
> >           return false;
> > @@ -235,11 +235,6 @@ bool vfio_devices_all_device_dirty_tracking(const 
> > VFIOContainerBase *bcontainer)
> >       return true;
> >   }
> >
> > -bool vfio_dma_unmap_dirty_sync_needed(const VFIOContainerBase *bcontainer)
> > -{
> > -    return vfio_devices_all_dirty_tracking_started(bcontainer);
> > -}
> > -
> Why not just renaming vfio_dma_unmap_dirty_sync_needed to
> vfio_devices_all_dirty_tracking_started() off from the start? We are introducing
> that helper name to just remove it again. And this patch wouldn't be needed 
> anymore

That's what I did at first, however there are several DPT helpers with 
similar names that check similar things but with subtle differences, so 
I thought that having a few changes in a single commit might be confusing.
To ease review and make it super clear I split it to be a step-by-step 
cleanup.

I like it this way, but if you think it's redundant I can merge it again.

Thanks.

> >   static bool vfio_listener_skipped_section(MemoryRegionSection *section)
> >   {
> >       return (!memory_region_is_ram(section->mr) &&
> > diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> > index 8107873534..15deffe3e4 100644
> > --- a/hw/vfio/container.c
> > +++ b/hw/vfio/container.c
> > @@ -131,7 +131,7 @@ static int vfio_legacy_dma_unmap(const VFIOContainerBase 
> > *bcontainer,
> >       int ret;
> >       Error *local_err = NULL;
> >
> > -    if (iotlb && vfio_dma_unmap_dirty_sync_needed(bcontainer)) {
> > +    if (iotlb && vfio_devices_all_dirty_tracking_started(bcontainer)) {
> >           if (!vfio_devices_all_device_dirty_tracking(bcontainer) &&
> >               bcontainer->dirty_pages_supported) {
> >               return vfio_dma_unmap_bitmap(container, iova, size, iotlb);