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

[Marcel Apfelbaum <marcel.apfelbaum@gmail.com>]

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 <peterx@redhat.com>
> ---
>   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,
>