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

[Eric Auger]

Hi Cédric,

On 9/27/23 12:12, Cédric Le Goater wrote:
> 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;
I agree this would be logical to return -errno. However in the original
code this path ends up to a return and not to
goto fail;

So if we return an error we do a functional change here. And if we keep
the existing behavior I agree we should add a comment.

Thanks

Eric
>
> 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)
>