| Message ID | 20240228035900.1085727-9-zhenzhong.duan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add a host IOMMU device abstraction | expand |
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 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); > g_free(vdev->emulated_config_bits); > g_free(vdev->rom); > /*
>-----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); >> /*
On 3/19/24 04:46, Duan, Zhenzhong wrote: > >> -----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: Oh yes I remember now. I had exactly the same question some months ago :-/ So that's fine then Eric > > --- 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); >>> /*
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 */ + host_iommu_device_create(vbasedev); + 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); g_free(vdev->emulated_config_bits); g_free(vdev->rom); /*
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- hw/vfio/pci.c | 4 ++++ 1 file changed, 4 insertions(+)