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?

Looking at this, machine_done seems to be happening too late. It seems
IOMMU regions are setup by 'pcidev->realize'( at which time you need
to have supplied the bus-IOMMU-regions-setup callback).
pci_iommu_setup is the function that sets up the
bus-IOMMU-regions-setup callback. There could be some other way am
missing but the problem that comes to my head is that if we are not
guaranteed the IOMMU device will be created first. we could still have
problems with some devices having IOMMU regions while others don't.

>
>> >
>> > It's necessary if we are to support multiple IOMMUs, anyway.
>> >
>> >> >
>> >> >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
>> >> >>
>> >> >