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 <m...@redhat.com> 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 <davidkiar...@gmail.com> > >> >>--- > >> >> 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. > > > >> > > >> >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 > >> >> > >> >