[Qemu-devel] [PATCH v2 0/3] enable iommu with -device

[Qemu-devel] [PATCH v2 0/3] enable iommu with -device

[Marcel Apfelbaum]

Create the iommu device with '-device intel-iommu' instead of '-machine,iommu=on'.

The device is part of the machine properties because we wanted
to ensure it is created before any other PCI device.

The alternative is to skip the bus_master_enable_region at
the time the device is created. We can create this region
at machine_done phase. (patch 1)

Patch 2 moves the init proces into iommu's realize function.

Then we need to enable sysbus devices for PC machines (patch 3).

This creates a new problem since we have now a bunch of
new devices that can be created with -device on Q35:
	name "q35-pcihost", bus System
	name "sysbus-ohci", bus System, desc "OHCI USB Controller"
	name "allwinner-ahci", bus System
	name "cfi.pflash01", bus System
	name "esp", bus System
	name "SUNW,fdtwo", bus System
	name "sysbus-ahci", bus System
	name "sysbus-fdc", bus System
	name "vfio-amd-xgbe", bus System, desc "VFIO AMD XGBE"
	name "vfio-calxeda-xgmac", bus System, desc "VFIO Calxeda XGMAC"
	name "virtio-mmio", bus System
	name "fw_cfg", bus System
	name "fw_cfg_io", bus System
	name "fw_cfg_mem", bus System
	name "generic-sdhci", bus System
	name "hpet", bus System
	name "i440FX-pcihost", bus System
	name "intel-iommu", bus System
	name "ioapic", bus System
	name "isabus-bridge", bus System
	name "kvm-ioapic", bus System
	name "kvmclock", bus System
	name "kvmvapic", bus System
	name "pxb-host", bus System

Took care of the ones creating immediate issues (like crashes) by marking them
as 'cannot_instantiate_with_device_add_yet'. I didn't mark them all because:
  - libvirt will mask them anyway
  - some of them have already a "protection" in place
  - it is possible that some of them can be actually used with -device on other platform. 
  - those are not 'interesting' scenarios.
If somebody spots devices in the list that cannot be added with -device on any platform
please let me know and I'll mark them.


v1 -> v2:
  - Enable bus_master also on init if the guest OS already booted to enable hotplug (Paolo).
  - Add a machine_done notifier to PCIBus instead of adding functionality
    for q35 machine_done callback. The main reason is we don't want to replicate
    the code for all platforms that support PCI and is also cleaner this way.
  - Added 'cannot_instantiate_with_device_add_yet' to sysbus devices that lead
    to crashes if added with -device.
  - Rebased on master

Thanks,
Marcel

Marcel Apfelbaum (2):
  hw/pci: delay bus_master_enable_region initialization
  hw/iommu: enable iommu with -device
  q35: allow dynamic sysbus

 hw/core/machine.c                   | 20 ----------------
 hw/i386/intel_iommu.c               | 17 ++++++++++++++
 hw/i386/pc_q35.c                    |  1 +
 hw/pci-bridge/pci_expander_bridge.c |  1 +
 hw/pci-host/piix.c                  |  1 +
 hw/pci-host/q35.c                   | 29 +----------------------
 hw/pci/pci.c                        | 46 +++++++++++++++++++++++++++++--------
 include/hw/pci/pci_bus.h            |  2 ++
 qemu-options.hx                     |  3 ---
 9 files changed, 59 insertions(+), 61 deletions(-)

-- 
2.4.3

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

[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 <marcel@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(&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);
+}
+
 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

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

[Paolo Bonzini]

----- Original Message -----
> From: "Marcel Apfelbaum" <marcel@redhat.com>
> To: qemu-devel@nongnu.org
> Cc: marcel@redhat.com, mst@redhat.com, pbonzini@redhat.com, ehabkost@redhat.com, peterx@redhat.com,
> davidkiarie4@gmail.com, "jan kiszka" <jan.kiszka@web.de>, "bd aviv" <bd.aviv@gmail.com>, "alex williamson"
> <alex.williamson@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 <marcel@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(&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
> 
> 

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

[Marcel Apfelbaum]

On 06/08/2016 02:16 PM, Paolo Bonzini wrote:
>
>
> ----- Original Message -----
>> From: "Marcel Apfelbaum" <marcel@redhat.com>
>> To: qemu-devel@nongnu.org
>> Cc: marcel@redhat.com, mst@redhat.com, pbonzini@redhat.com, ehabkost@redhat.com, peterx@redhat.com,
>> davidkiarie4@gmail.com, "jan kiszka" <jan.kiszka@web.de>, "bd aviv" <bd.aviv@gmail.com>, "alex williamson"
>> <alex.williamson@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 <marcel@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(&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.
>

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(&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
>>
>>

[Qemu-devel] [PATCH v2 2/3] hw/iommu: enable iommu with -device

[Marcel Apfelbaum]

Use the standard '-device iommu' instead of '-machine,iommu=on'
to create the IOMMU device.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/core/machine.c     | 20 --------------------
 hw/i386/intel_iommu.c | 17 +++++++++++++++++
 hw/pci-host/q35.c     | 28 ----------------------------
 qemu-options.hx       |  3 ---
 4 files changed, 17 insertions(+), 51 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index ccdd5fa..8f94301 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -300,20 +300,6 @@ static void machine_set_firmware(Object *obj, const char *value, Error **errp)
     ms->firmware = g_strdup(value);
 }
 
-static bool machine_get_iommu(Object *obj, Error **errp)
-{
-    MachineState *ms = MACHINE(obj);
-
-    return ms->iommu;
-}
-
-static void machine_set_iommu(Object *obj, bool value, Error **errp)
-{
-    MachineState *ms = MACHINE(obj);
-
-    ms->iommu = value;
-}
-
 static void machine_set_suppress_vmdesc(Object *obj, bool value, Error **errp)
 {
     MachineState *ms = MACHINE(obj);
@@ -493,12 +479,6 @@ static void machine_initfn(Object *obj)
     object_property_set_description(obj, "firmware",
                                     "Firmware image",
                                     NULL);
-    object_property_add_bool(obj, "iommu",
-                             machine_get_iommu,
-                             machine_set_iommu, NULL);
-    object_property_set_description(obj, "iommu",
-                                    "Set on/off to enable/disable Intel IOMMU (VT-d)",
-                                    NULL);
     object_property_add_bool(obj, "suppress-vmdesc",
                              machine_get_suppress_vmdesc,
                              machine_set_suppress_vmdesc, NULL);
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 347718f..9af5d6b 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -24,6 +24,8 @@
 #include "exec/address-spaces.h"
 #include "intel_iommu_internal.h"
 #include "hw/pci/pci.h"
+#include "hw/pci/pci_bus.h"
+#include "hw/i386/pc.h"
 
 /*#define DEBUG_INTEL_IOMMU*/
 #ifdef DEBUG_INTEL_IOMMU
@@ -2014,8 +2016,20 @@ static void vtd_reset(DeviceState *dev)
     vtd_init(s);
 }
 
+static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
+{
+    IntelIOMMUState *s = opaque;
+    VTDAddressSpace *vtd_as;
+
+    assert(0 <= devfn && devfn <= VTD_PCI_DEVFN_MAX);
+
+    vtd_as = vtd_find_add_as(s, bus, devfn);
+    return &vtd_as->as;
+}
+
 static void vtd_realize(DeviceState *dev, Error **errp)
 {
+    PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus;
     IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev);
 
     VTD_DPRINTF(GENERAL, "");
@@ -2029,6 +2043,9 @@ static void vtd_realize(DeviceState *dev, Error **errp)
     s->vtd_as_by_busptr = g_hash_table_new_full(vtd_uint64_hash, vtd_uint64_equal,
                                               g_free, g_free);
     vtd_init(s);
+    sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, Q35_HOST_BRIDGE_IOMMU_ADDR);
+    bus->iommu_fn = vtd_host_dma_iommu;
+    bus->iommu_opaque = dev;
 }
 
 static void vtd_class_init(ObjectClass *klass, void *data)
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 70f897e..ea684c7 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -424,30 +424,6 @@ static void mch_reset(DeviceState *qdev)
     mch_update(mch);
 }
 
