[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v1 04/22] vfio/common: Introduce vfio_container_add|del_secti
|
From: |
Cédric Le Goater |
|
Subject: |
Re: [PATCH v1 04/22] vfio/common: Introduce vfio_container_add|del_section_window() |
|
Date: |
Thu, 21 Sep 2023 10:28:41 +0200 |
|
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 |
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.