Re: [Qemu-devel] [PATCH v2 1/3] hw/pci: delay bus_master_enable_region initialization

[Paolo Bonzini]

----- Original Message -----
> From: "Marcel Apfelbaum" <address@hidden>
> To: address@hidden
> Cc: address@hidden, address@hidden, address@hidden, address@hidden, 
> address@hidden,
> address@hidden, "jan kiszka" <address@hidden>, "bd aviv" <address@hidden>, 
> "alex williamson"
> <address@hidden>
> 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 <address@hidden>
> ---
>  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
> 
>