-static AddressSpace *q35_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
-{
-    IntelIOMMUState *s = opaque;
-    VTDAddressSpace *vtd_as;
-
-    assert(0 <= devfn && devfn <= VTD_PCI_DEVFN_MAX);
-
-    vtd_as = vtd_find_add_as(s, bus, devfn);
-    return &vtd_as->as;
-}
-
-static void mch_init_dmar(MCHPCIState *mch)
-{
-    PCIBus *pci_bus = PCI_BUS(qdev_get_parent_bus(DEVICE(mch)));
-
-    mch->iommu = INTEL_IOMMU_DEVICE(qdev_create(NULL, TYPE_INTEL_IOMMU_DEVICE));
-    object_property_add_child(OBJECT(mch), "intel-iommu",
-                              OBJECT(mch->iommu), NULL);
-    qdev_init_nofail(DEVICE(mch->iommu));
-    sysbus_mmio_map(SYS_BUS_DEVICE(mch->iommu), 0, Q35_HOST_BRIDGE_IOMMU_ADDR);
-
-    pci_setup_iommu(pci_bus, q35_host_dma_iommu, mch->iommu);
-}
-
 static void mch_realize(PCIDevice *d, Error **errp)
 {
     int i;
@@ -506,10 +482,6 @@ static void mch_realize(PCIDevice *d, Error **errp)
                  mch->pci_address_space, &mch->pam_regions[i+1],
                  PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
     }
-    /* Intel IOMMU (VT-d) */
-    if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) {
-        mch_init_dmar(mch);
-    }
 }
 
 uint64_t mch_mcfg_base(void)
diff --git a/qemu-options.hx b/qemu-options.hx
index 6106520..2953baf 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -38,7 +38,6 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
     "                kvm_shadow_mem=size of KVM shadow MMU\n"
     "                dump-guest-core=on|off include guest memory in a core dump (default=on)\n"
     "                mem-merge=on|off controls memory merge support (default: on)\n"
-    "                iommu=on|off controls emulated Intel IOMMU (VT-d) support (default=off)\n"
     "                igd-passthru=on|off controls IGD GFX passthrough support (default=off)\n"
     "                aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n"
     "                dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n"
@@ -73,8 +72,6 @@ Include guest memory in a core dump. The default is on.
 Enables or disables memory merge support. This feature, when supported by
 the host, de-duplicates identical memory pages among VMs instances
 (enabled by default).
-@item iommu=on|off
-Enables or disables emulated Intel IOMMU (VT-d) support. The default is off.
 @item aes-key-wrap=on|off
 Enables or disables AES key wrapping support on s390-ccw hosts. This feature
 controls whether AES wrapping keys will be created to allow
-- 
2.4.3

Re: [Qemu-devel] [PATCH v2 2/3] hw/iommu: enable iommu with -device

[Michael S. Tsirkin]

On Thu, Jun 02, 2016 at 11:15:54PM +0300, Marcel Apfelbaum wrote:
> Use the standard '-device iommu' instead of '-machine,iommu=on'
> to create the IOMMU device.
> 
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>

Why can't we keep support for the old flag?

