| Message ID | 1464898555-14914-3-git-send-email-marcel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 02, 2016 at 11:15:54PM +0300, Marcel Apfelbaum wrote: > Use the standard '-device iommu' instead of '-machine,iommu=on' > to create the IOMMU device. > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> Why can't we keep support for the old flag? > --- > hw/core/machine.c | 20 -------------------- > hw/i386/intel_iommu.c | 17 +++++++++++++++++ > hw/pci-host/q35.c | 28 ---------------------------- > qemu-options.hx | 3 --- > 4 files changed, 17 insertions(+), 51 deletions(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index ccdd5fa..8f94301 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -300,20 +300,6 @@ static void machine_set_firmware(Object *obj, const char *value, Error **errp) > ms->firmware = g_strdup(value); > } > > -static bool machine_get_iommu(Object *obj, Error **errp) > -{ > - MachineState *ms = MACHINE(obj); > - > - return ms->iommu; > -} > - > -static void machine_set_iommu(Object *obj, bool value, Error **errp) > -{ > - MachineState *ms = MACHINE(obj); > - > - ms->iommu = value; > -} > - > static void machine_set_suppress_vmdesc(Object *obj, bool value, Error **errp) > { > MachineState *ms = MACHINE(obj); > @@ -493,12 +479,6 @@ static void machine_initfn(Object *obj) > object_property_set_description(obj, "firmware", > "Firmware image", > NULL); > - object_property_add_bool(obj, "iommu", > - machine_get_iommu, > - machine_set_iommu, NULL); > - object_property_set_description(obj, "iommu", > - "Set on/off to enable/disable Intel IOMMU (VT-d)", > - NULL); > object_property_add_bool(obj, "suppress-vmdesc", > machine_get_suppress_vmdesc, > machine_set_suppress_vmdesc, NULL); > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index 347718f..9af5d6b 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -24,6 +24,8 @@ > #include "exec/address-spaces.h" > #include "intel_iommu_internal.h" > #include "hw/pci/pci.h" > +#include "hw/pci/pci_bus.h" > +#include "hw/i386/pc.h" > > /*#define DEBUG_INTEL_IOMMU*/ > #ifdef DEBUG_INTEL_IOMMU > @@ -2014,8 +2016,20 @@ static void vtd_reset(DeviceState *dev) > vtd_init(s); > } > > +static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn) > +{ > + IntelIOMMUState *s = opaque; > + VTDAddressSpace *vtd_as; > + > + assert(0 <= devfn && devfn <= VTD_PCI_DEVFN_MAX); > + > + vtd_as = vtd_find_add_as(s, bus, devfn); > + return &vtd_as->as; > +} > + > static void vtd_realize(DeviceState *dev, Error **errp) > { > + PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus; > IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev); > > VTD_DPRINTF(GENERAL, ""); > @@ -2029,6 +2043,9 @@ static void vtd_realize(DeviceState *dev, Error **errp) > s->vtd_as_by_busptr = g_hash_table_new_full(vtd_uint64_hash, vtd_uint64_equal, > g_free, g_free); > vtd_init(s); > + sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, Q35_HOST_BRIDGE_IOMMU_ADDR); > + bus->iommu_fn = vtd_host_dma_iommu; > + bus->iommu_opaque = dev; > } > > static void vtd_class_init(ObjectClass *klass, void *data) > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c > index 70f897e..ea684c7 100644 > --- a/hw/pci-host/q35.c > +++ b/hw/pci-host/q35.c > @@ -424,30 +424,6 @@ static void mch_reset(DeviceState *qdev) > mch_update(mch); > } > > -static AddressSpace *q35_host_dma_iommu(PCIBus *bus, void *opaque, int devfn) > -{ > - IntelIOMMUState *s = opaque; > - VTDAddressSpace *vtd_as; > - > - assert(0 <= devfn && devfn <= VTD_PCI_DEVFN_MAX); > - > - vtd_as = vtd_find_add_as(s, bus, devfn); > - return &vtd_as->as; > -} > - > -static void mch_init_dmar(MCHPCIState *mch) > -{ > - 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); > - qdev_init_nofail(DEVICE(mch->iommu)); > - sysbus_mmio_map(SYS_BUS_DEVICE(mch->iommu), 0, Q35_HOST_BRIDGE_IOMMU_ADDR); > - > - pci_setup_iommu(pci_bus, q35_host_dma_iommu, mch->iommu); > -} > - > static void mch_realize(PCIDevice *d, Error **errp) > { > int i; > @@ -506,10 +482,6 @@ static void mch_realize(PCIDevice *d, Error **errp) > mch->pci_address_space, &mch->pam_regions[i+1], > PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE); > } > - /* Intel IOMMU (VT-d) */ > - if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) { > - mch_init_dmar(mch); > - } > } > > uint64_t mch_mcfg_base(void) > diff --git a/qemu-options.hx b/qemu-options.hx > index 6106520..2953baf 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -38,7 +38,6 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \ > " kvm_shadow_mem=size of KVM shadow MMU\n" > " dump-guest-core=on|off include guest memory in a core dump (default=on)\n" > " mem-merge=on|off controls memory merge support (default: on)\n" > - " iommu=on|off controls emulated Intel IOMMU (VT-d) support (default=off)\n" > " igd-passthru=on|off controls IGD GFX passthrough support (default=off)\n" > " aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n" > " dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n" > @@ -73,8 +72,6 @@ Include guest memory in a core dump. The default is on. > Enables or disables memory merge support. This feature, when supported by > the host, de-duplicates identical memory pages among VMs instances > (enabled by default). > -@item iommu=on|off > -Enables or disables emulated Intel IOMMU (VT-d) support. The default is off. > @item aes-key-wrap=on|off > Enables or disables AES key wrapping support on s390-ccw hosts. This feature > controls whether AES wrapping keys will be created to allow > -- > 2.4.3
On 06/03/2016 07:07 PM, Michael S. Tsirkin wrote: > On Thu, Jun 02, 2016 at 11:15:54PM +0300, Marcel Apfelbaum wrote: >> Use the standard '-device iommu' instead of '-machine,iommu=on' >> to create the IOMMU device. >> >> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > Hi Michael, Thank you for the review. > Why can't we keep support for the old flag? > We can, but IMO we don't need it for several reasons: The current vIOMMU before the fantastic work of Aviv and Peter is not really usable, is there only as a "lab" feature with no clear interesting scenario. If we keep it, we should also support the "x-iommu-type" for AMD IOMMU, so we add "legacy" code we don't want. It is easy to add additional options with -device, but how will we add them to -machine,iommu=on? an extra machine option? Finally, if we do have current users, asking them for a minimum command line change is not such a big deal. Thanks, Marcel >> --- >> hw/core/machine.c | 20 -------------------- >> hw/i386/intel_iommu.c | 17 +++++++++++++++++ >> hw/pci-host/q35.c | 28 ---------------------------- >> qemu-options.hx | 3 --- >> 4 files changed, 17 insertions(+), 51 deletions(-) >> >> diff --git a/hw/core/machine.c b/hw/core/machine.c >> index ccdd5fa..8f94301 100644 >> --- a/hw/core/machine.c >> +++ b/hw/core/machine.c >> @@ -300,20 +300,6 @@ static void machine_set_firmware(Object *obj, const char *value, Error **errp) >> ms->firmware = g_strdup(value); >> } >> >> -static bool machine_get_iommu(Object *obj, Error **errp) >> -{ >> - MachineState *ms = MACHINE(obj); >> - >> - return ms->iommu; >> -} >> - >> -static void machine_set_iommu(Object *obj, bool value, Error **errp) >> -{ >> - MachineState *ms = MACHINE(obj); >> - >> - ms->iommu = value; >> -} >> - >> static void machine_set_suppress_vmdesc(Object *obj, bool value, Error **errp) >> { >> MachineState *ms = MACHINE(obj); >> @@ -493,12 +479,6 @@ static void machine_initfn(Object *obj) >> object_property_set_description(obj, "firmware", >> "Firmware image", >> NULL); >> - object_property_add_bool(obj, "iommu", >> - machine_get_iommu, >> - machine_set_iommu, NULL); >> - object_property_set_description(obj, "iommu", >> - "Set on/off to enable/disable Intel IOMMU (VT-d)", >> - NULL); >> object_property_add_bool(obj, "suppress-vmdesc", >> machine_get_suppress_vmdesc, >> machine_set_suppress_vmdesc, NULL); >> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >> index 347718f..9af5d6b 100644 >> --- a/hw/i386/intel_iommu.c >> +++ b/hw/i386/intel_iommu.c >> @@ -24,6 +24,8 @@ >> #include "exec/address-spaces.h" >> #include "intel_iommu_internal.h" >> #include "hw/pci/pci.h" >> +#include "hw/pci/pci_bus.h" >> +#include "hw/i386/pc.h" >> >> /*#define DEBUG_INTEL_IOMMU*/ >> #ifdef DEBUG_INTEL_IOMMU >> @@ -2014,8 +2016,20 @@ static void vtd_reset(DeviceState *dev) >> vtd_init(s); >> } >> >> +static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn) >> +{ >> + IntelIOMMUState *s = opaque; >> + VTDAddressSpace *vtd_as; >> + >> + assert(0 <= devfn && devfn <= VTD_PCI_DEVFN_MAX); >> + >> + vtd_as = vtd_find_add_as(s, bus, devfn); >> + return &vtd_as->as; >> +} >> + >> static void vtd_realize(DeviceState *dev, Error **errp) >> { >> + PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus; >> IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev); >> >> VTD_DPRINTF(GENERAL, ""); >> @@ -2029,6 +2043,9 @@ static void vtd_realize(DeviceState *dev, Error **errp) >> s->vtd_as_by_busptr = g_hash_table_new_full(vtd_uint64_hash, vtd_uint64_equal, >> g_free, g_free); >> vtd_init(s); >> + sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, Q35_HOST_BRIDGE_IOMMU_ADDR); >> + bus->iommu_fn = vtd_host_dma_iommu; >> + bus->iommu_opaque = dev; >> } >> >> static void vtd_class_init(ObjectClass *klass, void *data) >> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c >> index 70f897e..ea684c7 100644 >> --- a/hw/pci-host/q35.c >> +++ b/hw/pci-host/q35.c >> @@ -424,30 +424,6 @@ static void mch_reset(DeviceState *qdev) >> mch_update(mch); >> } >> >> -static AddressSpace *q35_host_dma_iommu(PCIBus *bus, void *opaque, int devfn) >> -{ >> - IntelIOMMUState *s = opaque; >> - VTDAddressSpace *vtd_as; >> - >> - assert(0 <= devfn && devfn <= VTD_PCI_DEVFN_MAX); >> - >> - vtd_as = vtd_find_add_as(s, bus, devfn); >> - return &vtd_as->as; >> -} >> - >> -static void mch_init_dmar(MCHPCIState *mch) >> -{ >> - 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); >> - qdev_init_nofail(DEVICE(mch->iommu)); >> - sysbus_mmio_map(SYS_BUS_DEVICE(mch->iommu), 0, Q35_HOST_BRIDGE_IOMMU_ADDR); >> - >> - pci_setup_iommu(pci_bus, q35_host_dma_iommu, mch->iommu); >> -} >> - >> static void mch_realize(PCIDevice *d, Error **errp) >> { >> int i; >> @@ -506,10 +482,6 @@ static void mch_realize(PCIDevice *d, Error **errp) >> mch->pci_address_space, &mch->pam_regions[i+1], >> PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE); >> } >> - /* Intel IOMMU (VT-d) */ >> - if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) { >> - mch_init_dmar(mch); >> - } >> } >> >> uint64_t mch_mcfg_base(void) >> diff --git a/qemu-options.hx b/qemu-options.hx >> index 6106520..2953baf 100644 >> --- a/qemu-options.hx >> +++ b/qemu-options.hx >> @@ -38,7 +38,6 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \ >> " kvm_shadow_mem=size of KVM shadow MMU\n" >> " dump-guest-core=on|off include guest memory in a core dump (default=on)\n" >> " mem-merge=on|off controls memory merge support (default: on)\n" >> - " iommu=on|off controls emulated Intel IOMMU (VT-d) support (default=off)\n" >> " igd-passthru=on|off controls IGD GFX passthrough support (default=off)\n" >> " aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n" >> " dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n" >> @@ -73,8 +72,6 @@ Include guest memory in a core dump. The default is on. >> Enables or disables memory merge support. This feature, when supported by >> the host, de-duplicates identical memory pages among VMs instances >> (enabled by default). >> -@item iommu=on|off >> -Enables or disables emulated Intel IOMMU (VT-d) support. The default is off. >> @item aes-key-wrap=on|off >> Enables or disables AES key wrapping support on s390-ccw hosts. This feature >> controls whether AES wrapping keys will be created to allow >> -- >> 2.4.3
On Sun, Jun 05, 2016 at 11:46:13AM +0300, Marcel Apfelbaum wrote: > On 06/03/2016 07:07 PM, Michael S. Tsirkin wrote: > >On Thu, Jun 02, 2016 at 11:15:54PM +0300, Marcel Apfelbaum wrote: > >>Use the standard '-device iommu' instead of '-machine,iommu=on' > >>to create the IOMMU device. > >> > >>Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > > > > Hi Michael, > Thank you for the review. > > >Why can't we keep support for the old flag? > > > > We can, but IMO we don't need it for several reasons: > > The current vIOMMU before the fantastic work of Aviv and Peter > is not really usable, is there only as a "lab" feature with no > clear interesting scenario. > > If we keep it, we should also support the "x-iommu-type" for > AMD IOMMU, so we add "legacy" code we don't want. > It is easy to add additional options with -device, > but how will we add them to -machine,iommu=on? an extra machine option? > > Finally, if we do have current users, asking them for a minimum command line > change is not such a big deal. > > Thanks, > Marcel Could you separate -device support from dropping the iommu flag? iommu flag would keep meaning intel with no options for compatibility. > >>--- > >> hw/core/machine.c | 20 -------------------- > >> hw/i386/intel_iommu.c | 17 +++++++++++++++++ > >> hw/pci-host/q35.c | 28 ---------------------------- > >> qemu-options.hx | 3 --- > >> 4 files changed, 17 insertions(+), 51 deletions(-) > >> > >>diff --git a/hw/core/machine.c b/hw/core/machine.c > >>index ccdd5fa..8f94301 100644 > >>--- a/hw/core/machine.c > >>+++ b/hw/core/machine.c > >>@@ -300,20 +300,6 @@ static void machine_set_firmware(Object *obj, const char *value, Error **errp) > >> ms->firmware = g_strdup(value); > >> } > >> > >>-static bool machine_get_iommu(Object *obj, Error **errp) > >>-{ > >>- MachineState *ms = MACHINE(obj); > >>- > >>- return ms->iommu; > >>-} > >>- > >>-static void machine_set_iommu(Object *obj, bool value, Error **errp) > >>-{ > >>- MachineState *ms = MACHINE(obj); > >>- > >>- ms->iommu = value; > >>-} > >>- > >> static void machine_set_suppress_vmdesc(Object *obj, bool value, Error **errp) > >> { > >> MachineState *ms = MACHINE(obj); > >>@@ -493,12 +479,6 @@ static void machine_initfn(Object *obj) > >> object_property_set_description(obj, "firmware", > >> "Firmware image", > >> NULL); > >>- object_property_add_bool(obj, "iommu", > >>- machine_get_iommu, > >>- machine_set_iommu, NULL); > >>- object_property_set_description(obj, "iommu", > >>- "Set on/off to enable/disable Intel IOMMU (VT-d)", > >>- NULL); > >> object_property_add_bool(obj, "suppress-vmdesc", > >> machine_get_suppress_vmdesc, > >> machine_set_suppress_vmdesc, NULL); > >>diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > >>index 347718f..9af5d6b 100644 > >>--- a/hw/i386/intel_iommu.c > >>+++ b/hw/i386/intel_iommu.c > >>@@ -24,6 +24,8 @@ > >> #include "exec/address-spaces.h" > >> #include "intel_iommu_internal.h" > >> #include "hw/pci/pci.h" > >>+#include "hw/pci/pci_bus.h" > >>+#include "hw/i386/pc.h" > >> > >> /*#define DEBUG_INTEL_IOMMU*/ > >> #ifdef DEBUG_INTEL_IOMMU > >>@@ -2014,8 +2016,20 @@ static void vtd_reset(DeviceState *dev) > >> vtd_init(s); > >> } > >> > >>+static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn) > >>+{ > >>+ IntelIOMMUState *s = opaque; > >>+ VTDAddressSpace *vtd_as; > >>+ > >>+ assert(0 <= devfn && devfn <= VTD_PCI_DEVFN_MAX); > >>+ > >>+ vtd_as = vtd_find_add_as(s, bus, devfn); > >>+ return &vtd_as->as; > >>+} > >>+ > >> static void vtd_realize(DeviceState *dev, Error **errp) > >> { > >>+ PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus; > >> IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev); > >> > >> VTD_DPRINTF(GENERAL, ""); > >>@@ -2029,6 +2043,9 @@ static void vtd_realize(DeviceState *dev, Error **errp) > >> s->vtd_as_by_busptr = g_hash_table_new_full(vtd_uint64_hash, vtd_uint64_equal, > >> g_free, g_free); > >> vtd_init(s); > >>+ sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, Q35_HOST_BRIDGE_IOMMU_ADDR); > >>+ bus->iommu_fn = vtd_host_dma_iommu; > >>+ bus->iommu_opaque = dev; > >> } > >> > >> static void vtd_class_init(ObjectClass *klass, void *data) > >>diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c > >>index 70f897e..ea684c7 100644 > >>--- a/hw/pci-host/q35.c > >>+++ b/hw/pci-host/q35.c > >>@@ -424,30 +424,6 @@ static void mch_reset(DeviceState *qdev) > >> mch_update(mch); > >> } > >> > >>-static AddressSpace *q35_host_dma_iommu(PCIBus *bus, void *opaque, int devfn) > >>-{ > >>- IntelIOMMUState *s = opaque; > >>- VTDAddressSpace *vtd_as; > >>- > >>- assert(0 <= devfn && devfn <= VTD_PCI_DEVFN_MAX); > >>- > >>- vtd_as = vtd_find_add_as(s, bus, devfn); > >>- return &vtd_as->as; > >>-} > >>- > >>-static void mch_init_dmar(MCHPCIState *mch) > >>-{ > >>- 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); > >>- qdev_init_nofail(DEVICE(mch->iommu)); > >>- sysbus_mmio_map(SYS_BUS_DEVICE(mch->iommu), 0, Q35_HOST_BRIDGE_IOMMU_ADDR); > >>- > >>- pci_setup_iommu(pci_bus, q35_host_dma_iommu, mch->iommu); > >>-} > >>- > >> static void mch_realize(PCIDevice *d, Error **errp) > >> { > >> int i; > >>@@ -506,10 +482,6 @@ static void mch_realize(PCIDevice *d, Error **errp) > >> mch->pci_address_space, &mch->pam_regions[i+1], > >> PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE); > >> } > >>- /* Intel IOMMU (VT-d) */ > >>- if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) { > >>- mch_init_dmar(mch); > >>- } > >> } > >> > >> uint64_t mch_mcfg_base(void) > >>diff --git a/qemu-options.hx b/qemu-options.hx > >>index 6106520..2953baf 100644 > >>--- a/qemu-options.hx > >>+++ b/qemu-options.hx > >>@@ -38,7 +38,6 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \ > >> " kvm_shadow_mem=size of KVM shadow MMU\n" > >> " dump-guest-core=on|off include guest memory in a core dump (default=on)\n" > >> " mem-merge=on|off controls memory merge support (default: on)\n" > >>- " iommu=on|off controls emulated Intel IOMMU (VT-d) support (default=off)\n" > >> " igd-passthru=on|off controls IGD GFX passthrough support (default=off)\n" > >> " aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n" > >> " dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n" > >>@@ -73,8 +72,6 @@ Include guest memory in a core dump. The default is on. > >> Enables or disables memory merge support. This feature, when supported by > >> the host, de-duplicates identical memory pages among VMs instances > >> (enabled by default). > >>-@item iommu=on|off > >>-Enables or disables emulated Intel IOMMU (VT-d) support. The default is off. > >> @item aes-key-wrap=on|off > >> Enables or disables AES key wrapping support on s390-ccw hosts. This feature > >> controls whether AES wrapping keys will be created to allow > >>-- > >>2.4.3
On 06/05/2016 12:59 PM, Michael S. Tsirkin wrote: > On Sun, Jun 05, 2016 at 11:46:13AM +0300, Marcel Apfelbaum wrote: >> On 06/03/2016 07:07 PM, Michael S. Tsirkin wrote: >>> On Thu, Jun 02, 2016 at 11:15:54PM +0300, Marcel Apfelbaum wrote: >>>> Use the standard '-device iommu' instead of '-machine,iommu=on' >>>> to create the IOMMU device. >>>> >>>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> >>> >> >> Hi Michael, >> Thank you for the review. >> >>> Why can't we keep support for the old flag? >>> >> >> We can, but IMO we don't need it for several reasons: >> >> The current vIOMMU before the fantastic work of Aviv and Peter >> is not really usable, is there only as a "lab" feature with no >> clear interesting scenario. >> >> If we keep it, we should also support the "x-iommu-type" for >> AMD IOMMU, so we add "legacy" code we don't want. >> It is easy to add additional options with -device, >> but how will we add them to -machine,iommu=on? an extra machine option? >> >> Finally, if we do have current users, asking them for a minimum command line >> change is not such a big deal. >> >> Thanks, >> Marcel > > Could you separate -device support from dropping the iommu flag? > iommu flag would keep meaning intel with no options for compatibility. > Yes, is possible. But are you sure the compatibility worth having an iommu machine option supporting only Intel IOMMU (without AMD) with no options? Anyway, I will split this patch in two, first one allowing iommu creation in both ways, the other one removing the iommu=on option. We can decide what later if we want the legacy part or not. Thanks, Marcel >>>> --- >>>> hw/core/machine.c | 20 -------------------- >>>> hw/i386/intel_iommu.c | 17 +++++++++++++++++ >>>> hw/pci-host/q35.c | 28 ---------------------------- >>>> qemu-options.hx | 3 --- >>>> 4 files changed, 17 insertions(+), 51 deletions(-) >>>> >>>> diff --git a/hw/core/machine.c b/hw/core/machine.c >>>> index ccdd5fa..8f94301 100644 >>>> --- a/hw/core/machine.c >>>> +++ b/hw/core/machine.c >>>> @@ -300,20 +300,6 @@ static void machine_set_firmware(Object *obj, const char *value, Error **errp) >>>> ms->firmware = g_strdup(value); >>>> } >>>> >>>> -static bool machine_get_iommu(Object *obj, Error **errp) >>>> -{ >>>> - MachineState *ms = MACHINE(obj); >>>> - >>>> - return ms->iommu; >>>> -} >>>> - >>>> -static void machine_set_iommu(Object *obj, bool value, Error **errp) >>>> -{ >>>> - MachineState *ms = MACHINE(obj); >>>> - >>>> - ms->iommu = value; >>>> -} >>>> - >>>> static void machine_set_suppress_vmdesc(Object *obj, bool value, Error **errp) >>>> { >>>> MachineState *ms = MACHINE(obj); >>>> @@ -493,12 +479,6 @@ static void machine_initfn(Object *obj) >>>> object_property_set_description(obj, "firmware", >>>> "Firmware image", >>>> NULL); >>>> - object_property_add_bool(obj, "iommu", >>>> - machine_get_iommu, >>>> - machine_set_iommu, NULL); >>>> - object_property_set_description(obj, "iommu", >>>> - "Set on/off to enable/disable Intel IOMMU (VT-d)", >>>> - NULL); >>>> object_property_add_bool(obj, "suppress-vmdesc", >>>> machine_get_suppress_vmdesc, >>>> machine_set_suppress_vmdesc, NULL); >>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >>>> index 347718f..9af5d6b 100644 >>>> --- a/hw/i386/intel_iommu.c >>>> +++ b/hw/i386/intel_iommu.c >>>> @@ -24,6 +24,8 @@ >>>> #include "exec/address-spaces.h" >>>> #include "intel_iommu_internal.h" >>>> #include "hw/pci/pci.h" >>>> +#include "hw/pci/pci_bus.h" >>>> +#include "hw/i386/pc.h" >>>> >>>> /*#define DEBUG_INTEL_IOMMU*/ >>>> #ifdef DEBUG_INTEL_IOMMU >>>> @@ -2014,8 +2016,20 @@ static void vtd_reset(DeviceState *dev) >>>> vtd_init(s); >>>> } >>>> >>>> +static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn) >>>> +{ >>>> + IntelIOMMUState *s = opaque; >>>> + VTDAddressSpace *vtd_as; >>>> + >>>> + assert(0 <= devfn && devfn <= VTD_PCI_DEVFN_MAX); >>>> + >>>> + vtd_as = vtd_find_add_as(s, bus, devfn); >>>> + return &vtd_as->as; >>>> +} >>>> + >>>> static void vtd_realize(DeviceState *dev, Error **errp) >>>> { >>>> + PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus; >>>> IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev); >>>> >>>> VTD_DPRINTF(GENERAL, ""); >>>> @@ -2029,6 +2043,9 @@ static void vtd_realize(DeviceState *dev, Error **errp) >>>> s->vtd_as_by_busptr = g_hash_table_new_full(vtd_uint64_hash, vtd_uint64_equal, >>>> g_free, g_free); >>>> vtd_init(s); >>>> + sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, Q35_HOST_BRIDGE_IOMMU_ADDR); >>>> + bus->iommu_fn = vtd_host_dma_iommu; >>>> + bus->iommu_opaque = dev; >>>> } >>>> >>>> static void vtd_class_init(ObjectClass *klass, void *data) >>>> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c >>>> index 70f897e..ea684c7 100644 >>>> --- a/hw/pci-host/q35.c >>>> +++ b/hw/pci-host/q35.c >>>> @@ -424,30 +424,6 @@ static void mch_reset(DeviceState *qdev) >>>> mch_update(mch); >>>> } >>>> >>>> -static AddressSpace *q35_host_dma_iommu(PCIBus *bus, void *opaque, int devfn) >>>> -{ >>>> - IntelIOMMUState *s = opaque; >>>> - VTDAddressSpace *vtd_as; >>>> - >>>> - assert(0 <= devfn && devfn <= VTD_PCI_DEVFN_MAX); >>>> - >>>> - vtd_as = vtd_find_add_as(s, bus, devfn); >>>> - return &vtd_as->as; >>>> -} >>>> - >>>> -static void mch_init_dmar(MCHPCIState *mch) >>>> -{ >>>> - 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); >>>> - qdev_init_nofail(DEVICE(mch->iommu)); >>>> - sysbus_mmio_map(SYS_BUS_DEVICE(mch->iommu), 0, Q35_HOST_BRIDGE_IOMMU_ADDR); >>>> - >>>> - pci_setup_iommu(pci_bus, q35_host_dma_iommu, mch->iommu); >>>> -} >>>> - >>>> static void mch_realize(PCIDevice *d, Error **errp) >>>> { >>>> int i; >>>> @@ -506,10 +482,6 @@ static void mch_realize(PCIDevice *d, Error **errp) >>>> mch->pci_address_space, &mch->pam_regions[i+1], >>>> PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE); >>>> } >>>> - /* Intel IOMMU (VT-d) */ >>>> - if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) { >>>> - mch_init_dmar(mch); >>>> - } >>>> } >>>> >>>> uint64_t mch_mcfg_base(void) >>>> diff --git a/qemu-options.hx b/qemu-options.hx >>>> index 6106520..2953baf 100644 >>>> --- a/qemu-options.hx >>>> +++ b/qemu-options.hx >>>> @@ -38,7 +38,6 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \ >>>> " kvm_shadow_mem=size of KVM shadow MMU\n" >>>> " dump-guest-core=on|off include guest memory in a core dump (default=on)\n" >>>> " mem-merge=on|off controls memory merge support (default: on)\n" >>>> - " iommu=on|off controls emulated Intel IOMMU (VT-d) support (default=off)\n" >>>> " igd-passthru=on|off controls IGD GFX passthrough support (default=off)\n" >>>> " aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n" >>>> " dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n" >>>> @@ -73,8 +72,6 @@ Include guest memory in a core dump. The default is on. >>>> Enables or disables memory merge support. This feature, when supported by >>>> the host, de-duplicates identical memory pages among VMs instances >>>> (enabled by default). >>>> -@item iommu=on|off >>>> -Enables or disables emulated Intel IOMMU (VT-d) support. The default is off. >>>> @item aes-key-wrap=on|off >>>> Enables or disables AES key wrapping support on s390-ccw hosts. This feature >>>> controls whether AES wrapping keys will be created to allow >>>> -- >>>> 2.4.3
On Thu, Jun 02, 2016 at 11:15:54PM +0300, Marcel Apfelbaum wrote: [...] > static void vtd_realize(DeviceState *dev, Error **errp) > { > + PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus; > IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev); > > VTD_DPRINTF(GENERAL, ""); > @@ -2029,6 +2043,9 @@ static void vtd_realize(DeviceState *dev, Error **errp) > s->vtd_as_by_busptr = g_hash_table_new_full(vtd_uint64_hash, vtd_uint64_equal, > g_free, g_free); > vtd_init(s); > + sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, Q35_HOST_BRIDGE_IOMMU_ADDR); > + bus->iommu_fn = vtd_host_dma_iommu; > + bus->iommu_opaque = dev; Here, shall we still use pci_setup_iommu() to keep the two fields private for pci framework? Btw, I am rebasing Intel IR work onto this patchset, but encountered issues (guest hang, or errornous interrupts) when guest specify more than 1 vcpus (everything is cool as long as vcpu=1). Maybe there is something wrong during the rebase, still investigating. Please shoot if there is any clue. Thanks, -- peterx
On 06/12/2016 07:27 AM, Peter Xu wrote: > On Thu, Jun 02, 2016 at 11:15:54PM +0300, Marcel Apfelbaum wrote: > > [...] > >> static void vtd_realize(DeviceState *dev, Error **errp) >> { >> + PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus; >> IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev); >> >> VTD_DPRINTF(GENERAL, ""); >> @@ -2029,6 +2043,9 @@ static void vtd_realize(DeviceState *dev, Error **errp) >> s->vtd_as_by_busptr = g_hash_table_new_full(vtd_uint64_hash, vtd_uint64_equal, >> g_free, g_free); >> vtd_init(s); >> + sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, Q35_HOST_BRIDGE_IOMMU_ADDR); >> + bus->iommu_fn = vtd_host_dma_iommu; >> + bus->iommu_opaque = dev; > > Here, shall we still use pci_setup_iommu() to keep the two fields > private for pci framework? > I've already spotted it and took care of it, thanks :) ! > Btw, I am rebasing Intel IR work onto this patchset, but encountered > issues (guest hang, or errornous interrupts) when guest specify more > than 1 vcpus (everything is cool as long as vcpu=1). Maybe there is > something wrong during the rebase, still investigating. Please shoot > if there is any clue. > I am running with 2 vcpus and I didn't see any problem, I'll let you know if can reproduce. Thanks, Marcel > Thanks, > > -- peterx >
On Mon, Jun 13, 2016 at 01:20:11PM +0300, Marcel Apfelbaum wrote: > On 06/12/2016 07:27 AM, Peter Xu wrote: > >On Thu, Jun 02, 2016 at 11:15:54PM +0300, Marcel Apfelbaum wrote: > > > >[...] > > > >> static void vtd_realize(DeviceState *dev, Error **errp) > >> { > >>+ PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus; > >> IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev); > >> > >> VTD_DPRINTF(GENERAL, ""); > >>@@ -2029,6 +2043,9 @@ static void vtd_realize(DeviceState *dev, Error **errp) > >> s->vtd_as_by_busptr = g_hash_table_new_full(vtd_uint64_hash, vtd_uint64_equal, > >> g_free, g_free); > >> vtd_init(s); > >>+ sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, Q35_HOST_BRIDGE_IOMMU_ADDR); > >>+ bus->iommu_fn = vtd_host_dma_iommu; > >>+ bus->iommu_opaque = dev; > > > >Here, shall we still use pci_setup_iommu() to keep the two fields > >private for pci framework? > > > > I've already spotted it and took care of it, thanks :) ! Cool. :) Btw, have we removed MachineState.iommu variable as well? > > >Btw, I am rebasing Intel IR work onto this patchset, but encountered > >issues (guest hang, or errornous interrupts) when guest specify more > >than 1 vcpus (everything is cool as long as vcpu=1). Maybe there is > >something wrong during the rebase, still investigating. Please shoot > >if there is any clue. > > > > I am running with 2 vcpus and I didn't see any problem, I'll let you > know if can reproduce. My fault during rebase. It's very easy to lost lines of codes during rebase, especially where there is function move from one place to another, while in which function I did some changes... It's all good now. Thanks! -- peterx
diff --git a/hw/core/machine.c b/hw/core/machine.c index ccdd5fa..8f94301 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -300,20 +300,6 @@ static void machine_set_firmware(Object *obj, const char *value, Error **errp) ms->firmware = g_strdup(value); } -static bool machine_get_iommu(Object *obj, Error **errp) -{ - MachineState *ms = MACHINE(obj); - - return ms->iommu; -} - -static void machine_set_iommu(Object *obj, bool value, Error **errp) -{ - MachineState *ms = MACHINE(obj); - - ms->iommu = value; -} - static void machine_set_suppress_vmdesc(Object *obj, bool value, Error **errp) { MachineState *ms = MACHINE(obj); @@ -493,12 +479,6 @@ static void machine_initfn(Object *obj) object_property_set_description(obj, "firmware", "Firmware image", NULL); - object_property_add_bool(obj, "iommu", - machine_get_iommu, - machine_set_iommu, NULL); - object_property_set_description(obj, "iommu", - "Set on/off to enable/disable Intel IOMMU (VT-d)", - NULL); object_property_add_bool(obj, "suppress-vmdesc", machine_get_suppress_vmdesc, machine_set_suppress_vmdesc, NULL); diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 347718f..9af5d6b 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -24,6 +24,8 @@ #include "exec/address-spaces.h" #include "intel_iommu_internal.h" #include "hw/pci/pci.h" +#include "hw/pci/pci_bus.h" +#include "hw/i386/pc.h" /*#define DEBUG_INTEL_IOMMU*/ #ifdef DEBUG_INTEL_IOMMU @@ -2014,8 +2016,20 @@ static void vtd_reset(DeviceState *dev) vtd_init(s); } +static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn) +{ + IntelIOMMUState *s = opaque; + VTDAddressSpace *vtd_as; + + assert(0 <= devfn && devfn <= VTD_PCI_DEVFN_MAX); + + vtd_as = vtd_find_add_as(s, bus, devfn); + return &vtd_as->as; +} + static void vtd_realize(DeviceState *dev, Error **errp) { + PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus; IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev); VTD_DPRINTF(GENERAL, ""); @@ -2029,6 +2043,9 @@ static void vtd_realize(DeviceState *dev, Error **errp) s->vtd_as_by_busptr = g_hash_table_new_full(vtd_uint64_hash, vtd_uint64_equal, g_free, g_free); vtd_init(s); + sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, Q35_HOST_BRIDGE_IOMMU_ADDR); + bus->iommu_fn = vtd_host_dma_iommu; + bus->iommu_opaque = dev; } static void vtd_class_init(ObjectClass *klass, void *data) diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c index 70f897e..ea684c7 100644 --- a/hw/pci-host/q35.c +++ b/hw/pci-host/q35.c @@ -424,30 +424,6 @@ static void mch_reset(DeviceState *qdev) mch_update(mch); } -static AddressSpace *q35_host_dma_iommu(PCIBus *bus, void *opaque, int devfn) -{ - IntelIOMMUState *s = opaque; - VTDAddressSpace *vtd_as; - - assert(0 <= devfn && devfn <= VTD_PCI_DEVFN_MAX); - - vtd_as = vtd_find_add_as(s, bus, devfn); - return &vtd_as->as; -} - -static void mch_init_dmar(MCHPCIState *mch) -{ - 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); - qdev_init_nofail(DEVICE(mch->iommu)); - sysbus_mmio_map(SYS_BUS_DEVICE(mch->iommu), 0, Q35_HOST_BRIDGE_IOMMU_ADDR); - - pci_setup_iommu(pci_bus, q35_host_dma_iommu, mch->iommu); -} - static void mch_realize(PCIDevice *d, Error **errp) { int i; @@ -506,10 +482,6 @@ static void mch_realize(PCIDevice *d, Error **errp) mch->pci_address_space, &mch->pam_regions[i+1], PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE); } - /* Intel IOMMU (VT-d) */ - if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) { - mch_init_dmar(mch); - } } uint64_t mch_mcfg_base(void) diff --git a/qemu-options.hx b/qemu-options.hx index 6106520..2953baf 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -38,7 +38,6 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \ " kvm_shadow_mem=size of KVM shadow MMU\n" " dump-guest-core=on|off include guest memory in a core dump (default=on)\n" " mem-merge=on|off controls memory merge support (default: on)\n" - " iommu=on|off controls emulated Intel IOMMU (VT-d) support (default=off)\n" " igd-passthru=on|off controls IGD GFX passthrough support (default=off)\n" " aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n" " dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n" @@ -73,8 +72,6 @@ Include guest memory in a core dump. The default is on. Enables or disables memory merge support. This feature, when supported by the host, de-duplicates identical memory pages among VMs instances (enabled by default). -@item iommu=on|off -Enables or disables emulated Intel IOMMU (VT-d) support. The default is off. @item aes-key-wrap=on|off Enables or disables AES key wrapping support on s390-ccw hosts. This feature controls whether AES wrapping keys will be created to allow
Use the standard '-device iommu' instead of '-machine,iommu=on' to create the IOMMU device. Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> --- hw/core/machine.c | 20 -------------------- hw/i386/intel_iommu.c | 17 +++++++++++++++++ hw/pci-host/q35.c | 28 ---------------------------- qemu-options.hx | 3 --- 4 files changed, 17 insertions(+), 51 deletions(-)