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.au...@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.au...@redhat.com>
>> Signed-off-by: Yi Liu <yi.l....@intel.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.d...@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)¶m,
>> + };
>> +
>> + if (!memory_region_iommu_get_attr(iommu_mr,
>> IOMMU_ATTR_SPAPR_TCE_FD,
>> + ¶m.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 <c...@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)¶m,
>> - };
>> -
>> - if (!memory_region_iommu_get_attr(iommu_mr,
>> IOMMU_ATTR_SPAPR_TCE_FD,
>> - ¶m.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)
>
