Re: [PATCH v5 04/18] pci: create and free isolated PCI buses

From: Stefan Hajnoczi <stefanha@redhat.com>
To: Jagannathan Raman <jag.raman@oracle.com>
Cc: eduardo@habkost.net, elena.ufimtseva@oracle.com,
	john.g.johnson@oracle.com, berrange@redhat.com, bleal@redhat.com,
	john.levon@nutanix.com, mst@redhat.com, armbru@redhat.com,
	quintela@redhat.com, qemu-devel@nongnu.org, f4bug@amsat.org,
	marcandre.lureau@gmail.com, thanos.makatos@nutanix.com,
	pbonzini@redhat.com, eblake@redhat.com, dgilbert@redhat.com
Subject: Re: [PATCH v5 04/18] pci: create and free isolated PCI buses
Date: Tue, 25 Jan 2022 10:25:01 +0000	[thread overview]
Message-ID: <Ye/P/dUQ1Ngz24kI@stefanha-x1.localdomain> (raw)
In-Reply-To: <566b2ccf8a7a5ce2ccb21f66c988a0feee83ee8f.1642626515.git.jag.raman@oracle.com>

[-- Attachment #1: Type: text/plain, Size: 5223 bytes --]

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().

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]