[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [V6 4/4] hw/pci-host: Emulate AMD IOMMU

From: Marcel Apfelbaum
Subject: Re: [Qemu-devel] [V6 4/4] hw/pci-host: Emulate AMD IOMMU
Date: Thu, 3 Mar 2016 14:06:29 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0

On 03/03/2016 02:02 PM, Marcel Apfelbaum wrote:
On 03/03/2016 01:47 PM, David Kiarie wrote:
On Thu, Mar 3, 2016 at 12:49 PM, Michael S. Tsirkin <address@hidden> wrote:
On Thu, Mar 03, 2016 at 01:04:31AM +0300, David Kiarie wrote:
On Thu, Mar 3, 2016 at 12:17 AM, Michael S. Tsirkin <address@hidden> wrote:
On Thu, Mar 03, 2016 at 12:09:28AM +0300, David Kiarie wrote:

On 22/02/16 14:22, Marcel Apfelbaum wrote:
On 02/21/2016 08:11 PM, David Kiarie wrote:
Add AMD IOMMU emulation support to q35 chipset

Signed-off-by: David Kiarie <address@hidden>
  hw/pci-host/piix.c            |  1 +
  hw/pci-host/q35.c             | 14 ++++++++++++--
  include/hw/i386/intel_iommu.h |  1 +
  3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 41aa66f..ab2e24a 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -36,6 +36,7 @@
  #include "hw/i386/ioapic.h"
  #include "qapi/visitor.h"
  #include "qemu/error-report.h"
+#include "hw/i386/amd_iommu.h"


I think you don't need this include anymore.

   * I440FX chipset data sheet.
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 115fb8c..355fb32 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -31,6 +31,7 @@
  #include "hw/hw.h"
  #include "hw/pci-host/q35.h"
  #include "qapi/visitor.h"
+#include "hw/i386/amd_iommu.h"

   * Q35 host
@@ -505,9 +506,18 @@ 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)) {
+    if (g_strcmp0(MACHINE(qdev_get_machine())->iommu, INTEL_IOMMU_STR)
== 0) {
+        /* Intel IOMMU (VT-d) */
+    } else if (g_strcmp0(MACHINE(qdev_get_machine())->iommu,
+               == 0) {
+        AMDIOMMUState *iommu_state;
+        PCIDevice *iommu;
+        PCIBus *bus = PCI_BUS(qdev_get_parent_bus(DEVICE(mch)));
+        iommu = pci_create_simple(bus, 0x20, TYPE_AMD_IOMMU_DEVICE);

Pls don't hardcode paths like this. Set addr property instead.

+        iommu_state = AMD_IOMMU_DEVICE(iommu);
+        pci_setup_iommu(bus, bridge_host_amd_iommu, iommu_state);

pci_setup_iommu third parameter is void*, so you don't need to cast to
before passing it.

This include is necessary for the definition of "AMD_IOMMU_STR" either way
so am leaving this as is.

This option parsing is just too ugly.

Looks like it was a mistake to support the iommu
machine property, but I see no reason to add to the
existing mess.

Can't users create iommu with -device amd-iommu ?

You mean getting rid of the above code and starting device with
'-device amd-iommu' ? This way am not able to setup IOMMU regions for
devices correctly. IIRC 'pci_setup_iommu' when called from IOMMU code
sets up IOMMU region for IOMMU only. Calling this from the bus sets up
IOMMU regions for all devices though.

I need to setup IOMMU regions for all devices from the bus, to be able
to do that I need to have created the IOMMU device first.

I agree it's unfortunate, but I don't think internal limitations
should affect the user interface like this.

I wish we had a way to specify initialization order for devices in some
way, such that system devices are created before others.

For now, can you call pci_setup_iommu in the machine done callback?

It's necessary if we are to support multiple IOMMUs, anyway.

If this is mainly to allow users run multiple IOMMUs I think the
greatest limitation is that Qemu only have one PCI bus which can only
host one IOMMU, AFAIK so unless there are huge structural changes
running multiple IOMMU on Qemu may not be possible.

Actually we have good news on this front.
You can use as many pxb-pcie devices as you want to create extra PCI root buses.
Assigning them different IOMMUs would be great. (long term, of course)

Even without pxb-pcie, you can add a pci-bridge to expose a secondary PCI bus
and limit IOMMU scope to that PCI bridge.





diff --git a/include/hw/i386/intel_iommu.h
index b024ffa..539530c 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -27,6 +27,7 @@
  #define TYPE_INTEL_IOMMU_DEVICE "intel-iommu"
  #define INTEL_IOMMU_DEVICE(obj) \
+#define INTEL_IOMMU_STR "intel"

  /* DMAR Hardware Unit Definition address (IOMMU unit) */
  #define Q35_HOST_BRIDGE_IOMMU_ADDR  0xfed90000ULL

reply via email to

[Prev in Thread] Current Thread [Next in Thread]