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

[Marcel Apfelbaum]

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

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
> anything?

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 
> > > (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.

right


Thanks,
Marcel

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