> ---
>  hw/core/machine.c     | 20 --------------------
>  hw/i386/intel_iommu.c | 17 +++++++++++++++++
>  hw/pci-host/q35.c     | 28 ----------------------------
>  qemu-options.hx       |  3 ---
>  4 files changed, 17 insertions(+), 51 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index ccdd5fa..8f94301 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -300,20 +300,6 @@ static void machine_set_firmware(Object *obj, const char *value, Error **errp)
>      ms->firmware = g_strdup(value);
>  }
>  
> -static bool machine_get_iommu(Object *obj, Error **errp)
> -{
> -    MachineState *ms = MACHINE(obj);
> -
> -    return ms->iommu;
> -}
> -
> -static void machine_set_iommu(Object *obj, bool value, Error **errp)
> -{
> -    MachineState *ms = MACHINE(obj);
> -
> -    ms->iommu = value;
> -}
> -
>  static void machine_set_suppress_vmdesc(Object *obj, bool value, Error **errp)
>  {
>      MachineState *ms = MACHINE(obj);
> @@ -493,12 +479,6 @@ static void machine_initfn(Object *obj)
>      object_property_set_description(obj, "firmware",
>                                      "Firmware image",
>                                      NULL);
> -    object_property_add_bool(obj, "iommu",
> -                             machine_get_iommu,
> -                             machine_set_iommu, NULL);
> -    object_property_set_description(obj, "iommu",
> -                                    "Set on/off to enable/disable Intel IOMMU (VT-d)",
> -                                    NULL);
>      object_property_add_bool(obj, "suppress-vmdesc",
>                               machine_get_suppress_vmdesc,
>                               machine_set_suppress_vmdesc, NULL);
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 347718f..9af5d6b 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -24,6 +24,8 @@
>  #include "exec/address-spaces.h"
>  #include "intel_iommu_internal.h"
>  #include "hw/pci/pci.h"
> +#include "hw/pci/pci_bus.h"
> +#include "hw/i386/pc.h"
>  
>  /*#define DEBUG_INTEL_IOMMU*/
>  #ifdef DEBUG_INTEL_IOMMU
> @@ -2014,8 +2016,20 @@ static void vtd_reset(DeviceState *dev)
>      vtd_init(s);
>  }
>  
> +static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
> +{
> +    IntelIOMMUState *s = opaque;
> +    VTDAddressSpace *vtd_as;
> +
> +    assert(0 <= devfn && devfn <= VTD_PCI_DEVFN_MAX);
> +
> +    vtd_as = vtd_find_add_as(s, bus, devfn);
> +    return &vtd_as->as;
> +}
> +
>  static void vtd_realize(DeviceState *dev, Error **errp)
>  {
> +    PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus;
>      IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev);
>  
>      VTD_DPRINTF(GENERAL, "");
> @@ -2029,6 +2043,9 @@ static void vtd_realize(DeviceState *dev, Error **errp)
>      s->vtd_as_by_busptr = g_hash_table_new_full(vtd_uint64_hash, vtd_uint64_equal,
>                                                g_free, g_free);
>      vtd_init(s);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, Q35_HOST_BRIDGE_IOMMU_ADDR);
> +    bus->iommu_fn = vtd_host_dma_iommu;
> +    bus->iommu_opaque = dev;
>  }
>  
>  static void vtd_class_init(ObjectClass *klass, void *data)
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 70f897e..ea684c7 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -424,30 +424,6 @@ static void mch_reset(DeviceState *qdev)
>      mch_update(mch);
>  }
>  
> -static AddressSpace *q35_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
> -{
> -    IntelIOMMUState *s = opaque;
> -    VTDAddressSpace *vtd_as;
> -
> -    assert(0 <= devfn && devfn <= VTD_PCI_DEVFN_MAX);
> -
> -    vtd_as = vtd_find_add_as(s, bus, devfn);
> -    return &vtd_as->as;
> -}
> -
> -static void mch_init_dmar(MCHPCIState *mch)
> -{
> -    PCIBus *pci_bus = PCI_BUS(qdev_get_parent_bus(DEVICE(mch)));
> -
> -    mch->iommu = INTEL_IOMMU_DEVICE(qdev_create(NULL, TYPE_INTEL_IOMMU_DEVICE));
> -    object_property_add_child(OBJECT(mch), "intel-iommu",
> -                              OBJECT(mch->iommu), NULL);
> -    qdev_init_nofail(DEVICE(mch->iommu));
> -    sysbus_mmio_map(SYS_BUS_DEVICE(mch->iommu), 0, Q35_HOST_BRIDGE_IOMMU_ADDR);
> -
> -    pci_setup_iommu(pci_bus, q35_host_dma_iommu, mch->iommu);
> -}
> -
>  static void mch_realize(PCIDevice *d, Error **errp)
>  {
>      int i;
> @@ -506,10 +482,6 @@ static void mch_realize(PCIDevice *d, Error **errp)
>                   mch->pci_address_space, &mch->pam_regions[i+1],
>                   PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
>      }
> -    /* Intel IOMMU (VT-d) */
> -    if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) {
> -        mch_init_dmar(mch);
> -    }
>  }
>  
>  uint64_t mch_mcfg_base(void)
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 6106520..2953baf 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -38,7 +38,6 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
>      "                kvm_shadow_mem=size of KVM shadow MMU\n"
>      "                dump-guest-core=on|off include guest memory in a core dump (default=on)\n"
>      "                mem-merge=on|off controls memory merge support (default: on)\n"
> -    "                iommu=on|off controls emulated Intel IOMMU (VT-d) support (default=off)\n"
>      "                igd-passthru=on|off controls IGD GFX passthrough support (default=off)\n"
>      "                aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n"
>      "                dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n"
> @@ -73,8 +72,6 @@ Include guest memory in a core dump. The default is on.
>  Enables or disables memory merge support. This feature, when supported by
>  the host, de-duplicates identical memory pages among VMs instances
>  (enabled by default).
> -@item iommu=on|off
> -Enables or disables emulated Intel IOMMU (VT-d) support. The default is off.
>  @item aes-key-wrap=on|off
>  Enables or disables AES key wrapping support on s390-ccw hosts. This feature
>  controls whether AES wrapping keys will be created to allow
> -- 
> 2.4.3

Re: [Qemu-devel] [PATCH v2 2/3] hw/iommu: enable iommu with -device

[Marcel Apfelbaum]

On 06/03/2016 07:07 PM, Michael S. Tsirkin wrote:
> On Thu, Jun 02, 2016 at 11:15:54PM +0300, Marcel Apfelbaum wrote:
>> Use the standard '-device iommu' instead of '-machine,iommu=on'
>> to create the IOMMU device.
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>

Hi Michael,
Thank you for the review.

> Why can't we keep support for the old flag?
>

We can, but IMO we don't need it for several reasons:

The current vIOMMU before the fantastic work of Aviv and Peter
is not really usable, is there only as a "lab" feature with no
clear interesting scenario.

If we keep it, we should also support the "x-iommu-type" for
AMD IOMMU, so we add "legacy" code we don't want.

It is easy to add additional options with -device,
but how will we add them to -machine,iommu=on? an extra machine option?

Finally, if we do have current users, asking them for a minimum command line
change is not such a big deal.

Thanks,
Marcel

