From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34732) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bAbn2-0000of-UW for qemu-devel@nongnu.org; Wed, 08 Jun 2016 07:36:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bAbmz-0001x7-Q3 for qemu-devel@nongnu.org; Wed, 08 Jun 2016 07:36:52 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41141) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bAbmz-0001wp-HI for qemu-devel@nongnu.org; Wed, 08 Jun 2016 07:36:49 -0400 References: <1464898555-14914-1-git-send-email-marcel@redhat.com> <1464898555-14914-2-git-send-email-marcel@redhat.com> <628084411.20790043.1465384610818.JavaMail.zimbra@redhat.com> From: Marcel Apfelbaum Message-ID: <5758034C.8080501@redhat.com> Date: Wed, 8 Jun 2016 14:36:44 +0300 MIME-Version: 1.0 In-Reply-To: <628084411.20790043.1465384610818.JavaMail.zimbra@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 1/3] hw/pci: delay bus_master_enable_region initialization List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org, mst@redhat.com, ehabkost@redhat.com, peterx@redhat.com, davidkiarie4@gmail.com, jan kiszka , bd aviv , alex williamson On 06/08/2016 02:16 PM, Paolo Bonzini wrote: > > > ----- Original Message ----- >> From: "Marcel Apfelbaum" >> 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" , "bd aviv" , "alex williamson" >> >> 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 >> --- >> 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 >> >>