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

[Duan, Zhenzhong]

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.

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

Thanks
Zhenzhong