>> ---
>>   hw/core/machine.c     | 20 --------------------
>>   hw/i386/intel_iommu.c | 17 +++++++++++++++++
>>   hw/pci-host/q35.c     | 28 ----------------------------
>>   qemu-options.hx       |  3 ---
>>   4 files changed, 17 insertions(+), 51 deletions(-)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index ccdd5fa..8f94301 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -300,20 +300,6 @@ static void machine_set_firmware(Object *obj, const char *value, Error **errp)
>>       ms->firmware = g_strdup(value);
>>   }
>>
>> -static bool machine_get_iommu(Object *obj, Error **errp)
>> -{
>> -    MachineState *ms = MACHINE(obj);
>> -
>> -    return ms->iommu;
>> -}
>> -
>> -static void machine_set_iommu(Object *obj, bool value, Error **errp)
>> -{
>> -    MachineState *ms = MACHINE(obj);
>> -
>> -    ms->iommu = value;
>> -}
>> -
>>   static void machine_set_suppress_vmdesc(Object *obj, bool value, Error **errp)
>>   {
>>       MachineState *ms = MACHINE(obj);
>> @@ -493,12 +479,6 @@ static void machine_initfn(Object *obj)
>>       object_property_set_description(obj, "firmware",
>>                                       "Firmware image",
>>                                       NULL);
>> -    object_property_add_bool(obj, "iommu",
>> -                             machine_get_iommu,
>> -                             machine_set_iommu, NULL);
>> -    object_property_set_description(obj, "iommu",
>> -                                    "Set on/off to enable/disable Intel IOMMU (VT-d)",
>> -                                    NULL);
>>       object_property_add_bool(obj, "suppress-vmdesc",
>>                                machine_get_suppress_vmdesc,
>>                                machine_set_suppress_vmdesc, NULL);
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index 347718f..9af5d6b 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -24,6 +24,8 @@
>>   #include "exec/address-spaces.h"
>>   #include "intel_iommu_internal.h"
>>   #include "hw/pci/pci.h"
>> +#include "hw/pci/pci_bus.h"
>> +#include "hw/i386/pc.h"
>>
>>   /*#define DEBUG_INTEL_IOMMU*/
>>   #ifdef DEBUG_INTEL_IOMMU
>> @@ -2014,8 +2016,20 @@ static void vtd_reset(DeviceState *dev)
>>       vtd_init(s);
>>   }
>>
>> +static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>> +{
>> +    IntelIOMMUState *s = opaque;
>> +    VTDAddressSpace *vtd_as;
>> +
>> +    assert(0 <= devfn && devfn <= VTD_PCI_DEVFN_MAX);
>> +
>> +    vtd_as = vtd_find_add_as(s, bus, devfn);
>> +    return &vtd_as->as;
>> +}
>> +
>>   static void vtd_realize(DeviceState *dev, Error **errp)
>>   {
>> +    PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus;
>>       IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev);
>>
>>       VTD_DPRINTF(GENERAL, "");
>> @@ -2029,6 +2043,9 @@ static void vtd_realize(DeviceState *dev, Error **errp)
>>       s->vtd_as_by_busptr = g_hash_table_new_full(vtd_uint64_hash, vtd_uint64_equal,
>>                                                 g_free, g_free);
>>       vtd_init(s);
>> +    sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, Q35_HOST_BRIDGE_IOMMU_ADDR);
>> +    bus->iommu_fn = vtd_host_dma_iommu;
>> +    bus->iommu_opaque = dev;
>>   }
>>
>>   static void vtd_class_init(ObjectClass *klass, void *data)
>> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
>> index 70f897e..ea684c7 100644
>> --- a/hw/pci-host/q35.c
>> +++ b/hw/pci-host/q35.c
>> @@ -424,30 +424,6 @@ static void mch_reset(DeviceState *qdev)
>>       mch_update(mch);
>>   }
>>
>> -static AddressSpace *q35_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>> -{
>> -    IntelIOMMUState *s = opaque;
>> -    VTDAddressSpace *vtd_as;
>> -
>> -    assert(0 <= devfn && devfn <= VTD_PCI_DEVFN_MAX);
>> -
>> -    vtd_as = vtd_find_add_as(s, bus, devfn);
>> -    return &vtd_as->as;
>> -}
>> -
>> -static void mch_init_dmar(MCHPCIState *mch)
>> -{
>> -    PCIBus *pci_bus = PCI_BUS(qdev_get_parent_bus(DEVICE(mch)));
>> -
>> -    mch->iommu = INTEL_IOMMU_DEVICE(qdev_create(NULL, TYPE_INTEL_IOMMU_DEVICE));
>> -    object_property_add_child(OBJECT(mch), "intel-iommu",
>> -                              OBJECT(mch->iommu), NULL);
>> -    qdev_init_nofail(DEVICE(mch->iommu));
>> -    sysbus_mmio_map(SYS_BUS_DEVICE(mch->iommu), 0, Q35_HOST_BRIDGE_IOMMU_ADDR);
>> -
>> -    pci_setup_iommu(pci_bus, q35_host_dma_iommu, mch->iommu);
>> -}
>> -
>>   static void mch_realize(PCIDevice *d, Error **errp)
>>   {
>>       int i;
>> @@ -506,10 +482,6 @@ static void mch_realize(PCIDevice *d, Error **errp)
>>                    mch->pci_address_space, &mch->pam_regions[i+1],
>>                    PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
>>       }
>> -    /* Intel IOMMU (VT-d) */
>> -    if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) {
>> -        mch_init_dmar(mch);
>> -    }
>>   }
>>
>>   uint64_t mch_mcfg_base(void)
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 6106520..2953baf 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -38,7 +38,6 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
>>       "                kvm_shadow_mem=size of KVM shadow MMU\n"
>>       "                dump-guest-core=on|off include guest memory in a core dump (default=on)\n"
>>       "                mem-merge=on|off controls memory merge support (default: on)\n"
>> -    "                iommu=on|off controls emulated Intel IOMMU (VT-d) support (default=off)\n"
>>       "                igd-passthru=on|off controls IGD GFX passthrough support (default=off)\n"
>>       "                aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n"
>>       "                dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n"
>> @@ -73,8 +72,6 @@ Include guest memory in a core dump. The default is on.
>>   Enables or disables memory merge support. This feature, when supported by
>>   the host, de-duplicates identical memory pages among VMs instances
>>   (enabled by default).
>> -@item iommu=on|off
>> -Enables or disables emulated Intel IOMMU (VT-d) support. The default is off.
>>   @item aes-key-wrap=on|off
>>   Enables or disables AES key wrapping support on s390-ccw hosts. This feature
>>   controls whether AES wrapping keys will be created to allow
>> --
>> 2.4.3

Re: [Qemu-devel] [PATCH v2 2/3] hw/iommu: enable iommu with -device

[Michael S. Tsirkin]

On Sun, Jun 05, 2016 at 11:46:13AM +0300, Marcel Apfelbaum wrote:
> On 06/03/2016 07:07 PM, Michael S. Tsirkin wrote:
> >On Thu, Jun 02, 2016 at 11:15:54PM +0300, Marcel Apfelbaum wrote:
> >>Use the standard '-device iommu' instead of '-machine,iommu=on'
> >>to create the IOMMU device.
> >>
> >>Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> >
> 
> Hi Michael,
> Thank you for the review.
> 
> >Why can't we keep support for the old flag?
> >
> 
> We can, but IMO we don't need it for several reasons:
> 
> The current vIOMMU before the fantastic work of Aviv and Peter
> is not really usable, is there only as a "lab" feature with no
> clear interesting scenario.
> 
> If we keep it, we should also support the "x-iommu-type" for
> AMD IOMMU, so we add "legacy" code we don't want.
> It is easy to add additional options with -device,
> but how will we add them to -machine,iommu=on? an extra machine option?
> 
> Finally, if we do have current users, asking them for a minimum command line
> change is not such a big deal.
> 
> Thanks,
> Marcel

Could you separate -device support from dropping the iommu flag?
iommu flag would keep meaning intel with no options for compatibility.

