| Message ID | 1455852618-5224-2-git-send-email-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
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, >
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. > > 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. Hi, Marcel, Really appreciated for all of your review comments for the series! As I worked on this without noticing that Jan/Rita is working on this too, I may need to halt here and wait for her patches. So please ignore this patchset for now, and let's all wait for Rita's first version. Sorry for wasting your time on this! Thanks! Peter
On 02/23/2016 05:48 AM, Peter Xu wrote: > 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. >> >> 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. > > Hi, Marcel, > > Really appreciated for all of your review comments for the series! > > As I worked on this without noticing that Jan/Rita is working on > this too, I may need to halt here and wait for her patches. So > please ignore this patchset for now, and let's all wait for Rita's > first version. > Hi Peter, > Sorry for wasting your time on this! No problem, really, it happens :) Now you can much easier review their work and improve it as you see fit. Good luck, Marcel > > Thanks! > Peter >
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
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 >
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; +} + +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); 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)); } } 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,
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 To be more clear, the following command: $ qemu-system-x86_64 -M q35,iommu=on Will enable Intel IOMMU only, without interrupt remapping support. 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(-)