Re: [PATCH for-9.0 03/10] vfio/container: Initialize VFIOIOMMUOps under vfio_init_container()

[Cédric Le Goater]

On 12/11/23 06:59, Duan, Zhenzhong wrote:
> > -----Original Message-----
> > From: Cédric Le Goater <clg@redhat.com>
> > Sent: Friday, December 8, 2023 4:46 PM
> > Subject: [PATCH for-9.0 03/10] vfio/container: Initialize VFIOIOMMUOps
> > under vfio_init_container()
> >
> > vfio_init_container() already defines the IOMMU type of the container.
> > Do the same for the VFIOIOMMUOps struct. This prepares ground for the
> > following patches that will deduce the associated VFIOIOMMUOps struct
> > from the IOMMU type.
> >
> > Signed-off-by: Cédric Le Goater <clg@redhat.com>
> > ---
> > hw/vfio/container.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> > index
> > afcfe8048805c58291d1104ff0ef20bdc457f99c..f4a0434a5239bfb6a17b91c8
> > 879cb98e686afccc 100644
> > --- a/hw/vfio/container.c
> > +++ b/hw/vfio/container.c
> > @@ -370,7 +370,7 @@ static int vfio_get_iommu_type(VFIOContainer
> > *container,
> > }
> >
> > static int vfio_init_container(VFIOContainer *container, int group_fd,
> > -                               Error **errp)
> > +                               VFIOAddressSpace *space, Error **errp)
> > {
> >      int iommu_type, ret;
> >
> > @@ -401,6 +401,7 @@ static int vfio_init_container(VFIOContainer
> > *container, int group_fd,
> >      }
> >
> >      container->iommu_type = iommu_type;
> > +    vfio_container_init(&container->bcontainer, space, &vfio_legacy_ops);
>
> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>
> Not related to this patch, not clear if it's deserved to rename
> vfio_container_init as vfio_bcontainer_init to distinguish
> with vfio_init_container.

I agree, the vfio_container_init() and vfio_init_container() names
are confusing. I would keep vfio_container_init() because it is
consistent with all routines handling 'VFIOContainerBase *' ops.

I would be tempted to rename vfio_init_container() to vfio_set_iommu() ?

Also, I will introduce a vfio_connect_setup() helper in v2 doing the
assert as the other routines.

Thanks,

C.





> Thanks
> Zhenzhong
>
> >      return 0;
> > }
> >
> > @@ -583,9 +584,8 @@ static int vfio_connect_container(VFIOGroup *group,
> > AddressSpace *as,
> >      container = g_malloc0(sizeof(*container));
> >      container->fd = fd;
> >      bcontainer = &container->bcontainer;
> > -    vfio_container_init(bcontainer, space, &vfio_legacy_ops);
> >
> > -    ret = vfio_init_container(container, group->fd, errp);
> > +    ret = vfio_init_container(container, group->fd, space, errp);
> >      if (ret) {
> >          goto free_container_exit;
> >      }
> > --
> > 2.43.0