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 <clg@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.auger@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