RE: [PATCH v1 08/11] vfio/pci: Allocate and initialize HostIOMMUDevice after attachment

From: "Duan, Zhenzhong" <zhenzhong.duan@intel.com>
To: "eric.auger@redhat.com" <eric.auger@redhat.com>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>
Cc: "alex.williamson@redhat.com" <alex.williamson@redhat.com>,
	"clg@redhat.com" <clg@redhat.com>,
	"peterx@redhat.com" <peterx@redhat.com>,
	"jasowang@redhat.com" <jasowang@redhat.com>,
	"mst@redhat.com" <mst@redhat.com>,
	"jgg@nvidia.com" <jgg@nvidia.com>,
	"nicolinc@nvidia.com" <nicolinc@nvidia.com>,
	"joao.m.martins@oracle.com" <joao.m.martins@oracle.com>,
	"Tian, Kevin" <kevin.tian@intel.com>,
	"Liu, Yi L" <yi.l.liu@intel.com>,
	"Sun, Yi Y" <yi.y.sun@intel.com>,
	"Peng, Chao P" <chao.p.peng@intel.com>
Subject: RE: [PATCH v1 08/11] vfio/pci: Allocate and initialize HostIOMMUDevice after attachment
Date: Tue, 19 Mar 2024 03:46:31 +0000	[thread overview]
Message-ID: <SJ0PR11MB6744FC5D025D73D474838F90922C2@SJ0PR11MB6744.namprd11.prod.outlook.com> (raw)
In-Reply-To: <750d1d70-37cd-4e00-a092-204f0dbbe61c@redhat.com>



>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>Subject: Re: [PATCH v1 08/11] vfio/pci: Allocate and initialize
>HostIOMMUDevice after attachment
>
>
>
>On 2/28/24 04:58, Zhenzhong Duan wrote:
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>  hw/vfio/pci.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 4fa387f043..6cc7de5d10 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -3006,6 +3006,9 @@ static void vfio_realize(PCIDevice *pdev, Error
>**errp)
>>          goto error;
>>      }
>>
>> +    /* Allocate and initialize HostIOMMUDevice after attachment succeed
>*/
>after successful attachment?
>> +    host_iommu_device_create(vbasedev);
>> +
>you shall free on error: as well

I free it in vfio_instance_finalize().
Vfio-pci's design is special, it didn't free all allocated resources in realize's error path,
They are freed in _finalize(). e.g., vdev->emulated_config_bits, vdev->rom, devices and group resources(vfio_detach_device).
I'm following the same way. I'm fine to free it as you suggested something like below:

--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3246,6 +3246,7 @@ out_teardown:
     vfio_teardown_msi(vdev);
     vfio_bars_exit(vdev);
 error:
+    g_free(vdev->vbasedev.base_hdev);
     error_prepend(errp, VFIO_MSG_PREFIX, vbasedev->name);
 }

@@ -3288,6 +3289,7 @@ static void vfio_exitfn(PCIDevice *pdev)
     vfio_bars_exit(vdev);
     vfio_migration_exit(vbasedev);
     pci_device_unset_iommu_device(pdev);
+    g_free(vdev->vbasedev.base_hdev);
 }

>
>Eric
>>      vfio_populate_device(vdev, &err);
>>      if (err) {
>>          error_propagate(errp, err);
>> @@ -3244,6 +3247,7 @@ static void vfio_instance_finalize(Object *obj)
>>
>>      vfio_display_finalize(vdev);
>>      vfio_bars_finalize(vdev);
>> +    g_free(vdev->vbasedev.base_hdev);

I free it here.

Thanks
Zhenzhong

>>      g_free(vdev->emulated_config_bits);
>>      g_free(vdev->rom);
>>      /*

