qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Marcel Apfelbaum
Subject: Re: [Qemu-devel] [PATCH v2 2/3] hw/iommu: enable iommu with -device
Date: Sun, 5 Jun 2016 13:21:20 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0 
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 <address@hidden>




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).
address@hidden 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
reply via email to
[Prev in Thread] Current Thread [Next in Thread]