[Qemu-devel] [PATCH v4 0/4] enable iommu with -device

[Qemu-devel] [PATCH v4 0/4] 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 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)

Then we need to enable sysbus devices(*) for PC machines (patch 2),
since intel-iommu is a sysbus device.

Patch 3 moves the IOMMU init proces into iommu's realize function
and allows the device creation in both ways.

Finally patch 4 removes the iommu machine property.

v3 -> v4:
  - Rebased on mst/pci tree (Michael).

v2 -> v3:
  - Add machine_done notifier in pci_bus realize and remove it unrealize (Paolo).
  - Add comments for 'cannot_instantiate_with_device_add_yet' (Markus).
  - Split adding the -device iommu support and removing the iommu machine property (Michael).
  - Use pci_setup_iommu as before (Peter)
  - Mark intel-iommu as not hot-pluggable (Peter)
  - Rebased on master

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


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


Thanks,
Marcel

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

 hw/core/machine.c                   | 20 ------------------
 hw/i386/intel_iommu.c               | 16 +++++++++++++++
 hw/i386/pc_q35.c                    |  2 +-
 hw/pci-bridge/pci_expander_bridge.c |  2 ++
 hw/pci-host/piix.c                  |  2 ++
 hw/pci-host/q35.c                   | 31 +++-------------------------
 hw/pci/pci.c                        | 41 +++++++++++++++++++++++++++++--------
 include/hw/pci-host/q35.h           |  1 -
 include/hw/pci/pci_bus.h            |  2 ++
 include/sysemu/sysemu.h             |  1 +
 qemu-options.hx                     |  3 ---
 vl.c                                |  5 +++++
 12 files changed, 64 insertions(+), 62 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v4 1/4] 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             | 41 ++++++++++++++++++++++++++++++++---------
 include/hw/pci/pci_bus.h |  2 ++
 include/sysemu/sysemu.h  |  1 +
 vl.c                     |  5 +++++
 4 files changed, 40 insertions(+), 9 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 4b585f4..ee385eb 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -78,10 +78,37 @@ static const VMStateDescription vmstate_pcibus = {
     }
 };
 
+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 pci_bus_realize(BusState *qbus, Error **errp)
 {
     PCIBus *bus = PCI_BUS(qbus);
 
+    bus->machine_done.notify = pcibus_machine_done;
+    qemu_add_machine_init_done_notifier(&bus->machine_done);
+
     vmstate_register(NULL, -1, &vmstate_pcibus, bus);
 }
 
@@ -89,6 +116,8 @@ static void pci_bus_unrealize(BusState *qbus, Error **errp)
 {
     PCIBus *bus = PCI_BUS(qbus);
 
+    qemu_remove_machine_init_done_notifier(&bus->machine_done);
+
     vmstate_unregister(NULL, &vmstate_pcibus, bus);
 }
 
@@ -920,7 +949,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;
@@ -961,15 +989,10 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
 
     pci_dev->devfn = devfn;
     pci_dev->requester_id_cache = pci_req_id_cache_get(pci_dev);
-    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;
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 9428141..604ec31 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -75,6 +75,7 @@ void qemu_add_exit_notifier(Notifier *notify);
 void qemu_remove_exit_notifier(Notifier *notify);
 
 void qemu_add_machine_init_done_notifier(Notifier *notify);
+void qemu_remove_machine_init_done_notifier(Notifier *notify);
 
 void hmp_savevm(Monitor *mon, const QDict *qdict);
 int load_vmstate(const char *name);
diff --git a/vl.c b/vl.c
index 2f74fe8..6eb7245 100644
--- a/vl.c
+++ b/vl.c
@@ -2704,6 +2704,11 @@ void qemu_add_machine_init_done_notifier(Notifier *notify)
     }
 }
 
