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

[Peter Xu <peterx@redhat.com>]

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