| Message ID | 20250411101707.3460429-2-zhenzhong.duan@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | cleanup interfaces | expand |
On 11/04/2025 11:17, Zhenzhong Duan wrote: > The saved caps copy can be used to check dirty tracking capability. > > The capabilities is gotten through IOMMUFD interface, so define a > new structure HostIOMMUDeviceIOMMUFDCaps which contains vendor > caps raw data in "include/system/iommufd.h". > > This is a prepare work for moving .realize() after .attach_device(). > > Suggested-by: Cédric Le Goater <clg@redhat.com> > Suggested-by: Eric Auger <eric.auger@redhat.com> > Suggested-by: Nicolin Chen <nicolinc@nvidia.com> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > include/hw/vfio/vfio-device.h | 1 + > include/system/iommufd.h | 22 ++++++++++++++++++++++ > hw/vfio/iommufd.c | 10 +++++++++- > 3 files changed, 32 insertions(+), 1 deletion(-) > > diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h > index 66797b4c92..09a7af891a 100644 > --- a/include/hw/vfio/vfio-device.h > +++ b/include/hw/vfio/vfio-device.h > @@ -77,6 +77,7 @@ typedef struct VFIODevice { > bool dirty_tracking; /* Protected by BQL */ > bool iommu_dirty_tracking; > HostIOMMUDevice *hiod; > + HostIOMMUDeviceIOMMUFDCaps caps; > int devid; > IOMMUFDBackend *iommufd; > VFIOIOASHwpt *hwpt; > diff --git a/include/system/iommufd.h b/include/system/iommufd.h > index cbab75bfbf..0f337585c9 100644 > --- a/include/system/iommufd.h > +++ b/include/system/iommufd.h > @@ -18,6 +18,9 @@ > #include "exec/hwaddr.h" > #include "exec/cpu-common.h" > #include "system/host_iommu_device.h" > +#ifdef CONFIG_LINUX > +#include <linux/iommufd.h> > +#endif > > #define TYPE_IOMMUFD_BACKEND "iommufd" > OBJECT_DECLARE_TYPE(IOMMUFDBackend, IOMMUFDBackendClass, IOMMUFD_BACKEND) > @@ -63,4 +66,23 @@ bool iommufd_backend_get_dirty_bitmap(IOMMUFDBackend *be, uint32_t hwpt_id, > Error **errp); > > #define TYPE_HOST_IOMMU_DEVICE_IOMMUFD TYPE_HOST_IOMMU_DEVICE "-iommufd" > + > +typedef union VendorCaps { > + struct iommu_hw_info_vtd vtd; > + struct iommu_hw_info_arm_smmuv3 smmuv3; > +} VendorCaps; > + > +/** > + * struct HostIOMMUDeviceIOMMUFDCaps - Define host IOMMU device capabilities. > + * > + * @type: host platform IOMMU type. > + * > + * @hw_caps: host platform IOMMU capabilities (e.g. on IOMMUFD this represents > + * the @out_capabilities value returned from IOMMU_GET_HW_INFO ioctl) > + */ > +typedef struct HostIOMMUDeviceIOMMUFDCaps { > + uint32_t type; > + uint64_t hw_caps; > + VendorCaps vendor_caps; > +} HostIOMMUDeviceIOMMUFDCaps; > #endif > diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c > index 48db105422..530cde6740 100644 > --- a/hw/vfio/iommufd.c > +++ b/hw/vfio/iommufd.c > @@ -324,7 +324,7 @@ static bool iommufd_cdev_autodomains_get(VFIODevice *vbasedev, > * vfio_migration_realize() may decide to use VF dirty tracking > * instead. > */ > - if (vbasedev->hiod->caps.hw_caps & IOMMU_HW_CAP_DIRTY_TRACKING) { > + if (vbasedev->caps.hw_caps & IOMMU_HW_CAP_DIRTY_TRACKING) { > flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING; > } > > @@ -475,6 +475,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, > int ret, devfd; > uint32_t ioas_id; > Error *err = NULL; > + HostIOMMUDeviceIOMMUFDCaps *caps = &vbasedev->caps; > const VFIOIOMMUClass *iommufd_vioc = > VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD)); > > @@ -505,6 +506,13 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, > goto err_alloc_ioas; > } > > + if (!iommufd_backend_get_device_info(vbasedev->iommufd, vbasedev->devid, > + &caps->type, &caps->vendor_caps, > + sizeof(VendorCaps), &caps->hw_caps, > + errp)) { > + goto err_alloc_ioas; > + } > + I think this will fail on mdev (and thus fail the attachment mistakengly as there's no IOMMUFDDevice with mdev) ? In case it fails, you can just do: if (!vbasedev->mdev && !iommufd_backend_get_device_info(...)) {
On 4/11/25 12:17, Zhenzhong Duan wrote: > The saved caps copy can be used to check dirty tracking capability. > > The capabilities is gotten through IOMMUFD interface, so define a > new structure HostIOMMUDeviceIOMMUFDCaps which contains vendor > caps raw data in "include/system/iommufd.h". > > This is a prepare work for moving .realize() after .attach_device(). > > Suggested-by: Cédric Le Goater <clg@redhat.com> > Suggested-by: Eric Auger <eric.auger@redhat.com> > Suggested-by: Nicolin Chen <nicolinc@nvidia.com> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > include/hw/vfio/vfio-device.h | 1 + > include/system/iommufd.h | 22 ++++++++++++++++++++++ > hw/vfio/iommufd.c | 10 +++++++++- > 3 files changed, 32 insertions(+), 1 deletion(-) > > diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h > index 66797b4c92..09a7af891a 100644 > --- a/include/hw/vfio/vfio-device.h > +++ b/include/hw/vfio/vfio-device.h > @@ -77,6 +77,7 @@ typedef struct VFIODevice { > bool dirty_tracking; /* Protected by BQL */ > bool iommu_dirty_tracking; > HostIOMMUDevice *hiod; > + HostIOMMUDeviceIOMMUFDCaps caps; IMO, these capabilities belong to HostIOMMUDevice and not VFIODevice. I would simply call iommufd_backend_get_device_info() twice where needed : iommufd_cdev_autodomains_get() and hiod_iommufd_vfio_realize() Thanks, C. > int devid; > IOMMUFDBackend *iommufd; > VFIOIOASHwpt *hwpt; > diff --git a/include/system/iommufd.h b/include/system/iommufd.h > index cbab75bfbf..0f337585c9 100644 > --- a/include/system/iommufd.h > +++ b/include/system/iommufd.h > @@ -18,6 +18,9 @@ > #include "exec/hwaddr.h" > #include "exec/cpu-common.h" > #include "system/host_iommu_device.h" > +#ifdef CONFIG_LINUX > +#include <linux/iommufd.h> > +#endif > > #define TYPE_IOMMUFD_BACKEND "iommufd" > OBJECT_DECLARE_TYPE(IOMMUFDBackend, IOMMUFDBackendClass, IOMMUFD_BACKEND) > @@ -63,4 +66,23 @@ bool iommufd_backend_get_dirty_bitmap(IOMMUFDBackend *be, uint32_t hwpt_id, > Error **errp); > > #define TYPE_HOST_IOMMU_DEVICE_IOMMUFD TYPE_HOST_IOMMU_DEVICE "-iommufd" > + > +typedef union VendorCaps { > + struct iommu_hw_info_vtd vtd; > + struct iommu_hw_info_arm_smmuv3 smmuv3; > +} VendorCaps; > + > +/** > + * struct HostIOMMUDeviceIOMMUFDCaps - Define host IOMMU device capabilities. > + * > + * @type: host platform IOMMU type. > + * > + * @hw_caps: host platform IOMMU capabilities (e.g. on IOMMUFD this represents > + * the @out_capabilities value returned from IOMMU_GET_HW_INFO ioctl) > + */ > +typedef struct HostIOMMUDeviceIOMMUFDCaps { > + uint32_t type; > + uint64_t hw_caps; > + VendorCaps vendor_caps; > +} HostIOMMUDeviceIOMMUFDCaps; > #endif > diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c > index 48db105422..530cde6740 100644 > --- a/hw/vfio/iommufd.c > +++ b/hw/vfio/iommufd.c > @@ -324,7 +324,7 @@ static bool iommufd_cdev_autodomains_get(VFIODevice *vbasedev, > * vfio_migration_realize() may decide to use VF dirty tracking > * instead. > */ > - if (vbasedev->hiod->caps.hw_caps & IOMMU_HW_CAP_DIRTY_TRACKING) { > + if (vbasedev->caps.hw_caps & IOMMU_HW_CAP_DIRTY_TRACKING) { > flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING; > } > > @@ -475,6 +475,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, > int ret, devfd; > uint32_t ioas_id; > Error *err = NULL; > + HostIOMMUDeviceIOMMUFDCaps *caps = &vbasedev->caps; > const VFIOIOMMUClass *iommufd_vioc = > VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD)); > > @@ -505,6 +506,13 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, > goto err_alloc_ioas; > } > > + if (!iommufd_backend_get_device_info(vbasedev->iommufd, vbasedev->devid, > + &caps->type, &caps->vendor_caps, > + sizeof(VendorCaps), &caps->hw_caps, > + errp)) { > + goto err_alloc_ioas; > + } > + > /* try to attach to an existing container in this space */ > QLIST_FOREACH(bcontainer, &space->containers, next) { > container = container_of(bcontainer, VFIOIOMMUFDContainer, bcontainer);
>-----Original Message----- >From: Joao Martins <joao.m.martins@oracle.com> >Subject: Re: [PATCH 1/5] vfio/iommufd: Save host iommu capabilities in >VFIODevice.caps > >On 11/04/2025 11:17, Zhenzhong Duan wrote: >> The saved caps copy can be used to check dirty tracking capability. >> >> The capabilities is gotten through IOMMUFD interface, so define a >> new structure HostIOMMUDeviceIOMMUFDCaps which contains vendor >> caps raw data in "include/system/iommufd.h". >> >> This is a prepare work for moving .realize() after .attach_device(). >> >> Suggested-by: Cédric Le Goater <clg@redhat.com> >> Suggested-by: Eric Auger <eric.auger@redhat.com> >> Suggested-by: Nicolin Chen <nicolinc@nvidia.com> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> include/hw/vfio/vfio-device.h | 1 + >> include/system/iommufd.h | 22 ++++++++++++++++++++++ >> hw/vfio/iommufd.c | 10 +++++++++- >> 3 files changed, 32 insertions(+), 1 deletion(-) >> >> diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h >> index 66797b4c92..09a7af891a 100644 >> --- a/include/hw/vfio/vfio-device.h >> +++ b/include/hw/vfio/vfio-device.h >> @@ -77,6 +77,7 @@ typedef struct VFIODevice { >> bool dirty_tracking; /* Protected by BQL */ >> bool iommu_dirty_tracking; >> HostIOMMUDevice *hiod; >> + HostIOMMUDeviceIOMMUFDCaps caps; >> int devid; >> IOMMUFDBackend *iommufd; >> VFIOIOASHwpt *hwpt; >> diff --git a/include/system/iommufd.h b/include/system/iommufd.h >> index cbab75bfbf..0f337585c9 100644 >> --- a/include/system/iommufd.h >> +++ b/include/system/iommufd.h >> @@ -18,6 +18,9 @@ >> #include "exec/hwaddr.h" >> #include "exec/cpu-common.h" >> #include "system/host_iommu_device.h" >> +#ifdef CONFIG_LINUX >> +#include <linux/iommufd.h> >> +#endif >> >> #define TYPE_IOMMUFD_BACKEND "iommufd" >> OBJECT_DECLARE_TYPE(IOMMUFDBackend, IOMMUFDBackendClass, >IOMMUFD_BACKEND) >> @@ -63,4 +66,23 @@ bool >iommufd_backend_get_dirty_bitmap(IOMMUFDBackend *be, uint32_t hwpt_id, >> Error **errp); >> >> #define TYPE_HOST_IOMMU_DEVICE_IOMMUFD >TYPE_HOST_IOMMU_DEVICE "-iommufd" >> + >> +typedef union VendorCaps { >> + struct iommu_hw_info_vtd vtd; >> + struct iommu_hw_info_arm_smmuv3 smmuv3; >> +} VendorCaps; >> + >> +/** >> + * struct HostIOMMUDeviceIOMMUFDCaps - Define host IOMMU device >capabilities. >> + * >> + * @type: host platform IOMMU type. >> + * >> + * @hw_caps: host platform IOMMU capabilities (e.g. on IOMMUFD this >represents >> + * the @out_capabilities value returned from IOMMU_GET_HW_INFO >ioctl) >> + */ >> +typedef struct HostIOMMUDeviceIOMMUFDCaps { >> + uint32_t type; >> + uint64_t hw_caps; >> + VendorCaps vendor_caps; >> +} HostIOMMUDeviceIOMMUFDCaps; >> #endif >> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c >> index 48db105422..530cde6740 100644 >> --- a/hw/vfio/iommufd.c >> +++ b/hw/vfio/iommufd.c >> @@ -324,7 +324,7 @@ static bool >iommufd_cdev_autodomains_get(VFIODevice *vbasedev, >> * vfio_migration_realize() may decide to use VF dirty tracking >> * instead. >> */ >> - if (vbasedev->hiod->caps.hw_caps & IOMMU_HW_CAP_DIRTY_TRACKING) { >> + if (vbasedev->caps.hw_caps & IOMMU_HW_CAP_DIRTY_TRACKING) { >> flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING; >> } >> >> @@ -475,6 +475,7 @@ static bool iommufd_cdev_attach(const char *name, >VFIODevice *vbasedev, >> int ret, devfd; >> uint32_t ioas_id; >> Error *err = NULL; >> + HostIOMMUDeviceIOMMUFDCaps *caps = &vbasedev->caps; >> const VFIOIOMMUClass *iommufd_vioc = >> >VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD)); >> >> @@ -505,6 +506,13 @@ static bool iommufd_cdev_attach(const char *name, >VFIODevice *vbasedev, >> goto err_alloc_ioas; >> } >> >> + if (!iommufd_backend_get_device_info(vbasedev->iommufd, vbasedev- >>devid, >> + &caps->type, &caps->vendor_caps, >> + sizeof(VendorCaps), &caps->hw_caps, >> + errp)) { >> + goto err_alloc_ioas; >> + } >> + > >I think this will fail on mdev (and thus fail the attachment mistakengly as >there's no IOMMUFDDevice with mdev) ? In case it fails, you can just do: > > if (!vbasedev->mdev && !iommufd_backend_get_device_info(...)) { Indeed, thanks for caching this. BRs, Zhenzhong
>-----Original Message----- >From: Cédric Le Goater <clg@redhat.com> >Subject: Re: [PATCH 1/5] vfio/iommufd: Save host iommu capabilities in >VFIODevice.caps > >On 4/11/25 12:17, Zhenzhong Duan wrote: >> The saved caps copy can be used to check dirty tracking capability. >> >> The capabilities is gotten through IOMMUFD interface, so define a >> new structure HostIOMMUDeviceIOMMUFDCaps which contains vendor >> caps raw data in "include/system/iommufd.h". >> >> This is a prepare work for moving .realize() after .attach_device(). >> >> Suggested-by: Cédric Le Goater <clg@redhat.com> >> Suggested-by: Eric Auger <eric.auger@redhat.com> >> Suggested-by: Nicolin Chen <nicolinc@nvidia.com> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> include/hw/vfio/vfio-device.h | 1 + >> include/system/iommufd.h | 22 ++++++++++++++++++++++ >> hw/vfio/iommufd.c | 10 +++++++++- >> 3 files changed, 32 insertions(+), 1 deletion(-) >> >> diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h >> index 66797b4c92..09a7af891a 100644 >> --- a/include/hw/vfio/vfio-device.h >> +++ b/include/hw/vfio/vfio-device.h >> @@ -77,6 +77,7 @@ typedef struct VFIODevice { >> bool dirty_tracking; /* Protected by BQL */ >> bool iommu_dirty_tracking; >> HostIOMMUDevice *hiod; >> + HostIOMMUDeviceIOMMUFDCaps caps; > >IMO, these capabilities belong to HostIOMMUDevice and not VFIODevice. This was trying to address suggestions in [1], caps is generated by IOMMUFD backend and is only used by hiod_iommufd_get_cap(), hiod_legacy_vfio_get_cap() never check it. By putting it in VFIODevice, I can save vendor caps in a union and raw data format, hiod_iommufd_get_cap() recognizes the raw data format and can check it for a cap support. If keep caps in HostIOMMUDevice, I can think of a change like below to address Eric and Nicolin's suggestion: https://github.com/yiliu1765/qemu/commit/e05f91b2a724cefa8356969cb43284f7c3ec11d1 https://github.com/yiliu1765/qemu/commit/e05f91b2a724cefa8356969cb43284f7c3ec11d1 Does the change make sense for you? [1] https://lists.gnu.org/archive/html/qemu-devel/2025-03/msg01552.html > >I would simply call iommufd_backend_get_device_info() twice where needed : >iommufd_cdev_autodomains_get() and hiod_iommufd_vfio_realize() OK, will do, that's simpler than current change. Thanks Zhenzhong
diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h index 66797b4c92..09a7af891a 100644 --- a/include/hw/vfio/vfio-device.h +++ b/include/hw/vfio/vfio-device.h @@ -77,6 +77,7 @@ typedef struct VFIODevice { bool dirty_tracking; /* Protected by BQL */ bool iommu_dirty_tracking; HostIOMMUDevice *hiod; + HostIOMMUDeviceIOMMUFDCaps caps; int devid; IOMMUFDBackend *iommufd; VFIOIOASHwpt *hwpt; diff --git a/include/system/iommufd.h b/include/system/iommufd.h index cbab75bfbf..0f337585c9 100644 --- a/include/system/iommufd.h +++ b/include/system/iommufd.h @@ -18,6 +18,9 @@ #include "exec/hwaddr.h" #include "exec/cpu-common.h" #include "system/host_iommu_device.h" +#ifdef CONFIG_LINUX +#include <linux/iommufd.h> +#endif #define TYPE_IOMMUFD_BACKEND "iommufd" OBJECT_DECLARE_TYPE(IOMMUFDBackend, IOMMUFDBackendClass, IOMMUFD_BACKEND) @@ -63,4 +66,23 @@ bool iommufd_backend_get_dirty_bitmap(IOMMUFDBackend *be, uint32_t hwpt_id, Error **errp); #define TYPE_HOST_IOMMU_DEVICE_IOMMUFD TYPE_HOST_IOMMU_DEVICE "-iommufd" + +typedef union VendorCaps { + struct iommu_hw_info_vtd vtd; + struct iommu_hw_info_arm_smmuv3 smmuv3; +} VendorCaps; + +/** + * struct HostIOMMUDeviceIOMMUFDCaps - Define host IOMMU device capabilities. + * + * @type: host platform IOMMU type. + * + * @hw_caps: host platform IOMMU capabilities (e.g. on IOMMUFD this represents + * the @out_capabilities value returned from IOMMU_GET_HW_INFO ioctl) + */ +typedef struct HostIOMMUDeviceIOMMUFDCaps { + uint32_t type; + uint64_t hw_caps; + VendorCaps vendor_caps; +} HostIOMMUDeviceIOMMUFDCaps; #endif diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c index 48db105422..530cde6740 100644 --- a/hw/vfio/iommufd.c +++ b/hw/vfio/iommufd.c @@ -324,7 +324,7 @@ static bool iommufd_cdev_autodomains_get(VFIODevice *vbasedev, * vfio_migration_realize() may decide to use VF dirty tracking * instead. */ - if (vbasedev->hiod->caps.hw_caps & IOMMU_HW_CAP_DIRTY_TRACKING) { + if (vbasedev->caps.hw_caps & IOMMU_HW_CAP_DIRTY_TRACKING) { flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING; } @@ -475,6 +475,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, int ret, devfd; uint32_t ioas_id; Error *err = NULL; + HostIOMMUDeviceIOMMUFDCaps *caps = &vbasedev->caps; const VFIOIOMMUClass *iommufd_vioc = VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD)); @@ -505,6 +506,13 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, goto err_alloc_ioas; } + if (!iommufd_backend_get_device_info(vbasedev->iommufd, vbasedev->devid, + &caps->type, &caps->vendor_caps, + sizeof(VendorCaps), &caps->hw_caps, + errp)) { + goto err_alloc_ioas; + } + /* try to attach to an existing container in this space */ QLIST_FOREACH(bcontainer, &space->containers, next) { container = container_of(bcontainer, VFIOIOMMUFDContainer, bcontainer);
The saved caps copy can be used to check dirty tracking capability. The capabilities is gotten through IOMMUFD interface, so define a new structure HostIOMMUDeviceIOMMUFDCaps which contains vendor caps raw data in "include/system/iommufd.h". This is a prepare work for moving .realize() after .attach_device(). Suggested-by: Cédric Le Goater <clg@redhat.com> Suggested-by: Eric Auger <eric.auger@redhat.com> Suggested-by: Nicolin Chen <nicolinc@nvidia.com> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- include/hw/vfio/vfio-device.h | 1 + include/system/iommufd.h | 22 ++++++++++++++++++++++ hw/vfio/iommufd.c | 10 +++++++++- 3 files changed, 32 insertions(+), 1 deletion(-)