Re: [PATCH v2 04/17] vfio/common: Extract vIOMMU code from vfio_sync_dirty_bitmap()

[Eric Auger <eric.auger@redhat.com>]

Hi Cédric,

On 6/17/24 08:33, Cédric Le Goater wrote:
> From: Avihai Horon <avihaih@nvidia.com>
>
> Extract vIOMMU code from vfio_sync_dirty_bitmap() to a new function and
> restructure the code.
>
> This is done in preparation for optimizing vIOMMU deviice dirty page
device
> tracking. No functional changes intended.
>
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> [ clg: - Rebased on upstream ]
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  hw/vfio/common.c | 63 +++++++++++++++++++++++++++++-------------------
>  1 file changed, 38 insertions(+), 25 deletions(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index fe215918bdf66ddbe3c5db803e10ce1aa9756b90..f28641bad5cf4b71fcdc0a6c9d42b24c8d786248 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1302,37 +1302,50 @@ vfio_sync_ram_discard_listener_dirty_bitmap(VFIOContainerBase *bcontainer,
>                                                  &vrdl);
>  }
>  
> +static int vfio_sync_iommu_dirty_bitmap(VFIOContainerBase *bcontainer,
> +                                        MemoryRegionSection *section)
> +{
> +    VFIOGuestIOMMU *giommu;
> +    bool found = false;
> +    Int128 llend;
> +    vfio_giommu_dirty_notifier gdn;
> +    int idx;
> +
> +    QLIST_FOREACH(giommu, &bcontainer->giommu_list, giommu_next) {
> +        if (MEMORY_REGION(giommu->iommu_mr) == section->mr &&
> +            giommu->n.start == section->offset_within_region) {
> +            found = true;
> +            break;
> +        }
> +    }
> +
> +    if (!found) {
> +        return 0;
> +    }
> +
> +    gdn.giommu = giommu;
> +    idx = memory_region_iommu_attrs_to_index(giommu->iommu_mr,
> +                                             MEMTXATTRS_UNSPECIFIED);
> +
> +    llend = int128_add(int128_make64(section->offset_within_region),
> +                       section->size);
> +    llend = int128_sub(llend, int128_one());
> +
> +    iommu_notifier_init(&gdn.n, vfio_iommu_map_dirty_notify, IOMMU_NOTIFIER_MAP,
> +                        section->offset_within_region, int128_get64(llend),
> +                        idx);
> +    memory_region_iommu_replay(giommu->iommu_mr, &gdn.n);
> +
> +    return 0;
if this does not return anything else than 0, shouldn't it be void?

Thanks

Eric
> +}
> +
>  static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer,
>                                    MemoryRegionSection *section, Error **errp)
>  {
>      ram_addr_t ram_addr;
>  
>      if (memory_region_is_iommu(section->mr)) {
> -        VFIOGuestIOMMU *giommu;
> -
> -        QLIST_FOREACH(giommu, &bcontainer->giommu_list, giommu_next) {
> -            if (MEMORY_REGION(giommu->iommu_mr) == section->mr &&
> -                giommu->n.start == section->offset_within_region) {
> -                Int128 llend;
> -                vfio_giommu_dirty_notifier gdn = { .giommu = giommu };
> -                int idx = memory_region_iommu_attrs_to_index(giommu->iommu_mr,
> -                                                       MEMTXATTRS_UNSPECIFIED);
> -
> -                llend = int128_add(int128_make64(section->offset_within_region),
> -                                   section->size);
> -                llend = int128_sub(llend, int128_one());
> -
> -                iommu_notifier_init(&gdn.n,
> -                                    vfio_iommu_map_dirty_notify,
> -                                    IOMMU_NOTIFIER_MAP,
> -                                    section->offset_within_region,
> -                                    int128_get64(llend),
> -                                    idx);
> -                memory_region_iommu_replay(giommu->iommu_mr, &gdn.n);
> -                break;
> -            }
> -        }
> -        return 0;
> +        return vfio_sync_iommu_dirty_bitmap(bcontainer, section);
>      } else if (memory_region_has_ram_discard_manager(section->mr)) {
>          int ret;
>