| Message ID | 20231011200934.549735-1-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | vfio/pci: Remove vfio_detach_device from vfio_realize error path | expand |
>-----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 >
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); > }
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); }
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(-)