| Message ID | 20231026103104.1686921-8-zhenzhong.duan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio: Adopt iommufd | expand |
On 10/26/23 12:30, Zhenzhong Duan wrote: > This empty VFIOIOMMUOps named vfio_legacy_ops will hold all general > IOMMU ops of legacy container. > > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > include/hw/vfio/vfio-common.h | 2 +- > hw/vfio/container.c | 5 +++++ > 2 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h > index d8f293cb57..8ded5cd8e4 100644 > --- a/include/hw/vfio/vfio-common.h > +++ b/include/hw/vfio/vfio-common.h > @@ -255,7 +255,7 @@ typedef QLIST_HEAD(VFIOGroupList, VFIOGroup) VFIOGroupList; > typedef QLIST_HEAD(VFIODeviceList, VFIODevice) VFIODeviceList; > extern VFIOGroupList vfio_group_list; > extern VFIODeviceList vfio_device_list; > - > +extern const VFIOIOMMUOps vfio_legacy_ops; why does it need to be external ? Thanks, C. > extern const MemoryListener vfio_memory_listener; > extern int vfio_kvm_device_fd; > > diff --git a/hw/vfio/container.c b/hw/vfio/container.c > index 242010036a..4bc43ddfa4 100644 > --- a/hw/vfio/container.c > +++ b/hw/vfio/container.c > @@ -472,6 +472,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as, > Error **errp) > { > VFIOContainer *container; > + VFIOContainerBase *bcontainer; > int ret, fd; > VFIOAddressSpace *space; > > @@ -552,6 +553,8 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as, > container->iova_ranges = NULL; > QLIST_INIT(&container->giommu_list); > QLIST_INIT(&container->vrdl_list); > + bcontainer = &container->bcontainer; > + bcontainer->ops = &vfio_legacy_ops; > > ret = vfio_init_container(container, group->fd, errp); > if (ret) { > @@ -933,3 +936,5 @@ void vfio_detach_device(VFIODevice *vbasedev) > vfio_put_base_device(vbasedev); > vfio_put_group(group); > } > + > +const VFIOIOMMUOps vfio_legacy_ops;
>-----Original Message----- >From: Cédric Le Goater <clg@redhat.com> >Sent: Friday, October 27, 2023 10:21 PM >Subject: Re: [PATCH v3 07/37] vfio/container: Introduce a empty >VFIOIOMMUOps > >On 10/26/23 12:30, Zhenzhong Duan wrote: >> This empty VFIOIOMMUOps named vfio_legacy_ops will hold all general >> IOMMU ops of legacy container. >> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> include/hw/vfio/vfio-common.h | 2 +- >> hw/vfio/container.c | 5 +++++ >> 2 files changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h >> index d8f293cb57..8ded5cd8e4 100644 >> --- a/include/hw/vfio/vfio-common.h >> +++ b/include/hw/vfio/vfio-common.h >> @@ -255,7 +255,7 @@ typedef QLIST_HEAD(VFIOGroupList, VFIOGroup) >VFIOGroupList; >> typedef QLIST_HEAD(VFIODeviceList, VFIODevice) VFIODeviceList; >> extern VFIOGroupList vfio_group_list; >> extern VFIODeviceList vfio_device_list; >> - >> +extern const VFIOIOMMUOps vfio_legacy_ops; > > >why does it need to be external ? It is referenced by vfio_connect_container() and vfio_attach_device(). Thanks Zhenzhong
On 10/30/23 03:43, Duan, Zhenzhong wrote: > > >> -----Original Message----- >> From: Cédric Le Goater <clg@redhat.com> >> Sent: Friday, October 27, 2023 10:21 PM >> Subject: Re: [PATCH v3 07/37] vfio/container: Introduce a empty >> VFIOIOMMUOps >> >> On 10/26/23 12:30, Zhenzhong Duan wrote: >>> This empty VFIOIOMMUOps named vfio_legacy_ops will hold all general >>> IOMMU ops of legacy container. >>> >>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>> --- >>> include/hw/vfio/vfio-common.h | 2 +- >>> hw/vfio/container.c | 5 +++++ >>> 2 files changed, 6 insertions(+), 1 deletion(-) >>> >>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h >>> index d8f293cb57..8ded5cd8e4 100644 >>> --- a/include/hw/vfio/vfio-common.h >>> +++ b/include/hw/vfio/vfio-common.h >>> @@ -255,7 +255,7 @@ typedef QLIST_HEAD(VFIOGroupList, VFIOGroup) >> VFIOGroupList; >>> typedef QLIST_HEAD(VFIODeviceList, VFIODevice) VFIODeviceList; >>> extern VFIOGroupList vfio_group_list; >>> extern VFIODeviceList vfio_device_list; >>> - >>> +extern const VFIOIOMMUOps vfio_legacy_ops; >> >> >> why does it need to be external ? > > It is referenced by vfio_connect_container() and vfio_attach_device(). Yes. I realized that later on. The backend is chosen when the device id attached : int vfio_attach_device(char *name, VFIODevice *vbasedev, AddressSpace *as, Error **errp) { const VFIOIOMMUOps *ops; if (vbasedev->iommufd) { ops = &vfio_iommufd_ops; } else { ops = &vfio_legacy_ops; } return ops->attach_device(name, vbasedev, as, errp); } To be noted that we don't need the backend ops but the attach_device() handler only. And then, the backend ops is assigned to the base container deeper in the call stack with vfio_container_init(), which is a bit like a chicken and egg problem to me. vfio_legacy_ops.attach_device = vfio_legacy_attach_device() vfio_get_group() vfio_connect_container() vfio_container_init(&vfio_legacy_ops) vfio_iommufd_ops.attach_device = iommufd_attach_device() vfio_container_init(&vfio_iommufd_ops) vfio_legacy_attach_device() and iommufd_attach_device() are similar but have different requirements. I don't see a good alternative. Unless we introduce a QOM object wrapping the IOMMUFDBackend and the legacy one to hold the VFIOIOMMUOps struct. Looks overkill. Thanks, C.
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h index d8f293cb57..8ded5cd8e4 100644 --- a/include/hw/vfio/vfio-common.h +++ b/include/hw/vfio/vfio-common.h @@ -255,7 +255,7 @@ typedef QLIST_HEAD(VFIOGroupList, VFIOGroup) VFIOGroupList; typedef QLIST_HEAD(VFIODeviceList, VFIODevice) VFIODeviceList; extern VFIOGroupList vfio_group_list; extern VFIODeviceList vfio_device_list; - +extern const VFIOIOMMUOps vfio_legacy_ops; extern const MemoryListener vfio_memory_listener; extern int vfio_kvm_device_fd; diff --git a/hw/vfio/container.c b/hw/vfio/container.c index 242010036a..4bc43ddfa4 100644 --- a/hw/vfio/container.c +++ b/hw/vfio/container.c @@ -472,6 +472,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as, Error **errp) { VFIOContainer *container; + VFIOContainerBase *bcontainer; int ret, fd; VFIOAddressSpace *space; @@ -552,6 +553,8 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as, container->iova_ranges = NULL; QLIST_INIT(&container->giommu_list); QLIST_INIT(&container->vrdl_list); + bcontainer = &container->bcontainer; + bcontainer->ops = &vfio_legacy_ops; ret = vfio_init_container(container, group->fd, errp); if (ret) { @@ -933,3 +936,5 @@ void vfio_detach_device(VFIODevice *vbasedev) vfio_put_base_device(vbasedev); vfio_put_group(group); } + +const VFIOIOMMUOps vfio_legacy_ops;
This empty VFIOIOMMUOps named vfio_legacy_ops will hold all general IOMMU ops of legacy container. Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- include/hw/vfio/vfio-common.h | 2 +- hw/vfio/container.c | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-)