From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59224) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abS1e-00073h-9G for qemu-devel@nongnu.org; Thu, 03 Mar 2016 07:06:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1abS1Z-0004PN-4N for qemu-devel@nongnu.org; Thu, 03 Mar 2016 07:06:38 -0500 Received: from mx1.redhat.com ([209.132.183.28]:3274) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abS1Y-0004P5-Tq for qemu-devel@nongnu.org; Thu, 03 Mar 2016 07:06:33 -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> <56D827BF.10501@redhat.com> From: Marcel Apfelbaum Message-ID: <56D828C5.4030803@redhat.com> Date: Thu, 3 Mar 2016 14:06:29 +0200 MIME-Version: 1.0 In-Reply-To: <56D827BF.10501@redhat.com> 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 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 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) > 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 >>>>>>>> >>>>>>> >