| Message ID | 20230926113255.1177834-5-zhenzhong.duan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Prerequisite change for IOMMUFD support | expand |
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)¶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; 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)¶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)
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)¶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 <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)¶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) >
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; + } + 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)