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

[David Kiarie]

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"
>> >> >
>> >> >Hi,
>> >> >
>> >> >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) */
>> >> >>          mch_init_dmar(mch);
>> >> >>+    } else if (g_strcmp0(MACHINE(qdev_get_machine())->iommu,
>> >> >>AMD_IOMMU_STR)
>> >> >>+               == 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
>> >> >AMDIOMMUState
>> >> >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.

>> >
>> >> >
>> >> >Thanks,
>> >> >Marcel
>> >> >
>> >> >>      }
>> >> >>  }
>> >> >>
>> >> >>diff --git a/include/hw/i386/intel_iommu.h
>> >> >>b/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) \
>> >> >>       OBJECT_CHECK(IntelIOMMUState, (obj), TYPE_INTEL_IOMMU_DEVICE)
>> >> >>+#define INTEL_IOMMU_STR "intel"
>> >> >>
>> >> >>  /* DMAR Hardware Unit Definition address (IOMMU unit) */
>> >> >>  #define Q35_HOST_BRIDGE_IOMMU_ADDR  0xfed90000ULL
>> >> >>
>> >> >