> >>---
> >>  hw/core/machine.c     | 20 --------------------
> >>  hw/i386/intel_iommu.c | 17 +++++++++++++++++
> >>  hw/pci-host/q35.c     | 28 ----------------------------
> >>  qemu-options.hx       |  3 ---
> >>  4 files changed, 17 insertions(+), 51 deletions(-)
> >>
> >>diff --git a/hw/core/machine.c b/hw/core/machine.c
> >>index ccdd5fa..8f94301 100644
> >>--- a/hw/core/machine.c
> >>+++ b/hw/core/machine.c
> >>@@ -300,20 +300,6 @@ static void machine_set_firmware(Object *obj, const char *value, Error **errp)
> >>      ms->firmware = g_strdup(value);
> >>  }
> >>
> >>-static bool machine_get_iommu(Object *obj, Error **errp)
> >>-{
> >>-    MachineState *ms = MACHINE(obj);
> >>-
> >>-    return ms->iommu;
> >>-}
> >>-
> >>-static void machine_set_iommu(Object *obj, bool value, Error **errp)
> >>-{
> >>-    MachineState *ms = MACHINE(obj);
> >>-
> >>-    ms->iommu = value;
> >>-}
> >>-
> >>  static void machine_set_suppress_vmdesc(Object *obj, bool value, Error **errp)
> >>  {
> >>      MachineState *ms = MACHINE(obj);
> >>@@ -493,12 +479,6 @@ static void machine_initfn(Object *obj)
> >>      object_property_set_description(obj, "firmware",
> >>                                      "Firmware image",
> >>                                      NULL);
> >>-    object_property_add_bool(obj, "iommu",
> >>-                             machine_get_iommu,
> >>-                             machine_set_iommu, NULL);
> >>-    object_property_set_description(obj, "iommu",
> >>-                                    "Set on/off to enable/disable Intel IOMMU (VT-d)",
> >>-                                    NULL);
> >>      object_property_add_bool(obj, "suppress-vmdesc",
> >>                               machine_get_suppress_vmdesc,
> >>                               machine_set_suppress_vmdesc, NULL);
> >>diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> >>index 347718f..9af5d6b 100644
> >>--- a/hw/i386/intel_iommu.c
> >>+++ b/hw/i386/intel_iommu.c
> >>@@ -24,6 +24,8 @@
> >>  #include "exec/address-spaces.h"
> >>  #include "intel_iommu_internal.h"
> >>  #include "hw/pci/pci.h"
> >>+#include "hw/pci/pci_bus.h"
> >>+#include "hw/i386/pc.h"
> >>
> >>  /*#define DEBUG_INTEL_IOMMU*/
> >>  #ifdef DEBUG_INTEL_IOMMU
> >>@@ -2014,8 +2016,20 @@ static void vtd_reset(DeviceState *dev)
> >>      vtd_init(s);
> >>  }
> >>
> >>+static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
> >>+{
> >>+    IntelIOMMUState *s = opaque;
> >>+    VTDAddressSpace *vtd_as;
> >>+
> >>+    assert(0 <= devfn && devfn <= VTD_PCI_DEVFN_MAX);
> >>+
> >>+    vtd_as = vtd_find_add_as(s, bus, devfn);
> >>+    return &vtd_as->as;
> >>+}
> >>+
> >>  static void vtd_realize(DeviceState *dev, Error **errp)
> >>  {
> >>+    PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus;
> >>      IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev);
> >>
> >>      VTD_DPRINTF(GENERAL, "");
> >>@@ -2029,6 +2043,9 @@ static void vtd_realize(DeviceState *dev, Error **errp)
> >>      s->vtd_as_by_busptr = g_hash_table_new_full(vtd_uint64_hash, vtd_uint64_equal,
> >>                                                g_free, g_free);
> >>      vtd_init(s);
> >>+    sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, Q35_HOST_BRIDGE_IOMMU_ADDR);
> >>+    bus->iommu_fn = vtd_host_dma_iommu;
> >>+    bus->iommu_opaque = dev;
> >>  }
> >>
> >>  static void vtd_class_init(ObjectClass *klass, void *data)
> >>diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> >>index 70f897e..ea684c7 100644
> >>--- a/hw/pci-host/q35.c
> >>+++ b/hw/pci-host/q35.c
> >>@@ -424,30 +424,6 @@ static void mch_reset(DeviceState *qdev)
> >>      mch_update(mch);
> >>  }
> >>
> >>-static AddressSpace *q35_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
> >>-{
> >>-    IntelIOMMUState *s = opaque;
> >>-    VTDAddressSpace *vtd_as;
> >>-
> >>-    assert(0 <= devfn && devfn <= VTD_PCI_DEVFN_MAX);
> >>-
> >>-    vtd_as = vtd_find_add_as(s, bus, devfn);
> >>-    return &vtd_as->as;
> >>-}
> >>-
> >>-static void mch_init_dmar(MCHPCIState *mch)
> >>-{
> >>-    PCIBus *pci_bus = PCI_BUS(qdev_get_parent_bus(DEVICE(mch)));
> >>-
> >>-    mch->iommu = INTEL_IOMMU_DEVICE(qdev_create(NULL, TYPE_INTEL_IOMMU_DEVICE));
> >>-    object_property_add_child(OBJECT(mch), "intel-iommu",
> >>-                              OBJECT(mch->iommu), NULL);
> >>-    qdev_init_nofail(DEVICE(mch->iommu));
> >>-    sysbus_mmio_map(SYS_BUS_DEVICE(mch->iommu), 0, Q35_HOST_BRIDGE_IOMMU_ADDR);
> >>-
> >>-    pci_setup_iommu(pci_bus, q35_host_dma_iommu, mch->iommu);
> >>-}
> >>-
> >>  static void mch_realize(PCIDevice *d, Error **errp)
> >>  {
> >>      int i;
> >>@@ -506,10 +482,6 @@ static void mch_realize(PCIDevice *d, Error **errp)
> >>                   mch->pci_address_space, &mch->pam_regions[i+1],
> >>                   PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
> >>      }
> >>-    /* Intel IOMMU (VT-d) */
> >>-    if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) {
> >>-        mch_init_dmar(mch);
> >>-    }
> >>  }
> >>
> >>  uint64_t mch_mcfg_base(void)
> >>diff --git a/qemu-options.hx b/qemu-options.hx
> >>index 6106520..2953baf 100644
> >>--- a/qemu-options.hx
> >>+++ b/qemu-options.hx
> >>@@ -38,7 +38,6 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
> >>      "                kvm_shadow_mem=size of KVM shadow MMU\n"
> >>      "                dump-guest-core=on|off include guest memory in a core dump (default=on)\n"
> >>      "                mem-merge=on|off controls memory merge support (default: on)\n"
> >>-    "                iommu=on|off controls emulated Intel IOMMU (VT-d) support (default=off)\n"
> >>      "                igd-passthru=on|off controls IGD GFX passthrough support (default=off)\n"
> >>      "                aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n"
> >>      "                dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n"
> >>@@ -73,8 +72,6 @@ Include guest memory in a core dump. The default is on.
> >>  Enables or disables memory merge support. This feature, when supported by
> >>  the host, de-duplicates identical memory pages among VMs instances
> >>  (enabled by default).
> >>-@item iommu=on|off
> >>-Enables or disables emulated Intel IOMMU (VT-d) support. The default is off.
> >>  @item aes-key-wrap=on|off
> >>  Enables or disables AES key wrapping support on s390-ccw hosts. This feature
> >>  controls whether AES wrapping keys will be created to allow
> >>--
> >>2.4.3

Re: [Qemu-devel] [PATCH v2 2/3] hw/iommu: enable iommu with -device

[Marcel Apfelbaum]

On 06/05/2016 12:59 PM, Michael S. Tsirkin wrote:
> On Sun, Jun 05, 2016 at 11:46:13AM +0300, Marcel Apfelbaum wrote:
>> On 06/03/2016 07:07 PM, Michael S. Tsirkin wrote:
>>> On Thu, Jun 02, 2016 at 11:15:54PM +0300, Marcel Apfelbaum wrote:
>>>> Use the standard '-device iommu' instead of '-machine,iommu=on'
>>>> to create the IOMMU device.
>>>>
>>>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>>>
>>
>> Hi Michael,
>> Thank you for the review.
>>
>>> Why can't we keep support for the old flag?
>>>
>>
>> We can, but IMO we don't need it for several reasons:
>>
>> The current vIOMMU before the fantastic work of Aviv and Peter
>> is not really usable, is there only as a "lab" feature with no
>> clear interesting scenario.
>>
>> If we keep it, we should also support the "x-iommu-type" for
>> AMD IOMMU, so we add "legacy" code we don't want.
>> It is easy to add additional options with -device,
>> but how will we add them to -machine,iommu=on? an extra machine option?
>>
>> Finally, if we do have current users, asking them for a minimum command line
>> change is not such a big deal.
>>
>> Thanks,
>> Marcel
>
> Could you separate -device support from dropping the iommu flag?
> iommu flag would keep meaning intel with no options for compatibility.
>

