RE: [PATCH v1 04/22] vfio/common: Introduce vfio_container_add|del_section_window()

[Duan, Zhenzhong]

Hi Cédric,

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Sent: Thursday, September 21, 2023 4:29 PM
>Subject: Re: [PATCH v1 04/22] vfio/common: Introduce
>vfio_container_add|del_section_window()
>
>Hello Zhenzhong,
>
>On 8/30/23 12:37, Zhenzhong Duan wrote:
>> From: Eric Auger <eric.auger@redhat.com>
>>
>> Introduce helper functions that isolate the code used for
>> VFIO_SPAPR_TCE_v2_IOMMU. This code reliance is IOMMU backend
>> specific whereas the rest of the code in the callers, ie.
>> vfio_listener_region_add|del is not.
>>
>> 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 9ca695837f..67150e4575 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -796,6 +796,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;
>> +    }
>
>This test makes me think that we should register a specific backend
>for the pseries machines, implementing the add/del_window handler,
>since others do not need it. Correct ?

Yes, introducing a specific backend could help removing above check.
But each backend has a VFIOIOMMUBackendOps, we need same check
as above to select Ops.

>
>It would avoid this ugly test. Let's keep that in mind when the
>backends are introduced.
>
>> +
>> +    /* 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
>
>the ifdef test doesn't seem useful because the compiler should compile
>out the section below since, in that case, kvm_enabled() is defined as :
>
>   #define kvm_enabled()           (0)

Looks so, I'll remove it in v2.

>
>> +    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;
>
>hmm, the code bails out directly without undoing previous actions. we should
>return some error at least.

I think Eric doesn't intend any functional change in this patch, just refactor 
these
code into two wrapper functions. In fact the original code just return void,
if ioctl() fails. Not clear if that's intentional or a bug.

>
>> +                }
>> +                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)
>>   {
>> @@ -822,62 +908,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;
>
>That's not exactly the same as the return above when the ioctl call
>fails. there doesn't seem to be much consequences though. Let's keep
>it that way.
OK.

>
>>       }
>>
>>       hostwin = vfio_find_hostwin(container, iova, end);
>> @@ -1094,17 +1126,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)
>
>PPC is in the way. May be we could move these two routines in pseries to
>help a little. I will look into it.
Do you mean PPC cleanup?

Thanks
Zhenzhong