| Message ID | 1456078260-6669-5-git-send-email-davidkiarie4@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/21/2016 08:11 PM, David Kiarie wrote: > Add AMD IOMMU emulation support to q35 chipset > > Signed-off-by: David Kiarie <davidkiarie4@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); > + 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. 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 >
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 <davidkiarie4@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 ? 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 > >> > >
On Thu, Mar 3, 2016 at 12:17 AM, Michael S. Tsirkin <mst@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 <davidkiarie4@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. > > 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 >> >> >> >
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 <mst@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 <davidkiarie4@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 > >> >> > >> >
On Thu, Mar 3, 2016 at 12:49 PM, Michael S. Tsirkin <mst@redhat.com> 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 <mst@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 <davidkiarie4@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. 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 >> >> >> >> >> >
On 03/03/2016 01:47 PM, David Kiarie wrote: > On Thu, Mar 3, 2016 at 12:49 PM, Michael S. Tsirkin <mst@redhat.com> 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 <mst@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 <davidkiarie4@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. > > 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) Thanks, Marcel > >>>> >>>>>> >>>>>> 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 >>>>>>> >>>>>>
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 <mst@redhat.com> 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 <mst@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 <davidkiarie4@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. >> >> 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. Thanks, Marcel > > Thanks, > Marcel > >> >>>>> >>>>>>> >>>>>>> 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 >>>>>>>> >>>>>>> >
On Thu, Mar 3, 2016 at 3:06 PM, Marcel Apfelbaum <marcel@redhat.com> wrote: > 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 <mst@redhat.com> >>> 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 <mst@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 <davidkiarie4@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. >>> >>> >>> 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. Hmmh, good to know. Will look at that. > > Thanks, > Marcel > > >> >> Thanks, >> Marcel >> >>> >>>>>> >>>>>>>> >>>>>>>> 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 >>>>>>>>> >>>>>>>> >> >
On Thu, Mar 03, 2016 at 03:18:18PM +0300, David Kiarie wrote: > >> 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. > > Hmmh, good to know. Will look at that. OK but there's no rush wrt that. What I chiefly would like is getting command line right so we don't need to maintain legacy command line syntax. Support for many iommus can come later,
On Thu, Mar 3, 2016 at 12:49 PM, Michael S. Tsirkin <mst@redhat.com> 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 <mst@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 <davidkiarie4@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? 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 >> >> >> >> >> >
On Sun, Feb 21, 2016 at 09:11:00PM +0300, David Kiarie wrote: > Add AMD IOMMU emulation support to q35 chipset > > Signed-off-by: David Kiarie <davidkiarie4@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" > > /* > * I440FX chipset data sheet. Why is this needed? > 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); address can be set through a property. > + iommu_state = AMD_IOMMU_DEVICE(iommu); > + pci_setup_iommu(bus, bridge_host_amd_iommu, iommu_state); It would be better to move this chunk to a separate function. > } > } > > 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 > -- > 2.1.4
On Fri, Mar 11, 2016 at 4:22 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Sun, Feb 21, 2016 at 09:11:00PM +0300, David Kiarie wrote: >> Add AMD IOMMU emulation support to q35 chipset >> >> Signed-off-by: David Kiarie <davidkiarie4@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" >> >> /* >> * I440FX chipset data sheet. > > Why is this needed? > >> 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); > > address can be set through a property. I missed something here, what is the problem ?Is it the hardcoded address ? > >> + iommu_state = AMD_IOMMU_DEVICE(iommu); >> + pci_setup_iommu(bus, bridge_host_amd_iommu, iommu_state); > > > It would be better to move this chunk to a separate function. > >> } >> } >> >> 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 >> -- >> 2.1.4
On Sun, Mar 13, 2016 at 03:14:37AM +0300, David Kiarie wrote: > On Fri, Mar 11, 2016 at 4:22 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Sun, Feb 21, 2016 at 09:11:00PM +0300, David Kiarie wrote: > >> Add AMD IOMMU emulation support to q35 chipset > >> > >> Signed-off-by: David Kiarie <davidkiarie4@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" > >> > >> /* > >> * I440FX chipset data sheet. > > > > Why is this needed? > > > >> 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); > > > > address can be set through a property. > > I missed something here, what is the problem ?Is it the hardcoded address ? As long as you use pci_create_simple, it's not a problem. If eventually we manage to switch to using -device, it'll be useful that by setting address property machine can influence address for the device. > > > >> + iommu_state = AMD_IOMMU_DEVICE(iommu); > >> + pci_setup_iommu(bus, bridge_host_amd_iommu, iommu_state); > > > > > > It would be better to move this chunk to a separate function. > > > >> } > >> } > >> > >> 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 > >> -- > >> 2.1.4
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" /* * 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); + iommu_state = AMD_IOMMU_DEVICE(iommu); + pci_setup_iommu(bus, bridge_host_amd_iommu, iommu_state); } } 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
Add AMD IOMMU emulation support to q35 chipset Signed-off-by: David Kiarie <davidkiarie4@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(-)