| Message ID | 20240429065046.3688701-14-zhenzhong.duan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add a host IOMMU device abstraction to check with vIOMMU | expand |
On 4/29/24 08:50, Zhenzhong Duan wrote: > Create host IOMMU device instance in vfio_attach_device() and call > .realize() to initialize it further. > > Suggested-by: Cédric Le Goater <clg@redhat.com> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > include/hw/vfio/vfio-common.h | 1 + > hw/vfio/common.c | 18 +++++++++++++++++- > 2 files changed, 18 insertions(+), 1 deletion(-) > > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h > index 0943add3bc..b204b93a55 100644 > --- a/include/hw/vfio/vfio-common.h > +++ b/include/hw/vfio/vfio-common.h > @@ -126,6 +126,7 @@ typedef struct VFIODevice { > OnOffAuto pre_copy_dirty_page_tracking; > bool dirty_pages_supported; > bool dirty_tracking; > + HostIOMMUDevice *hiod; > int devid; > IOMMUFDBackend *iommufd; > } VFIODevice; > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index 8f9cbdc026..0be8b70ebd 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -1497,6 +1497,8 @@ int vfio_attach_device(char *name, VFIODevice *vbasedev, > { > const VFIOIOMMUClass *ops = > VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY)); > + HostIOMMUDevice *hiod; > + int ret; > > if (vbasedev->iommufd) { > ops = VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD)); > @@ -1504,7 +1506,20 @@ int vfio_attach_device(char *name, VFIODevice *vbasedev, > > assert(ops); > > - return ops->attach_device(name, vbasedev, as, errp); > + ret = ops->attach_device(name, vbasedev, as, errp); > + if (ret < 0) { > + return ret; hmm, I wonder if we should change the return value of vfio_attach_device() to be a bool. Thanks, C. > + } > + > + hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename)); > + if (!HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp)) { > + object_unref(hiod); > + ops->detach_device(vbasedev); > + return -EINVAL; > + } > + vbasedev->hiod = hiod; > + > + return 0; > } > > void vfio_detach_device(VFIODevice *vbasedev) > @@ -1512,5 +1527,6 @@ void vfio_detach_device(VFIODevice *vbasedev) > if (!vbasedev->bcontainer) { > return; > } > + object_unref(vbasedev->hiod); > vbasedev->bcontainer->ops->detach_device(vbasedev); > }
>-----Original Message----- >From: Cédric Le Goater <clg@redhat.com> >Subject: Re: [PATCH v3 13/19] vfio: Create host IOMMU device instance > >On 4/29/24 08:50, Zhenzhong Duan wrote: >> Create host IOMMU device instance in vfio_attach_device() and call >> .realize() to initialize it further. >> >> Suggested-by: Cédric Le Goater <clg@redhat.com> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> include/hw/vfio/vfio-common.h | 1 + >> hw/vfio/common.c | 18 +++++++++++++++++- >> 2 files changed, 18 insertions(+), 1 deletion(-) >> >> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio- >common.h >> index 0943add3bc..b204b93a55 100644 >> --- a/include/hw/vfio/vfio-common.h >> +++ b/include/hw/vfio/vfio-common.h >> @@ -126,6 +126,7 @@ typedef struct VFIODevice { >> OnOffAuto pre_copy_dirty_page_tracking; >> bool dirty_pages_supported; >> bool dirty_tracking; >> + HostIOMMUDevice *hiod; >> int devid; >> IOMMUFDBackend *iommufd; >> } VFIODevice; >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index 8f9cbdc026..0be8b70ebd 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -1497,6 +1497,8 @@ int vfio_attach_device(char *name, VFIODevice >*vbasedev, >> { >> const VFIOIOMMUClass *ops = >> >VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY)); >> + HostIOMMUDevice *hiod; >> + int ret; >> >> if (vbasedev->iommufd) { >> ops = >VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUF >D)); >> @@ -1504,7 +1506,20 @@ int vfio_attach_device(char *name, >VFIODevice *vbasedev, >> >> assert(ops); >> >> - return ops->attach_device(name, vbasedev, as, errp); >> + ret = ops->attach_device(name, vbasedev, as, errp); >> + if (ret < 0) { >> + return ret; > > >hmm, I wonder if we should change the return value of vfio_attach_device() >to be a bool. I see, also VFIOIOMMUClass:: setup and VFIOIOMMUClass::add_window. I can add cleanup patches to fix them if you have no other plan. Thanks Zhenzhong > > >Thanks, > >C. > > > >> + } >> + >> + hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename)); >> + if (!HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, >errp)) { >> + object_unref(hiod); >> + ops->detach_device(vbasedev); >> + return -EINVAL; >> + } >> + vbasedev->hiod = hiod; >> + >> + return 0; >> } >> >> void vfio_detach_device(VFIODevice *vbasedev) >> @@ -1512,5 +1527,6 @@ void vfio_detach_device(VFIODevice *vbasedev) >> if (!vbasedev->bcontainer) { >> return; >> } >> + object_unref(vbasedev->hiod); >> vbasedev->bcontainer->ops->detach_device(vbasedev); >> }
On 4/30/24 12:16, Duan, Zhenzhong wrote: > > >> -----Original Message----- >> From: Cédric Le Goater <clg@redhat.com> >> Subject: Re: [PATCH v3 13/19] vfio: Create host IOMMU device instance >> >> On 4/29/24 08:50, Zhenzhong Duan wrote: >>> Create host IOMMU device instance in vfio_attach_device() and call >>> .realize() to initialize it further. >>> >>> Suggested-by: Cédric Le Goater <clg@redhat.com> >>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>> --- >>> include/hw/vfio/vfio-common.h | 1 + >>> hw/vfio/common.c | 18 +++++++++++++++++- >>> 2 files changed, 18 insertions(+), 1 deletion(-) >>> >>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio- >> common.h >>> index 0943add3bc..b204b93a55 100644 >>> --- a/include/hw/vfio/vfio-common.h >>> +++ b/include/hw/vfio/vfio-common.h >>> @@ -126,6 +126,7 @@ typedef struct VFIODevice { >>> OnOffAuto pre_copy_dirty_page_tracking; >>> bool dirty_pages_supported; >>> bool dirty_tracking; >>> + HostIOMMUDevice *hiod; >>> int devid; >>> IOMMUFDBackend *iommufd; >>> } VFIODevice; >>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >>> index 8f9cbdc026..0be8b70ebd 100644 >>> --- a/hw/vfio/common.c >>> +++ b/hw/vfio/common.c >>> @@ -1497,6 +1497,8 @@ int vfio_attach_device(char *name, VFIODevice >> *vbasedev, >>> { >>> const VFIOIOMMUClass *ops = >>> >> VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY)); >>> + HostIOMMUDevice *hiod; >>> + int ret; >>> >>> if (vbasedev->iommufd) { >>> ops = >> VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUF >> D)); >>> @@ -1504,7 +1506,20 @@ int vfio_attach_device(char *name, >> VFIODevice *vbasedev, >>> >>> assert(ops); >>> >>> - return ops->attach_device(name, vbasedev, as, errp); >>> + ret = ops->attach_device(name, vbasedev, as, errp); >>> + if (ret < 0) { >>> + return ret; >> >> >> hmm, I wonder if we should change the return value of vfio_attach_device() >> to be a bool. > > I see, also VFIOIOMMUClass:: setup and VFIOIOMMUClass::add_window. > I can add cleanup patches to fix them if you have no other plan. Yes please. I don't have plans for changes in that area. The only pending patches are from the series "[v4] migration: Improve error reporting" [*]. Thanks, C. [*] https://lore.kernel.org/qemu-devel/20240306133441.2351700-1-clg@redhat.com/
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h index 0943add3bc..b204b93a55 100644 --- a/include/hw/vfio/vfio-common.h +++ b/include/hw/vfio/vfio-common.h @@ -126,6 +126,7 @@ typedef struct VFIODevice { OnOffAuto pre_copy_dirty_page_tracking; bool dirty_pages_supported; bool dirty_tracking; + HostIOMMUDevice *hiod; int devid; IOMMUFDBackend *iommufd; } VFIODevice; diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 8f9cbdc026..0be8b70ebd 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -1497,6 +1497,8 @@ int vfio_attach_device(char *name, VFIODevice *vbasedev, { const VFIOIOMMUClass *ops = VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY)); + HostIOMMUDevice *hiod; + int ret; if (vbasedev->iommufd) { ops = VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD)); @@ -1504,7 +1506,20 @@ int vfio_attach_device(char *name, VFIODevice *vbasedev, assert(ops); - return ops->attach_device(name, vbasedev, as, errp); + ret = ops->attach_device(name, vbasedev, as, errp); + if (ret < 0) { + return ret; + } + + hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename)); + if (!HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp)) { + object_unref(hiod); + ops->detach_device(vbasedev); + return -EINVAL; + } + vbasedev->hiod = hiod; + + return 0; } void vfio_detach_device(VFIODevice *vbasedev) @@ -1512,5 +1527,6 @@ void vfio_detach_device(VFIODevice *vbasedev) if (!vbasedev->bcontainer) { return; } + object_unref(vbasedev->hiod); vbasedev->bcontainer->ops->detach_device(vbasedev); }
Create host IOMMU device instance in vfio_attach_device() and call .realize() to initialize it further. Suggested-by: Cédric Le Goater <clg@redhat.com> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- include/hw/vfio/vfio-common.h | 1 + hw/vfio/common.c | 18 +++++++++++++++++- 2 files changed, 18 insertions(+), 1 deletion(-)