Yes, is possible. But are you sure the compatibility worth having
an iommu machine option supporting only Intel IOMMU (without AMD) with no options?

Anyway, I will split this patch in two, first one allowing iommu creation
in both ways, the other one  removing the iommu=on option.
We can decide what later if we want the legacy part or not.

Thanks,
Marcel

>>>> ---
>>>>   hw/core/machine.c     | 20 --------------------
>>>>   hw/i386/intel_iommu.c | 17 +++++++++++++++++
>>>>   hw/pci-host/q35.c     | 28 ----------------------------
>>>>   qemu-options.hx       |  3 ---
>>>>   4 files changed, 17 insertions(+), 51 deletions(-)
>>>>
>>>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>>>> index ccdd5fa..8f94301 100644
>>>> --- a/hw/core/machine.c
>>>> +++ b/hw/core/machine.c
>>>> @@ -300,20 +300,6 @@ static void machine_set_firmware(Object *obj, const char *value, Error **errp)
>>>>       ms->firmware = g_strdup(value);
>>>>   }
>>>>
>>>> -static bool machine_get_iommu(Object *obj, Error **errp)
>>>> -{
>>>> -    MachineState *ms = MACHINE(obj);
>>>> -
>>>> -    return ms->iommu;
>>>> -}
>>>> -
>>>> -static void machine_set_iommu(Object *obj, bool value, Error **errp)
>>>> -{
>>>> -    MachineState *ms = MACHINE(obj);
>>>> -
>>>> -    ms->iommu = value;
>>>> -}
>>>> -
>>>>   static void machine_set_suppress_vmdesc(Object *obj, bool value, Error **errp)
>>>>   {
>>>>       MachineState *ms = MACHINE(obj);
>>>> @@ -493,12 +479,6 @@ static void machine_initfn(Object *obj)
>>>>       object_property_set_description(obj, "firmware",
>>>>                                       "Firmware image",
>>>>                                       NULL);
>>>> -    object_property_add_bool(obj, "iommu",
>>>> -                             machine_get_iommu,
>>>> -                             machine_set_iommu, NULL);
>>>> -    object_property_set_description(obj, "iommu",
>>>> -                                    "Set on/off to enable/disable Intel IOMMU (VT-d)",
>>>> -                                    NULL);
>>>>       object_property_add_bool(obj, "suppress-vmdesc",
>>>>                                machine_get_suppress_vmdesc,
>>>>                                machine_set_suppress_vmdesc, NULL);
>>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>>> index 347718f..9af5d6b 100644
>>>> --- a/hw/i386/intel_iommu.c
>>>> +++ b/hw/i386/intel_iommu.c
>>>> @@ -24,6 +24,8 @@
>>>>   #include "exec/address-spaces.h"
>>>>   #include "intel_iommu_internal.h"
>>>>   #include "hw/pci/pci.h"
>>>> +#include "hw/pci/pci_bus.h"
>>>> +#include "hw/i386/pc.h"
>>>>
>>>>   /*#define DEBUG_INTEL_IOMMU*/
>>>>   #ifdef DEBUG_INTEL_IOMMU
>>>> @@ -2014,8 +2016,20 @@ static void vtd_reset(DeviceState *dev)
>>>>       vtd_init(s);
>>>>   }
>>>>
>>>> +static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>>>> +{
>>>> +    IntelIOMMUState *s = opaque;
>>>> +    VTDAddressSpace *vtd_as;
>>>> +
>>>> +    assert(0 <= devfn && devfn <= VTD_PCI_DEVFN_MAX);
>>>> +
>>>> +    vtd_as = vtd_find_add_as(s, bus, devfn);
>>>> +    return &vtd_as->as;
>>>> +}
>>>> +
>>>>   static void vtd_realize(DeviceState *dev, Error **errp)
>>>>   {
>>>> +    PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus;
>>>>       IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev);
>>>>
>>>>       VTD_DPRINTF(GENERAL, "");
>>>> @@ -2029,6 +2043,9 @@ static void vtd_realize(DeviceState *dev, Error **errp)
>>>>       s->vtd_as_by_busptr = g_hash_table_new_full(vtd_uint64_hash, vtd_uint64_equal,
>>>>                                                 g_free, g_free);
>>>>       vtd_init(s);
>>>> +    sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, Q35_HOST_BRIDGE_IOMMU_ADDR);
>>>> +    bus->iommu_fn = vtd_host_dma_iommu;
>>>> +    bus->iommu_opaque = dev;
>>>>   }
>>>>
>>>>   static void vtd_class_init(ObjectClass *klass, void *data)
>>>> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
>>>> index 70f897e..ea684c7 100644
>>>> --- a/hw/pci-host/q35.c
>>>> +++ b/hw/pci-host/q35.c
>>>> @@ -424,30 +424,6 @@ static void mch_reset(DeviceState *qdev)
>>>>       mch_update(mch);
>>>>   }
>>>>
>>>> -static AddressSpace *q35_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>>>> -{
>>>> -    IntelIOMMUState *s = opaque;
>>>> -    VTDAddressSpace *vtd_as;
>>>> -
>>>> -    assert(0 <= devfn && devfn <= VTD_PCI_DEVFN_MAX);
>>>> -
>>>> -    vtd_as = vtd_find_add_as(s, bus, devfn);
>>>> -    return &vtd_as->as;
>>>> -}
>>>> -
>>>> -static void mch_init_dmar(MCHPCIState *mch)
>>>> -{
>>>> -    PCIBus *pci_bus = PCI_BUS(qdev_get_parent_bus(DEVICE(mch)));
>>>> -
>>>> -    mch->iommu = INTEL_IOMMU_DEVICE(qdev_create(NULL, TYPE_INTEL_IOMMU_DEVICE));
>>>> -    object_property_add_child(OBJECT(mch), "intel-iommu",
>>>> -                              OBJECT(mch->iommu), NULL);
>>>> -    qdev_init_nofail(DEVICE(mch->iommu));
>>>> -    sysbus_mmio_map(SYS_BUS_DEVICE(mch->iommu), 0, Q35_HOST_BRIDGE_IOMMU_ADDR);
>>>> -
>>>> -    pci_setup_iommu(pci_bus, q35_host_dma_iommu, mch->iommu);
>>>> -}
>>>> -
>>>>   static void mch_realize(PCIDevice *d, Error **errp)
>>>>   {
>>>>       int i;
>>>> @@ -506,10 +482,6 @@ static void mch_realize(PCIDevice *d, Error **errp)
>>>>                    mch->pci_address_space, &mch->pam_regions[i+1],
>>>>                    PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
>>>>       }
>>>> -    /* Intel IOMMU (VT-d) */
>>>> -    if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) {
>>>> -        mch_init_dmar(mch);
>>>> -    }
>>>>   }
>>>>
>>>>   uint64_t mch_mcfg_base(void)
>>>> diff --git a/qemu-options.hx b/qemu-options.hx
>>>> index 6106520..2953baf 100644
>>>> --- a/qemu-options.hx
>>>> +++ b/qemu-options.hx
>>>> @@ -38,7 +38,6 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
>>>>       "                kvm_shadow_mem=size of KVM shadow MMU\n"
>>>>       "                dump-guest-core=on|off include guest memory in a core dump (default=on)\n"
>>>>       "                mem-merge=on|off controls memory merge support (default: on)\n"
>>>> -    "                iommu=on|off controls emulated Intel IOMMU (VT-d) support (default=off)\n"
>>>>       "                igd-passthru=on|off controls IGD GFX passthrough support (default=off)\n"
>>>>       "                aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n"
>>>>       "                dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n"
>>>> @@ -73,8 +72,6 @@ Include guest memory in a core dump. The default is on.
>>>>   Enables or disables memory merge support. This feature, when supported by
>>>>   the host, de-duplicates identical memory pages among VMs instances
>>>>   (enabled by default).
>>>> -@item iommu=on|off
>>>> -Enables or disables emulated Intel IOMMU (VT-d) support. The default is off.
>>>>   @item aes-key-wrap=on|off
>>>>   Enables or disables AES key wrapping support on s390-ccw hosts. This feature
>>>>   controls whether AES wrapping keys will be created to allow
>>>> --
>>>> 2.4.3

