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

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

On 9/21/23 12:14, Duan, Zhenzhong wrote:
> 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?

I will see if we can move out of VFIO the implementation of the spapr routines.
Don't wait for me. It can be addressed in parallel.

Thanks,

C.


> 
> Thanks
> Zhenzhong
>