On Thu, Mar 3, 2016 at 3:06 PM, Marcel Apfelbaum <mar...@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 <m...@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 <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. >>> >>> >>> 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 >>>>>>>>> >>>>>>>> >> >