[Qemu-devel] [PATCH RFC 0/2] enable iommu with -device

[Qemu-devel] [PATCH RFC 0/2] enable iommu with -device

[Marcel Apfelbaum]

This is a proposal on how to create the iommu with
'-device intel-iommu' instead of '-machine,iommu=on'.

The device is part of the machine properties because we wanted
to ensure it is created before any other PCI device.

The alternative is to skip the bus_master_enable_region at
the time the device is created. We can create this region
at machine_done phase. (patch 1)

Then we can enable sysbus devices for PC machines and make all the
init steps inside the iommu realize function. (patch 2)

The series is working, but a lot of issues are not resolved:
  - minimum testing was done
  - the iommu addr should be passed (maybe) in command line rather than hard-coded
  - enabling sysbus devices for PC machines is risky, I am not aware yet
    of the side effects of this modification.
  - I am not sure moving the bus_master_enable_region to machine_done
    is with no undesired effects. 

Thanks,
Marcel

Marcel Apfelbaum (2):
  hw/pci: delay bus_master_enable_region initialization
  hw/iommu: enable iommu with -device

 hw/core/machine.c     | 20 --------------------
 hw/i386/intel_iommu.c | 17 +++++++++++++++++
 hw/i386/pc.c          | 17 +++++++++++++++++
 hw/i386/pc_q35.c      |  1 +
 hw/pci-host/q35.c     | 28 ----------------------------
 hw/pci/pci.c          | 22 ++++++++++++----------
 include/hw/pci/pci.h  |  2 ++
 7 files changed, 49 insertions(+), 58 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH RFC 1/2] hw/pci: delay bus_master_enable_region initialization

[Marcel Apfelbaum]

Skip bus_master_enable region creation on PCI devices init
in order to be sure the IOMMU device (if present) would
be created in advance. Add this memory region at machine_done time.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/i386/pc.c         | 17 +++++++++++++++++
 hw/pci/pci.c         | 22 ++++++++++++----------
 include/hw/pci/pci.h |  2 ++
 3 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 99437e0..f9ac543 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -32,6 +32,7 @@
 #include "hw/ide.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_bus.h"
+#include "hw/pci/pci_bridge.h"
 #include "hw/nvram/fw_cfg.h"
 #include "hw/timer/hpet.h"
 #include "hw/smbios/smbios.h"
@@ -1156,12 +1157,26 @@ typedef struct PcRomPciInfo {
     uint64_t w64_max;
 } PcRomPciInfo;
 