Re: [Qemu-devel] [PATCH v2 2/3] hw/iommu: enable iommu with -device

[Peter Xu]

On Thu, Jun 02, 2016 at 11:15:54PM +0300, Marcel Apfelbaum wrote:

[...]

>  static void vtd_realize(DeviceState *dev, Error **errp)
>  {
> +    PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus;
>      IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev);
>  
>      VTD_DPRINTF(GENERAL, "");
> @@ -2029,6 +2043,9 @@ static void vtd_realize(DeviceState *dev, Error **errp)
>      s->vtd_as_by_busptr = g_hash_table_new_full(vtd_uint64_hash, vtd_uint64_equal,
>                                                g_free, g_free);
>      vtd_init(s);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, Q35_HOST_BRIDGE_IOMMU_ADDR);
> +    bus->iommu_fn = vtd_host_dma_iommu;
> +    bus->iommu_opaque = dev;

Here, shall we still use pci_setup_iommu() to keep the two fields
private for pci framework?

Btw, I am rebasing Intel IR work onto this patchset, but encountered
issues (guest hang, or errornous interrupts) when guest specify more
than 1 vcpus (everything is cool as long as vcpu=1). Maybe there is
something wrong during the rebase, still investigating. Please shoot
if there is any clue.

Thanks,

-- peterx

Re: [Qemu-devel] [PATCH v2 2/3] hw/iommu: enable iommu with -device

[Marcel Apfelbaum]

On 06/12/2016 07:27 AM, Peter Xu wrote:
> On Thu, Jun 02, 2016 at 11:15:54PM +0300, Marcel Apfelbaum wrote:
>
> [...]
>
>>   static void vtd_realize(DeviceState *dev, Error **errp)
>>   {
>> +    PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus;
>>       IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev);
>>
>>       VTD_DPRINTF(GENERAL, "");
>> @@ -2029,6 +2043,9 @@ static void vtd_realize(DeviceState *dev, Error **errp)
>>       s->vtd_as_by_busptr = g_hash_table_new_full(vtd_uint64_hash, vtd_uint64_equal,
>>                                                 g_free, g_free);
>>       vtd_init(s);
>> +    sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, Q35_HOST_BRIDGE_IOMMU_ADDR);
>> +    bus->iommu_fn = vtd_host_dma_iommu;
>> +    bus->iommu_opaque = dev;
>
> Here, shall we still use pci_setup_iommu() to keep the two fields
> private for pci framework?
>

I've already spotted it and took care of it, thanks :) !

> Btw, I am rebasing Intel IR work onto this patchset, but encountered
> issues (guest hang, or errornous interrupts) when guest specify more
> than 1 vcpus (everything is cool as long as vcpu=1). Maybe there is
> something wrong during the rebase, still investigating. Please shoot
> if there is any clue.
>

I am running with 2 vcpus and I didn't see any problem, I'll let you
know if can reproduce.

Thanks,
Marcel

> Thanks,
>
> -- peterx
>

Re: [Qemu-devel] [PATCH v2 2/3] hw/iommu: enable iommu with -device

[Peter Xu]

On Mon, Jun 13, 2016 at 01:20:11PM +0300, Marcel Apfelbaum wrote:
> On 06/12/2016 07:27 AM, Peter Xu wrote:
> >On Thu, Jun 02, 2016 at 11:15:54PM +0300, Marcel Apfelbaum wrote:
> >
> >[...]
> >
> >>  static void vtd_realize(DeviceState *dev, Error **errp)
> >>  {
> >>+    PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus;
> >>      IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev);
> >>
> >>      VTD_DPRINTF(GENERAL, "");
> >>@@ -2029,6 +2043,9 @@ static void vtd_realize(DeviceState *dev, Error **errp)
> >>      s->vtd_as_by_busptr = g_hash_table_new_full(vtd_uint64_hash, vtd_uint64_equal,
> >>                                                g_free, g_free);
> >>      vtd_init(s);
> >>+    sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, Q35_HOST_BRIDGE_IOMMU_ADDR);
> >>+    bus->iommu_fn = vtd_host_dma_iommu;
> >>+    bus->iommu_opaque = dev;
> >
> >Here, shall we still use pci_setup_iommu() to keep the two fields
> >private for pci framework?
> >
> 
> I've already spotted it and took care of it, thanks :) !

Cool. :)

Btw, have we removed MachineState.iommu variable as well?

> 
> >Btw, I am rebasing Intel IR work onto this patchset, but encountered
> >issues (guest hang, or errornous interrupts) when guest specify more
> >than 1 vcpus (everything is cool as long as vcpu=1). Maybe there is
> >something wrong during the rebase, still investigating. Please shoot
> >if there is any clue.
> >
> 
> I am running with 2 vcpus and I didn't see any problem, I'll let you
> know if can reproduce.

My fault during rebase. It's very easy to lost lines of codes during
rebase, especially where there is function move from one place to
another, while in which function I did some changes... It's all good
now. Thanks!

-- peterx

[Qemu-devel] [PATCH v2 3/3] q35: allow dynamic sysbus

[Marcel Apfelbaum]

Allow adding sysbus devices with -device on Q35.

At first Q35 will support only intel-iommu to be added this way,
however the command line will support all sysbus devices.

Mark with 'cannot_instantiate_with_device_add_yet' the ones
causing immediate problems (e.g. crashes).

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/i386/pc_q35.c                    | 1 +
 hw/pci-bridge/pci_expander_bridge.c | 1 +
 hw/pci-host/piix.c                  | 1 +
 hw/pci-host/q35.c                   | 1 +
 4 files changed, 4 insertions(+)

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 04aae89..431eaed 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -281,6 +281,7 @@ static void pc_q35_machine_options(MachineClass *m)
     m->default_machine_opts = "firmware=bios-256k.bin";
     m->default_display = "std";
     m->no_floppy = 1;
