| Message ID | 566b2ccf8a7a5ce2ccb21f66c988a0feee83ee8f.1642626515.git.jag.raman@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio-user server in QEMU | expand |
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.ufimtseva@oracle.com> > Signed-off-by: John G Johnson <john.g.johnson@oracle.com> > Signed-off-by: Jagannathan Raman <jag.raman@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()? > + > /** > * 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. > + 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().
> On Jan 25, 2022, at 5:25 AM, Stefan Hajnoczi <stefanha@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.ufimtseva@oracle.com> >> Signed-off-by: John G Johnson <john.g.johnson@oracle.com> >> Signed-off-by: Jagannathan Raman <jag.raman@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
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); + /** * 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); + 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)); + memory_region_init(pci_bus->address_space_io, NULL, + io_mr_name, UINT64_MAX); + address_space_init(pci_bus->isol_as_io, + pci_bus->address_space_io, io_as_name); +} + +PCIBus *pci_isol_bus_new(BusState *parent_bus, const char *new_bus_type, + Error **errp) +{ + ERRP_GUARD(); + PCIBus *parent_pci_bus = NULL; + DeviceState *pcie_root_port = NULL; + g_autofree char *new_bus_name = NULL; + PCIBus *new_pci_bus = NULL; + HotplugHandler *hotplug_handler = NULL; + uint32_t devfn, slot; + + if (!parent_bus) { + error_setg(errp, "parent PCI bus not found"); + return NULL; + } + + if (!new_bus_type || + (strcmp(new_bus_type, TYPE_PCIE_BUS) && + strcmp(new_bus_type, TYPE_PCI_BUS))) { + error_setg(errp, "bus type must be %s or %s", TYPE_PCIE_BUS, + TYPE_PCI_BUS); + return NULL; + } + + if (!object_dynamic_cast(OBJECT(parent_bus), TYPE_PCI_BUS)) { + error_setg(errp, "Unsupported root bus type"); + return NULL; + } + + parent_pci_bus = PCI_BUS(parent_bus); + + /** + * Create TYPE_GEN_PCIE_ROOT_PORT device to interface parent and + * new buses. + */ + for (devfn = 0; devfn < (PCI_SLOT_MAX * PCI_FUNC_MAX); + devfn += PCI_FUNC_MAX) { + if (!parent_pci_bus->devices[devfn]) { + break; + } + } + if (devfn == (PCI_SLOT_MAX * PCI_FUNC_MAX)) { + error_setg(errp, "parent PCI slots full"); + return NULL; + } + + slot = devfn / PCI_FUNC_MAX; + pcie_root_port = qdev_new("pcie-root-port"); + if (!object_property_set_int(OBJECT(pcie_root_port), "slot", + slot, errp)){ + error_prepend(errp, "Failed to set slot property for root port: "); + goto fail_rp_init; + } + if (!qdev_realize(pcie_root_port, parent_bus, errp)) { + goto fail_rp_init; + } + + /** + * Create new PCI bus and plug it to the root port + */ + new_bus_name = g_strdup_printf("pci-bus-%d", (slot + 1)); + new_pci_bus = PCI_BUS(qbus_new(new_bus_type, pcie_root_port, new_bus_name)); + new_pci_bus->parent_dev = PCI_DEVICE(pcie_root_port); + hotplug_handler = qdev_get_bus_hotplug_handler(pcie_root_port); + qbus_set_hotplug_handler(BUS(new_pci_bus), OBJECT(hotplug_handler)); + pci_default_write_config(new_pci_bus->parent_dev, PCI_SECONDARY_BUS, + (slot + 1), 1); + pci_default_write_config(new_pci_bus->parent_dev, PCI_SUBORDINATE_BUS, + (slot + 1), 1); + pci_bus_init_isol_mem(new_pci_bus, (slot + 1)); + + QLIST_INIT(&new_pci_bus->child); + QLIST_INSERT_HEAD(&parent_pci_bus->child, new_pci_bus, sibling); + + if (!qbus_realize(BUS(new_pci_bus), errp)) { + QLIST_REMOVE(new_pci_bus, sibling); + pci_bus_free_isol_mem(new_pci_bus); + object_unparent(OBJECT(new_pci_bus)); + new_pci_bus = NULL; + goto fail_rp_init; + } + + return new_pci_bus; + +fail_rp_init: + qdev_unrealize(pcie_root_port); + object_unparent(OBJECT(pcie_root_port)); + pcie_root_port = NULL; + return NULL; +} + +bool pci_isol_bus_free(PCIBus *pci_bus, Error **errp) +{ + ERRP_GUARD(); + PCIDevice *pcie_root_port = pci_bus->parent_dev; + + if (!pcie_root_port) { + error_setg(errp, "Can't unplug root bus"); + return false; + } + + if (!QLIST_EMPTY(&pci_bus->child)) { + error_setg(errp, "Bus has attached device"); + return false; + } + + QLIST_REMOVE(pci_bus, sibling); + pci_bus_free_isol_mem(pci_bus); + qbus_unrealize(BUS(pci_bus)); + object_unparent(OBJECT(pci_bus)); + qdev_unrealize(DEVICE(pcie_root_port)); + object_unparent(OBJECT(pcie_root_port)); + return true; +} + void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq, void *irq_opaque, int nirq) { diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c index 01f3834db5..7306074019 100644 --- a/softmmu/qdev-monitor.c +++ b/softmmu/qdev-monitor.c @@ -64,6 +64,9 @@ typedef struct QDevAlias #define QEMU_ARCH_VIRTIO_CCW (QEMU_ARCH_S390X) #define QEMU_ARCH_VIRTIO_MMIO (QEMU_ARCH_M68K) +static QDevGetBusFunc *qdev_get_bus; +static QDevPutBusFunc *qdev_put_bus; + /* Please keep this table sorted by typename. */ static const QDevAlias qdev_alias_table[] = { { "AC97", "ac97" }, /* -soundhw name */ @@ -450,7 +453,7 @@ static inline bool qbus_is_full(BusState *bus) * If more than one exists, prefer one that can take another device. * Return the bus if found, else %NULL. */ -static BusState *qbus_find_recursive(BusState *bus, const char *name, +BusState *qbus_find_recursive(BusState *bus, const char *name, const char *bus_typename) { BusChild *kid; @@ -608,6 +611,20 @@ const char *qdev_set_id(DeviceState *dev, char *id, Error **errp) return prop->name; } + +bool qdev_set_bus_cbs(QDevGetBusFunc *get_bus, QDevPutBusFunc *put_bus, + Error **errp) +{ + if (qdev_get_bus || qdev_put_bus) { + error_setg(errp, "callbacks already set"); + return false; + } + + qdev_get_bus = get_bus; + qdev_put_bus = put_bus; + return true; +} + DeviceState *qdev_device_add_from_qdict(const QDict *opts, bool from_json, Error **errp) { @@ -642,7 +659,13 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts, driver, object_get_typename(OBJECT(bus))); return NULL; } - } else if (dc->bus_type != NULL) { + } else if (dc->bus_type != NULL && qdev_get_bus != NULL) { + if (!qdev_get_bus(dc->bus_type, &bus, errp)) { + return NULL; + } + } + + if (!bus && dc->bus_type != NULL) { bus = qbus_find_recursive(sysbus_get_default(), NULL, dc->bus_type); if (!bus || qbus_is_full(bus)) { error_setg(errp, "No '%s' bus found for device '%s'", @@ -891,10 +914,12 @@ static DeviceState *find_device_state(const char *id, Error **errp) void qdev_unplug(DeviceState *dev, Error **errp) { + ERRP_GUARD(); DeviceClass *dc = DEVICE_GET_CLASS(dev); HotplugHandler *hotplug_ctrl; HotplugHandlerClass *hdc; Error *local_err = NULL; + BusState *parent_bus = qdev_get_parent_bus(dev); if (dev->parent_bus && !qbus_is_hotpluggable(dev->parent_bus)) { error_setg(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name); @@ -930,7 +955,15 @@ void qdev_unplug(DeviceState *dev, Error **errp) object_unparent(OBJECT(dev)); } } - error_propagate(errp, local_err); + + if (local_err) { + error_propagate(errp, local_err); + return; + } + + if (qdev_put_bus) { + qdev_put_bus(parent_bus, errp); + } } void qmp_device_del(const char *id, Error **errp)