>-----Original Message-----
>From: Cédric Le Goater <c...@redhat.com>
>Sent: Tuesday, November 7, 2023 1:33 AM
>Subject: Re: [PATCH v4 22/41] vfio/spapr: switch to spapr IOMMU BE
>add/del_section_window
>
>On 11/2/23 08:12, Zhenzhong Duan wrote:
>> No fucntional change intended.
>>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.d...@intel.com>
>> ---
>> include/hw/vfio/vfio-common.h | 5 -----
>> include/hw/vfio/vfio-container-base.h | 5 +++++
>> hw/vfio/common.c | 8 ++------
>> hw/vfio/container-base.c | 21 +++++++++++++++++++++
>> hw/vfio/spapr.c | 19 ++++++++++++++-----
>> 5 files changed, 42 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>> index b9e5a0e64b..055f679363 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -169,11 +169,6 @@ VFIOAddressSpace
>*vfio_get_address_space(AddressSpace *as);
>> void vfio_put_address_space(VFIOAddressSpace *space);
>>
>> /* SPAPR specific */
>> -int vfio_container_add_section_window(VFIOContainer *container,
>> - MemoryRegionSection *section,
>> - Error **errp);
>> -void vfio_container_del_section_window(VFIOContainer *container,
>> - MemoryRegionSection *section);
>> int vfio_spapr_container_init(VFIOContainer *container, Error **errp);
>> void vfio_spapr_container_deinit(VFIOContainer *container);
>>
>> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-
>container-base.h
>> index f62a14ac73..4b6f017c6f 100644
>> --- a/include/hw/vfio/vfio-container-base.h
>> +++ b/include/hw/vfio/vfio-container-base.h
>> @@ -75,6 +75,11 @@ int vfio_container_dma_map(VFIOContainerBase
>*bcontainer,
>> int vfio_container_dma_unmap(VFIOContainerBase *bcontainer,
>> hwaddr iova, ram_addr_t size,
>> IOMMUTLBEntry *iotlb);
>> +int vfio_container_add_section_window(VFIOContainerBase *bcontainer,
>> + MemoryRegionSection *section,
>> + Error **errp);
>> +void vfio_container_del_section_window(VFIOContainerBase *bcontainer,
>> + MemoryRegionSection *section);
>> int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
>> bool start);
>> int vfio_container_query_dirty_bitmap(VFIOContainerBase *bcontainer,
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 483ba82089..572ae7c934 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -571,8 +571,6 @@ static void vfio_listener_region_add(MemoryListener
>*listener,
>> {
>> VFIOContainerBase *bcontainer = container_of(listener,
>> VFIOContainerBase,
>> listener);
>> - VFIOContainer *container = container_of(bcontainer, VFIOContainer,
>> - bcontainer);
>> hwaddr iova, end;
>> Int128 llend, llsize;
>> void *vaddr;
>> @@ -595,7 +593,7 @@ static void vfio_listener_region_add(MemoryListener
>*listener,
>> return;
>> }
>>
>> - if (vfio_container_add_section_window(container, section, &err)) {
>> + if (vfio_container_add_section_window(bcontainer, section, &err)) {
>> goto fail;
>> }
>>
>> @@ -738,8 +736,6 @@ static void vfio_listener_region_del(MemoryListener
>*listener,
>> {
>> VFIOContainerBase *bcontainer = container_of(listener,
>> VFIOContainerBase,
>> listener);
>> - VFIOContainer *container = container_of(bcontainer, VFIOContainer,
>> - bcontainer);
>> hwaddr iova, end;
>> Int128 llend, llsize;
>> int ret;
>> @@ -818,7 +814,7 @@ static void vfio_listener_region_del(MemoryListener
>*listener,
>>
>> memory_region_unref(section->mr);
>>
>> - vfio_container_del_section_window(container, section);
>> + vfio_container_del_section_window(bcontainer, section);
>> }
>>
>> typedef struct VFIODirtyRanges {
>> diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
>> index 0177f43741..71f7274973 100644
>> --- a/hw/vfio/container-base.c
>> +++ b/hw/vfio/container-base.c
>> @@ -31,6 +31,27 @@ int vfio_container_dma_unmap(VFIOContainerBase
>*bcontainer,
>> return bcontainer->ops->dma_unmap(bcontainer, iova, size, iotlb);
>> }
>>
>> +int vfio_container_add_section_window(VFIOContainerBase *bcontainer,
>> + MemoryRegionSection *section,
>> + Error **errp)
>> +{
>> + if (!bcontainer->ops->add_window) {
>> + return 0;
>> + }
>
>These should an assert right ? because only called on the pseries
>platform which defines the handlers.
Because we use a unified vfio_memory_listener for legacy, spapr and iommufd
backend, so we need the check for legacy and iommufd backend.
Another choice is to introduce separate region_add/del callbacks for spapr,
then we can add assert. But that way we will have redundant code.
Thanks
Zhenzhong
