Re: [PATCH v1 04/22] vfio/common: Introduce vfio_container_add|del_section_window()

[Cédric Le Goater]

On 9/27/23 04:08, Duan, Zhenzhong wrote:
> Hi Cédric,
>
> > -----Original Message-----
> > From: Duan, Zhenzhong
> > Sent: Thursday, September 21, 2023 6:14 PM
> > Subject: RE: [PATCH v1 04/22] vfio/common: Introduce
> > vfio_container_add|del_section_window()
> >
> > Hi Cédric,
> >
> > > -----Original Message-----
> > > From: Cédric Le Goater <clg@redhat.com>
> > > Sent: Thursday, September 21, 2023 4:29 PM
> > > Subject: Re: [PATCH v1 04/22] vfio/common: Introduce
> > > vfio_container_add|del_section_window()
> > >
> > > Hello Zhenzhong,
> > >
> > > On 8/30/23 12:37, Zhenzhong Duan wrote:
> > > > From: Eric Auger <eric.auger@redhat.com>
> > > >
> > > > Introduce helper functions that isolate the code used for
> > > > VFIO_SPAPR_TCE_v2_IOMMU. This code reliance is IOMMU backend
> > > > specific whereas the rest of the code in the callers, ie.
> > > > vfio_listener_region_add|del is not.
> > > >
> > > > Signed-off-by: Eric Auger <eric.auger@redhat.com>
> > > > Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> > > > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> > > > ---
> > > >    hw/vfio/common.c | 156 +++++++++++++++++++++++++++--------------------
> > > >    1 file changed, 89 insertions(+), 67 deletions(-)
> > > >
> > > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > > > index 9ca695837f..67150e4575 100644
> > > > --- a/hw/vfio/common.c
> > > > +++ b/hw/vfio/common.c
> > > > @@ -796,6 +796,92 @@ static bool
> > > vfio_get_section_iova_range(VFIOContainer *container,
> > > >        return true;
> > > >    }
> > > >
> > > > +static int vfio_container_add_section_window(VFIOContainer *container,
> > > > +                                             MemoryRegionSection *section,
> > > > +                                             Error **errp)
> > > > +{
> > > > +    VFIOHostDMAWindow *hostwin;
> > > > +    hwaddr pgsize = 0;
> > > > +    int ret;
> > > > +
> > > > +    if (container->iommu_type != VFIO_SPAPR_TCE_v2_IOMMU) {
> > > > +        return 0;
> > > > +    }
> > >
> > > This test makes me think that we should register a specific backend
> > > for the pseries machines, implementing the add/del_window handler,
> > > since others do not need it. Correct ?
> >
> > Yes, introducing a specific backend could help removing above check.
> > But each backend has a VFIOIOMMUBackendOps, we need same check
> > as above to select Ops.
> >
> > > It would avoid this ugly test. Let's keep that in mind when the
> > > backends are introduced.
> > >
> > > > +
> > > > +    /* For now intersections are not allowed, we may relax this later */
> > > > +    QLIST_FOREACH(hostwin, &container->hostwin_list, hostwin_next) {
> > > > +        if (ranges_overlap(hostwin->min_iova,
> > > > +                           hostwin->max_iova - hostwin->min_iova + 1,
> > > > +                           section->offset_within_address_space,
> > > > +                           int128_get64(section->size))) {
> > > > +            error_setg(errp,
> > > > +                "region [0x%"PRIx64",0x%"PRIx64"] overlaps with existing"
> > > > +                "host DMA window [0x%"PRIx64",0x%"PRIx64"]",
> > > > +                section->offset_within_address_space,
> > > > +                section->offset_within_address_space +
> > > > +                    int128_get64(section->size) - 1,
> > > > +                hostwin->min_iova, hostwin->max_iova);
> > > > +            return -EINVAL;
> > > > +        }
> > > > +    }
> > > > +
> > > > +    ret = vfio_spapr_create_window(container, section, &pgsize);
> > > > +    if (ret) {
> > > > +        error_setg_errno(errp, -ret, "Failed to create SPAPR window");
> > > > +        return ret;
> > > > +    }
> > > > +
> > > > +    vfio_host_win_add(container, section->offset_within_address_space,
> > > > +                      section->offset_within_address_space +
> > > > +                      int128_get64(section->size) - 1, pgsize);
> > > > +#ifdef CONFIG_KVM
> > >
> > > the ifdef test doesn't seem useful because the compiler should compile
> > > out the section below since, in that case, kvm_enabled() is defined as :
> > >
> > >    #define kvm_enabled()           (0)
> >
> > Looks so, I'll remove it in v2.
>
> Forgot to let you know, finally I failed to remove the ifdef test in v2 due to
> many "undeclared" compile errors. I guess the reason is grammatical check
> Is triggered before optimization in compiler.
>
> For example:
> error: ‘KVM_DEV_VFIO_GROUP’ undeclared
> error: ‘vfio_kvm_device_fd’ undeclared


Yes. It would need helpers to hide the kernel structs and defined.
Let's address it later, after the backends are introduced.

Thanks for looking into it.

C.