+    m->has_dynamic_sysbus = true;
 }
 
 static void pc_q35_2_6_machine_options(MachineClass *m)
diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
index ba320bd..40518a2 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -149,6 +149,7 @@ static void pxb_host_class_init(ObjectClass *class, void *data)
     PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(class);
 
     dc->fw_name = "pci";
+    dc->cannot_instantiate_with_device_add_yet = true;
     sbc->explicit_ofw_unit_address = pxb_host_ofw_unit_address;
     hc->root_bus_path = pxb_host_root_bus_path;
 }
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index df2b0e2..fea7f59 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -865,6 +865,7 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
     dc->realize = i440fx_pcihost_realize;
     dc->fw_name = "pci";
     dc->props = i440fx_props;
+    dc->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo i440fx_pcihost_info = {
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index ea684c7..9d82d30 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -138,6 +138,7 @@ static void q35_host_class_init(ObjectClass *klass, void *data)
     hc->root_bus_path = q35_host_root_bus_path;
     dc->realize = q35_host_realize;
     dc->props = mch_props;
+    dc->cannot_instantiate_with_device_add_yet = true;
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
     dc->fw_name = "pci";
 }
-- 
2.4.3

Re: [Qemu-devel] [PATCH v2 3/3] q35: allow dynamic sysbus

[Markus Armbruster]

Marcel Apfelbaum <marcel@redhat.com> writes:

> Allow adding sysbus devices with -device on Q35.
>
> At first Q35 will support only intel-iommu to be added this way,
> however the command line will support all sysbus devices.
>
> Mark with 'cannot_instantiate_with_device_add_yet' the ones
> causing immediate problems (e.g. crashes).
>
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> ---
>  hw/i386/pc_q35.c                    | 1 +
>  hw/pci-bridge/pci_expander_bridge.c | 1 +
>  hw/pci-host/piix.c                  | 1 +
>  hw/pci-host/q35.c                   | 1 +
>  4 files changed, 4 insertions(+)
>
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 04aae89..431eaed 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -281,6 +281,7 @@ static void pc_q35_machine_options(MachineClass *m)
>      m->default_machine_opts = "firmware=bios-256k.bin";
>      m->default_display = "std";
>      m->no_floppy = 1;
> +    m->has_dynamic_sysbus = true;
>  }
>  
>  static void pc_q35_2_6_machine_options(MachineClass *m)
> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
> index ba320bd..40518a2 100644
> --- a/hw/pci-bridge/pci_expander_bridge.c
> +++ b/hw/pci-bridge/pci_expander_bridge.c
> @@ -149,6 +149,7 @@ static void pxb_host_class_init(ObjectClass *class, void *data)
>      PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(class);
>  
>      dc->fw_name = "pci";
> +    dc->cannot_instantiate_with_device_add_yet = true;
>      sbc->explicit_ofw_unit_address = pxb_host_ofw_unit_address;
>      hc->root_bus_path = pxb_host_root_bus_path;
>  }

Any assignment to cannot_instantiate_with_device_add_yet must have a
comment, like this:

       /* Reason: frobnicates some frobs backwards */
       dc->cannot_instantiate_with_device_add_yet = true;

We have one offender in master: hw/ppc/spapr_pci.c (commit 09aa9a52),
which I'll try to get fixed.  Please don't add more.

[...]

Re: [Qemu-devel] [PATCH v2 3/3] q35: allow dynamic sysbus

[Marcel Apfelbaum]

On 06/03/2016 09:33 AM, Markus Armbruster wrote:
> Marcel Apfelbaum <marcel@redhat.com> writes:
>
>> Allow adding sysbus devices with -device on Q35.
>>
>> At first Q35 will support only intel-iommu to be added this way,
>> however the command line will support all sysbus devices.
>>
>> Mark with 'cannot_instantiate_with_device_add_yet' the ones
>> causing immediate problems (e.g. crashes).
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>> ---
>>   hw/i386/pc_q35.c                    | 1 +
>>   hw/pci-bridge/pci_expander_bridge.c | 1 +
>>   hw/pci-host/piix.c                  | 1 +
>>   hw/pci-host/q35.c                   | 1 +
>>   4 files changed, 4 insertions(+)
>>
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index 04aae89..431eaed 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -281,6 +281,7 @@ static void pc_q35_machine_options(MachineClass *m)
>>       m->default_machine_opts = "firmware=bios-256k.bin";
>>       m->default_display = "std";
>>       m->no_floppy = 1;
>> +    m->has_dynamic_sysbus = true;
>>   }
>>
>>   static void pc_q35_2_6_machine_options(MachineClass *m)
>> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
>> index ba320bd..40518a2 100644
>> --- a/hw/pci-bridge/pci_expander_bridge.c
>> +++ b/hw/pci-bridge/pci_expander_bridge.c
>> @@ -149,6 +149,7 @@ static void pxb_host_class_init(ObjectClass *class, void *data)
>>       PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(class);
>>
>>       dc->fw_name = "pci";
>> +    dc->cannot_instantiate_with_device_add_yet = true;
>>       sbc->explicit_ofw_unit_address = pxb_host_ofw_unit_address;
>>       hc->root_bus_path = pxb_host_root_bus_path;
>>   }
>
> Any assignment to cannot_instantiate_with_device_add_yet must have a
> comment, like this:
>
>         /* Reason: frobnicates some frobs backwards */
>         dc->cannot_instantiate_with_device_add_yet = true;
>
> We have one offender in master: hw/ppc/spapr_pci.c (commit 09aa9a52),
> which I'll try to get fixed.  Please don't add more.
>

Sure, I'll take care of it and send a V2.

Thanks,
Marcel


> [...]
>

Re: [Qemu-devel] [PATCH v2 3/3] q35: allow dynamic sysbus

[Peter Xu]

On Thu, Jun 02, 2016 at 11:15:55PM +0300, Marcel Apfelbaum wrote:
> Allow adding sysbus devices with -device on Q35.
> 
> At first Q35 will support only intel-iommu to be added this way,
> however the command line will support all sysbus devices.
> 
> Mark with 'cannot_instantiate_with_device_add_yet' the ones
> causing immediate problems (e.g. crashes).

What happens if we do dynamic device_add for IOMMU? Guessing we should
not allow that as well?

-- peterx

Re: [Qemu-devel] [PATCH v2 3/3] q35: allow dynamic sysbus

[Marcel Apfelbaum]

On 06/08/2016 05:56 AM, Peter Xu wrote:
> On Thu, Jun 02, 2016 at 11:15:55PM +0300, Marcel Apfelbaum wrote:
>> Allow adding sysbus devices with -device on Q35.
>>
>> At first Q35 will support only intel-iommu to be added this way,
>> however the command line will support all sysbus devices.
>>
>> Mark with 'cannot_instantiate_with_device_add_yet' the ones
>> causing immediate problems (e.g. crashes).
>
> What happens if we do dynamic device_add for IOMMU? Guessing we should
> not allow that as well?
>

Sure, hot-plug is not supported, at least for now.
I can think about a device that has an IOMMU built-in,
but this is not our use-case.

Thanks,
Marcel


> -- peterx
>