From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37768) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aXRPc-0004QP-Hl for qemu-devel@nongnu.org; Sun, 21 Feb 2016 05:38:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aXRPX-0003mQ-Hd for qemu-devel@nongnu.org; Sun, 21 Feb 2016 05:38:48 -0500 Received: from mail-wm0-x22f.google.com ([2a00:1450:400c:c09::22f]:36972) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aXRPX-0003mL-7Z for qemu-devel@nongnu.org; Sun, 21 Feb 2016 05:38:43 -0500 Received: by mail-wm0-x22f.google.com with SMTP id g62so126513519wme.0 for ; Sun, 21 Feb 2016 02:38:42 -0800 (PST) References: <1455852618-5224-1-git-send-email-peterx@redhat.com> <1455852618-5224-2-git-send-email-peterx@redhat.com> From: Marcel Apfelbaum Message-ID: <56C993AE.7010703@gmail.com> Date: Sun, 21 Feb 2016 12:38:38 +0200 MIME-Version: 1.0 In-Reply-To: <1455852618-5224-2-git-send-email-peterx@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 01/13] q35: add "int-remap" flag to enable intr Reply-To: marcel@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Xu , qemu-devel@nongnu.org Cc: ehabkost@redhat.com, mst@redhat.com, jasowang@redhat.com, imammedo@redhat.com, pbonzini@redhat.com, David Kiarie , rth@twiddle.net On 02/19/2016 05:30 AM, Peter Xu wrote: > One flag is added to specify whether to enable INTR for emulated > IOMMU. By default, interrupt remapping is not supportted. To enable it, > we should specify something like: > > $ qemu-system-x86_64 -M q35,iommu=on,int_remap=on Hi Peter, Please be aware that there is an AMD IOMMU emulation series on the list, so now we will have iommu=intel/amd/off. As far as I know we prefer int-remap instead of int_remap and also the "int" prefix reminds me integer, I would use ir=on or the clean interrupt-remapping=on. > > To be more clear, the following command: > > $ qemu-system-x86_64 -M q35,iommu=on > > Will enable Intel IOMMU only, without interrupt remapping support. Since AMD IOMMU also supports interrupt remapping please sync with the other project. > > Signed-off-by: Peter Xu > --- > hw/core/machine.c | 20 ++++++++++++++++++++ > hw/pci-host/q35.c | 6 ++++-- > include/hw/boards.h | 1 + > include/hw/i386/intel_iommu.h | 3 +++ > 4 files changed, 28 insertions(+), 2 deletions(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index 6d1a0d8..852cee2 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -298,6 +298,20 @@ static void machine_set_iommu(Object *obj, bool value, Error **errp) > ms->iommu = value; > } > > +static bool machine_get_int_remap(Object *obj, Error **errp) > +{ > + MachineState *ms = MACHINE(obj); > + > + return ms->int_remap; > +} I am starting to wonder if "iommu" and now "interrupt-remapping" should be part of machine and not the pc machine. Both implementations we have are only for the PC machines. > + > +static void machine_set_int_remap(Object *obj, bool value, Error **errp) > +{ > + MachineState *ms = MACHINE(obj); > + > + ms->int_remap = value; > +} > + > static void machine_set_suppress_vmdesc(Object *obj, bool value, Error **errp) > { > MachineState *ms = MACHINE(obj); > @@ -461,6 +475,12 @@ static void machine_initfn(Object *obj) > object_property_set_description(obj, "iommu", > "Set on/off to enable/disable Intel IOMMU (VT-d)", > NULL); > + object_property_add_bool(obj, "int-remap", > + machine_get_int_remap, > + machine_set_int_remap, NULL); > + object_property_set_description(obj, "int-remap", > + "Set on/off to enable/disable Intel IOMMU INTR", > + NULL); Here the same, I would rename int-remap to interrupt-remapping and keep in mind that would not be for Intel IOMMU only. > object_property_add_bool(obj, "suppress-vmdesc", > machine_get_suppress_vmdesc, > machine_set_suppress_vmdesc, NULL); > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c > index 115fb8c..7cd4d87 100644 > --- a/hw/pci-host/q35.c > +++ b/hw/pci-host/q35.c > @@ -434,13 +434,14 @@ static AddressSpace *q35_host_dma_iommu(PCIBus *bus, void *opaque, int devfn) > return &vtd_as->as; > } > > -static void mch_init_dmar(MCHPCIState *mch) > +static void mch_init_dmar(MCHPCIState *mch, bool intr_supported) > { > PCIBus *pci_bus = PCI_BUS(qdev_get_parent_bus(DEVICE(mch))); > > mch->iommu = INTEL_IOMMU_DEVICE(qdev_create(NULL, TYPE_INTEL_IOMMU_DEVICE)); > object_property_add_child(OBJECT(mch), "intel-iommu", > OBJECT(mch->iommu), NULL); > + mch->iommu->intr_supported = intr_supported; > qdev_init_nofail(DEVICE(mch->iommu)); > sysbus_mmio_map(SYS_BUS_DEVICE(mch->iommu), 0, Q35_HOST_BRIDGE_IOMMU_ADDR); > > @@ -507,7 +508,8 @@ static void mch_realize(PCIDevice *d, Error **errp) > } > /* Intel IOMMU (VT-d) */ > if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) { > - mch_init_dmar(mch); > + mch_init_dmar(mch, object_property_get_bool(qdev_get_machine(), > + "int-remap", false)); Here you can directly call qdev_get_machine()->interrupt-remapping instead of object_property_get_bool. Thanks, Marcel > } > } > > diff --git a/include/hw/boards.h b/include/hw/boards.h > index 0f30959..d488e40 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -127,6 +127,7 @@ struct MachineState { > bool igd_gfx_passthru; > char *firmware; > bool iommu; > + bool int_remap; > bool suppress_vmdesc; > > ram_addr_t ram_size; > diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h > index b024ffa..6e52c6b 100644 > --- a/include/hw/i386/intel_iommu.h > +++ b/include/hw/i386/intel_iommu.h > @@ -123,6 +123,9 @@ struct IntelIOMMUState { > MemoryRegionIOMMUOps iommu_ops; > GHashTable *vtd_as_by_busptr; /* VTDBus objects indexed by PCIBus* reference */ > VTDBus *vtd_as_by_bus_num[VTD_PCI_BUS_MAX]; /* VTDBus objects indexed by bus number */ > + > + /* interrupt remapping */ > + bool intr_supported; /* Whether IR is supported */ > }; > > /* Find the VTD Address space associated with the given bus pointer, >