Re: [PATCH v3 14/37] vfio/container: Move vrdl_list, pgsizes and dma_max_mappings to base container

[Cédric Le Goater]

On 10/30/23 04:14, Duan, Zhenzhong wrote:
Hi Cédric,
-----Original Message-----
From: Cédric Le Goater <c...@redhat.com>
Sent: Friday, October 27, 2023 11:52 PM
Subject: Re: [PATCH v3 14/37] vfio/container: Move vrdl_list, pgsizes and
dma_max_mappings to base container

On 10/26/23 12:30, Zhenzhong Duan wrote:
From: Eric Auger <eric.au...@redhat.com>

...

   void vfio_container_destroy(VFIOContainerBase *bcontainer)
   {
+    VFIORamDiscardListener *vrdl, *vrdl_tmp;
       VFIOGuestIOMMU *giommu, *tmp;

       QLIST_REMOVE(bcontainer, next);

+    QLIST_FOREACH_SAFE(vrdl, &bcontainer->vrdl_list, next, vrdl_tmp) {
+        RamDiscardManager *rdm;
+
+        rdm = memory_region_get_ram_discard_manager(vrdl->mr);
+        ram_discard_manager_unregister_listener(rdm, &vrdl->listener);
+        QLIST_REMOVE(vrdl, next);
+        g_free(vrdl);
+    }

Where was this done previously ?

Good catch! This should be removed.

May be the vrdl list should be handled
separatly from pgsizes and dma_max_mappings.

Good suggestion! Will do.


       QLIST_FOREACH_SAFE(giommu, &bcontainer->giommu_list, giommu_next,
tmp) {
           memory_region_unregister_iommu_notifier(
                   MEMORY_REGION(giommu->iommu_mr), &giommu->n);
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 8d5b408e86..0e265ffa67 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -154,7 +154,7 @@ static int vfio_legacy_dma_unmap(VFIOContainerBase
*bcontainer, hwaddr iova,
           if (errno == EINVAL && unmap.size && !(unmap.iova + unmap.size) &&
               container->iommu_type == VFIO_TYPE1v2_IOMMU) {
               trace_vfio_legacy_dma_unmap_overflow_workaround();
-            unmap.size -= 1ULL << ctz64(container->pgsizes);
+            unmap.size -= 1ULL << ctz64(container->bcontainer.pgsizes);
               continue;
           }
           error_report("VFIO_UNMAP_DMA failed: %s", strerror(errno));
@@ -559,9 +559,7 @@ static int vfio_connect_container(VFIOGroup *group,
AddressSpace *as,
       container = g_malloc0(sizeof(*container));
       container->fd = fd;
       container->error = NULL;
-    container->dma_max_mappings = 0;
       container->iova_ranges = NULL;
-    QLIST_INIT(&container->vrdl_list);
       bcontainer = &container->bcontainer;
       vfio_container_init(bcontainer, space, &vfio_legacy_ops);

@@ -589,13 +587,13 @@ static int vfio_connect_container(VFIOGroup *group,
AddressSpace *as,
           }

           if (info->flags & VFIO_IOMMU_INFO_PGSIZES) {
-            container->pgsizes = info->iova_pgsizes;
+            container->bcontainer.pgsizes = info->iova_pgsizes;
           } else {
-            container->pgsizes = qemu_real_host_page_size();
+            container->bcontainer.pgsizes = qemu_real_host_page_size();
           }

-        if (!vfio_get_info_dma_avail(info, &container->dma_max_mappings)) {
-            container->dma_max_mappings = 65535;
+        if (!vfio_get_info_dma_avail(info, &bcontainer->dma_max_mappings)) {
+            container->bcontainer.dma_max_mappings = 65535;
           }

           vfio_get_info_iova_range(info, container);
diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
index 3495737ab2..dbc4c24052 100644
--- a/hw/vfio/spapr.c
+++ b/hw/vfio/spapr.c
@@ -223,13 +223,13 @@ static int vfio_spapr_create_window(VFIOContainer
*container,
       if (pagesize > rampagesize) {
           pagesize = rampagesize;
       }
-    pgmask = container->pgsizes & (pagesize | (pagesize - 1));
+    pgmask = container->bcontainer.pgsizes & (pagesize | (pagesize - 1));
       pagesize = pgmask ? (1ULL << (63 - clz64(pgmask))) : 0;
       if (!pagesize) {
           error_report("Host doesn't support page size 0x%"PRIx64
                        ", the supported mask is 0x%lx",
                        memory_region_iommu_get_min_page_size(iommu_mr),
-                     container->pgsizes);
+                     container->bcontainer.pgsizes);
           return -EINVAL;
       }

@@ -385,7 +385,7 @@ void
vfio_container_del_section_window(VFIOContainer *container,

   bool vfio_spapr_container_init(VFIOContainer *container, Error **errp)
   {
-
+    VFIOContainerBase *bcontainer = &container->bcontainer;
       struct vfio_iommu_spapr_tce_info info;
       bool v2 = container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU;
       int ret, fd = container->fd;
@@ -424,7 +424,7 @@ bool vfio_spapr_container_init(VFIOContainer
*container, Error **errp)
       }

       if (v2) {
-        container->pgsizes = info.ddw.pgsizes;
+        bcontainer->pgsizes = info.ddw.pgsizes;
           /*
            * There is a default window in just created container.
            * To make region_add/del simpler, we better remove this
@@ -439,7 +439,7 @@ bool vfio_spapr_container_init(VFIOContainer
*container, Error **errp)
           }
       } else {
           /* The default table uses 4K pages */
-        container->pgsizes = 0x1000;
+        bcontainer->pgsizes = 0x1000;
           vfio_host_win_add(container, info.dma32_window_start,
                             info.dma32_window_start +
                             info.dma32_window_size - 1,
@@ -455,7 +455,15 @@ listener_unregister_exit:

   void vfio_spapr_container_deinit(VFIOContainer *container)
   {
+    VFIOHostDMAWindow *hostwin, *next;
+
       if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) {
           memory_listener_unregister(&container->prereg_listener);
       }
+    QLIST_FOREACH_SAFE(hostwin, &container->hostwin_list, hostwin_next,
+                       next) {
+        QLIST_REMOVE(hostwin, hostwin_next);
+        g_free(hostwin);
+    }
+
   }

I am sure this change  belongs to this patch.
       
"I am not sure" but you read my mind !


Good catch! Yes, I should move it into below patch.
"vfio/common: Move vfio_host_win_add/del into spapr.c"

yes.

Thanks,

C.





Thanks
Zhenzhong