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

[Qemu-devel] [PATCH v3 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.

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                        | 42 ++++++++++++++++++++++++++++---------
 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(+), 63 deletions(-)

-- 
2.4.3

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

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index bb605ef..c434c2b 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);
 }
 
@@ -845,7 +874,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 +913,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;
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 b0bcc25..65d79df 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 v3 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 v3 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 v3 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 0e42ba5..cda3170 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 in bytes\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 v3 0/4] enable iommu with -device

[Michael S. Tsirkin]

On Mon, Jun 13, 2016 at 05:36:56PM +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.

Looks good. I'm inclined to apply 1-3 initially, see how many
acks will 4 collect.
Could you rebase on my tree pls?

> Finally patch 4 removes the iommu machine property.
> 
> 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                        | 42 ++++++++++++++++++++++++++++---------
>  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(+), 63 deletions(-)
> 
> -- 
> 2.4.3

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

[Marcel Apfelbaum]

On 06/13/2016 11:51 PM, Michael S. Tsirkin wrote:
>
> On Mon, Jun 13, 2016 at 05:36:56PM +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.
>
> Looks good. I'm inclined to apply 1-3 initially, see how many
> acks will 4 collect.
> Could you rebase on my tree pls?
>

Sure, I'll resend it.
Thanks,
Marcel

>> Finally patch 4 removes the iommu machine property.
>>
>> 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                        | 42 ++++++++++++++++++++++++++++---------
>>   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(+), 63 deletions(-)
>>
>> --
>> 2.4.3

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

[Eduardo Habkost]

On Mon, Jun 13, 2016 at 05:37:00PM +0300, Marcel Apfelbaum wrote:
> 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>

On the one hand, the option is present since v2.2.0. On the other
hand, it is present only in q35, and the property really doesn't
belong to TYPE_MACHINE (so even if we decide to keep it for a few
releases, property registration needs to be moved to
pc_q35_machine_options().

What kind of usage the existing code has, currently? Development
and debugging only, or is there any chance somebody might be
using it in production?

-- 
Eduardo

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

[Paolo Bonzini]

On 14/06/2016 20:38, Eduardo Habkost wrote:
> On Mon, Jun 13, 2016 at 05:37:00PM +0300, Marcel Apfelbaum wrote:
>> 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>
> 
> On the one hand, the option is present since v2.2.0. On the other
> hand, it is present only in q35, and the property really doesn't
> belong to TYPE_MACHINE (so even if we decide to keep it for a few
> releases, property registration needs to be moved to
> pc_q35_machine_options().
> 
> What kind of usage the existing code has, currently? Development
> and debugging only, or is there any chance somebody might be
> using it in production?

Definitely development and debugging only.  Someone might be using it in
automated tests, but that's pretty much it.

Paolo

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

[Jan Kiszka]

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

On 2016-06-15 00:03, Paolo Bonzini wrote:
> 
> 
> On 14/06/2016 20:38, Eduardo Habkost wrote:
>> On Mon, Jun 13, 2016 at 05:37:00PM +0300, Marcel Apfelbaum wrote:
>>> 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>
>>
>> On the one hand, the option is present since v2.2.0. On the other
>> hand, it is present only in q35, and the property really doesn't
>> belong to TYPE_MACHINE (so even if we decide to keep it for a few
>> releases, property registration needs to be moved to
>> pc_q35_machine_options().
>>
>> What kind of usage the existing code has, currently? Development
>> and debugging only, or is there any chance somebody might be
>> using it in production?
> 
> Definitely development and debugging only.  Someone might be using it in
> automated tests, but that's pretty much it.

Agreed.

Even we didn't recommended the IOMMU and therefore also this switch to
Jailhouse users trying things out under QEMU (because we also need IR
which isn't mainline yet).

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]