From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57500) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abRxQ-0004Zq-6X for qemu-devel@nongnu.org; Thu, 03 Mar 2016 07:02:22 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1abRxK-0003RS-W7 for qemu-devel@nongnu.org; Thu, 03 Mar 2016 07:02:16 -0500 Received: from mx1.redhat.com ([209.132.183.28]:33860) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abRxK-0003RF-P1 for qemu-devel@nongnu.org; Thu, 03 Mar 2016 07:02:10 -0500 References: <1456078260-6669-1-git-send-email-davidkiarie4@gmail.com> <1456078260-6669-5-git-send-email-davidkiarie4@gmail.com> <56CAEF63.80007@redhat.com> <56D75688.1020500@gmail.com> <20160302231226-mutt-send-email-mst@redhat.com> <20160303114709-mutt-send-email-mst@redhat.com> From: Marcel Apfelbaum Message-ID: <56D827BF.10501@redhat.com> Date: Thu, 3 Mar 2016 14:02:07 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [V6 4/4] hw/pci-host: Emulate AMD IOMMU List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Kiarie , "Michael S. Tsirkin" Cc: Valentine Sinitsyn , Jan Kiszka , QEMU Developers On 03/03/2016 01:47 PM, David Kiarie wrote: > On Thu, Mar 3, 2016 at 12:49 PM, Michael S. Tsirkin 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 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 >>>>>>> --- >>>>>>> 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 >>>>>>> >>>>>>