RE: [PATCH v4 22/41] vfio/spapr: switch to spapr IOMMU BE add/del_section_window

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@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.duan@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