[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 01/13] q35: add "int-remap" flag to enable intr

From: Marcel Apfelbaum
Subject: Re: [Qemu-devel] [PATCH 01/13] q35: add "int-remap" flag to enable intr
Date: Mon, 11 Apr 2016 13:07:03 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0

On 04/08/2016 10:30 AM, Peter Xu wrote:
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.

Hi Peter,

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?

I agree

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 

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.

I would go with "interrupt-remapping", but is only me. If "intr"
is OK for the others developers, I would be OK with it.


+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

I meant that IOMMU implementation should be in PC Machines, and not in the base 
Machine class
because the only implementations we have are for x86. Not all architectures 
support IOMMU.
But it is out of the scope of this series, I think. It is only a thought :)

+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 
+    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 
+                                    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_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, 
      object_property_add_child(OBJECT(mch), "intel-iommu",
                                OBJECT(mch->iommu), NULL);
+    mch->iommu->intr_supported = intr_supported;
      sysbus_mmio_map(SYS_BUS_DEVICE(mch->iommu), 0, 

@@ -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 

Will do.


-- peterx

reply via email to

[Prev in Thread] Current Thread [Next in Thread]