From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52913) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aoQs7-0005lh-3Y for qemu-devel@nongnu.org; Fri, 08 Apr 2016 03:30:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aoQs3-00069i-TM for qemu-devel@nongnu.org; Fri, 08 Apr 2016 03:30:27 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38090) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aoQs3-00069d-LO for qemu-devel@nongnu.org; Fri, 08 Apr 2016 03:30:23 -0400 Date: Fri, 8 Apr 2016 15:30:16 +0800 From: Peter Xu Message-ID: <20160408073016.GA26923@pxdev.xzpeter.org> References: <1455852618-5224-1-git-send-email-peterx@redhat.com> <1455852618-5224-2-git-send-email-peterx@redhat.com> <56C993AE.7010703@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <56C993AE.7010703@gmail.com> Subject: Re: [Qemu-devel] [PATCH 01/13] q35: add "int-remap" flag to enable intr List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: marcel@redhat.com Cc: qemu-devel@nongnu.org, ehabkost@redhat.com, mst@redhat.com, jasowang@redhat.com, pbonzini@redhat.com, imammedo@redhat.com, rth@twiddle.net, David Kiarie It's a long time from previous post... However I will start to pick this up. Several questions on the comments... On Sun, Feb 21, 2016 at 12:38:38PM +0200, Marcel Apfelbaum wrote: > 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. I had a look on the AMD series. I see that x-iommu-type is introduced to specify AMD IOMMUs. So maybe I should keep this interface unchanged for now? > > 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. I suppose "ir" is a little bit hard for people to think about interrupt remaping, and "interrupt-remapping" might be too long. If you would not mind, I'd like to use "intr" in next version. [...] > >+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. Do you mean that "both implementation we have are for the machines" rather than PC machines? It seems that both of us are modifying machine_initfn() rather than pc_machine_initfn()? Am I missing anything? > > >+ > >+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. Will keep that in mind. Thanks. However, we do not need to specify the type here, right? Since we can specify the type of IOMMU via x-iommu-type, and the type of IR will always follow the type of IOMMU. > > > 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. Will do. Thanks! -- peterx