| Message ID | 1390322271-3310-1-git-send-email-marcel.a@redhat.com |
|---|---|
| State | New |
| Headers | show |
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;
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.
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
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;
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(-)