+void qemu_remove_machine_init_done_notifier(Notifier *notify)
+{
+    notifier_remove(notify);
+}
+
 static void qemu_run_machine_init_done_notifiers(void)
 {
     notifier_list_notify(&machine_init_done_notifiers, NULL);
-- 
2.4.3

[Qemu-devel] [PATCH v4 2/4] 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 | 2 ++
 hw/pci-host/piix.c                  | 2 ++
 hw/pci-host/q35.c                   | 2 ++
 4 files changed, 7 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..ab86121 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -149,6 +149,8 @@ static void pxb_host_class_init(ObjectClass *class, void *data)
     PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(class);
 
     dc->fw_name = "pci";
+    /* Reason: Internal part of the pxb/pxb-pcie device, not usable by itself */
+    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..7167e58 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -865,6 +865,8 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
     dc->realize = i440fx_pcihost_realize;
     dc->fw_name = "pci";
     dc->props = i440fx_props;
+    /* Reason: needs to be wired up by pc_init1 */
+    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 70f897e..141ba5b 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -138,6 +138,8 @@ 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;
+    /* Reason: needs to be wired up by pc_q35_init */
+    dc->cannot_instantiate_with_device_add_yet = true;
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
     dc->fw_name = "pci";
 }
-- 
2.4.3

[Qemu-devel] [PATCH v4 3/4] hw/iommu: enable iommu with -device

[Marcel Apfelbaum]

Use the standard '-device iommu' to create the IOMMU device.
The legacy '-machine,iommu=on' can still be used.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/i386/intel_iommu.c | 16 ++++++++++++++++
 hw/i386/pc_q35.c      |  1 -
 hw/pci-host/q35.c     | 17 +----------------
 3 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 347718f..ea25585 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -24,6 +24,7 @@
 #include "exec/address-spaces.h"
 #include "intel_iommu_internal.h"
 #include "hw/pci/pci.h"
+#include "hw/i386/pc.h"
 
 /*#define DEBUG_INTEL_IOMMU*/
 #ifdef DEBUG_INTEL_IOMMU
@@ -2014,8 +2015,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 +2042,8 @@ 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);
+    pci_setup_iommu(bus, vtd_host_dma_iommu, dev);
 }
 
 static void vtd_class_init(ObjectClass *klass, void *data)
@@ -2039,6 +2054,7 @@ static void vtd_class_init(ObjectClass *klass, void *data)
     dc->realize = vtd_realize;
     dc->vmsd = &vtd_vmstate;
     dc->props = vtd_properties;
+    dc->hotpluggable = false;
 }
 
 static const TypeInfo vtd_info = {
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 431eaed..47e93d4 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -169,7 +169,6 @@ static void pc_q35_init(MachineState *machine)
     qdev_init_nofail(DEVICE(q35_host));
     phb = PCI_HOST_BRIDGE(q35_host);
     host_bus = phb->bus;
-    pcms->bus = phb->bus;
     /* create ISA bus */
     lpc = pci_create_simple_multifunction(host_bus, PCI_DEVFN(ICH9_LPC_DEV,
                                           ICH9_LPC_FUNC), true,
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 141ba5b..4bd5fb5 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -52,6 +52,7 @@ static void q35_host_realize(DeviceState *dev, Error **errp)
     pci->bus = pci_bus_new(DEVICE(s), "pcie.0",
                            s->mch.pci_address_space, s->mch.address_space_io,
                            0, TYPE_PCIE_BUS);
+    PC_MACHINE(qdev_get_machine())->bus = pci->bus;
     qdev_set_parent_bus(DEVICE(&s->mch), BUS(pci->bus));
     qdev_init_nofail(DEVICE(&s->mch));
 }
@@ -426,28 +427,12 @@ 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)
-- 
2.4.3

[Qemu-devel] [PATCH v4 4/4] machine: remove iommu property

[Marcel Apfelbaum]

Since iommu devices can be created with '-device' there is
no need to keep iommu as machine and mch property.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/core/machine.c         | 20 --------------------
 hw/pci-host/q35.c         | 12 ------------
 include/hw/pci-host/q35.h |  1 -
 qemu-options.hx           |  3 ---
 4 files changed, 36 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/pci-host/q35.c b/hw/pci-host/q35.c
index 4bd5fb5..181bc3b 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -427,14 +427,6 @@ static void mch_reset(DeviceState *qdev)
     mch_update(mch);
 }
 
