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

2016-06-08 Thread Marcel Apfelbaum 

On 06/08/2016 02:16 PM, Paolo Bonzini wrote:



- Original Message -

From: "Marcel Apfelbaum" <mar...@redhat.com>
To: qemu-devel@nongnu.org
Cc: mar...@redhat.com, m...@redhat.com, pbonz...@redhat.com, 
ehabk...@redhat.com, pet...@redhat.com,
davidkiar...@gmail.com, "jan kiszka" <jan.kis...@web.de>, "bd aviv" <bd.a...@gmail.com>, 
"alex williamson"
<alex.william...@redhat.com>
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 <mar...@redhat.com>
---
  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(_dev->bus_master_enable_region,
+ OBJECT(pci_dev), "bus master",
+ dma_as->root, 0,
memory_region_size(dma_as->root));
+memory_region_set_enabled(_dev->bus_master_enable_region, false);
+address_space_init(_dev->bus_master_as,
+   _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(>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(_dev->bus_master_enable_region,
- OBJECT(pci_dev), "bus master",
- dma_as->root, 0,
memory_region_size(dma_as->root));
-memory_region_set_enabled(_dev->bus_master_enable_region, false);
-address_space_init(_dev->bus_master_as,
_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

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

2016-06-08 Thread Paolo Bonzini 


- Original Message -
> From: "Marcel Apfelbaum" <mar...@redhat.com>
> To: qemu-devel@nongnu.org
> Cc: mar...@redhat.com, m...@redhat.com, pbonz...@redhat.com, 
> ehabk...@redhat.com, pet...@redhat.com,
> davidkiar...@gmail.com, "jan kiszka" <jan.kis...@web.de>, "bd aviv" 
> <bd.a...@gmail.com>, "alex williamson"
> <alex.william...@redhat.com>
> 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 <mar...@redhat.com>
> ---
>  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(_dev->bus_master_enable_region,
> + OBJECT(pci_dev), "bus master",
> + dma_as->root, 0,
> memory_region_size(dma_as->root));
> +memory_region_set_enabled(_dev->bus_master_enable_region, false);
> +address_space_init(_dev->bus_master_as,
> +   _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(>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(_dev->bus_master_enable_region,
> - OBJECT(pci_dev), "bus master",
> - dma_as->root, 0,
> memory_region_size(dma_as->root));
> -memory_region_set_enabled(_dev->bus_master_enable_region, false);
> -address_space_init(_dev->bus_master_as,
> _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
> 
>

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

2016-06-02 Thread Marcel Apfelbaum 
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(_dev->bus_master_enable_region,
+ OBJECT(pci_dev), "bus master",
+ dma_as->root, 0, 
memory_region_size(dma_as->root));
+memory_region_set_enabled(_dev->bus_master_enable_region, false);
+address_space_init(_dev->bus_master_as,
+   _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(>machine_done);
+}
+
 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(_dev->bus_master_enable_region,
- OBJECT(pci_dev), "bus master",
- dma_as->root, 0, 
memory_region_size(dma_as->root));
-memory_region_set_enabled(_dev->bus_master_enable_region, false);
-address_space_init(_dev->bus_master_as, 
_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