hw/vfio/pci.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)
In vfio_realize, on the error path, we currently call
vfio_detach_device() after a successful vfio_attach_device.
While this looks natural, vfio_instance_finalize also induces
a vfio_detach_device(), and it seems to be the right place
instead as other resources are released there which happen
to be a prerequisite to a successful UNSET_CONTAINER.
So let's rely on the finalize vfio_detach_device call to free
all the relevant resources.
Fixes: a28e06621170 ("vfio/pci: Introduce vfio_[attach/detach]_device")
Reported-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
This applies on top of vfio-next
Note I am not sure the SHA1 of
vfio/pci: Introduce vfio_[attach/detach]_device
is stable.
---
hw/vfio/pci.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 40ae46266e..6e3f6aba28 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3115,7 +3115,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
vfio_populate_device(vdev, &err);
if (err) {
error_propagate(errp, err);
- goto out_detach;
+ goto error;
}
/* Get a copy of config space */
@@ -3125,7 +3125,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
if (ret < (int)MIN(pci_config_size(&vdev->pdev), vdev->config_size)) {
ret = ret < 0 ? -errno : -EFAULT;
error_setg_errno(errp, -ret, "failed to read device config space");
- goto out_detach;
+ goto error;
}
/* vfio emulates a lot for us, but some bits need extra love */
@@ -3144,7 +3144,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
if (vdev->vendor_id != PCI_ANY_ID) {
if (vdev->vendor_id >= 0xffff) {
error_setg(errp, "invalid PCI vendor ID provided");
- goto out_detach;
+ goto error;
}
vfio_add_emulated_word(vdev, PCI_VENDOR_ID, vdev->vendor_id, ~0);
trace_vfio_pci_emulated_vendor_id(vbasedev->name, vdev->vendor_id);
@@ -3155,7 +3155,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
if (vdev->device_id != PCI_ANY_ID) {
if (vdev->device_id > 0xffff) {
error_setg(errp, "invalid PCI device ID provided");
- goto out_detach;
+ goto error;
}
vfio_add_emulated_word(vdev, PCI_DEVICE_ID, vdev->device_id, ~0);
trace_vfio_pci_emulated_device_id(vbasedev->name, vdev->device_id);
@@ -3166,7 +3166,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
if (vdev->sub_vendor_id != PCI_ANY_ID) {
if (vdev->sub_vendor_id > 0xffff) {
error_setg(errp, "invalid PCI subsystem vendor ID provided");
- goto out_detach;
+ goto error;
}
vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_VENDOR_ID,
vdev->sub_vendor_id, ~0);
@@ -3177,7 +3177,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
if (vdev->sub_device_id != PCI_ANY_ID) {
if (vdev->sub_device_id > 0xffff) {
error_setg(errp, "invalid PCI subsystem device ID provided");
- goto out_detach;
+ goto error;
}
vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_ID, vdev->sub_device_id, ~0);
trace_vfio_pci_emulated_sub_device_id(vbasedev->name,
@@ -3210,7 +3210,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
vfio_msix_early_setup(vdev, &err);
if (err) {
error_propagate(errp, err);
- goto out_detach;
+ goto error;
}
vfio_bars_register(vdev);
@@ -3326,8 +3326,6 @@ out_deregister:
out_teardown:
vfio_teardown_msi(vdev);
vfio_bars_exit(vdev);
-out_detach:
- vfio_detach_device(vbasedev);
error:
error_prepend(errp, VFIO_MSG_PREFIX, vbasedev->name);
}
--
2.41.0
On 10/11/23 22:09, Eric Auger wrote: > In vfio_realize, on the error path, we currently call > vfio_detach_device() after a successful vfio_attach_device. > While this looks natural, vfio_instance_finalize also induces > a vfio_detach_device(), and it seems to be the right place > instead as other resources are released there which happen > to be a prerequisite to a successful UNSET_CONTAINER. > > So let's rely on the finalize vfio_detach_device call to free > all the relevant resources. > > Fixes: a28e06621170 ("vfio/pci: Introduce vfio_[attach/detach]_device") > Reported-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > Signed-off-by: Eric Auger <eric.auger@redhat.com> > > --- > > This applies on top of vfio-next Applied to vfio-next. > Note I am not sure the SHA1 of > vfio/pci: Introduce vfio_[attach/detach]_device > is stable. It should if I only rebase on master. If I need to re-apply, I will drop the Fixes tag. Thanks, C. > --- > hw/vfio/pci.c | 16 +++++++--------- > 1 file changed, 7 insertions(+), 9 deletions(-) > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index 40ae46266e..6e3f6aba28 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -3115,7 +3115,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > vfio_populate_device(vdev, &err); > if (err) { > error_propagate(errp, err); > - goto out_detach; > + goto error; > } > > /* Get a copy of config space */ > @@ -3125,7 +3125,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > if (ret < (int)MIN(pci_config_size(&vdev->pdev), vdev->config_size)) { > ret = ret < 0 ? -errno : -EFAULT; > error_setg_errno(errp, -ret, "failed to read device config space"); > - goto out_detach; > + goto error; > } > > /* vfio emulates a lot for us, but some bits need extra love */ > @@ -3144,7 +3144,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > if (vdev->vendor_id != PCI_ANY_ID) { > if (vdev->vendor_id >= 0xffff) { > error_setg(errp, "invalid PCI vendor ID provided"); > - goto out_detach; > + goto error; > } > vfio_add_emulated_word(vdev, PCI_VENDOR_ID, vdev->vendor_id, ~0); > trace_vfio_pci_emulated_vendor_id(vbasedev->name, vdev->vendor_id); > @@ -3155,7 +3155,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > if (vdev->device_id != PCI_ANY_ID) { > if (vdev->device_id > 0xffff) { > error_setg(errp, "invalid PCI device ID provided"); > - goto out_detach; > + goto error; > } > vfio_add_emulated_word(vdev, PCI_DEVICE_ID, vdev->device_id, ~0); > trace_vfio_pci_emulated_device_id(vbasedev->name, vdev->device_id); > @@ -3166,7 +3166,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > if (vdev->sub_vendor_id != PCI_ANY_ID) { > if (vdev->sub_vendor_id > 0xffff) { > error_setg(errp, "invalid PCI subsystem vendor ID provided"); > - goto out_detach; > + goto error; > } > vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_VENDOR_ID, > vdev->sub_vendor_id, ~0); > @@ -3177,7 +3177,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > if (vdev->sub_device_id != PCI_ANY_ID) { > if (vdev->sub_device_id > 0xffff) { > error_setg(errp, "invalid PCI subsystem device ID provided"); > - goto out_detach; > + goto error; > } > vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_ID, vdev->sub_device_id, ~0); > trace_vfio_pci_emulated_sub_device_id(vbasedev->name, > @@ -3210,7 +3210,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > vfio_msix_early_setup(vdev, &err); > if (err) { > error_propagate(errp, err); > - goto out_detach; > + goto error; > } > > vfio_bars_register(vdev); > @@ -3326,8 +3326,6 @@ out_deregister: > out_teardown: > vfio_teardown_msi(vdev); > vfio_bars_exit(vdev); > -out_detach: > - vfio_detach_device(vbasedev); > error: > error_prepend(errp, VFIO_MSG_PREFIX, vbasedev->name); > }
>-----Original Message----- >From: Eric Auger <eric.auger@redhat.com> >Sent: Thursday, October 12, 2023 4:10 AM >To: eric.auger.pro@gmail.com; eric.auger@redhat.com; qemu- >devel@nongnu.org; alex.williamson@redhat.com; clg@redhat.com; Duan, >Zhenzhong <zhenzhong.duan@intel.com>; Liu, Yi L <yi.l.liu@intel.com>; >yanghliu@redhat.com >Subject: [PATCH] vfio/pci: Remove vfio_detach_device from vfio_realize error >path > >In vfio_realize, on the error path, we currently call >vfio_detach_device() after a successful vfio_attach_device. >While this looks natural, vfio_instance_finalize also induces >a vfio_detach_device(), and it seems to be the right place >instead as other resources are released there which happen >to be a prerequisite to a successful UNSET_CONTAINER. > >So let's rely on the finalize vfio_detach_device call to free >all the relevant resources. > >Fixes: a28e06621170 ("vfio/pci: Introduce vfio_[attach/detach]_device") >Reported-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >Signed-off-by: Eric Auger <eric.auger@redhat.com> Tested-by: Zhenzhong Duan <zhenzhong.duan@intel.com> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com> Thanks Zhenzhong
Hi Zhenzhong, On 10/12/23 04:34, Duan, Zhenzhong wrote: > >> -----Original Message----- >> From: Eric Auger <eric.auger@redhat.com> >> Sent: Thursday, October 12, 2023 4:10 AM >> To: eric.auger.pro@gmail.com; eric.auger@redhat.com; qemu- >> devel@nongnu.org; alex.williamson@redhat.com; clg@redhat.com; Duan, >> Zhenzhong <zhenzhong.duan@intel.com>; Liu, Yi L <yi.l.liu@intel.com>; >> yanghliu@redhat.com >> Subject: [PATCH] vfio/pci: Remove vfio_detach_device from vfio_realize error >> path >> >> In vfio_realize, on the error path, we currently call >> vfio_detach_device() after a successful vfio_attach_device. >> While this looks natural, vfio_instance_finalize also induces >> a vfio_detach_device(), and it seems to be the right place >> instead as other resources are released there which happen >> to be a prerequisite to a successful UNSET_CONTAINER. >> >> So let's rely on the finalize vfio_detach_device call to free >> all the relevant resources. >> >> Fixes: a28e06621170 ("vfio/pci: Introduce vfio_[attach/detach]_device") >> Reported-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> > Tested-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com> Thanks! Eric > > Thanks > Zhenzhong >
© 2016 - 2024 Red Hat, Inc.