| Message ID | 1464898555-14914-2-git-send-email-marcel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
----- Original Message ----- > From: "Marcel Apfelbaum" <marcel@redhat.com> > To: qemu-devel@nongnu.org > Cc: marcel@redhat.com, mst@redhat.com, pbonzini@redhat.com, ehabkost@redhat.com, peterx@redhat.com, > davidkiarie4@gmail.com, "jan kiszka" <jan.kiszka@web.de>, "bd aviv" <bd.aviv@gmail.com>, "alex williamson" > <alex.williamson@redhat.com> > Sent: Thursday, June 2, 2016 10:15:53 PM > Subject: [Qemu-devel] [PATCH v2 1/3] hw/pci: delay bus_master_enable_region initialization > > Skip bus_master_enable region creation on PCI device init > in order to be sure the IOMMU device (if present) would > be created in advance. Add this memory region at machine_done time. > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > --- > hw/pci/pci.c | 46 ++++++++++++++++++++++++++++++++++++---------- > include/hw/pci/pci_bus.h | 2 ++ > 2 files changed, 38 insertions(+), 10 deletions(-) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index bb605ef..29dcbcf 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -127,11 +127,44 @@ static void pci_bus_class_init(ObjectClass *klass, void > *data) > pbc->numa_node = pcibus_numa_node; > } > > +static void pci_init_bus_master(PCIDevice *pci_dev) > +{ > + AddressSpace *dma_as = pci_device_iommu_address_space(pci_dev); > + > + memory_region_init_alias(&pci_dev->bus_master_enable_region, > + OBJECT(pci_dev), "bus master", > + dma_as->root, 0, > memory_region_size(dma_as->root)); > + memory_region_set_enabled(&pci_dev->bus_master_enable_region, false); > + address_space_init(&pci_dev->bus_master_as, > + &pci_dev->bus_master_enable_region, pci_dev->name); > +} > + > +static void pcibus_machine_done(Notifier *notifier, void *data) > +{ > + PCIBus *bus = container_of(notifier, PCIBus, machine_done); > + int i; > + > + for (i = 0; i < ARRAY_SIZE(bus->devices); ++i) { > + if (bus->devices[i]) { > + pci_init_bus_master(bus->devices[i]); > + } > + } > +} > + > +static void pcibus_initfn(Object *obj) > +{ > + PCIBus *bus = PCI_BUS(obj); > + > + bus->machine_done.notify = pcibus_machine_done; > + qemu_add_machine_init_done_notifier(&bus->machine_done); > +} This should be done at realize time, otherwise object_unref(object_new(TYPE_PCI_BUS)); leaves a dangling reference. On one hand I think it's fair to assume that there's no unrealize between realize and machine done; if something fails to realize QEMU will exit. On the other hand it might break in weird ways in the future. So if you could add a qemu_remove_machine_init_done_notifier and call it from bus unrealize, it would be best. Thanks, Paolo > static const TypeInfo pci_bus_info = { > .name = TYPE_PCI_BUS, > .parent = TYPE_BUS, > .instance_size = sizeof(PCIBus), > .class_size = sizeof(PCIBusClass), > + .instance_init = pcibus_initfn, > .class_init = pci_bus_class_init, > }; > > @@ -845,7 +878,6 @@ static PCIDevice *do_pci_register_device(PCIDevice > *pci_dev, PCIBus *bus, > PCIConfigReadFunc *config_read = pc->config_read; > PCIConfigWriteFunc *config_write = pc->config_write; > Error *local_err = NULL; > - AddressSpace *dma_as; > DeviceState *dev = DEVICE(pci_dev); > > pci_dev->bus = bus; > @@ -885,15 +917,9 @@ static PCIDevice *do_pci_register_device(PCIDevice > *pci_dev, PCIBus *bus, > } > > pci_dev->devfn = devfn; > - dma_as = pci_device_iommu_address_space(pci_dev); > - > - memory_region_init_alias(&pci_dev->bus_master_enable_region, > - OBJECT(pci_dev), "bus master", > - dma_as->root, 0, > memory_region_size(dma_as->root)); > - memory_region_set_enabled(&pci_dev->bus_master_enable_region, false); > - address_space_init(&pci_dev->bus_master_as, > &pci_dev->bus_master_enable_region, > - name); > - > + if (qdev_hotplug) { > + pci_init_bus_master(pci_dev); > + } > pstrcpy(pci_dev->name, sizeof(pci_dev->name), name); > pci_dev->irq_state = 0; > pci_config_alloc(pci_dev); > diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h > index 403fec6..5484a9b 100644 > --- a/include/hw/pci/pci_bus.h > +++ b/include/hw/pci/pci_bus.h > @@ -39,6 +39,8 @@ struct PCIBus { > Keep a count of the number of devices with raised IRQs. */ > int nirq; > int *irq_count; > + > + Notifier machine_done; > }; > > typedef struct PCIBridgeWindows PCIBridgeWindows; > -- > 2.4.3 > >
On 06/08/2016 02:16 PM, Paolo Bonzini wrote: > > > ----- Original Message ----- >> From: "Marcel Apfelbaum" <marcel@redhat.com> >> To: qemu-devel@nongnu.org >> Cc: marcel@redhat.com, mst@redhat.com, pbonzini@redhat.com, ehabkost@redhat.com, peterx@redhat.com, >> davidkiarie4@gmail.com, "jan kiszka" <jan.kiszka@web.de>, "bd aviv" <bd.aviv@gmail.com>, "alex williamson" >> <alex.williamson@redhat.com> >> Sent: Thursday, June 2, 2016 10:15:53 PM >> Subject: [Qemu-devel] [PATCH v2 1/3] hw/pci: delay bus_master_enable_region initialization >> >> Skip bus_master_enable region creation on PCI device init >> in order to be sure the IOMMU device (if present) would >> be created in advance. Add this memory region at machine_done time. >> >> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> >> --- >> hw/pci/pci.c | 46 ++++++++++++++++++++++++++++++++++++---------- >> include/hw/pci/pci_bus.h | 2 ++ >> 2 files changed, 38 insertions(+), 10 deletions(-) >> >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c >> index bb605ef..29dcbcf 100644 >> --- a/hw/pci/pci.c >> +++ b/hw/pci/pci.c >> @@ -127,11 +127,44 @@ static void pci_bus_class_init(ObjectClass *klass, void >> *data) >> pbc->numa_node = pcibus_numa_node; >> } >> >> +static void pci_init_bus_master(PCIDevice *pci_dev) >> +{ >> + AddressSpace *dma_as = pci_device_iommu_address_space(pci_dev); >> + >> + memory_region_init_alias(&pci_dev->bus_master_enable_region, >> + OBJECT(pci_dev), "bus master", >> + dma_as->root, 0, >> memory_region_size(dma_as->root)); >> + memory_region_set_enabled(&pci_dev->bus_master_enable_region, false); >> + address_space_init(&pci_dev->bus_master_as, >> + &pci_dev->bus_master_enable_region, pci_dev->name); >> +} >> + >> +static void pcibus_machine_done(Notifier *notifier, void *data) >> +{ >> + PCIBus *bus = container_of(notifier, PCIBus, machine_done); >> + int i; >> + >> + for (i = 0; i < ARRAY_SIZE(bus->devices); ++i) { >> + if (bus->devices[i]) { >> + pci_init_bus_master(bus->devices[i]); >> + } >> + } >> +} >> + >> +static void pcibus_initfn(Object *obj) >> +{ >> + PCIBus *bus = PCI_BUS(obj); >> + >> + bus->machine_done.notify = pcibus_machine_done; >> + qemu_add_machine_init_done_notifier(&bus->machine_done); >> +} > > This should be done at realize time, otherwise > > object_unref(object_new(TYPE_PCI_BUS)); > > leaves a dangling reference. > > On one hand I think it's fair to assume that there's no unrealize > between realize and machine done; if something fails to realize > QEMU will exit. > > On the other hand it might break in weird ways in the future. > So if you could add a qemu_remove_machine_init_done_notifier and > call it from bus unrealize, it would be best. > I can do that, sure. I'll also move the pcibus_initfn code in the realize function. Thanks, Marcel > Thanks, > > Paolo > >> static const TypeInfo pci_bus_info = { >> .name = TYPE_PCI_BUS, >> .parent = TYPE_BUS, >> .instance_size = sizeof(PCIBus), >> .class_size = sizeof(PCIBusClass), >> + .instance_init = pcibus_initfn, >> .class_init = pci_bus_class_init, >> }; >> >> @@ -845,7 +878,6 @@ static PCIDevice *do_pci_register_device(PCIDevice >> *pci_dev, PCIBus *bus, >> PCIConfigReadFunc *config_read = pc->config_read; >> PCIConfigWriteFunc *config_write = pc->config_write; >> Error *local_err = NULL; >> - AddressSpace *dma_as; >> DeviceState *dev = DEVICE(pci_dev); >> >> pci_dev->bus = bus; >> @@ -885,15 +917,9 @@ static PCIDevice *do_pci_register_device(PCIDevice >> *pci_dev, PCIBus *bus, >> } >> >> pci_dev->devfn = devfn; >> - dma_as = pci_device_iommu_address_space(pci_dev); >> - >> - memory_region_init_alias(&pci_dev->bus_master_enable_region, >> - OBJECT(pci_dev), "bus master", >> - dma_as->root, 0, >> memory_region_size(dma_as->root)); >> - memory_region_set_enabled(&pci_dev->bus_master_enable_region, false); >> - address_space_init(&pci_dev->bus_master_as, >> &pci_dev->bus_master_enable_region, >> - name); >> - >> + if (qdev_hotplug) { >> + pci_init_bus_master(pci_dev); >> + } >> pstrcpy(pci_dev->name, sizeof(pci_dev->name), name); >> pci_dev->irq_state = 0; >> pci_config_alloc(pci_dev); >> diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h >> index 403fec6..5484a9b 100644 >> --- a/include/hw/pci/pci_bus.h >> +++ b/include/hw/pci/pci_bus.h >> @@ -39,6 +39,8 @@ struct PCIBus { >> Keep a count of the number of devices with raised IRQs. */ >> int nirq; >> int *irq_count; >> + >> + Notifier machine_done; >> }; >> >> typedef struct PCIBridgeWindows PCIBridgeWindows; >> -- >> 2.4.3 >> >>
diff --git a/hw/pci/pci.c b/hw/pci/pci.c index bb605ef..29dcbcf 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -127,11 +127,44 @@ static void pci_bus_class_init(ObjectClass *klass, void *data) pbc->numa_node = pcibus_numa_node; } +static void pci_init_bus_master(PCIDevice *pci_dev) +{ + AddressSpace *dma_as = pci_device_iommu_address_space(pci_dev); + + memory_region_init_alias(&pci_dev->bus_master_enable_region, + OBJECT(pci_dev), "bus master", + dma_as->root, 0, memory_region_size(dma_as->root)); + memory_region_set_enabled(&pci_dev->bus_master_enable_region, false); + address_space_init(&pci_dev->bus_master_as, + &pci_dev->bus_master_enable_region, pci_dev->name); +} + +static void pcibus_machine_done(Notifier *notifier, void *data) +{ + PCIBus *bus = container_of(notifier, PCIBus, machine_done); + int i; + + for (i = 0; i < ARRAY_SIZE(bus->devices); ++i) { + if (bus->devices[i]) { + pci_init_bus_master(bus->devices[i]); + } + } +} + +static void pcibus_initfn(Object *obj) +{ + PCIBus *bus = PCI_BUS(obj); + + bus->machine_done.notify = pcibus_machine_done; + qemu_add_machine_init_done_notifier(&bus->machine_done); +} + static const TypeInfo pci_bus_info = { .name = TYPE_PCI_BUS, .parent = TYPE_BUS, .instance_size = sizeof(PCIBus), .class_size = sizeof(PCIBusClass), + .instance_init = pcibus_initfn, .class_init = pci_bus_class_init, }; @@ -845,7 +878,6 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, PCIConfigReadFunc *config_read = pc->config_read; PCIConfigWriteFunc *config_write = pc->config_write; Error *local_err = NULL; - AddressSpace *dma_as; DeviceState *dev = DEVICE(pci_dev); pci_dev->bus = bus; @@ -885,15 +917,9 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, } pci_dev->devfn = devfn; - dma_as = pci_device_iommu_address_space(pci_dev); - - memory_region_init_alias(&pci_dev->bus_master_enable_region, - OBJECT(pci_dev), "bus master", - dma_as->root, 0, memory_region_size(dma_as->root)); - memory_region_set_enabled(&pci_dev->bus_master_enable_region, false); - address_space_init(&pci_dev->bus_master_as, &pci_dev->bus_master_enable_region, - name); - + if (qdev_hotplug) { + pci_init_bus_master(pci_dev); + } pstrcpy(pci_dev->name, sizeof(pci_dev->name), name); pci_dev->irq_state = 0; pci_config_alloc(pci_dev); diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h index 403fec6..5484a9b 100644 --- a/include/hw/pci/pci_bus.h +++ b/include/hw/pci/pci_bus.h @@ -39,6 +39,8 @@ struct PCIBus { Keep a count of the number of devices with raised IRQs. */ int nirq; int *irq_count; + + Notifier machine_done; }; typedef struct PCIBridgeWindows PCIBridgeWindows;
Skip bus_master_enable region creation on PCI device init in order to be sure the IOMMU device (if present) would be created in advance. Add this memory region at machine_done time. Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> --- hw/pci/pci.c | 46 ++++++++++++++++++++++++++++++++++++---------- include/hw/pci/pci_bus.h | 2 ++ 2 files changed, 38 insertions(+), 10 deletions(-)