| Message ID | 20230830103754.36461-5-zhenzhong.duan@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | vfio: Adopt iommufd | expand |
Hi 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. this last sentence should be rephrased into something like Those helpers hide implementation details beneath the container object and make the vfio_listener_region_add/del() implementations more readable ( I think). No code change intended. Thanks Eric > 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; > + } > + > + /* 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) > { > @@ -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)¶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); > @@ -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)
>-----Original Message----- >From: Eric Auger <eric.auger@redhat.com> >Sent: Wednesday, September 20, 2023 7:23 PM >Subject: Re: [PATCH v1 04/22] vfio/common: Introduce >vfio_container_add|del_section_window() > >Hi 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. >this last sentence should be rephrased into something like >Those helpers hide implementation details beneath the container object >and make the vfio_listener_region_add/del() implementations more >readable ( I think). No code change intended. Thanks for your suggestion, will use it in v2. BR. Zhenzhong
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 ? 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) > + 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; hmm, the code bails out directly without undoing previous actions. we should return some error at least. > + } > + 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)¶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; 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. > } > > 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. Thanks, C.
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)¶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; > >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)¶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; > >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
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)¶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; >> >> 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)¶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; >> >> 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 >
Hi Cédric, >-----Original Message----- >From: Duan, Zhenzhong >Sent: Thursday, September 21, 2023 6:14 PM >Subject: RE: [PATCH v1 04/22] vfio/common: Introduce >vfio_container_add|del_section_window() > >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. Forgot to let you know, finally I failed to remove the ifdef test in v2 due to many "undeclared" compile errors. I guess the reason is grammatical check Is triggered before optimization in compiler. For example: error: ‘KVM_DEV_VFIO_GROUP’ undeclared error: ‘vfio_kvm_device_fd’ undeclared ... Thanks Zhenzhong
On 9/27/23 04:08, Duan, Zhenzhong wrote: > Hi Cédric, > >> -----Original Message----- >> From: Duan, Zhenzhong >> Sent: Thursday, September 21, 2023 6:14 PM >> Subject: RE: [PATCH v1 04/22] vfio/common: Introduce >> vfio_container_add|del_section_window() >> >> 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. > > Forgot to let you know, finally I failed to remove the ifdef test in v2 due to > many "undeclared" compile errors. I guess the reason is grammatical check > Is triggered before optimization in compiler. > > For example: > error: ‘KVM_DEV_VFIO_GROUP’ undeclared > error: ‘vfio_kvm_device_fd’ undeclared Yes. It would need helpers to hide the kernel structs and defined. Let's address it later, after the backends are introduced. Thanks for looking into it. C.
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; + } + + /* 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) { @@ -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)¶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); @@ -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)