Re: [PATCH v2 04/12] vfio/common: Introduce vfio_container_add|del_section_window()

["Cédric Le Goater" <clg@redhat.com>]

On 9/26/23 13:32, Zhenzhong Duan wrote:
> From: Eric Auger <eric.auger@redhat.com>
> 
> Introduce helper functions that isolate the code used for
> VFIO_SPAPR_TCE_v2_IOMMU.
> 
> Those helpers hide implementation details beneath the container object
> and make the vfio_listener_region_add/del() implementations more
> readable. No code change intended.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   hw/vfio/common.c | 156 +++++++++++++++++++++++++++--------------------
>   1 file changed, 89 insertions(+), 67 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 4e122fc4e4..de6b4a86e2 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -807,6 +807,92 @@ static bool vfio_get_section_iova_range(VFIOContainer *container,
>       return true;
>   }
>   
> +static int vfio_container_add_section_window(VFIOContainer *container,
> +                                             MemoryRegionSection *section,
> +                                             Error **errp)
> +{
> +    VFIOHostDMAWindow *hostwin;
> +    hwaddr pgsize = 0;
> +    int ret;
> +
> +    if (container->iommu_type != VFIO_SPAPR_TCE_v2_IOMMU) {
> +        return 0;
> +    }
> +
> +    /* For now intersections are not allowed, we may relax this later */
> +    QLIST_FOREACH(hostwin, &container->hostwin_list, hostwin_next) {
> +        if (ranges_overlap(hostwin->min_iova,
> +                           hostwin->max_iova - hostwin->min_iova + 1,
> +                           section->offset_within_address_space,
> +                           int128_get64(section->size))) {
> +            error_setg(errp,
> +                "region [0x%"PRIx64",0x%"PRIx64"] overlaps with existing"
> +                "host DMA window [0x%"PRIx64",0x%"PRIx64"]",
> +                section->offset_within_address_space,
> +                section->offset_within_address_space +
> +                    int128_get64(section->size) - 1,
> +                hostwin->min_iova, hostwin->max_iova);
> +            return -EINVAL;
> +        }
> +    }
> +
> +    ret = vfio_spapr_create_window(container, section, &pgsize);
> +    if (ret) {
> +        error_setg_errno(errp, -ret, "Failed to create SPAPR window");
> +        return ret;
> +    }
> +
> +    vfio_host_win_add(container, section->offset_within_address_space,
> +                      section->offset_within_address_space +
> +                      int128_get64(section->size) - 1, pgsize);
> +#ifdef CONFIG_KVM
> +    if (kvm_enabled()) {
> +        VFIOGroup *group;
> +        IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(section->mr);
> +        struct kvm_vfio_spapr_tce param;
> +        struct kvm_device_attr attr = {
> +            .group = KVM_DEV_VFIO_GROUP,
> +            .attr = KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE,
> +            .addr = (uint64_t)(unsigned long)&param,
> +        };
> +
> +        if (!memory_region_iommu_get_attr(iommu_mr, IOMMU_ATTR_SPAPR_TCE_FD,
> +                                          &param.tablefd)) {
> +            QLIST_FOREACH(group, &container->group_list, container_next) {
> +                param.groupfd = group->fd;
> +                if (ioctl(vfio_kvm_device_fd, KVM_SET_DEVICE_ATTR, &attr)) {
> +                    error_report("vfio: failed to setup fd %d "
> +                                 "for a group with fd %d: %s",
> +                                 param.tablefd, param.groupfd,
> +                                 strerror(errno));
> +                    return 0;

please return errno;

Otherwise,

Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.

> +                }
> +                trace_vfio_spapr_group_attach(param.groupfd, param.tablefd);
> +            }
> +        }
> +    }
> +#endif
> +    return 0;
> +}
> +
> +static void vfio_container_del_section_window(VFIOContainer *container,
> +                                              MemoryRegionSection *section)
> +{
> +    if (container->iommu_type != VFIO_SPAPR_TCE_v2_IOMMU) {
> +        return;
> +    }
> +
> +    vfio_spapr_remove_window(container,
> +                             section->offset_within_address_space);
> +    if (vfio_host_win_del(container,
> +                          section->offset_within_address_space,
> +                          section->offset_within_address_space +
> +                          int128_get64(section->size) - 1) < 0) {
> +        hw_error("%s: Cannot delete missing window at %"HWADDR_PRIx,
> +                 __func__, section->offset_within_address_space);
> +    }
> +}
> +
>   static void vfio_listener_region_add(MemoryListener *listener,
>                                        MemoryRegionSection *section)
>   {
> @@ -833,62 +919,8 @@ static void vfio_listener_region_add(MemoryListener *listener,
>           return;
>       }
>   
> -    if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) {
> -        hwaddr pgsize = 0;
> -
> -        /* For now intersections are not allowed, we may relax this later */
> -        QLIST_FOREACH(hostwin, &container->hostwin_list, hostwin_next) {
> -            if (ranges_overlap(hostwin->min_iova,
> -                               hostwin->max_iova - hostwin->min_iova + 1,
> -                               section->offset_within_address_space,
> -                               int128_get64(section->size))) {
> -                error_setg(&err,
> -                    "region [0x%"PRIx64",0x%"PRIx64"] overlaps with existing"
> -                    "host DMA window [0x%"PRIx64",0x%"PRIx64"]",
> -                    section->offset_within_address_space,
> -                    section->offset_within_address_space +
> -                        int128_get64(section->size) - 1,
> -                    hostwin->min_iova, hostwin->max_iova);
> -                goto fail;
> -            }
> -        }
> -
> -        ret = vfio_spapr_create_window(container, section, &pgsize);
> -        if (ret) {
> -            error_setg_errno(&err, -ret, "Failed to create SPAPR window");
> -            goto fail;
> -        }
> -
> -        vfio_host_win_add(container, section->offset_within_address_space,
> -                          section->offset_within_address_space +
> -                          int128_get64(section->size) - 1, pgsize);
> -#ifdef CONFIG_KVM
> -        if (kvm_enabled()) {
> -            VFIOGroup *group;
> -            IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(section->mr);
> -            struct kvm_vfio_spapr_tce param;
> -            struct kvm_device_attr attr = {
> -                .group = KVM_DEV_VFIO_GROUP,
> -                .attr = KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE,
> -                .addr = (uint64_t)(unsigned long)&param,
> -            };
> -
> -            if (!memory_region_iommu_get_attr(iommu_mr, IOMMU_ATTR_SPAPR_TCE_FD,
> -                                              &param.tablefd)) {
> -                QLIST_FOREACH(group, &container->group_list, container_next) {
> -                    param.groupfd = group->fd;
> -                    if (ioctl(vfio_kvm_device_fd, KVM_SET_DEVICE_ATTR, &attr)) {
> -                        error_report("vfio: failed to setup fd %d "
> -                                     "for a group with fd %d: %s",
> -                                     param.tablefd, param.groupfd,
> -                                     strerror(errno));
> -                        return;
> -                    }
> -                    trace_vfio_spapr_group_attach(param.groupfd, param.tablefd);
> -                }
> -            }
> -        }
> -#endif
> +    if (vfio_container_add_section_window(container, section, &err)) {
> +        goto fail;
>       }
>   
>       hostwin = vfio_find_hostwin(container, iova, end);
> @@ -1105,17 +1137,7 @@ static void vfio_listener_region_del(MemoryListener *listener,
>   
>       memory_region_unref(section->mr);
>   
> -    if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) {
> -        vfio_spapr_remove_window(container,
> -                                 section->offset_within_address_space);
> -        if (vfio_host_win_del(container,
> -                              section->offset_within_address_space,
> -                              section->offset_within_address_space +
> -                              int128_get64(section->size) - 1) < 0) {
> -            hw_error("%s: Cannot delete missing window at %"HWADDR_PRIx,
> -                     __func__, section->offset_within_address_space);
> -        }
> -    }
> +    vfio_container_del_section_window(container, section);
>   }
>   
>   static int vfio_set_dirty_page_tracking(VFIOContainer *container, bool start)