-static void mch_init_dmar(MCHPCIState *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));
-}
-
 static void mch_realize(PCIDevice *d, Error **errp)
 {
     int i;
@@ -493,10 +485,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/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
index c5c073d..3dee058 100644
--- a/include/hw/pci-host/q35.h
+++ b/include/hw/pci-host/q35.h
@@ -60,7 +60,6 @@ typedef struct MCHPCIState {
     ram_addr_t above_4g_mem_size;
     uint64_t pci_hole64_size;
     uint32_t short_root_bus;
-    IntelIOMMUState *iommu;
 } MCHPCIState;
 
 typedef struct Q35PCIHost {
diff --git a/qemu-options.hx b/qemu-options.hx
index 9f33361..28e1978 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 v4 3/4] hw/iommu: enable iommu with -device

[Paolo Bonzini]

On 14/06/2016 09:19, Marcel Apfelbaum wrote:
> Use the standard '-device iommu' to create the IOMMU device.

It's "-device intel-iommu", apart from this it looks good, thanks!

Paolo

> The legacy '-machine,iommu=on' can still be used.
> 
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> ---
>  hw/i386/intel_iommu.c | 16 ++++++++++++++++
>  hw/i386/pc_q35.c      |  1 -
>  hw/pci-host/q35.c     | 17 +----------------
>  3 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 347718f..ea25585 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -24,6 +24,7 @@
>  #include "exec/address-spaces.h"
>  #include "intel_iommu_internal.h"
>  #include "hw/pci/pci.h"
> +#include "hw/i386/pc.h"
>  
>  /*#define DEBUG_INTEL_IOMMU*/
>  #ifdef DEBUG_INTEL_IOMMU
> @@ -2014,8 +2015,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 +2042,8 @@ 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);
> +    pci_setup_iommu(bus, vtd_host_dma_iommu, dev);
>  }
>  
>  static void vtd_class_init(ObjectClass *klass, void *data)
> @@ -2039,6 +2054,7 @@ static void vtd_class_init(ObjectClass *klass, void *data)
>      dc->realize = vtd_realize;
>      dc->vmsd = &vtd_vmstate;
>      dc->props = vtd_properties;
> +    dc->hotpluggable = false;
>  }
>  
>  static const TypeInfo vtd_info = {
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 431eaed..47e93d4 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -169,7 +169,6 @@ static void pc_q35_init(MachineState *machine)
>      qdev_init_nofail(DEVICE(q35_host));
>      phb = PCI_HOST_BRIDGE(q35_host);
>      host_bus = phb->bus;
> -    pcms->bus = phb->bus;
>      /* create ISA bus */
>      lpc = pci_create_simple_multifunction(host_bus, PCI_DEVFN(ICH9_LPC_DEV,
>                                            ICH9_LPC_FUNC), true,
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 141ba5b..4bd5fb5 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -52,6 +52,7 @@ static void q35_host_realize(DeviceState *dev, Error **errp)
>      pci->bus = pci_bus_new(DEVICE(s), "pcie.0",
>                             s->mch.pci_address_space, s->mch.address_space_io,
>                             0, TYPE_PCIE_BUS);
> +    PC_MACHINE(qdev_get_machine())->bus = pci->bus;
>      qdev_set_parent_bus(DEVICE(&s->mch), BUS(pci->bus));
>      qdev_init_nofail(DEVICE(&s->mch));
>  }
> @@ -426,28 +427,12 @@ 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)
> 

Re: [Qemu-devel] [PATCH v4 3/4] hw/iommu: enable iommu with -device

[Marcel Apfelbaum]

On 06/21/2016 07:54 PM, Paolo Bonzini wrote:
>
>
> On 14/06/2016 09:19, Marcel Apfelbaum wrote:
>> Use the standard '-device iommu' to create the IOMMU device.
>
> It's "-device intel-iommu", apart from this it looks good, thanks!
>

Thanks Paolo!

Michael, can you please correct the patch message?
Is already rebased on your tree. If you prefer another re-spin please let me know.

Thanks,
Marcel

