> On Jan 25, 2022, at 5:25 AM, Stefan Hajnoczi <stefa...@redhat.com> wrote: > > On Wed, Jan 19, 2022 at 04:41:53PM -0500, Jagannathan Raman wrote: >> Adds pci_isol_bus_new() and pci_isol_bus_free() functions to manage >> creation and destruction of isolated PCI buses. Also adds qdev_get_bus >> and qdev_put_bus callbacks to allow the choice of parent bus. >> >> Signed-off-by: Elena Ufimtseva <elena.ufimts...@oracle.com> >> Signed-off-by: John G Johnson <john.g.john...@oracle.com> >> Signed-off-by: Jagannathan Raman <jag.ra...@oracle.com> >> --- >> include/hw/pci/pci.h | 4 + >> include/hw/qdev-core.h | 16 ++++ >> hw/pci/pci.c | 169 +++++++++++++++++++++++++++++++++++++++++ >> softmmu/qdev-monitor.c | 39 +++++++++- >> 4 files changed, 225 insertions(+), 3 deletions(-) >> >> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h >> index 9bb4472abc..8c18f10d9d 100644 >> --- a/include/hw/pci/pci.h >> +++ b/include/hw/pci/pci.h >> @@ -452,6 +452,10 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, PCIBus >> *rootbus, >> >> PCIDevice *pci_vga_init(PCIBus *bus); >> >> +PCIBus *pci_isol_bus_new(BusState *parent_bus, const char *new_bus_type, >> + Error **errp); >> +bool pci_isol_bus_free(PCIBus *pci_bus, Error **errp); >> + >> static inline PCIBus *pci_get_bus(const PCIDevice *dev) >> { >> return PCI_BUS(qdev_get_parent_bus(DEVICE(dev))); >> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h >> index 92c3d65208..eed2983072 100644 >> --- a/include/hw/qdev-core.h >> +++ b/include/hw/qdev-core.h >> @@ -419,6 +419,20 @@ void qdev_simple_device_unplug_cb(HotplugHandler >> *hotplug_dev, >> void qdev_machine_creation_done(void); >> bool qdev_machine_modified(void); >> >> +/** >> + * Find parent bus - these callbacks are used during device addition >> + * and deletion. >> + * >> + * During addition, if no parent bus is specified in the options, >> + * these callbacks provide a way to figure it out based on the >> + * bus type. If these callbacks are not defined, defaults to >> + * finding the parent bus starting from default system bus >> + */ >> +typedef bool (QDevGetBusFunc)(const char *type, BusState **bus, Error >> **errp); >> +typedef bool (QDevPutBusFunc)(BusState *bus, Error **errp); >> +bool qdev_set_bus_cbs(QDevGetBusFunc *get_bus, QDevPutBusFunc *put_bus, >> + Error **errp); > > Where is this used, it doesn't seem related to pci_isol_bus_new()?
Yes, this is no directly related to pci_isol_bus_new() - will move it to a separate patch. > >> + >> /** >> * GpioPolarity: Polarity of a GPIO line >> * >> @@ -691,6 +705,8 @@ BusState *qdev_get_parent_bus(DeviceState *dev); >> /*** BUS API. ***/ >> >> DeviceState *qdev_find_recursive(BusState *bus, const char *id); >> +BusState *qbus_find_recursive(BusState *bus, const char *name, >> + const char *bus_typename); >> >> /* Returns 0 to walk children, > 0 to skip walk, < 0 to terminate walk. */ >> typedef int (qbus_walkerfn)(BusState *bus, void *opaque); >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c >> index d5f1c6c421..63ec1e47b5 100644 >> --- a/hw/pci/pci.c >> +++ b/hw/pci/pci.c >> @@ -493,6 +493,175 @@ void pci_root_bus_cleanup(PCIBus *bus) >> qbus_unrealize(BUS(bus)); >> } >> >> +static void pci_bus_free_isol_mem(PCIBus *pci_bus) >> +{ >> + if (pci_bus->address_space_mem) { >> + memory_region_unref(pci_bus->address_space_mem); > > memory_region_unref() already does a NULL pointer check so the if > statements in this function aren't needed. Got it, thank you! > >> + pci_bus->address_space_mem = NULL; >> + } >> + >> + if (pci_bus->isol_as_mem) { >> + address_space_destroy(pci_bus->isol_as_mem); >> + pci_bus->isol_as_mem = NULL; >> + } >> + >> + if (pci_bus->address_space_io) { >> + memory_region_unref(pci_bus->address_space_io); >> + pci_bus->address_space_io = NULL; >> + } >> + >> + if (pci_bus->isol_as_io) { >> + address_space_destroy(pci_bus->isol_as_io); >> + pci_bus->isol_as_io = NULL; >> + } >> +} >> + >> +static void pci_bus_init_isol_mem(PCIBus *pci_bus, uint32_t unique_id) >> +{ >> + g_autofree char *mem_mr_name = NULL; >> + g_autofree char *mem_as_name = NULL; >> + g_autofree char *io_mr_name = NULL; >> + g_autofree char *io_as_name = NULL; >> + >> + if (!pci_bus) { >> + return; >> + } >> + >> + mem_mr_name = g_strdup_printf("mem-mr-%u", unique_id); >> + mem_as_name = g_strdup_printf("mem-as-%u", unique_id); >> + io_mr_name = g_strdup_printf("io-mr-%u", unique_id); >> + io_as_name = g_strdup_printf("io-as-%u", unique_id); >> + >> + pci_bus->address_space_mem = g_malloc0(sizeof(MemoryRegion)); >> + pci_bus->isol_as_mem = g_malloc0(sizeof(AddressSpace)); >> + memory_region_init(pci_bus->address_space_mem, NULL, >> + mem_mr_name, UINT64_MAX); >> + address_space_init(pci_bus->isol_as_mem, >> + pci_bus->address_space_mem, mem_as_name); >> + >> + pci_bus->address_space_io = g_malloc0(sizeof(MemoryRegion)); >> + pci_bus->isol_as_io = g_malloc0(sizeof(AddressSpace)); > > Where are address_space_mem, isol_as_mem, address_space_io, and > isol_as_io freed? I think the unref calls won't free them because the > objects were created with object_initialize() instead of object_new(). Ah OK got it, thank you! Will fix it. I think we could set the owner of the the memory regions to the PCI bus, as opposed to NULL. We could also add an ‘instance_finalize’ function to PCI bus which would free them. -- Jag