>-----Original Message-----
>From: Cédric Le Goater <c...@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 <c...@redhat.com>
>> Suggested-by: Donald Dutile <ddut...@redhat.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.d...@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
