Previously device attaching depends on realize() getting host iommu
capabilities to check dirty tracking support.
Now we save a caps copy in VFIODevice and check that copy for dirty
tracking support, there is no dependency any more, move realize()
call after attach_device() call in vfio_device_attach().
Drop vfio_device_hiod_realize() which looks redundant now.
Suggested-by: Cédric Le Goater <clg@redhat.com>
Suggested-by: Donald Dutile <ddutile@redhat.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
include/hw/vfio/vfio-device.h | 1 -
hw/vfio/container.c | 4 ----
hw/vfio/device.c | 28 +++++++++++-----------------
hw/vfio/iommufd.c | 4 ----
4 files changed, 11 insertions(+), 26 deletions(-)
diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
index 09a7af891a..14559733c6 100644
--- a/include/hw/vfio/vfio-device.h
+++ b/include/hw/vfio/vfio-device.h
@@ -124,7 +124,6 @@ bool vfio_device_irq_set_signaling(VFIODevice *vbasedev, int index, int subindex
void vfio_device_reset_handler(void *opaque);
bool vfio_device_is_mdev(VFIODevice *vbasedev);
-bool vfio_device_hiod_realize(VFIODevice *vbasedev, Error **errp);
bool vfio_device_attach(char *name, VFIODevice *vbasedev,
AddressSpace *as, Error **errp);
void vfio_device_detach(VFIODevice *vbasedev);
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 23a3373470..676e88cef4 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -883,10 +883,6 @@ static bool vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev,
trace_vfio_device_attach(vbasedev->name, groupid);
- if (!vfio_device_hiod_realize(vbasedev, errp)) {
- return false;
- }
-
group = vfio_group_get(groupid, as, errp);
if (!group) {
return false;
diff --git a/hw/vfio/device.c b/hw/vfio/device.c
index 4de6948cf4..6154d3f443 100644
--- a/hw/vfio/device.c
+++ b/hw/vfio/device.c
@@ -347,17 +347,6 @@ bool vfio_device_is_mdev(VFIODevice *vbasedev)
return subsys && (strcmp(subsys, "/sys/bus/mdev") == 0);
}
-bool vfio_device_hiod_realize(VFIODevice *vbasedev, Error **errp)
-{
- HostIOMMUDevice *hiod = vbasedev->hiod;
-
- if (!hiod) {
- return true;
- }
-
- return HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp);
-}
-
VFIODevice *vfio_get_vfio_device(Object *obj)
{
if (object_dynamic_cast(obj, TYPE_VFIO_PCI)) {
@@ -372,6 +361,7 @@ bool vfio_device_attach(char *name, VFIODevice *vbasedev,
{
const VFIOIOMMUClass *ops =
VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY));
+ HostIOMMUDeviceClass *hiodc;
HostIOMMUDevice *hiod = NULL;
if (vbasedev->iommufd) {
@@ -380,16 +370,20 @@ bool vfio_device_attach(char *name, VFIODevice *vbasedev,
assert(ops);
+ if (!ops->attach_device(name, vbasedev, as, errp)) {
+ return false;
+ }
if (!vbasedev->mdev) {
hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename));
- vbasedev->hiod = hiod;
- }
+ hiodc = HOST_IOMMU_DEVICE_GET_CLASS(hiod);
- if (!ops->attach_device(name, vbasedev, as, errp)) {
- object_unref(hiod);
- vbasedev->hiod = NULL;
- return false;
+ if (!hiodc->realize(hiod, vbasedev, errp)) {
+ object_unref(hiod);
+ ops->detach_device(vbasedev);
+ return false;
+ }
+ vbasedev->hiod = hiod;
}
return true;
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 530cde6740..e05b472e35 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -502,10 +502,6 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
* FD to be connected and having a devid to be able to successfully call
* iommufd_backend_get_device_info().
*/
- if (!vfio_device_hiod_realize(vbasedev, errp)) {
- goto err_alloc_ioas;
- }
-
if (!iommufd_backend_get_device_info(vbasedev->iommufd, vbasedev->devid,
&caps->type, &caps->vendor_caps,
sizeof(VendorCaps), &caps->hw_caps,
--
2.34.1
On 4/11/25 6:17 AM, Zhenzhong Duan wrote: > Previously device attaching depends on realize() getting host iommu capabilities to check dirty tracking support. Now we save a caps copy in VFIODevice and check that copy for dirty tracking support, there is no dependency any more, move realize() call after attach_device() call in vfio_device_attach(). Drop vfio_device_hiod_realize() which looks redundant now. Suggested-by: Cédric Le Goater <clg@redhat.com> Suggested-by: Donald Dutile <ddutile@redhat.com> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> Reviewed-by: Donald Dutile <ddutile@redhat.com>
On 4/11/25 12:17, Zhenzhong Duan wrote:
> Previously device attaching depends on realize() getting host iommu
> capabilities to check dirty tracking support.
>
> Now we save a caps copy in VFIODevice and check that copy for dirty
> tracking support, there is no dependency any more, move realize()
> call after attach_device() call in vfio_device_attach().
>
> Drop vfio_device_hiod_realize() which looks redundant now.
>
> Suggested-by: Cédric Le Goater <clg@redhat.com>
> Suggested-by: Donald Dutile <ddutile@redhat.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
> include/hw/vfio/vfio-device.h | 1 -
> hw/vfio/container.c | 4 ----
> hw/vfio/device.c | 28 +++++++++++-----------------
> hw/vfio/iommufd.c | 4 ----
> 4 files changed, 11 insertions(+), 26 deletions(-)
>
> diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
> index 09a7af891a..14559733c6 100644
> --- a/include/hw/vfio/vfio-device.h
> +++ b/include/hw/vfio/vfio-device.h
> @@ -124,7 +124,6 @@ bool vfio_device_irq_set_signaling(VFIODevice *vbasedev, int index, int subindex
>
> void vfio_device_reset_handler(void *opaque);
> bool vfio_device_is_mdev(VFIODevice *vbasedev);
> -bool vfio_device_hiod_realize(VFIODevice *vbasedev, Error **errp);
> bool vfio_device_attach(char *name, VFIODevice *vbasedev,
> AddressSpace *as, Error **errp);
> void vfio_device_detach(VFIODevice *vbasedev);
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index 23a3373470..676e88cef4 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -883,10 +883,6 @@ static bool vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev,
>
> trace_vfio_device_attach(vbasedev->name, groupid);
>
> - if (!vfio_device_hiod_realize(vbasedev, errp)) {
> - return false;
> - }
> -
> group = vfio_group_get(groupid, as, errp);
> if (!group) {
> return false;
> diff --git a/hw/vfio/device.c b/hw/vfio/device.c
> index 4de6948cf4..6154d3f443 100644
> --- a/hw/vfio/device.c
> +++ b/hw/vfio/device.c
> @@ -347,17 +347,6 @@ bool vfio_device_is_mdev(VFIODevice *vbasedev)
> return subsys && (strcmp(subsys, "/sys/bus/mdev") == 0);
> }
>
> -bool vfio_device_hiod_realize(VFIODevice *vbasedev, Error **errp)
> -{
> - HostIOMMUDevice *hiod = vbasedev->hiod;
> -
> - if (!hiod) {
> - return true;
> - }
> -
> - return HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp);
> -}
> -
> VFIODevice *vfio_get_vfio_device(Object *obj)
> {
> if (object_dynamic_cast(obj, TYPE_VFIO_PCI)) {
> @@ -372,6 +361,7 @@ bool vfio_device_attach(char *name, VFIODevice *vbasedev,
> {
> const VFIOIOMMUClass *ops =
> VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY));
> + HostIOMMUDeviceClass *hiodc;
> HostIOMMUDevice *hiod = NULL;
>
> if (vbasedev->iommufd) {
> @@ -380,16 +370,20 @@ bool vfio_device_attach(char *name, VFIODevice *vbasedev,
>
> assert(ops);
>
> + if (!ops->attach_device(name, vbasedev, as, errp)) {
> + return false;
> + }
>
> if (!vbasedev->mdev) {
> hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename));
> - vbasedev->hiod = hiod;
> - }
> + hiodc = HOST_IOMMU_DEVICE_GET_CLASS(hiod);
>
> - if (!ops->attach_device(name, vbasedev, as, errp)) {
> - object_unref(hiod);
> - vbasedev->hiod = NULL;
> - return false;
> + if (!hiodc->realize(hiod, vbasedev, errp)) {
> + object_unref(hiod);
> + ops->detach_device(vbasedev);
> + return false;
> + }
> + vbasedev->hiod = hiod;
This is not what I meant. I was not clear enough. Sorry about that.
hiodc->realize can be called under each container backend: legacy
and iommufd. I don't see much much value to make it common and
it would remove the unref/detach sequence to handle errors.
Thanks,
C.
> }
>
> return true;
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index 530cde6740..e05b472e35 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -502,10 +502,6 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
> * FD to be connected and having a devid to be able to successfully call
> * iommufd_backend_get_device_info().
> */
> - if (!vfio_device_hiod_realize(vbasedev, errp)) {
> - goto err_alloc_ioas;
> - }
> -
> if (!iommufd_backend_get_device_info(vbasedev->iommufd, vbasedev->devid,
> &caps->type, &caps->vendor_caps,
> sizeof(VendorCaps), &caps->hw_caps,
>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: Re: [PATCH 2/5] vfio: Move realize() after attach_device()
>
>On 4/11/25 12:17, Zhenzhong Duan wrote:
>> Previously device attaching depends on realize() getting host iommu
>> capabilities to check dirty tracking support.
>>
>> Now we save a caps copy in VFIODevice and check that copy for dirty
>> tracking support, there is no dependency any more, move realize()
>> call after attach_device() call in vfio_device_attach().
>>
>> Drop vfio_device_hiod_realize() which looks redundant now.
>>
>> Suggested-by: Cédric Le Goater <clg@redhat.com>
>> Suggested-by: Donald Dutile <ddutile@redhat.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>> include/hw/vfio/vfio-device.h | 1 -
>> hw/vfio/container.c | 4 ----
>> hw/vfio/device.c | 28 +++++++++++-----------------
>> hw/vfio/iommufd.c | 4 ----
>> 4 files changed, 11 insertions(+), 26 deletions(-)
>>
>> diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
>> index 09a7af891a..14559733c6 100644
>> --- a/include/hw/vfio/vfio-device.h
>> +++ b/include/hw/vfio/vfio-device.h
>> @@ -124,7 +124,6 @@ bool vfio_device_irq_set_signaling(VFIODevice
>*vbasedev, int index, int subindex
>>
>> void vfio_device_reset_handler(void *opaque);
>> bool vfio_device_is_mdev(VFIODevice *vbasedev);
>> -bool vfio_device_hiod_realize(VFIODevice *vbasedev, Error **errp);
>> bool vfio_device_attach(char *name, VFIODevice *vbasedev,
>> AddressSpace *as, Error **errp);
>> void vfio_device_detach(VFIODevice *vbasedev);
>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>> index 23a3373470..676e88cef4 100644
>> --- a/hw/vfio/container.c
>> +++ b/hw/vfio/container.c
>> @@ -883,10 +883,6 @@ static bool vfio_legacy_attach_device(const char
>*name, VFIODevice *vbasedev,
>>
>> trace_vfio_device_attach(vbasedev->name, groupid);
>>
>> - if (!vfio_device_hiod_realize(vbasedev, errp)) {
>> - return false;
>> - }
>> -
>> group = vfio_group_get(groupid, as, errp);
>> if (!group) {
>> return false;
>> diff --git a/hw/vfio/device.c b/hw/vfio/device.c
>> index 4de6948cf4..6154d3f443 100644
>> --- a/hw/vfio/device.c
>> +++ b/hw/vfio/device.c
>> @@ -347,17 +347,6 @@ bool vfio_device_is_mdev(VFIODevice *vbasedev)
>> return subsys && (strcmp(subsys, "/sys/bus/mdev") == 0);
>> }
>>
>> -bool vfio_device_hiod_realize(VFIODevice *vbasedev, Error **errp)
>> -{
>> - HostIOMMUDevice *hiod = vbasedev->hiod;
>> -
>> - if (!hiod) {
>> - return true;
>> - }
>> -
>> - return HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev,
>errp);
>> -}
>> -
>> VFIODevice *vfio_get_vfio_device(Object *obj)
>> {
>> if (object_dynamic_cast(obj, TYPE_VFIO_PCI)) {
>> @@ -372,6 +361,7 @@ bool vfio_device_attach(char *name, VFIODevice
>*vbasedev,
>> {
>> const VFIOIOMMUClass *ops =
>>
>VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY));
>> + HostIOMMUDeviceClass *hiodc;
>> HostIOMMUDevice *hiod = NULL;
>>
>> if (vbasedev->iommufd) {
>> @@ -380,16 +370,20 @@ bool vfio_device_attach(char *name, VFIODevice
>*vbasedev,
>>
>> assert(ops);
>>
>> + if (!ops->attach_device(name, vbasedev, as, errp)) {
>> + return false;
>> + }
>>
>> if (!vbasedev->mdev) {
>> hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename));
>> - vbasedev->hiod = hiod;
>> - }
>> + hiodc = HOST_IOMMU_DEVICE_GET_CLASS(hiod);
>>
>> - if (!ops->attach_device(name, vbasedev, as, errp)) {
>> - object_unref(hiod);
>> - vbasedev->hiod = NULL;
>> - return false;
>> + if (!hiodc->realize(hiod, vbasedev, errp)) {
>> + object_unref(hiod);
>> + ops->detach_device(vbasedev);
>> + return false;
>> + }
>> + vbasedev->hiod = hiod;
>
>This is not what I meant. I was not clear enough. Sorry about that.
>
>hiodc->realize can be called under each container backend: legacy
>and iommufd. I don't see much much value to make it common and
>it would remove the unref/detach sequence to handle errors.
OK, I'll switch to per backend change.
I was trying to avoid duplicate change for both backend, also realize() will not fail normally and unref/detach sequence is untouched.
Thanks
Zhenzhong
Hi, On 11/4/25 12:17, Zhenzhong Duan wrote: > Previously device attaching depends on realize() getting host iommu > capabilities to check dirty tracking support. > > Now we save a caps copy in VFIODevice and check that copy for dirty > tracking support, there is no dependency any more, move realize() > call after attach_device() call in vfio_device_attach(). > > Drop vfio_device_hiod_realize() which looks redundant now. > > Suggested-by: Cédric Le Goater <clg@redhat.com> > Suggested-by: Donald Dutile <ddutile@redhat.com> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > include/hw/vfio/vfio-device.h | 1 - > hw/vfio/container.c | 4 ---- > hw/vfio/device.c | 28 +++++++++++----------------- > hw/vfio/iommufd.c | 4 ---- > 4 files changed, 11 insertions(+), 26 deletions(-) > > diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h > index 09a7af891a..14559733c6 100644 > --- a/include/hw/vfio/vfio-device.h > +++ b/include/hw/vfio/vfio-device.h > @@ -124,7 +124,6 @@ bool vfio_device_irq_set_signaling(VFIODevice *vbasedev, int index, int subindex > > void vfio_device_reset_handler(void *opaque); > bool vfio_device_is_mdev(VFIODevice *vbasedev); > -bool vfio_device_hiod_realize(VFIODevice *vbasedev, Error **errp); Pre-existing, but can we add documentation about what vfio_device_attach does, in particular in which state is the device once attached (or if attachment failed)? > bool vfio_device_attach(char *name, VFIODevice *vbasedev, > AddressSpace *as, Error **errp); > void vfio_device_detach(VFIODevice *vbasedev);
>-----Original Message----- >From: Philippe Mathieu-Daudé <philmd@linaro.org> >Subject: Re: [PATCH 2/5] vfio: Move realize() after attach_device() > >Hi, > >On 11/4/25 12:17, Zhenzhong Duan wrote: >> Previously device attaching depends on realize() getting host iommu >> capabilities to check dirty tracking support. >> >> Now we save a caps copy in VFIODevice and check that copy for dirty >> tracking support, there is no dependency any more, move realize() >> call after attach_device() call in vfio_device_attach(). >> >> Drop vfio_device_hiod_realize() which looks redundant now. >> >> Suggested-by: Cédric Le Goater <clg@redhat.com> >> Suggested-by: Donald Dutile <ddutile@redhat.com> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> include/hw/vfio/vfio-device.h | 1 - >> hw/vfio/container.c | 4 ---- >> hw/vfio/device.c | 28 +++++++++++----------------- >> hw/vfio/iommufd.c | 4 ---- >> 4 files changed, 11 insertions(+), 26 deletions(-) >> >> diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h >> index 09a7af891a..14559733c6 100644 >> --- a/include/hw/vfio/vfio-device.h >> +++ b/include/hw/vfio/vfio-device.h >> @@ -124,7 +124,6 @@ bool vfio_device_irq_set_signaling(VFIODevice >*vbasedev, int index, int subindex >> >> void vfio_device_reset_handler(void *opaque); >> bool vfio_device_is_mdev(VFIODevice *vbasedev); >> -bool vfio_device_hiod_realize(VFIODevice *vbasedev, Error **errp); > >Pre-existing, but can we add documentation about what vfio_device_attach >does, in particular in which state is the device once attached (or if >attachment failed)? Sure, it can be in a separate patch. Thanks Zhenzhong > >> bool vfio_device_attach(char *name, VFIODevice *vbasedev, >> AddressSpace *as, Error **errp); >> void vfio_device_detach(VFIODevice *vbasedev);
© 2016 - 2025 Red Hat, Inc.