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

[Cédric Le Goater]

On 11/7/23 04:06, Duan, Zhenzhong wrote:
> > -----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.

Yeah. I thought so. I will take a look. I have more comments to come.

Thanks,

C.