> Paolo
>
>> The legacy '-machine,iommu=on' can still be used.
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>> ---
>>   hw/i386/intel_iommu.c | 16 ++++++++++++++++
>>   hw/i386/pc_q35.c      |  1 -
>>   hw/pci-host/q35.c     | 17 +----------------
>>   3 files changed, 17 insertions(+), 17 deletions(-)
>>
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index 347718f..ea25585 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -24,6 +24,7 @@
>>   #include "exec/address-spaces.h"
>>   #include "intel_iommu_internal.h"
>>   #include "hw/pci/pci.h"
>> +#include "hw/i386/pc.h"
>>
>>   /*#define DEBUG_INTEL_IOMMU*/
>>   #ifdef DEBUG_INTEL_IOMMU
>> @@ -2014,8 +2015,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 +2042,8 @@ 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);
>> +    pci_setup_iommu(bus, vtd_host_dma_iommu, dev);
>>   }
>>
>>   static void vtd_class_init(ObjectClass *klass, void *data)
>> @@ -2039,6 +2054,7 @@ static void vtd_class_init(ObjectClass *klass, void *data)
>>       dc->realize = vtd_realize;
>>       dc->vmsd = &vtd_vmstate;
>>       dc->props = vtd_properties;
>> +    dc->hotpluggable = false;
>>   }
>>
>>   static const TypeInfo vtd_info = {
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index 431eaed..47e93d4 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -169,7 +169,6 @@ static void pc_q35_init(MachineState *machine)
>>       qdev_init_nofail(DEVICE(q35_host));
>>       phb = PCI_HOST_BRIDGE(q35_host);
>>       host_bus = phb->bus;
>> -    pcms->bus = phb->bus;
>>       /* create ISA bus */
>>       lpc = pci_create_simple_multifunction(host_bus, PCI_DEVFN(ICH9_LPC_DEV,
>>                                             ICH9_LPC_FUNC), true,
>> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
>> index 141ba5b..4bd5fb5 100644
>> --- a/hw/pci-host/q35.c
>> +++ b/hw/pci-host/q35.c
>> @@ -52,6 +52,7 @@ static void q35_host_realize(DeviceState *dev, Error **errp)
>>       pci->bus = pci_bus_new(DEVICE(s), "pcie.0",
>>                              s->mch.pci_address_space, s->mch.address_space_io,
>>                              0, TYPE_PCIE_BUS);
>> +    PC_MACHINE(qdev_get_machine())->bus = pci->bus;
>>       qdev_set_parent_bus(DEVICE(&s->mch), BUS(pci->bus));
>>       qdev_init_nofail(DEVICE(&s->mch));
>>   }
>> @@ -426,28 +427,12 @@ 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)
>>

Re: [Qemu-devel] [PATCH v4 0/4] enable iommu with -device

[Michael S. Tsirkin]

On Tue, Jun 14, 2016 at 10:19:32AM +0300, Marcel Apfelbaum wrote:
> 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 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)
> 
> Then we need to enable sysbus devices(*) for PC machines (patch 2),
> since intel-iommu is a sysbus device.
> 
> Patch 3 moves the IOMMU init proces into iommu's realize function
> and allows the device creation in both ways.



This breaks make check on x86:

GTESTER check-qtest-ppc64
Broken pipe
GTester: last random seed: R02Sf26cb511ed533726cf1afef21a00bde4
/scm/qemu/tests/Makefile.include:668: recipe for target
'check-qtest-ppc64' failed
make: *** [check-qtest-ppc64] Error 1


