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?