+static void pci_bus_enable_bus_master(PCIBus *bus, PCIDevice *dev, void *opaque)
+{
+    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
+
+    pci_init_bus_master(dev);
+
+    if (pc->is_bridge) {
+        PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev));
+        pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
+                            pci_bus_enable_bus_master, opaque);
+    }
+}
+
 static
 void pc_machine_done(Notifier *notifier, void *data)
 {
     PCMachineState *pcms = container_of(notifier,
                                         PCMachineState, machine_done);
     PCIBus *bus = pcms->bus;
+    pci_for_each_device(bus, pci_bus_num(bus), pci_bus_enable_bus_master, NULL);
 
     if (bus) {
         int extra_hosts = 0;
@@ -1169,6 +1184,8 @@ void pc_machine_done(Notifier *notifier, void *data)
         QLIST_FOREACH(bus, &bus->child, sibling) {
             /* look for expander root buses */
             if (pci_bus_is_root(bus)) {
+                pci_for_each_device(bus, pci_bus_num(bus),
+                                    pci_bus_enable_bus_master, NULL);
                 extra_hosts++;
             }
         }
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index bb605ef..e22b1c8 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -828,6 +828,18 @@ static void pci_config_free(PCIDevice *pci_dev)
     g_free(pci_dev->used);
 }
 
+void pci_init_bus_master(PCIDevice *pci_dev)
+{
+    AddressSpace *dma_as = pci_device_iommu_address_space(pci_dev);
+
+    memory_region_init_alias(&pci_dev->bus_master_enable_region,
+                             OBJECT(pci_dev), "bus master",
+                             dma_as->root, 0, memory_region_size(dma_as->root));
+    memory_region_set_enabled(&pci_dev->bus_master_enable_region, false);
+    address_space_init(&pci_dev->bus_master_as, &pci_dev->bus_master_enable_region,
+                       pci_dev->name);
+}
+
 static void do_pci_unregister_device(PCIDevice *pci_dev)
 {
     pci_dev->bus->devices[pci_dev->devfn] = NULL;
@@ -845,7 +857,6 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
     PCIConfigReadFunc *config_read = pc->config_read;
     PCIConfigWriteFunc *config_write = pc->config_write;
     Error *local_err = NULL;
-    AddressSpace *dma_as;
     DeviceState *dev = DEVICE(pci_dev);
 
     pci_dev->bus = bus;
@@ -885,15 +896,6 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
     }
 
     pci_dev->devfn = devfn;
-    dma_as = pci_device_iommu_address_space(pci_dev);
-
-    memory_region_init_alias(&pci_dev->bus_master_enable_region,
-                             OBJECT(pci_dev), "bus master",
-                             dma_as->root, 0, memory_region_size(dma_as->root));
-    memory_region_set_enabled(&pci_dev->bus_master_enable_region, false);
-    address_space_init(&pci_dev->bus_master_as, &pci_dev->bus_master_enable_region,
-                       name);
-
     pstrcpy(pci_dev->name, sizeof(pci_dev->name), name);
     pci_dev->irq_state = 0;
     pci_config_alloc(pci_dev);
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index ef6ba51..95a930f 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -343,6 +343,8 @@ int pci_device_load(PCIDevice *s, QEMUFile *f);
 MemoryRegion *pci_address_space(PCIDevice *dev);
 MemoryRegion *pci_address_space_io(PCIDevice *dev);
 
+void pci_init_bus_master(PCIDevice *dev);
+
 /*
  * Should not normally be used by devices. For use by sPAPR target
  * where QEMU emulates firmware.
-- 
2.4.3

Re: [Qemu-devel] [PATCH RFC 1/2] hw/pci: delay bus_master_enable_region initialization

[Paolo Bonzini]

On 23/05/2016 16:01, Marcel Apfelbaum wrote:
> Skip bus_master_enable region creation on PCI devices init
> in order to be sure the IOMMU device (if present) would
> be created in advance. Add this memory region at machine_done time.
> 
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> ---
>  hw/i386/pc.c         | 17 +++++++++++++++++
>  hw/pci/pci.c         | 22 ++++++++++++----------
>  include/hw/pci/pci.h |  2 ++
>  3 files changed, 31 insertions(+), 10 deletions(-)

Does hotplug still work?

Thanks,

Paolo

Re: [Qemu-devel] [PATCH RFC 1/2] hw/pci: delay bus_master_enable_region initialization

[Marcel Apfelbaum]

On 05/23/2016 05:08 PM, Paolo Bonzini wrote:
>
>
> On 23/05/2016 16:01, Marcel Apfelbaum wrote:
>> Skip bus_master_enable region creation on PCI devices init
>> in order to be sure the IOMMU device (if present) would
>> be created in advance. Add this memory region at machine_done time.
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>> ---
>>   hw/i386/pc.c         | 17 +++++++++++++++++
>>   hw/pci/pci.c         | 22 ++++++++++++----------
>>   include/hw/pci/pci.h |  2 ++
>>   3 files changed, 31 insertions(+), 10 deletions(-)
>
> Does hotplug still work?

Hotplug does work, but the device can't be bus_master since I am adding
the bus_master_region only at machine_done...

Thank you for pointing that out, this can be easily solved by checking
the qdev_hotplug flag and enabling the bus_master region if we passed machine creation.

Other than that, does it seems to a you a feasible approach?

Thanks,
Marcel

>
> Thanks,
>
> Paolo
>

Re: [Qemu-devel] [PATCH RFC 1/2] hw/pci: delay bus_master_enable_region initialization

[Paolo Bonzini]

On 23/05/2016 16:22, Marcel Apfelbaum wrote:
> On 05/23/2016 05:08 PM, Paolo Bonzini wrote:
>>
>>
>> On 23/05/2016 16:01, Marcel Apfelbaum wrote:
>>> Skip bus_master_enable region creation on PCI devices init
>>> in order to be sure the IOMMU device (if present) would
>>> be created in advance. Add this memory region at machine_done time.
>>>
>>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>>> ---
>>>   hw/i386/pc.c         | 17 +++++++++++++++++
>>>   hw/pci/pci.c         | 22 ++++++++++++----------
>>>   include/hw/pci/pci.h |  2 ++
>>>   3 files changed, 31 insertions(+), 10 deletions(-)
>>
>> Does hotplug still work?
> 
> Hotplug does work, but the device can't be bus_master since I am adding
> the bus_master_region only at machine_done...
> 
> Thank you for pointing that out, this can be easily solved by checking
> the qdev_hotplug flag and enabling the bus_master region if we passed
> machine creation.
> 
> Other than that, does it seems to a you a feasible approach?

Yes, I guess it's okay.

Thanks,

Paolo

[Qemu-devel] [PATCH RFC 2/2] hw/iommu: enable iommu with -device

[Marcel Apfelbaum]

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/i386/pc_q35.c      |  1 +
 hw/pci-host/q35.c     | 28 ----------------------------
 4 files changed, 18 insertions(+), 48 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 6dbbc85..23456f8 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -286,20 +286,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);
@@ -472,12 +458,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/i386/pc_q35.c b/hw/i386/pc_q35.c
index 04aae89..431eaed 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -281,6 +281,7 @@ static void pc_q35_machine_options(MachineClass *m)
     m->default_machine_opts = "firmware=bios-256k.bin";
     m->default_display = "std";
     m->no_floppy = 1;
+    m->has_dynamic_sysbus = true;
 }
 
 static void pc_q35_2_6_machine_options(MachineClass *m)
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)
-- 
2.4.3

Re: [Qemu-devel] [PATCH RFC 0/2] enable iommu with -device

[Peter Xu]

On Mon, May 23, 2016 at 05:01:28PM +0300, Marcel Apfelbaum wrote:
> This is a proposal on how to create the iommu with
> '-device intel-iommu' instead of '-machine,iommu=on'.
> 
> The device is part of the machine properties because we wanted
> to ensure it is created before any other PCI device.
> 
> The alternative is to skip the bus_master_enable_region at
> the time the device is created. We can create this region
> at machine_done phase. (patch 1)
> 
> Then we can enable sysbus devices for PC machines and make all the
> init steps inside the iommu realize function. (patch 2)
> 
> The series is working, but a lot of issues are not resolved:
>   - minimum testing was done
>   - the iommu addr should be passed (maybe) in command line rather than hard-coded
>   - enabling sysbus devices for PC machines is risky, I am not aware yet
>     of the side effects of this modification.
>   - I am not sure moving the bus_master_enable_region to machine_done
>     is with no undesired effects. 

I gave it a shot on the patches and it works nicely (of course no
complex configurations, like hot plug).

Could you help introduce what will bring us if we use "-device" rather
than "-M" options?  Benefits I can see is that, we can specify
parameters with specific device, rather than messing them up in
"machine" options. Do we have any other benefits that I may have
missed?

Thanks!

-- peterx

Re: [Qemu-devel] [PATCH RFC 0/2] enable iommu with -device

[Marcel Apfelbaum]

On 05/30/2016 04:43 PM, Peter Xu wrote:
> On Mon, May 23, 2016 at 05:01:28PM +0300, Marcel Apfelbaum wrote:
>> This is a proposal on how to create the iommu with
>> '-device intel-iommu' instead of '-machine,iommu=on'.
>>
>> The device is part of the machine properties because we wanted
>> to ensure it is created before any other PCI device.
>>
>> The alternative is to skip the bus_master_enable_region at
>> the time the device is created. We can create this region
>> at machine_done phase. (patch 1)
>>
>> Then we can enable sysbus devices for PC machines and make all the
>> init steps inside the iommu realize function. (patch 2)
>>
>> The series is working, but a lot of issues are not resolved:
>>    - minimum testing was done
>>    - the iommu addr should be passed (maybe) in command line rather than hard-coded
>>    - enabling sysbus devices for PC machines is risky, I am not aware yet
>>      of the side effects of this modification.
>>    - I am not sure moving the bus_master_enable_region to machine_done
>>      is with no undesired effects.
>
> I gave it a shot on the patches and it works nicely (of course no
> complex configurations, like hot plug).
>
> Could you help introduce what will bring us if we use "-device" rather
> than "-M" options?  Benefits I can see is that, we can specify
> parameters with specific device, rather than messing them up in
> "machine" options. Do we have any other benefits that I may have
> missed?

Hi Peter,
Thanks for trying it!

Mainly is about not hard-coding device options (e.g. PCI address for AMD IOMMU),
but also to avoid having devices added as a side-effect of some machine option.
This will bring as closer to a cleaner model of a modular machine.

I plan to post a non-rfc version soon.
Thanks,
Marcel


>
> Thanks!
>
> -- peterx
>

Re: [Qemu-devel] [PATCH RFC 0/2] enable iommu with -device

[Peter Xu]

On Mon, May 30, 2016 at 05:14:15PM +0300, Marcel Apfelbaum wrote:
> On 05/30/2016 04:43 PM, Peter Xu wrote:
> >On Mon, May 23, 2016 at 05:01:28PM +0300, Marcel Apfelbaum wrote:
> >>This is a proposal on how to create the iommu with
> >>'-device intel-iommu' instead of '-machine,iommu=on'.
> >>
> >>The device is part of the machine properties because we wanted
> >>to ensure it is created before any other PCI device.
> >>
> >>The alternative is to skip the bus_master_enable_region at
> >>the time the device is created. We can create this region
> >>at machine_done phase. (patch 1)
> >>
> >>Then we can enable sysbus devices for PC machines and make all the
> >>init steps inside the iommu realize function. (patch 2)
> >>
> >>The series is working, but a lot of issues are not resolved:
> >>   - minimum testing was done
> >>   - the iommu addr should be passed (maybe) in command line rather than hard-coded
> >>   - enabling sysbus devices for PC machines is risky, I am not aware yet
> >>     of the side effects of this modification.
> >>   - I am not sure moving the bus_master_enable_region to machine_done
> >>     is with no undesired effects.
> >
> >I gave it a shot on the patches and it works nicely (of course no
> >complex configurations, like hot plug).
> >
> >Could you help introduce what will bring us if we use "-device" rather
> >than "-M" options?  Benefits I can see is that, we can specify
> >parameters with specific device, rather than messing them up in
> >"machine" options. Do we have any other benefits that I may have
> >missed?
> 
> Hi Peter,
> Thanks for trying it!
> 
> Mainly is about not hard-coding device options (e.g. PCI address for AMD IOMMU),
> but also to avoid having devices added as a side-effect of some machine option.
> This will bring as closer to a cleaner model of a modular machine.
> 
> I plan to post a non-rfc version soon.

I just thought about whether we should support multiple IOMMUs in the
future (never know whether there would be a use case for
that). Anyway, looking forward to your non-rfc version. :)

Thanks!

-- peterx

Re: [Qemu-devel] [PATCH RFC 0/2] enable iommu with -device

[Marcel Apfelbaum]

On 05/31/2016 04:44 AM, Peter Xu wrote:
> On Mon, May 30, 2016 at 05:14:15PM +0300, Marcel Apfelbaum wrote:
>> On 05/30/2016 04:43 PM, Peter Xu wrote:
>>> On Mon, May 23, 2016 at 05:01:28PM +0300, Marcel Apfelbaum wrote:
>>>> This is a proposal on how to create the iommu with
>>>> '-device intel-iommu' instead of '-machine,iommu=on'.
>>>>
>>>> The device is part of the machine properties because we wanted
>>>> to ensure it is created before any other PCI device.
>>>>
>>>> The alternative is to skip the bus_master_enable_region at
>>>> the time the device is created. We can create this region
>>>> at machine_done phase. (patch 1)
>>>>
>>>> Then we can enable sysbus devices for PC machines and make all the
>>>> init steps inside the iommu realize function. (patch 2)
>>>>
>>>> The series is working, but a lot of issues are not resolved:
>>>>    - minimum testing was done
>>>>    - the iommu addr should be passed (maybe) in command line rather than hard-coded
>>>>    - enabling sysbus devices for PC machines is risky, I am not aware yet
>>>>      of the side effects of this modification.
>>>>    - I am not sure moving the bus_master_enable_region to machine_done
>>>>      is with no undesired effects.
>>>
>>> I gave it a shot on the patches and it works nicely (of course no
>>> complex configurations, like hot plug).
>>>
>>> Could you help introduce what will bring us if we use "-device" rather
>>> than "-M" options?  Benefits I can see is that, we can specify
>>> parameters with specific device, rather than messing them up in
>>> "machine" options. Do we have any other benefits that I may have
>>> missed?
>>
>> Hi Peter,
>> Thanks for trying it!
>>
>> Mainly is about not hard-coding device options (e.g. PCI address for AMD IOMMU),
>> but also to avoid having devices added as a side-effect of some machine option.
>> This will bring as closer to a cleaner model of a modular machine.
>>
>> I plan to post a non-rfc version soon.
>
> I just thought about whether we should support multiple IOMMUs in the
> future (never know whether there would be a use case for
> that).

This is an interesting scenario, we may find a need for multiple IOMMUs.
I tried it a while ago, but the linux kernel has (had?) a known limitation supporting it, see:
     https://lkml.org/lkml/2015/11/22/100
Since is categorized as bug, it may be solved and the we can go for it.

  Anyway, looking forward to your non-rfc version. :)

I just posted a new version.
Thanks,
Marcel

>


> Thanks!
>
> -- peterx
>