> Finally patch 4 removes the iommu machine property.
> 
> v3 -> v4:
>   - Rebased on mst/pci tree (Michael).
> 
> v2 -> v3:
>   - Add machine_done notifier in pci_bus realize and remove it unrealize (Paolo).
>   - Add comments for 'cannot_instantiate_with_device_add_yet' (Markus).
>   - Split adding the -device iommu support and removing the iommu machine property (Michael).
>   - Use pci_setup_iommu as before (Peter)
>   - Mark intel-iommu as not hot-pluggable (Peter)
>   - Rebased on master
> 
> 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
> 
> 
> (*) 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.
> 
> 
> Thanks,
> Marcel
> 
> Marcel Apfelbaum (4):
>   hw/pci: delay bus_master_enable_region initialization
>   q35: allow dynamic sysbus
>   hw/iommu: enable iommu with -device
>   machine: remove iommu property
> 
>  hw/core/machine.c                   | 20 ------------------
>  hw/i386/intel_iommu.c               | 16 +++++++++++++++
>  hw/i386/pc_q35.c                    |  2 +-
>  hw/pci-bridge/pci_expander_bridge.c |  2 ++
>  hw/pci-host/piix.c                  |  2 ++
>  hw/pci-host/q35.c                   | 31 +++-------------------------
>  hw/pci/pci.c                        | 41 +++++++++++++++++++++++++++++--------
>  include/hw/pci-host/q35.h           |  1 -
>  include/hw/pci/pci_bus.h            |  2 ++
>  include/sysemu/sysemu.h             |  1 +
>  qemu-options.hx                     |  3 ---
>  vl.c                                |  5 +++++
>  12 files changed, 64 insertions(+), 62 deletions(-)
> 
> -- 
> 2.4.3

Re: [Qemu-devel] [PATCH v4 0/4] enable iommu with -device

[Marcel Apfelbaum]

On 06/24/2016 08:47 AM, Michael S. Tsirkin wrote:
> On Tue, Jun 14, 2016 at 10:19:32AM +0300, Marcel Apfelbaum wrote:
>> 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 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)
>>
>> Then we need to enable sysbus devices(*) for PC machines (patch 2),
>> since intel-iommu is a sysbus device.
>>
>> Patch 3 moves the IOMMU init proces into iommu's realize function
>> and allows the device creation in both ways.
>
>
>
> This breaks make check on x86:
>
> GTESTER check-qtest-ppc64
> Broken pipe
> GTester: last random seed: R02Sf26cb511ed533726cf1afef21a00bde4
> /scm/qemu/tests/Makefile.include:668: recipe for target
> 'check-qtest-ppc64' failed
> make: *** [check-qtest-ppc64] Error 1
>

I am sorry for that.
I'll take care of it and re-post.

Thanks,
Marcel

>
>> Finally patch 4 removes the iommu machine property.
>>
>> v3 -> v4:
>>    - Rebased on mst/pci tree (Michael).
>>
>> v2 -> v3:
>>    - Add machine_done notifier in pci_bus realize and remove it unrealize (Paolo).
>>    - Add comments for 'cannot_instantiate_with_device_add_yet' (Markus).
>>    - Split adding the -device iommu support and removing the iommu machine property (Michael).
>>    - Use pci_setup_iommu as before (Peter)
>>    - Mark intel-iommu as not hot-pluggable (Peter)
>>    - Rebased on master
>>
>> 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
>>
>>
>> (*) 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.
>>
>>
>> Thanks,
>> Marcel
>>
>> Marcel Apfelbaum (4):
>>    hw/pci: delay bus_master_enable_region initialization
>>    q35: allow dynamic sysbus
>>    hw/iommu: enable iommu with -device
>>    machine: remove iommu property
>>
>>   hw/core/machine.c                   | 20 ------------------
>>   hw/i386/intel_iommu.c               | 16 +++++++++++++++
>>   hw/i386/pc_q35.c                    |  2 +-
>>   hw/pci-bridge/pci_expander_bridge.c |  2 ++
>>   hw/pci-host/piix.c                  |  2 ++
>>   hw/pci-host/q35.c                   | 31 +++-------------------------
>>   hw/pci/pci.c                        | 41 +++++++++++++++++++++++++++++--------
>>   include/hw/pci-host/q35.h           |  1 -
>>   include/hw/pci/pci_bus.h            |  2 ++
>>   include/sysemu/sysemu.h             |  1 +
>>   qemu-options.hx                     |  3 ---
>>   vl.c                                |  5 +++++
>>   12 files changed, 64 insertions(+), 62 deletions(-)
>>
>> --
>> 2.4.3