| Message ID | 20240712114704.8708-1-joao.m.martins@oracle.com |
|---|---|
| Headers | show |
| Series | hw/iommufd: IOMMUFD Dirty Tracking | expand |
>-----Original Message----- >From: Joao Martins <joao.m.martins@oracle.com> >Subject: [PATCH v4 00/12] hw/iommufd: IOMMUFD Dirty Tracking > >This small series adds support for IOMMU dirty tracking support via the >IOMMUFD backend. The hardware capability is available on most recent x86 >hardware. The series is divided organized as follows: > >* Patch 1-2: Fixes a regression into mdev support with IOMMUFD. This > one is independent of the series but happened to cross it > while testing mdev with this series I guess VFIO ap/ccw may need fixes too. Will you help on that or I can take it if you want to focus on dirty tracking. The fix may be trivial, just assign VFIODevice->mdev = true. Thanks Zhenzhong > >* Patch 3: Adds a support to iommufd_get_device_info() for capabilities > >* Patches 4 - 10: IOMMUFD backend support for dirty tracking; > >Introduce auto domains -- Patch 5 goes into more detail, but the gist is that >we will find and attach a device to a compatible IOMMU domain, or allocate >a new >hardware pagetable *or* rely on kernel IOAS attach (for mdevs). Afterwards >the >workflow is relatively simple: > >1) Probe device and allow dirty tracking in the HWPT >2) Toggling dirty tracking on/off >3) Read-and-clear of Dirty IOVAs > >The heuristics selected for (1) were to always request the HWPT for >dirty tracking if supported, or rely on device dirty page tracking. This >is a little simplistic and we aren't necessarily utilizing IOMMU dirty >tracking even if we ask during hwpt allocation. > >The unmap case is deferred until further vIOMMU support with migration >is added[3] which will then introduce the usage of >IOMMU_HWPT_GET_DIRTY_BITMAP_NO_CLEAR in GET_DIRTY_BITMAP ioctl >in the >dma unmap bitmap flow. > >* Patches 11-12: Don't block live migration where there's no VF dirty >tracker, considering that we have IOMMU dirty tracking. > >Comments and feedback appreciated. Thanks for all the review thus far! > >Cheers, > Joao > >P.S. Suggest linux-next (or future v6.11) as hypervisor kernel as there's >some bugs fixed there with regards to IOMMU hugepage dirty tracking. > >Changes since v3[5]: >* Skip HostIOMMUDevice::realize for mdev, and introduce a helper to check >if the VFIO > device is mdev. (Zhenzhong) >* Skip setting IOMMU device for mdev (Zhenzhong) >* Add Zhenzhong review tag in patch 3 >* Utilize vbasedev::bcontainer::dirty_pages_supported instead of >introducing > a new HostIOMMUDevice capability and thus remove the cap patch from >the series (Zhenzhong) >* Move the HostIOMMUDevice::realize() to be part of VFIODevice >initialization in attach_device() >while skipping it all together for mdev. (Cedric) >* Due to the previous item, had to remove aw_bits because it depends on >device attach being >finished, instead defer it to when get_cap() gets called. >* Skip auto domains for mdev instead of purposedly erroring out >(Zhenzhong) >* Pass errp in all cases, and instead just free the error in case of -EINVAL > in most of all patches, and also pass Error* in >iommufd_backend_alloc_hwpt() amd > set/query dirty. This is made better thanks in part to skipping auto domains >for mdev (Cedric) > >Changes since RFCv2[4]: >* Always allocate hwpt with IOMMU_HWPT_ALLOC_DIRTY_TRACKING even >if >we end up not actually toggling dirty tracking. (Avihai) >* Fix error handling widely in auto domains logic and all patches (Avihai) >* Reuse iommufd_backend_get_device_info() for capabilities (Zhenzhong) >* New patches 1 and 2 taking into consideration previous comments. >* Store hwpt::flags to know if we have dirty tracking (Avihai) >* New patch 8, that allows to query dirty tracking support after >provisioning. This is a cleaner way to check IOMMU dirty tracking support >when vfio::migration is iniitalized, as opposed to RFCv2 via device caps. >device caps way is still used because at vfio attach we aren't yet with >a fully initialized migration state. >* Adopt error propagation in query,set dirty tracking >* Misc improvements overall broadly and Avihai >* Drop hugepages as it's a bit unrelated; I can pursue that patch >* separately. The main motivation is to provide a way to test >without hugepages similar to what vfio_type1_iommu.disable_hugepages=1 >does. > >Changes since RFCv1[2]: >* Remove intel/amd dirty tracking emulation enabling >* Remove the dirtyrate improvement for VF/IOMMU dirty tracking >[Will pursue these two in separate series] >* Introduce auto domains support >* Enforce dirty tracking following the IOMMUFD UAPI for this >* Add support for toggling hugepages in IOMMUFD >* Auto enable support when VF supports migration to use IOMMU >when it doesn't have VF dirty tracking >* Add a parameter to toggle VF dirty tracking > >[0] https://lore.kernel.org/qemu-devel/20240201072818.327930-1- >zhenzhong.duan@intel.com/ >[1] https://lore.kernel.org/qemu-devel/20240201072818.327930-10- >zhenzhong.duan@intel.com/ >[2] https://lore.kernel.org/qemu-devel/20220428211351.3897-1- >joao.m.martins@oracle.com/ >[3] https://lore.kernel.org/qemu-devel/20230622214845.3980-1- >joao.m.martins@oracle.com/ >[4] https://lore.kernel.org/qemu-devel/20240212135643.5858-1- >joao.m.martins@oracle.com/ >[5] https://lore.kernel.org/qemu-devel/20240708143420.16953-1- >joao.m.martins@oracle.com/ > >Joao Martins (12): > vfio/pci: Extract mdev check into an helper > vfio/iommufd: Don't initialize nor set a HOST_IOMMU_DEVICE with mdev > backends/iommufd: Extend iommufd_backend_get_device_info() to fetch >HW > capabilities > vfio/iommufd: Return errno in iommufd_cdev_attach_ioas_hwpt() > vfio/iommufd: Introduce auto domain creation > vfio/{iommufd,container}: Remove caps::aw_bits > vfio/{iommufd,container}: Initialize HostIOMMUDeviceCaps during > attach_device() > vfio/iommufd: Probe and request hwpt dirty tracking capability > vfio/iommufd: Implement VFIOIOMMUClass::set_dirty_tracking support > vfio/iommufd: Implement VFIOIOMMUClass::query_dirty_bitmap support > vfio/migration: Don't block migration device dirty tracking is > unsupported > vfio/common: Allow disabling device dirty page tracking > > include/hw/vfio/vfio-common.h | 13 +++ > include/sysemu/host_iommu_device.h | 2 +- > include/sysemu/iommufd.h | 14 ++- > backends/iommufd.c | 89 ++++++++++++++- > hw/vfio/common.c | 17 +-- > hw/vfio/container.c | 11 +- > hw/vfio/helpers.c | 18 +++ > hw/vfio/iommufd.c | 178 ++++++++++++++++++++++++++++- > hw/vfio/migration.c | 4 +- > hw/vfio/pci.c | 22 ++-- > backends/trace-events | 3 + > 11 files changed, 339 insertions(+), 32 deletions(-) > >-- >2.17.2
On 16/07/2024 09:20, Duan, Zhenzhong wrote: > > >> -----Original Message----- >> From: Joao Martins <joao.m.martins@oracle.com> >> Subject: [PATCH v4 00/12] hw/iommufd: IOMMUFD Dirty Tracking >> >> This small series adds support for IOMMU dirty tracking support via the >> IOMMUFD backend. The hardware capability is available on most recent x86 >> hardware. The series is divided organized as follows: >> >> * Patch 1-2: Fixes a regression into mdev support with IOMMUFD. This >> one is independent of the series but happened to cross it >> while testing mdev with this series > > I guess VFIO ap/ccw may need fixes too. > Will you help on that or I can take it if you want to focus on dirty tracking. > The fix may be trivial, just assign VFIODevice->mdev = true. > If you have something in mind already by all means go ahead. But from the code are we sure these are mdev bus devices? Certainly are grepping with 'mdev' but unclear if that's abbreviation for 'My Device' or actually bus mdev/mediated-device?
On 7/12/24 13:46, Joao Martins wrote: > Fetch IOMMU hw raw caps behind the device and thus move the > HostIOMMUDevice::realize() to be done during the attach of the device. It > allows it to cache the information obtained from IOMMU_GET_HW_INFO from > iommufd early on. However, while legacy HostIOMMUDevice caps > always return true and doesn't have dependency on other things, the IOMMUFD > backend requires the iommufd FD to be connected and having a devid to be > able to query capabilities. Hence when exactly is HostIOMMUDevice > initialized inside backend ::attach_device() implementation is backend > specific. > > This is in preparation to fetch parse hw capabilities and understand if > dirty tracking is supported by device backing IOMMU without necessarily > duplicating the amount of calls we do to IOMMU_GET_HW_INFO. > > Suggested-by: Cédric Le Goater <clg@redhat.com> > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > --- > include/sysemu/host_iommu_device.h | 1 + > hw/vfio/common.c | 16 ++++++---------- > hw/vfio/container.c | 6 ++++++ > hw/vfio/iommufd.c | 7 +++++++ > 4 files changed, 20 insertions(+), 10 deletions(-) > > diff --git a/include/sysemu/host_iommu_device.h b/include/sysemu/host_iommu_device.h > index 20e77cf54568..b1e5f4b8ac3e 100644 > --- a/include/sysemu/host_iommu_device.h > +++ b/include/sysemu/host_iommu_device.h > @@ -24,6 +24,7 @@ > */ > typedef struct HostIOMMUDeviceCaps { > uint32_t type; > + uint64_t hw_caps; > } HostIOMMUDeviceCaps; > > #define TYPE_HOST_IOMMU_DEVICE "host-iommu-device" > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index b0beed44116e..cc14f0e3fe24 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -1544,7 +1544,7 @@ bool vfio_attach_device(char *name, VFIODevice *vbasedev, > { > const VFIOIOMMUClass *ops = > VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY)); > - HostIOMMUDevice *hiod; > + HostIOMMUDevice *hiod = NULL; > > if (vbasedev->iommufd) { > ops = VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD)); > @@ -1552,21 +1552,17 @@ bool vfio_attach_device(char *name, VFIODevice *vbasedev, > > assert(ops); > > - if (!ops->attach_device(name, vbasedev, as, errp)) { > - return false; > - } > > - if (vbasedev->mdev) { > - return true; > + if (!vbasedev->mdev) { > + hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename)); > + vbasedev->hiod = hiod; > } > > - hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename)); > - if (!HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp)) { > + if (!ops->attach_device(name, vbasedev, as, errp)) { > object_unref(hiod); > - ops->detach_device(vbasedev); > + vbasedev->hiod = NULL; > return false; > } > - vbasedev->hiod = hiod; > > return true; > } > diff --git a/hw/vfio/container.c b/hw/vfio/container.c > index c27f448ba26e..29da261bbf3e 100644 > --- a/hw/vfio/container.c > +++ b/hw/vfio/container.c > @@ -907,6 +907,7 @@ static bool vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev, > AddressSpace *as, Error **errp) > { > int groupid = vfio_device_groupid(vbasedev, errp); > + HostIOMMUDevice *hiod = vbasedev->hiod; > VFIODevice *vbasedev_iter; > VFIOGroup *group; > VFIOContainerBase *bcontainer; > @@ -917,6 +918,11 @@ static bool vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev, > > trace_vfio_attach_device(vbasedev->name, groupid); > > + if (hiod && > + !HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp)) { > + return false; > + } > + Could you please introduce an helper : bool vfio_device_hiod_realize(VFIODevice *vbasedev, Error **errp); Thanks, C. > group = vfio_get_group(groupid, as, errp); > if (!group) { > return false; > diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c > index 873c919e319c..d34dc88231ec 100644 > --- a/hw/vfio/iommufd.c > +++ b/hw/vfio/iommufd.c > @@ -384,6 +384,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, > Error *err = NULL; > const VFIOIOMMUClass *iommufd_vioc = > VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD)); > + HostIOMMUDevice *hiod = vbasedev->hiod; > > if (vbasedev->fd < 0) { > devfd = iommufd_cdev_getfd(vbasedev->sysfsdev, errp); > @@ -401,6 +402,11 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, > > space = vfio_get_address_space(as); > > + if (hiod && > + !HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp)) { > + return false; > + } > + > /* try to attach to an existing container in this space */ > QLIST_FOREACH(bcontainer, &space->containers, next) { > container = container_of(bcontainer, VFIOIOMMUFDContainer, bcontainer); > @@ -722,6 +728,7 @@ static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque, > > hiod->name = g_strdup(vdev->name); > caps->type = type; > + caps->hw_caps = hw_caps; > > return true; > }
On 16/07/2024 11:20, Cédric Le Goater wrote: > On 7/12/24 13:46, Joao Martins wrote: >> Fetch IOMMU hw raw caps behind the device and thus move the >> HostIOMMUDevice::realize() to be done during the attach of the device. It >> allows it to cache the information obtained from IOMMU_GET_HW_INFO from >> iommufd early on. However, while legacy HostIOMMUDevice caps >> always return true and doesn't have dependency on other things, the IOMMUFD >> backend requires the iommufd FD to be connected and having a devid to be >> able to query capabilities. Hence when exactly is HostIOMMUDevice >> initialized inside backend ::attach_device() implementation is backend >> specific. >> >> This is in preparation to fetch parse hw capabilities and understand if >> dirty tracking is supported by device backing IOMMU without necessarily >> duplicating the amount of calls we do to IOMMU_GET_HW_INFO. >> >> Suggested-by: Cédric Le Goater <clg@redhat.com> >> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> >> --- >> include/sysemu/host_iommu_device.h | 1 + >> hw/vfio/common.c | 16 ++++++---------- >> hw/vfio/container.c | 6 ++++++ >> hw/vfio/iommufd.c | 7 +++++++ >> 4 files changed, 20 insertions(+), 10 deletions(-) >> >> diff --git a/include/sysemu/host_iommu_device.h >> b/include/sysemu/host_iommu_device.h >> index 20e77cf54568..b1e5f4b8ac3e 100644 >> --- a/include/sysemu/host_iommu_device.h >> +++ b/include/sysemu/host_iommu_device.h >> @@ -24,6 +24,7 @@ >> */ >> typedef struct HostIOMMUDeviceCaps { >> uint32_t type; >> + uint64_t hw_caps; >> } HostIOMMUDeviceCaps; >> #define TYPE_HOST_IOMMU_DEVICE "host-iommu-device" >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index b0beed44116e..cc14f0e3fe24 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -1544,7 +1544,7 @@ bool vfio_attach_device(char *name, VFIODevice *vbasedev, >> { >> const VFIOIOMMUClass *ops = >> VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY)); >> - HostIOMMUDevice *hiod; >> + HostIOMMUDevice *hiod = NULL; >> if (vbasedev->iommufd) { >> ops = VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD)); >> @@ -1552,21 +1552,17 @@ bool vfio_attach_device(char *name, VFIODevice *vbasedev, >> assert(ops); >> - if (!ops->attach_device(name, vbasedev, as, errp)) { >> - return false; >> - } >> - if (vbasedev->mdev) { >> - return true; >> + if (!vbasedev->mdev) { >> + hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename)); >> + vbasedev->hiod = hiod; >> } >> - hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename)); >> - if (!HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp)) { >> + if (!ops->attach_device(name, vbasedev, as, errp)) { >> object_unref(hiod); >> - ops->detach_device(vbasedev); >> + vbasedev->hiod = NULL; >> return false; >> } >> - vbasedev->hiod = hiod; >> return true; >> } >> diff --git a/hw/vfio/container.c b/hw/vfio/container.c >> index c27f448ba26e..29da261bbf3e 100644 >> --- a/hw/vfio/container.c >> +++ b/hw/vfio/container.c >> @@ -907,6 +907,7 @@ static bool vfio_legacy_attach_device(const char *name, >> VFIODevice *vbasedev, >> AddressSpace *as, Error **errp) >> { >> int groupid = vfio_device_groupid(vbasedev, errp); >> + HostIOMMUDevice *hiod = vbasedev->hiod; >> VFIODevice *vbasedev_iter; >> VFIOGroup *group; >> VFIOContainerBase *bcontainer; >> @@ -917,6 +918,11 @@ static bool vfio_legacy_attach_device(const char *name, >> VFIODevice *vbasedev, >> trace_vfio_attach_device(vbasedev->name, groupid); >> + if (hiod && >> + !HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp)) { >> + return false; >> + } >> + > > > Could you please introduce an helper : > > bool vfio_device_hiod_realize(VFIODevice *vbasedev, Error **errp); > Yeah, let me do that > > Thanks, > > C. > > > >> group = vfio_get_group(groupid, as, errp); >> if (!group) { >> return false; >> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c >> index 873c919e319c..d34dc88231ec 100644 >> --- a/hw/vfio/iommufd.c >> +++ b/hw/vfio/iommufd.c >> @@ -384,6 +384,7 @@ static bool iommufd_cdev_attach(const char *name, >> VFIODevice *vbasedev, >> Error *err = NULL; >> const VFIOIOMMUClass *iommufd_vioc = >> VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD)); >> + HostIOMMUDevice *hiod = vbasedev->hiod; >> if (vbasedev->fd < 0) { >> devfd = iommufd_cdev_getfd(vbasedev->sysfsdev, errp); >> @@ -401,6 +402,11 @@ static bool iommufd_cdev_attach(const char *name, >> VFIODevice *vbasedev, >> space = vfio_get_address_space(as); >> + if (hiod && >> + !HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp)) { >> + return false; >> + } >> + >> /* try to attach to an existing container in this space */ >> QLIST_FOREACH(bcontainer, &space->containers, next) { >> container = container_of(bcontainer, VFIOIOMMUFDContainer, bcontainer); >> @@ -722,6 +728,7 @@ static bool hiod_iommufd_vfio_realize(HostIOMMUDevice >> *hiod, void *opaque, >> hiod->name = g_strdup(vdev->name); >> caps->type = type; >> + caps->hw_caps = hw_caps; >> return true; >> } >
>-----Original Message----- >From: Joao Martins <joao.m.martins@oracle.com> >Subject: [PATCH v4 07/12] vfio/{iommufd,container}: Initialize >HostIOMMUDeviceCaps during attach_device() > >Fetch IOMMU hw raw caps behind the device and thus move the >HostIOMMUDevice::realize() to be done during the attach of the device. It >allows it to cache the information obtained from IOMMU_GET_HW_INFO >from >iommufd early on. However, while legacy HostIOMMUDevice caps >always return true and doesn't have dependency on other things, the >IOMMUFD >backend requires the iommufd FD to be connected and having a devid to be >able to query capabilities. Hence when exactly is HostIOMMUDevice >initialized inside backend ::attach_device() implementation is backend >specific. > >This is in preparation to fetch parse hw capabilities and understand if >dirty tracking is supported by device backing IOMMU without necessarily >duplicating the amount of calls we do to IOMMU_GET_HW_INFO. > >Suggested-by: Cédric Le Goater <clg@redhat.com> >Signed-off-by: Joao Martins <joao.m.martins@oracle.com> >--- > include/sysemu/host_iommu_device.h | 1 + > hw/vfio/common.c | 16 ++++++---------- > hw/vfio/container.c | 6 ++++++ > hw/vfio/iommufd.c | 7 +++++++ > 4 files changed, 20 insertions(+), 10 deletions(-) > >diff --git a/include/sysemu/host_iommu_device.h >b/include/sysemu/host_iommu_device.h >index 20e77cf54568..b1e5f4b8ac3e 100644 >--- a/include/sysemu/host_iommu_device.h >+++ b/include/sysemu/host_iommu_device.h >@@ -24,6 +24,7 @@ > */ > typedef struct HostIOMMUDeviceCaps { > uint32_t type; >+ uint64_t hw_caps; > } HostIOMMUDeviceCaps; > > #define TYPE_HOST_IOMMU_DEVICE "host-iommu-device" >diff --git a/hw/vfio/common.c b/hw/vfio/common.c >index b0beed44116e..cc14f0e3fe24 100644 >--- a/hw/vfio/common.c >+++ b/hw/vfio/common.c >@@ -1544,7 +1544,7 @@ bool vfio_attach_device(char *name, VFIODevice >*vbasedev, > { > const VFIOIOMMUClass *ops = > >VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY)); >- HostIOMMUDevice *hiod; >+ HostIOMMUDevice *hiod = NULL; No need to NULL it? > > if (vbasedev->iommufd) { > ops = >VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUF >D)); >@@ -1552,21 +1552,17 @@ bool vfio_attach_device(char *name, >VFIODevice *vbasedev, > > assert(ops); > >- if (!ops->attach_device(name, vbasedev, as, errp)) { >- return false; >- } > >- if (vbasedev->mdev) { >- return true; >+ if (!vbasedev->mdev) { >+ hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename)); >+ vbasedev->hiod = hiod; > } > >- hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename)); >- if (!HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, >errp)) { >+ if (!ops->attach_device(name, vbasedev, as, errp)) { > object_unref(hiod); >- ops->detach_device(vbasedev); >+ vbasedev->hiod = NULL; > return false; > } >- vbasedev->hiod = hiod; > > return true; > } >diff --git a/hw/vfio/container.c b/hw/vfio/container.c >index c27f448ba26e..29da261bbf3e 100644 >--- a/hw/vfio/container.c >+++ b/hw/vfio/container.c >@@ -907,6 +907,7 @@ static bool vfio_legacy_attach_device(const char >*name, VFIODevice *vbasedev, > AddressSpace *as, Error **errp) > { > int groupid = vfio_device_groupid(vbasedev, errp); >+ HostIOMMUDevice *hiod = vbasedev->hiod; Hiod is used only once in this func, may be use vbasedev->hiod directly? > VFIODevice *vbasedev_iter; > VFIOGroup *group; > VFIOContainerBase *bcontainer; >@@ -917,6 +918,11 @@ static bool vfio_legacy_attach_device(const char >*name, VFIODevice *vbasedev, > > trace_vfio_attach_device(vbasedev->name, groupid); > >+ if (hiod && >+ !HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, >errp)) { >+ return false; >+ } >+ > group = vfio_get_group(groupid, as, errp); > if (!group) { > return false; >diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c >index 873c919e319c..d34dc88231ec 100644 >--- a/hw/vfio/iommufd.c >+++ b/hw/vfio/iommufd.c >@@ -384,6 +384,7 @@ static bool iommufd_cdev_attach(const char *name, >VFIODevice *vbasedev, > Error *err = NULL; > const VFIOIOMMUClass *iommufd_vioc = > >VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUF >D)); >+ HostIOMMUDevice *hiod = vbasedev->hiod; Same here. > > if (vbasedev->fd < 0) { > devfd = iommufd_cdev_getfd(vbasedev->sysfsdev, errp); >@@ -401,6 +402,11 @@ static bool iommufd_cdev_attach(const char >*name, VFIODevice *vbasedev, > > space = vfio_get_address_space(as); > >+ if (hiod && >+ !HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, >errp)) { >+ return false; >+ } >+ > /* try to attach to an existing container in this space */ > QLIST_FOREACH(bcontainer, &space->containers, next) { > container = container_of(bcontainer, VFIOIOMMUFDContainer, >bcontainer); >@@ -722,6 +728,7 @@ static bool >hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque, > > hiod->name = g_strdup(vdev->name); > caps->type = type; >+ caps->hw_caps = hw_caps; > > return true; > } >-- >2.17.2
On 17/07/2024 03:05, Duan, Zhenzhong wrote: > > >> -----Original Message----- >> From: Joao Martins <joao.m.martins@oracle.com> >> Subject: [PATCH v4 07/12] vfio/{iommufd,container}: Initialize >> HostIOMMUDeviceCaps during attach_device() >> >> Fetch IOMMU hw raw caps behind the device and thus move the >> HostIOMMUDevice::realize() to be done during the attach of the device. It >> allows it to cache the information obtained from IOMMU_GET_HW_INFO >> from >> iommufd early on. However, while legacy HostIOMMUDevice caps >> always return true and doesn't have dependency on other things, the >> IOMMUFD >> backend requires the iommufd FD to be connected and having a devid to be >> able to query capabilities. Hence when exactly is HostIOMMUDevice >> initialized inside backend ::attach_device() implementation is backend >> specific. >> >> This is in preparation to fetch parse hw capabilities and understand if >> dirty tracking is supported by device backing IOMMU without necessarily >> duplicating the amount of calls we do to IOMMU_GET_HW_INFO. >> >> Suggested-by: Cédric Le Goater <clg@redhat.com> >> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> >> --- >> include/sysemu/host_iommu_device.h | 1 + >> hw/vfio/common.c | 16 ++++++---------- >> hw/vfio/container.c | 6 ++++++ >> hw/vfio/iommufd.c | 7 +++++++ >> 4 files changed, 20 insertions(+), 10 deletions(-) >> >> diff --git a/include/sysemu/host_iommu_device.h >> b/include/sysemu/host_iommu_device.h >> index 20e77cf54568..b1e5f4b8ac3e 100644 >> --- a/include/sysemu/host_iommu_device.h >> +++ b/include/sysemu/host_iommu_device.h >> @@ -24,6 +24,7 @@ >> */ >> typedef struct HostIOMMUDeviceCaps { >> uint32_t type; >> + uint64_t hw_caps; >> } HostIOMMUDeviceCaps; >> >> #define TYPE_HOST_IOMMU_DEVICE "host-iommu-device" >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index b0beed44116e..cc14f0e3fe24 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -1544,7 +1544,7 @@ bool vfio_attach_device(char *name, VFIODevice >> *vbasedev, >> { >> const VFIOIOMMUClass *ops = >> >> VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY)); >> - HostIOMMUDevice *hiod; >> + HostIOMMUDevice *hiod = NULL; > > No need to NULL it? > /me nods >> >> if (vbasedev->iommufd) { >> ops = >> VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUF >> D)); >> @@ -1552,21 +1552,17 @@ bool vfio_attach_device(char *name, >> VFIODevice *vbasedev, >> >> assert(ops); >> >> - if (!ops->attach_device(name, vbasedev, as, errp)) { >> - return false; >> - } >> >> - if (vbasedev->mdev) { >> - return true; >> + if (!vbasedev->mdev) { >> + hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename)); >> + vbasedev->hiod = hiod; >> } >> >> - hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename)); >> - if (!HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, >> errp)) { >> + if (!ops->attach_device(name, vbasedev, as, errp)) { >> object_unref(hiod); >> - ops->detach_device(vbasedev); >> + vbasedev->hiod = NULL; >> return false; >> } >> - vbasedev->hiod = hiod; >> >> return true; >> } >> diff --git a/hw/vfio/container.c b/hw/vfio/container.c >> index c27f448ba26e..29da261bbf3e 100644 >> --- a/hw/vfio/container.c >> +++ b/hw/vfio/container.c >> @@ -907,6 +907,7 @@ static bool vfio_legacy_attach_device(const char >> *name, VFIODevice *vbasedev, >> AddressSpace *as, Error **errp) >> { >> int groupid = vfio_device_groupid(vbasedev, errp); >> + HostIOMMUDevice *hiod = vbasedev->hiod; > > Hiod is used only once in this func, may be use vbasedev->hiod directly? > The problem is more of how the line below (...) > >> VFIODevice *vbasedev_iter; >> VFIOGroup *group; >> VFIOContainerBase *bcontainer; >> @@ -917,6 +918,11 @@ static bool vfio_legacy_attach_device(const char >> *name, VFIODevice *vbasedev, >> >> trace_vfio_attach_device(vbasedev->name, groupid); >> >> + if (hiod && >> + !HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, >> errp)) { >> + return false; >> + } >> + (...) would look like like really long. And I would end up deref-ing 3 times. But with the helper function that Cedric suggests might easy to accomodate your comment. >> group = vfio_get_group(groupid, as, errp); >> if (!group) { >> return false; >> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c >> index 873c919e319c..d34dc88231ec 100644 >> --- a/hw/vfio/iommufd.c >> +++ b/hw/vfio/iommufd.c >> @@ -384,6 +384,7 @@ static bool iommufd_cdev_attach(const char *name, >> VFIODevice *vbasedev, >> Error *err = NULL; >> const VFIOIOMMUClass *iommufd_vioc = >> >> VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUF >> D)); >> + HostIOMMUDevice *hiod = vbasedev->hiod; > > Same here. > >> >> if (vbasedev->fd < 0) { >> devfd = iommufd_cdev_getfd(vbasedev->sysfsdev, errp); >> @@ -401,6 +402,11 @@ static bool iommufd_cdev_attach(const char >> *name, VFIODevice *vbasedev, >> >> space = vfio_get_address_space(as); >> >> + if (hiod && >> + !HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, >> errp)) { >> + return false; >> + } >> + >> /* try to attach to an existing container in this space */ >> QLIST_FOREACH(bcontainer, &space->containers, next) { >> container = container_of(bcontainer, VFIOIOMMUFDContainer, >> bcontainer); >> @@ -722,6 +728,7 @@ static bool >> hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque, >> >> hiod->name = g_strdup(vdev->name); >> caps->type = type; >> + caps->hw_caps = hw_caps; >> >> return true; >> } >> -- >> 2.17.2 >
Hi Joao, On 7/12/24 13:46, Joao Martins wrote: > Fetch IOMMU hw raw caps behind the device and thus move the what does mean "Fetch IOMMU hw raw caps behind the device'" > HostIOMMUDevice::realize() to be done during the attach of the device. It > allows it to cache the information obtained from IOMMU_GET_HW_INFO from what do you mean by " It allows it to cache the information obtained from IOMMU_GET_HW_INFO from iommufd early on" > iommufd early on. However, while legacy HostIOMMUDevice caps what does mean "legacy HostIOMMUDevice caps always return true"? > always return true and doesn't have dependency on other things, the IOMMUFD > backend requires the iommufd FD to be connected and having a devid to be > able to query capabilities. Hence when exactly is HostIOMMUDevice > initialized inside backend ::attach_device() implementation is backend > specific. > > This is in preparation to fetch parse hw capabilities and understand if fetch parse? > dirty tracking is supported by device backing IOMMU without necessarily > duplicating the amount of calls we do to IOMMU_GET_HW_INFO. But we move code from generic place to BE specific place? Sorry I feel really hard to understand the commit msg in general Eric > > Suggested-by: Cédric Le Goater <clg@redhat.com> > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > --- > include/sysemu/host_iommu_device.h | 1 + > hw/vfio/common.c | 16 ++++++---------- > hw/vfio/container.c | 6 ++++++ > hw/vfio/iommufd.c | 7 +++++++ > 4 files changed, 20 insertions(+), 10 deletions(-) > > diff --git a/include/sysemu/host_iommu_device.h b/include/sysemu/host_iommu_device.h > index 20e77cf54568..b1e5f4b8ac3e 100644 > --- a/include/sysemu/host_iommu_device.h > +++ b/include/sysemu/host_iommu_device.h > @@ -24,6 +24,7 @@ > */ > typedef struct HostIOMMUDeviceCaps { > uint32_t type; > + uint64_t hw_caps; please also update the doc comment > } HostIOMMUDeviceCaps; > > #define TYPE_HOST_IOMMU_DEVICE "host-iommu-device" > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index b0beed44116e..cc14f0e3fe24 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -1544,7 +1544,7 @@ bool vfio_attach_device(char *name, VFIODevice *vbasedev, > { > const VFIOIOMMUClass *ops = > VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY)); > - HostIOMMUDevice *hiod; > + HostIOMMUDevice *hiod = NULL; > > if (vbasedev->iommufd) { > ops = VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD)); > @@ -1552,21 +1552,17 @@ bool vfio_attach_device(char *name, VFIODevice *vbasedev, > > assert(ops); > > - if (!ops->attach_device(name, vbasedev, as, errp)) { > - return false; > - } > > - if (vbasedev->mdev) { > - return true; > + if (!vbasedev->mdev) { > + hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename)); > + vbasedev->hiod = hiod; > } > > - hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename)); > - if (!HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp)) { > + if (!ops->attach_device(name, vbasedev, as, errp)) { > object_unref(hiod); > - ops->detach_device(vbasedev); > + vbasedev->hiod = NULL; > return false; > } > - vbasedev->hiod = hiod; > > return true; > } > diff --git a/hw/vfio/container.c b/hw/vfio/container.c > index c27f448ba26e..29da261bbf3e 100644 > --- a/hw/vfio/container.c > +++ b/hw/vfio/container.c > @@ -907,6 +907,7 @@ static bool vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev, > AddressSpace *as, Error **errp) > { > int groupid = vfio_device_groupid(vbasedev, errp); > + HostIOMMUDevice *hiod = vbasedev->hiod; > VFIODevice *vbasedev_iter; > VFIOGroup *group; > VFIOContainerBase *bcontainer; > @@ -917,6 +918,11 @@ static bool vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev, > > trace_vfio_attach_device(vbasedev->name, groupid); > > + if (hiod && > + !HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp)) { > + return false; > + } > + > group = vfio_get_group(groupid, as, errp); > if (!group) { > return false; > diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c > index 873c919e319c..d34dc88231ec 100644 > --- a/hw/vfio/iommufd.c > +++ b/hw/vfio/iommufd.c > @@ -384,6 +384,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, > Error *err = NULL; > const VFIOIOMMUClass *iommufd_vioc = > VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD)); > + HostIOMMUDevice *hiod = vbasedev->hiod; > > if (vbasedev->fd < 0) { > devfd = iommufd_cdev_getfd(vbasedev->sysfsdev, errp); > @@ -401,6 +402,11 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, > > space = vfio_get_address_space(as); > > + if (hiod && > + !HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp)) { > + return false; > + } > + > /* try to attach to an existing container in this space */ > QLIST_FOREACH(bcontainer, &space->containers, next) { > container = container_of(bcontainer, VFIOIOMMUFDContainer, bcontainer); > @@ -722,6 +728,7 @@ static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque, > > hiod->name = g_strdup(vdev->name); > caps->type = type; > + caps->hw_caps = hw_caps; > > return true; > }
On 17/07/2024 13:19, Eric Auger wrote: > Hi Joao, > > On 7/12/24 13:46, Joao Martins wrote: >> Fetch IOMMU hw raw caps behind the device and thus move the > what does mean "Fetch IOMMU hw raw caps behind the device'" Fetching the out_capabilities field from GET_HW_INFO which essentially tell us if the IOMMU behind the device supports dirty tracking. >> HostIOMMUDevice::realize() to be done during the attach of the device. It >> allows it to cache the information obtained from IOMMU_GET_HW_INFO from > what do you mean by " It allows it to cache the information obtained > from IOMMU_GET_HW_INFO from iommufd early on" /me nods >> iommufd early on. However, while legacy HostIOMMUDevice caps > what does mean "legacy HostIOMMUDevice caps always return true"? That means that it can't fail, and the data in there is synthetic: VFIODevice *vdev = opaque; hiod->name = g_strdup(vdev->name); hiod->agent = opaque; return true; The IOMMUFD one might fail if GET_HW_INFO fails. >> always return true and doesn't have dependency on other things, the IOMMUFD >> backend requires the iommufd FD to be connected and having a devid to be >> able to query capabilities. Hence when exactly is HostIOMMUDevice >> initialized inside backend ::attach_device() implementation is backend >> specific. >> >> This is in preparation to fetch parse hw capabilities and understand if > fetch parse? >> dirty tracking is supported by device backing IOMMU without necessarily >> duplicating the amount of calls we do to IOMMU_GET_HW_INFO. > But we move code from generic place to BE specific place? > No because in IOMMUFD needs the backend connected, while the legacy backend doesn't. Otherwise this patch wouldn't be needed to be backend specific. > Sorry I feel really hard to understand the commit msg in general > How about this: Fetch IOMMU hw raw caps behind the device and thus move the HostIOMMUDevice::realize() to be done during the attach of the device. This is in preparation to fetch parse hw capabilities and understand if dirty tracking is supported by device backing IOMMU without necessarily duplicating the amount of calls we do to IOMMU_GET_HW_INFO. Note that the HostIOMMUDevice data with legacy backend is synthetic and doesn't need any information from the (type1-iommu) backend. While the IOMMUFD backend requires the iommufd FD to be connected and having a devid to be able to query device capabilities seeded in HostIOMMUDevice. This means that HostIOMMUDevice initialization (i.e. ::realized() is invoked) is container backend specific. > Eric > > >> >> Suggested-by: Cédric Le Goater <clg@redhat.com> >> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> >> --- >> include/sysemu/host_iommu_device.h | 1 + >> hw/vfio/common.c | 16 ++++++---------- >> hw/vfio/container.c | 6 ++++++ >> hw/vfio/iommufd.c | 7 +++++++ >> 4 files changed, 20 insertions(+), 10 deletions(-) >> >> diff --git a/include/sysemu/host_iommu_device.h b/include/sysemu/host_iommu_device.h >> index 20e77cf54568..b1e5f4b8ac3e 100644 >> --- a/include/sysemu/host_iommu_device.h >> +++ b/include/sysemu/host_iommu_device.h >> @@ -24,6 +24,7 @@ >> */ >> typedef struct HostIOMMUDeviceCaps { >> uint32_t type; >> + uint64_t hw_caps; > please also update the doc comment OK >> } HostIOMMUDeviceCaps; >> >> #define TYPE_HOST_IOMMU_DEVICE "host-iommu-device" >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index b0beed44116e..cc14f0e3fe24 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -1544,7 +1544,7 @@ bool vfio_attach_device(char *name, VFIODevice *vbasedev, >> { >> const VFIOIOMMUClass *ops = >> VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY)); >> - HostIOMMUDevice *hiod; >> + HostIOMMUDevice *hiod = NULL; >> >> if (vbasedev->iommufd) { >> ops = VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD)); >> @@ -1552,21 +1552,17 @@ bool vfio_attach_device(char *name, VFIODevice *vbasedev, >> >> assert(ops); >> >> - if (!ops->attach_device(name, vbasedev, as, errp)) { >> - return false; >> - } >> >> - if (vbasedev->mdev) { >> - return true; >> + if (!vbasedev->mdev) { >> + hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename)); >> + vbasedev->hiod = hiod; >> } >> >> - hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename)); >> - if (!HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp)) { >> + if (!ops->attach_device(name, vbasedev, as, errp)) { >> object_unref(hiod); >> - ops->detach_device(vbasedev); >> + vbasedev->hiod = NULL; >> return false; >> } >> - vbasedev->hiod = hiod; >> >> return true; >> } >> diff --git a/hw/vfio/container.c b/hw/vfio/container.c >> index c27f448ba26e..29da261bbf3e 100644 >> --- a/hw/vfio/container.c >> +++ b/hw/vfio/container.c >> @@ -907,6 +907,7 @@ static bool vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev, >> AddressSpace *as, Error **errp) >> { >> int groupid = vfio_device_groupid(vbasedev, errp); >> + HostIOMMUDevice *hiod = vbasedev->hiod; >> VFIODevice *vbasedev_iter; >> VFIOGroup *group; >> VFIOContainerBase *bcontainer; >> @@ -917,6 +918,11 @@ static bool vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev, >> >> trace_vfio_attach_device(vbasedev->name, groupid); >> >> + if (hiod && >> + !HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp)) { >> + return false; >> + } >> + >> group = vfio_get_group(groupid, as, errp); >> if (!group) { >> return false; >> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c >> index 873c919e319c..d34dc88231ec 100644 >> --- a/hw/vfio/iommufd.c >> +++ b/hw/vfio/iommufd.c >> @@ -384,6 +384,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, >> Error *err = NULL; >> const VFIOIOMMUClass *iommufd_vioc = >> VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD)); >> + HostIOMMUDevice *hiod = vbasedev->hiod; >> >> if (vbasedev->fd < 0) { >> devfd = iommufd_cdev_getfd(vbasedev->sysfsdev, errp); >> @@ -401,6 +402,11 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, >> >> space = vfio_get_address_space(as); >> >> + if (hiod && >> + !HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp)) { >> + return false; >> + } >> + >> /* try to attach to an existing container in this space */ >> QLIST_FOREACH(bcontainer, &space->containers, next) { >> container = container_of(bcontainer, VFIOIOMMUFDContainer, bcontainer); >> @@ -722,6 +728,7 @@ static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque, >> >> hiod->name = g_strdup(vdev->name); >> caps->type = type; >> + caps->hw_caps = hw_caps; >> >> return true; >> } >
On 7/17/24 14:33, Joao Martins wrote: > On 17/07/2024 13:19, Eric Auger wrote: >> Hi Joao, >> >> On 7/12/24 13:46, Joao Martins wrote: >>> Fetch IOMMU hw raw caps behind the device and thus move the >> what does mean "Fetch IOMMU hw raw caps behind the device'" > Fetching the out_capabilities field from GET_HW_INFO which essentially tell us > if the IOMMU behind the device supports dirty tracking. that's much clearer than the 1st sentence > >>> HostIOMMUDevice::realize() to be done during the attach of the device. It >>> allows it to cache the information obtained from IOMMU_GET_HW_INFO from >> what do you mean by " It allows it to cache the information obtained >> from IOMMU_GET_HW_INFO from iommufd early on" > /me nods ? > >>> iommufd early on. However, while legacy HostIOMMUDevice caps >> what does mean "legacy HostIOMMUDevice caps always return true"? > That means that it can't fail, and the data in there is synthetic: > > VFIODevice *vdev = opaque; > > hiod->name = g_strdup(vdev->name); > hiod->agent = opaque; > > return true; > > The IOMMUFD one might fail if GET_HW_INFO fails. so you talk about hiod_legacy_vfio_realize() and not " legacy HostIOMMUDevice caps"! > >>> always return true and doesn't have dependency on other things, the IOMMUFD >>> backend requires the iommufd FD to be connected and having a devid to be >>> able to query capabilities. Hence when exactly is HostIOMMUDevice >>> initialized inside backend ::attach_device() implementation is backend >>> specific. >>> >>> This is in preparation to fetch parse hw capabilities and understand if >> fetch parse? >>> dirty tracking is supported by device backing IOMMU without necessarily >>> duplicating the amount of calls we do to IOMMU_GET_HW_INFO. >> But we move code from generic place to BE specific place? >> > No because in IOMMUFD needs the backend connected, while the legacy backend > doesn't. Otherwise this patch wouldn't be needed to be backend specific. > >> Sorry I feel really hard to understand the commit msg in general >> > How about this: > > Fetch IOMMU hw raw caps behind the device and thus move the You need to tell what the patch does and why. "Fetch IOMMU hw raw caps behind the device" sentence does not clearly fit in any. > HostIOMMUDevice::realize() to be done during the attach of the device. > > This is in preparation to fetch parse hw capabilities and understand if > dirty tracking is supported by device backing IOMMU without necessarily > duplicating the amount of calls we do to IOMMU_GET_HW_INFO. > > Note that the HostIOMMUDevice data with legacy backend is synthetic > and doesn't need any information from the (type1-iommu) backend. While the > IOMMUFD backend requires the iommufd FD to be connected and having a devid > to be able to query device capabilities seeded in HostIOMMUDevice. This > means that HostIOMMUDevice initialization (i.e. ::realized() is invoked) is > container backend specific. > > > > >> Eric >> >> >>> Suggested-by: Cédric Le Goater <clg@redhat.com> >>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> >>> --- >>> include/sysemu/host_iommu_device.h | 1 + >>> hw/vfio/common.c | 16 ++++++---------- >>> hw/vfio/container.c | 6 ++++++ >>> hw/vfio/iommufd.c | 7 +++++++ >>> 4 files changed, 20 insertions(+), 10 deletions(-) >>> >>> diff --git a/include/sysemu/host_iommu_device.h b/include/sysemu/host_iommu_device.h >>> index 20e77cf54568..b1e5f4b8ac3e 100644 >>> --- a/include/sysemu/host_iommu_device.h >>> +++ b/include/sysemu/host_iommu_device.h >>> @@ -24,6 +24,7 @@ >>> */ >>> typedef struct HostIOMMUDeviceCaps { >>> uint32_t type; >>> + uint64_t hw_caps; >> please also update the doc comment > OK > >>> } HostIOMMUDeviceCaps; >>> >>> #define TYPE_HOST_IOMMU_DEVICE "host-iommu-device" >>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >>> index b0beed44116e..cc14f0e3fe24 100644 >>> --- a/hw/vfio/common.c >>> +++ b/hw/vfio/common.c >>> @@ -1544,7 +1544,7 @@ bool vfio_attach_device(char *name, VFIODevice *vbasedev, >>> { >>> const VFIOIOMMUClass *ops = >>> VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY)); >>> - HostIOMMUDevice *hiod; >>> + HostIOMMUDevice *hiod = NULL; >>> >>> if (vbasedev->iommufd) { >>> ops = VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD)); >>> @@ -1552,21 +1552,17 @@ bool vfio_attach_device(char *name, VFIODevice *vbasedev, >>> >>> assert(ops); >>> >>> - if (!ops->attach_device(name, vbasedev, as, errp)) { >>> - return false; >>> - } >>> >>> - if (vbasedev->mdev) { >>> - return true; >>> + if (!vbasedev->mdev) { >>> + hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename)); >>> + vbasedev->hiod = hiod; >>> } >>> >>> - hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename)); >>> - if (!HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp)) { >>> + if (!ops->attach_device(name, vbasedev, as, errp)) { >>> object_unref(hiod); >>> - ops->detach_device(vbasedev); >>> + vbasedev->hiod = NULL; >>> return false; >>> } >>> - vbasedev->hiod = hiod; >>> >>> return true; >>> } >>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c >>> index c27f448ba26e..29da261bbf3e 100644 >>> --- a/hw/vfio/container.c >>> +++ b/hw/vfio/container.c >>> @@ -907,6 +907,7 @@ static bool vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev, >>> AddressSpace *as, Error **errp) >>> { >>> int groupid = vfio_device_groupid(vbasedev, errp); >>> + HostIOMMUDevice *hiod = vbasedev->hiod; >>> VFIODevice *vbasedev_iter; >>> VFIOGroup *group; >>> VFIOContainerBase *bcontainer; >>> @@ -917,6 +918,11 @@ static bool vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev, >>> >>> trace_vfio_attach_device(vbasedev->name, groupid); >>> >>> + if (hiod && >>> + !HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp)) { >>> + return false; >>> + } >>> + >>> group = vfio_get_group(groupid, as, errp); >>> if (!group) { >>> return false; >>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c >>> index 873c919e319c..d34dc88231ec 100644 >>> --- a/hw/vfio/iommufd.c >>> +++ b/hw/vfio/iommufd.c >>> @@ -384,6 +384,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, >>> Error *err = NULL; >>> const VFIOIOMMUClass *iommufd_vioc = >>> VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD)); >>> + HostIOMMUDevice *hiod = vbasedev->hiod; >>> >>> if (vbasedev->fd < 0) { >>> devfd = iommufd_cdev_getfd(vbasedev->sysfsdev, errp); >>> @@ -401,6 +402,11 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, >>> >>> space = vfio_get_address_space(as); >>> >>> + if (hiod && >>> + !HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp)) { >>> + return false; >>> + } >>> + >>> /* try to attach to an existing container in this space */ >>> QLIST_FOREACH(bcontainer, &space->containers, next) { >>> container = container_of(bcontainer, VFIOIOMMUFDContainer, bcontainer); >>> @@ -722,6 +728,7 @@ static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque, >>> >>> hiod->name = g_strdup(vdev->name); >>> caps->type = type; >>> + caps->hw_caps = hw_caps; >>> >>> return true; >>> }
On 17/07/2024 14:41, Eric Auger wrote: > On 7/17/24 14:33, Joao Martins wrote: >> On 17/07/2024 13:19, Eric Auger wrote: >>> Hi Joao, >>> >>> On 7/12/24 13:46, Joao Martins wrote: >>>> Fetch IOMMU hw raw caps behind the device and thus move the >>> what does mean "Fetch IOMMU hw raw caps behind the device'" >> Fetching the out_capabilities field from GET_HW_INFO which essentially tell us >> if the IOMMU behind the device supports dirty tracking. > that's much clearer than the 1st sentence >> >>>> HostIOMMUDevice::realize() to be done during the attach of the device. It >>>> allows it to cache the information obtained from IOMMU_GET_HW_INFO from >>> what do you mean by " It allows it to cache the information obtained >>> from IOMMU_GET_HW_INFO from iommufd early on" >> /me nods > ? By caching I mean that invoking realize() earlier allow us to store the value of @out_capabilities in HostIOMMUDevice::caps for later use and avoid having to call GET_HW_INFO Again. 'Early on' refers to me doing this at the beginning of attach_device(). >> >>>> iommufd early on. However, while legacy HostIOMMUDevice caps >>> what does mean "legacy HostIOMMUDevice caps always return true"? >> That means that it can't fail, and the data in there is synthetic: >> >> VFIODevice *vdev = opaque; >> >> hiod->name = g_strdup(vdev->name); >> hiod->agent = opaque; >> >> return true; >> >> The IOMMUFD one might fail if GET_HW_INFO fails. > so you talk about hiod_legacy_vfio_realize() and not " > > legacy HostIOMMUDevice caps"! > It's both. Legacy doesn't need to initialize @caps. Whereby in IOMMUFD we do and with actual info (the capabilities) and in order to do that, we need the backend initialized. And *that* ioctl() may fail. >> >>>> always return true and doesn't have dependency on other things, the IOMMUFD >>>> backend requires the iommufd FD to be connected and having a devid to be >>>> able to query capabilities. Hence when exactly is HostIOMMUDevice >>>> initialized inside backend ::attach_device() implementation is backend >>>> specific. >>>> >>>> This is in preparation to fetch parse hw capabilities and understand if >>> fetch parse? >>>> dirty tracking is supported by device backing IOMMU without necessarily >>>> duplicating the amount of calls we do to IOMMU_GET_HW_INFO. >>> But we move code from generic place to BE specific place? >>> >> No because in IOMMUFD needs the backend connected, while the legacy backend >> doesn't. Otherwise this patch wouldn't be needed to be backend specific. >> >>> Sorry I feel really hard to understand the commit msg in general >>> >> How about this: >> >> Fetch IOMMU hw raw caps behind the device and thus move the > You need to tell what the patch does and why. > IMHO, I already do that -- what we are having here is a parsing issue on my english (likely because it's a bit convoluted). Me asking you how it sounds is for me to calibrate against how you understand it or literality of the text (or lack of thereof). > "Fetch IOMMU hw raw caps behind the device" sentence does not clearly fit in any. > >> HostIOMMUDevice::realize() to be done during the attach of the device. >> >> This is in preparation to fetch parse hw capabilities and understand if >> dirty tracking is supported by device backing IOMMU without necessarily >> duplicating the amount of calls we do to IOMMU_GET_HW_INFO. >> >> Note that the HostIOMMUDevice data with legacy backend is synthetic >> and doesn't need any information from the (type1-iommu) backend. While the >> IOMMUFD backend requires the iommufd FD to be connected and having a devid >> to be able to query device capabilities seeded in HostIOMMUDevice. This >> means that HostIOMMUDevice initialization (i.e. ::realized() is invoked) is >> container backend specific. >> >> >>
>-----Original Message----- >From: Joao Martins <joao.m.martins@oracle.com> >Subject: Re: [PATCH v4 00/12] hw/iommufd: IOMMUFD Dirty Tracking > >On 16/07/2024 09:20, Duan, Zhenzhong wrote: >> >> >>> -----Original Message----- >>> From: Joao Martins <joao.m.martins@oracle.com> >>> Subject: [PATCH v4 00/12] hw/iommufd: IOMMUFD Dirty Tracking >>> >>> This small series adds support for IOMMU dirty tracking support via the >>> IOMMUFD backend. The hardware capability is available on most recent >x86 >>> hardware. The series is divided organized as follows: >>> >>> * Patch 1-2: Fixes a regression into mdev support with IOMMUFD. This >>> one is independent of the series but happened to cross it >>> while testing mdev with this series >> >> I guess VFIO ap/ccw may need fixes too. >> Will you help on that or I can take it if you want to focus on dirty tracking. >> The fix may be trivial, just assign VFIODevice->mdev = true. >> > >If you have something in mind already by all means go ahead. OK, will be after your 'dirty tracking' v5 as there is dependency. > >But from the code are we sure these are mdev bus devices? Certainly are >grepping >with 'mdev' but unclear if that's abbreviation for 'My Device' or actually bus >mdev/mediated-device? I think so, docs/system/s390x/vfio-[ap|ccw].rst shows /sys/bus/mdev Thanks Zhenzhong
This small series adds support for IOMMU dirty tracking support via the IOMMUFD backend. The hardware capability is available on most recent x86 hardware. The series is divided organized as follows: * Patch 1-2: Fixes a regression into mdev support with IOMMUFD. This one is independent of the series but happened to cross it while testing mdev with this series * Patch 3: Adds a support to iommufd_get_device_info() for capabilities * Patches 4 - 10: IOMMUFD backend support for dirty tracking; Introduce auto domains -- Patch 5 goes into more detail, but the gist is that we will find and attach a device to a compatible IOMMU domain, or allocate a new hardware pagetable *or* rely on kernel IOAS attach (for mdevs). Afterwards the workflow is relatively simple: 1) Probe device and allow dirty tracking in the HWPT 2) Toggling dirty tracking on/off 3) Read-and-clear of Dirty IOVAs The heuristics selected for (1) were to always request the HWPT for dirty tracking if supported, or rely on device dirty page tracking. This is a little simplistic and we aren't necessarily utilizing IOMMU dirty tracking even if we ask during hwpt allocation. The unmap case is deferred until further vIOMMU support with migration is added[3] which will then introduce the usage of IOMMU_HWPT_GET_DIRTY_BITMAP_NO_CLEAR in GET_DIRTY_BITMAP ioctl in the dma unmap bitmap flow. * Patches 11-12: Don't block live migration where there's no VF dirty tracker, considering that we have IOMMU dirty tracking. Comments and feedback appreciated. Thanks for all the review thus far! Cheers, Joao P.S. Suggest linux-next (or future v6.11) as hypervisor kernel as there's some bugs fixed there with regards to IOMMU hugepage dirty tracking. Changes since v3[5]: * Skip HostIOMMUDevice::realize for mdev, and introduce a helper to check if the VFIO device is mdev. (Zhenzhong) * Skip setting IOMMU device for mdev (Zhenzhong) * Add Zhenzhong review tag in patch 3 * Utilize vbasedev::bcontainer::dirty_pages_supported instead of introducing a new HostIOMMUDevice capability and thus remove the cap patch from the series (Zhenzhong) * Move the HostIOMMUDevice::realize() to be part of VFIODevice initialization in attach_device() while skipping it all together for mdev. (Cedric) * Due to the previous item, had to remove aw_bits because it depends on device attach being finished, instead defer it to when get_cap() gets called. * Skip auto domains for mdev instead of purposedly erroring out (Zhenzhong) * Pass errp in all cases, and instead just free the error in case of -EINVAL in most of all patches, and also pass Error* in iommufd_backend_alloc_hwpt() amd set/query dirty. This is made better thanks in part to skipping auto domains for mdev (Cedric) Changes since RFCv2[4]: * Always allocate hwpt with IOMMU_HWPT_ALLOC_DIRTY_TRACKING even if we end up not actually toggling dirty tracking. (Avihai) * Fix error handling widely in auto domains logic and all patches (Avihai) * Reuse iommufd_backend_get_device_info() for capabilities (Zhenzhong) * New patches 1 and 2 taking into consideration previous comments. * Store hwpt::flags to know if we have dirty tracking (Avihai) * New patch 8, that allows to query dirty tracking support after provisioning. This is a cleaner way to check IOMMU dirty tracking support when vfio::migration is iniitalized, as opposed to RFCv2 via device caps. device caps way is still used because at vfio attach we aren't yet with a fully initialized migration state. * Adopt error propagation in query,set dirty tracking * Misc improvements overall broadly and Avihai * Drop hugepages as it's a bit unrelated; I can pursue that patch * separately. The main motivation is to provide a way to test without hugepages similar to what vfio_type1_iommu.disable_hugepages=1 does. Changes since RFCv1[2]: * Remove intel/amd dirty tracking emulation enabling * Remove the dirtyrate improvement for VF/IOMMU dirty tracking [Will pursue these two in separate series] * Introduce auto domains support * Enforce dirty tracking following the IOMMUFD UAPI for this * Add support for toggling hugepages in IOMMUFD * Auto enable support when VF supports migration to use IOMMU when it doesn't have VF dirty tracking * Add a parameter to toggle VF dirty tracking [0] https://lore.kernel.org/qemu-devel/20240201072818.327930-1-zhenzhong.duan@intel.com/ [1] https://lore.kernel.org/qemu-devel/20240201072818.327930-10-zhenzhong.duan@intel.com/ [2] https://lore.kernel.org/qemu-devel/20220428211351.3897-1-joao.m.martins@oracle.com/ [3] https://lore.kernel.org/qemu-devel/20230622214845.3980-1-joao.m.martins@oracle.com/ [4] https://lore.kernel.org/qemu-devel/20240212135643.5858-1-joao.m.martins@oracle.com/ [5] https://lore.kernel.org/qemu-devel/20240708143420.16953-1-joao.m.martins@oracle.com/ Joao Martins (12): vfio/pci: Extract mdev check into an helper vfio/iommufd: Don't initialize nor set a HOST_IOMMU_DEVICE with mdev backends/iommufd: Extend iommufd_backend_get_device_info() to fetch HW capabilities vfio/iommufd: Return errno in iommufd_cdev_attach_ioas_hwpt() vfio/iommufd: Introduce auto domain creation vfio/{iommufd,container}: Remove caps::aw_bits vfio/{iommufd,container}: Initialize HostIOMMUDeviceCaps during attach_device() vfio/iommufd: Probe and request hwpt dirty tracking capability vfio/iommufd: Implement VFIOIOMMUClass::set_dirty_tracking support vfio/iommufd: Implement VFIOIOMMUClass::query_dirty_bitmap support vfio/migration: Don't block migration device dirty tracking is unsupported vfio/common: Allow disabling device dirty page tracking include/hw/vfio/vfio-common.h | 13 +++ include/sysemu/host_iommu_device.h | 2 +- include/sysemu/iommufd.h | 14 ++- backends/iommufd.c | 89 ++++++++++++++- hw/vfio/common.c | 17 +-- hw/vfio/container.c | 11 +- hw/vfio/helpers.c | 18 +++ hw/vfio/iommufd.c | 178 ++++++++++++++++++++++++++++- hw/vfio/migration.c | 4 +- hw/vfio/pci.c | 22 ++-- backends/trace-events | 3 + 11 files changed, 339 insertions(+), 32 deletions(-)