[Qemu-devel] [PATCH] hw/pci: fix error flow in pci multifunction init

[Qemu-devel] [PATCH] hw/pci: fix error flow in pci multifunction init

[Marcel Apfelbaum]

Scenario:
  - There is a non multifunction pci device A on 00:0X.0.
  - Hot-plug another multifunction pci device B at 00:0X.1.
  - The operation will fail of course.
  - Try to hot-plug the B device 2-3 more times, qemu will crash.

Reason: The error flow leaves the B's address space into global address spaces
list, but the device object is freed. Fixed that.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 hw/pci/pci.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index aa2a395..5f454dd 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -793,6 +793,15 @@ static void pci_config_free(PCIDevice *pci_dev)
     g_free(pci_dev->used);
 }
 
+static void do_pci_unregister_device(PCIDevice *pci_dev)
+{
+    pci_dev->bus->devices[pci_dev->devfn] = NULL;
+    pci_config_free(pci_dev);
+
+    address_space_destroy(&pci_dev->bus_master_as);
+    memory_region_destroy(&pci_dev->bus_master_enable_region);
+}
+
 /* -1 for devfn means auto assign */
 static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
                                          const char *name, int devfn)
@@ -858,7 +867,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
         pci_init_mask_bridge(pci_dev);
     }
     if (pci_init_multifunction(bus, pci_dev)) {
-        pci_config_free(pci_dev);
+        do_pci_unregister_device(pci_dev);
         return NULL;
     }
 
@@ -873,15 +882,6 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
     return pci_dev;
 }
 
-static void do_pci_unregister_device(PCIDevice *pci_dev)
-{
-    pci_dev->bus->devices[pci_dev->devfn] = NULL;
-    pci_config_free(pci_dev);
-
-    address_space_destroy(&pci_dev->bus_master_as);
-    memory_region_destroy(&pci_dev->bus_master_enable_region);
-}
-
 static void pci_unregister_io_regions(PCIDevice *pci_dev)
 {
     PCIIORegion *r;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] hw/pci: fix error flow in pci multifunction init

[Marcel Apfelbaum]

On Tue, 2014-01-21 at 18:37 +0200, Marcel Apfelbaum wrote:
> Scenario:
>   - There is a non multifunction pci device A on 00:0X.0.
>   - Hot-plug another multifunction pci device B at 00:0X.1.
>   - The operation will fail of course.
>   - Try to hot-plug the B device 2-3 more times, qemu will crash.
> 
> Reason: The error flow leaves the B's address space into global address spaces
> list, but the device object is freed. Fixed that.
Ping

Thanks,
Marcel

> 
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
>  hw/pci/pci.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index aa2a395..5f454dd 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -793,6 +793,15 @@ static void pci_config_free(PCIDevice *pci_dev)
>      g_free(pci_dev->used);
>  }
>  
> +static void do_pci_unregister_device(PCIDevice *pci_dev)
> +{
> +    pci_dev->bus->devices[pci_dev->devfn] = NULL;
> +    pci_config_free(pci_dev);
> +
> +    address_space_destroy(&pci_dev->bus_master_as);
> +    memory_region_destroy(&pci_dev->bus_master_enable_region);
> +}
> +
>  /* -1 for devfn means auto assign */
>  static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>                                           const char *name, int devfn)
> @@ -858,7 +867,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>          pci_init_mask_bridge(pci_dev);
>      }
>      if (pci_init_multifunction(bus, pci_dev)) {
> -        pci_config_free(pci_dev);
> +        do_pci_unregister_device(pci_dev);
>          return NULL;
>      }
>  
> @@ -873,15 +882,6 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>      return pci_dev;
>  }
>  
> -static void do_pci_unregister_device(PCIDevice *pci_dev)
> -{
> -    pci_dev->bus->devices[pci_dev->devfn] = NULL;
> -    pci_config_free(pci_dev);
> -
> -    address_space_destroy(&pci_dev->bus_master_as);
> -    memory_region_destroy(&pci_dev->bus_master_enable_region);
> -}
> -
>  static void pci_unregister_io_regions(PCIDevice *pci_dev)
>  {
>      PCIIORegion *r;

Re: [Qemu-devel] [PATCH] hw/pci: fix error flow in pci multifunction init

[Markus Armbruster]

Marcel Apfelbaum <marcel.a@redhat.com> writes:

> Scenario:
>   - There is a non multifunction pci device A on 00:0X.0.
>   - Hot-plug another multifunction pci device B at 00:0X.1.
>   - The operation will fail of course.
>   - Try to hot-plug the B device 2-3 more times, qemu will crash.
>
> Reason: The error flow leaves the B's address space into global address spaces
> list, but the device object is freed. Fixed that.
>
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>

Looks good to me, but I'm not a PCI expert.

Re: [Qemu-devel] [PATCH] hw/pci: fix error flow in pci multifunction init

[Michael S. Tsirkin]

On Tue, Jan 21, 2014 at 06:37:51PM +0200, Marcel Apfelbaum wrote:
> Scenario:
>   - There is a non multifunction pci device A on 00:0X.0.
>   - Hot-plug another multifunction pci device B at 00:0X.1.
>   - The operation will fail of course.
>   - Try to hot-plug the B device 2-3 more times, qemu will crash.
> 
> Reason: The error flow leaves the B's address space into global address spaces
> list, but the device object is freed. Fixed that.
> 
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>

Thanks, applied.

> ---
>  hw/pci/pci.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index aa2a395..5f454dd 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -793,6 +793,15 @@ static void pci_config_free(PCIDevice *pci_dev)
>      g_free(pci_dev->used);
>  }
>  
> +static void do_pci_unregister_device(PCIDevice *pci_dev)
> +{
> +    pci_dev->bus->devices[pci_dev->devfn] = NULL;
> +    pci_config_free(pci_dev);
> +
> +    address_space_destroy(&pci_dev->bus_master_as);
> +    memory_region_destroy(&pci_dev->bus_master_enable_region);
> +}
> +
>  /* -1 for devfn means auto assign */
>  static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>                                           const char *name, int devfn)
> @@ -858,7 +867,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>          pci_init_mask_bridge(pci_dev);
>      }
>      if (pci_init_multifunction(bus, pci_dev)) {
> -        pci_config_free(pci_dev);
> +        do_pci_unregister_device(pci_dev);
>          return NULL;
>      }
>  
> @@ -873,15 +882,6 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>      return pci_dev;
>  }
>  
> -static void do_pci_unregister_device(PCIDevice *pci_dev)
> -{
> -    pci_dev->bus->devices[pci_dev->devfn] = NULL;
> -    pci_config_free(pci_dev);
> -
> -    address_space_destroy(&pci_dev->bus_master_as);
> -    memory_region_destroy(&pci_dev->bus_master_enable_region);
> -}
> -
>  static void pci_unregister_io_regions(PCIDevice *pci_dev)
>  {
>      PCIIORegion *r;
> -- 
> 1.8.3.1