[Qemu-devel] [PATCH RFC 0/3] qdev: order devices by priority before creating them

[Qemu-devel] [PATCH RFC 0/3] qdev: order devices by priority before creating them

[Marcel Apfelbaum]

This series aims to allow more devices to be used with '-device'
by sorting the devices based on a predefined creation order flag
before creating them.

Devices like IOMMU need to be created before others, so they can leverage
the DeviceCreationPriority flag introduced by the first patch to DeviceClass.

The second patch sorts the devices by their DeviceCreationPriority
before creating them.

Finally, the last patch demonstrates how it can be used to ensure
the creation of host-bridges before the pci-bridges and pci-bridges before
the others.

I preferred to combine all the priorities into a single enum
to better manage the creation order.

This is an RFC because I only wanted to know if it seems like the right way to go.
Comments are appreciated,

Thanks,
Marcel

Marcel Apfelbaum (3):
  qdev: add device creation priority flag
  vl.c: create devices by their creation priority flag
  hw/pci-bridge: add the corresponding creation priority flag

 hw/pci-bridge/pci_bridge_dev.c      |  1 +
 hw/pci-bridge/pci_expander_bridge.c |  2 ++
 include/hw/qdev-core.h              | 13 +++++++++
 include/monitor/qdev.h              |  1 +
 qdev-monitor.c                      | 19 +++++++++++++
 vl.c                                | 55 +++++++++++++++++++++++++++++++------
 6 files changed, 82 insertions(+), 9 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH RFC 1/3] qdev: add device creation priority flag

[Marcel Apfelbaum]

Some devices need to be created before others. Add a flag
to DeviceClass and the means to query it.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 include/hw/qdev-core.h | 13 +++++++++++++
 include/monitor/qdev.h |  1 +
 qdev-monitor.c         | 19 +++++++++++++++++++
 3 files changed, 33 insertions(+)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 1ce02b2..f885d6c 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -29,6 +29,13 @@ typedef enum DeviceCategory {
     DEVICE_CATEGORY_MAX
 } DeviceCategory;
 
+
+typedef enum DeviceCreationPriority {
+    DEVICE_PRIO_NORMAL,
+    DEVICE_PRIO_BRIDGE,
+    DEVICE_PRIO_HOSTBRIDGE
+} DeviceCreationPriority;
+
 typedef int (*qdev_initfn)(DeviceState *dev);
 typedef int (*qdev_event)(DeviceState *dev);
 typedef void (*qdev_resetfn)(DeviceState *dev);
@@ -139,6 +146,12 @@ typedef struct DeviceClass {
     qdev_initfn init; /* TODO remove, once users are converted to realize */
     qdev_event exit; /* TODO remove, once users are converted to unrealize */
     const char *bus_type;
+
+    /*
+     * Some devices need to be created before others. Devices with higher
+     * priority will be created first.
+     */
+    DeviceCreationPriority creation_priority;
 } DeviceClass;
 
 typedef struct NamedGPIOList NamedGPIOList;
diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
index c4b8a05..7f35ee5 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -12,5 +12,6 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp);
 
 int qdev_device_help(QemuOpts *opts);
 DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
+int qdev_device_get_priority(QemuOpts *opts, Error **errp);
 
 #endif
diff --git a/qdev-monitor.c b/qdev-monitor.c
index e19617f..0edba77 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -626,6 +626,25 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
     return dev;
 }
 
+int qdev_device_get_priority(QemuOpts *opts, Error **errp)
+{
+    DeviceClass *dc;
+    const char *driver;
+
+    driver = qemu_opt_get(opts, "driver");
+    if (!driver) {
+        error_setg(errp, QERR_MISSING_PARAMETER, "driver");
+        return -1;
+    }
+
+    /* find driver */
+    dc = qdev_get_device_class(&driver, errp);
+    if (!dc) {
+        return -1;
+    }
+
+    return dc->creation_priority;
+}
 
 #define qdev_printf(fmt, ...) monitor_printf(mon, "%*s" fmt, indent, "", ## __VA_ARGS__)
 static void qbus_print(Monitor *mon, BusState *bus, int indent);
-- 
2.4.3

[Qemu-devel] [PATCH RFC 2/3] vl.c: create devices by their creation priority flag

[Marcel Apfelbaum]

Create the devices by their DeviceCreationPriority order instead
of the input order, however devices with the same priority will
be created in the same order as before.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 vl.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 46 insertions(+), 9 deletions(-)

diff --git a/vl.c b/vl.c
index 3629336..1c650b9 100644
--- a/vl.c
+++ b/vl.c
@@ -2341,20 +2341,58 @@ static int device_help_func(void *opaque, QemuOpts *opts, Error **errp)
     return qdev_device_help(opts);
 }
 
-static int device_init_func(void *opaque, QemuOpts *opts, Error **errp)
+static int device_insert_by_priority_func(void *opaque, QemuOpts *opts,
+                                          Error **errp)
 {
-    Error *err = NULL;
-    DeviceState *dev;
+    GList **dev_list = opaque;
+    GList *l;
+    int dev_prio = qdev_device_get_priority(opts, errp);
 
-    dev = qdev_device_add(opts, &err);
-    if (!dev) {
-        error_report_err(err);
+    if (dev_prio < 0) {
         return -1;
     }
-    object_unref(OBJECT(dev));
+
+    for (l = *dev_list; l != NULL; l = g_list_next(l)) {
+        int prio = qdev_device_get_priority((QemuOpts *)l->data, errp);
+        if (prio >= dev_prio) {
+            continue;
+        }
+        break;
+    }
+
+    *dev_list = g_list_insert_before(*dev_list, l, opts);
+
     return 0;
 }
 
+static int devices_init(void)
+{
+    GList *dev_list, *l;
+    Error *err = NULL;
+
+    if (qemu_opts_foreach(qemu_find_opts("device"),
+                          device_insert_by_priority_func, &dev_list, &err)) {
+        goto out_err;
+    }
+
+    for (l = dev_list; l != NULL; l = g_list_next(l)) {
+        DeviceState *dev = qdev_device_add((QemuOpts *)l->data, &err);
+        if (!dev) {
+            goto out_err;
+        }
+
+        object_unref(OBJECT(dev));
+    }
+
+    g_list_free(dev_list);
+    return 0;
+
+out_err:
+    g_list_free(dev_list);
+    error_report_err(err);
+    return -1;
+}
+
 static int chardev_init_func(void *opaque, QemuOpts *opts, Error **errp)
 {
     Error *local_err = NULL;
@@ -4535,8 +4573,7 @@ int main(int argc, char **argv, char **envp)
     igd_gfx_passthru();
 
     /* init generic devices */
-    if (qemu_opts_foreach(qemu_find_opts("device"),
-                          device_init_func, NULL, NULL)) {
+    if (devices_init()) {
         exit(1);
     }
 
-- 
2.4.3

[Qemu-devel] [PATCH RFC 3/3] hw/pci-bridge: add the corresponding creation priority flag

[Marcel Apfelbaum]

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/pci-bridge/pci_bridge_dev.c      | 1 +
 hw/pci-bridge/pci_expander_bridge.c | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index 7b582e9..eb552c4 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -210,6 +210,7 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
     dc->reset = qdev_pci_bridge_dev_reset;
     dc->props = pci_bridge_dev_properties;
     dc->vmsd = &pci_bridge_dev_vmstate;
+    dc->creation_priority = DEVICE_PRIO_BRIDGE;
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
     hc->plug = pci_bridge_dev_hotplug_cb;
     hc->unplug_request = pci_bridge_dev_hot_unplug_request_cb;
diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
index 5e7e546..e012eab 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -302,6 +302,7 @@ static void pxb_dev_class_init(ObjectClass *klass, void *data)
 
     dc->desc = "PCI Expander Bridge";
     dc->props = pxb_dev_properties;
+    dc->creation_priority = DEVICE_PRIO_HOSTBRIDGE;
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
 }
 
@@ -335,6 +336,7 @@ static void pxb_pcie_dev_class_init(ObjectClass *klass, void *data)
 
     dc->desc = "PCI Express Expander Bridge";
     dc->props = pxb_dev_properties;
+    dc->creation_priority = DEVICE_PRIO_HOSTBRIDGE;
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
 }
 
-- 
2.4.3

Re: [Qemu-devel] [PATCH RFC 0/3] qdev: order devices by priority before creating them

[Markus Armbruster]

Marcel Apfelbaum <marcel@redhat.com> writes:

> This series aims to allow more devices to be used with '-device'
> by sorting the devices based on a predefined creation order flag
> before creating them.
>
> Devices like IOMMU need to be created before others, so they can leverage
> the DeviceCreationPriority flag introduced by the first patch to DeviceClass.
>
> The second patch sorts the devices by their DeviceCreationPriority
> before creating them.
>
> Finally, the last patch demonstrates how it can be used to ensure
> the creation of host-bridges before the pci-bridges and pci-bridges before
> the others.
>
> I preferred to combine all the priorities into a single enum
> to better manage the creation order.
>
> This is an RFC because I only wanted to know if it seems like the right way to go.
> Comments are appreciated,

Can you explain why requiring the user to specify -device in a sane
order isn't good enough?

Re: [Qemu-devel] [PATCH RFC 0/3] qdev: order devices by priority before creating them

[Marcel Apfelbaum]

On 05/10/2016 11:28 AM, Markus Armbruster wrote:
> Marcel Apfelbaum <marcel@redhat.com> writes:
>
>> This series aims to allow more devices to be used with '-device'
>> by sorting the devices based on a predefined creation order flag
>> before creating them.
>>
>> Devices like IOMMU need to be created before others, so they can leverage
>> the DeviceCreationPriority flag introduced by the first patch to DeviceClass.
>>
>> The second patch sorts the devices by their DeviceCreationPriority
>> before creating them.
>>
>> Finally, the last patch demonstrates how it can be used to ensure
>> the creation of host-bridges before the pci-bridges and pci-bridges before
>> the others.
>>
>> I preferred to combine all the priorities into a single enum
>> to better manage the creation order.
>>
>> This is an RFC because I only wanted to know if it seems like the right way to go.
>> Comments are appreciated,
>

Hi Markus,
Thanks for looking into this.


> Can you explain why requiring the user to specify -device in a sane
> order isn't good enough?
>

Point taken, the truth is I didn't like the 'order' restriction in the first place.

If the device creation depends on the id of some other devices (e.g we need the bus id to plug a device into it),
for IOMMU devices it gets a little tricky. You can add the IOMMU device before other PCI devices but it will not
work (because some internal implementation). This is why we added using -machine pc,iommu=on.
I suppose we have other examples as well. This is not user friendly IMO.

To solve the specific IOMMU problem we can check that there are no PCI devices created yet,
but I am not sure is a better approach and is strictly related to this device.

The goal is to be able to add more devices with -device and I thought this kind of creation in steps may help.

Thanks,
Marcel

Re: [Qemu-devel] [PATCH RFC 0/3] qdev: order devices by priority before creating them

[Michael S. Tsirkin]

On Tue, May 10, 2016 at 10:28:43AM +0200, Markus Armbruster wrote:
> Marcel Apfelbaum <marcel@redhat.com> writes:
> 
> > This series aims to allow more devices to be used with '-device'
> > by sorting the devices based on a predefined creation order flag
> > before creating them.
> >
> > Devices like IOMMU need to be created before others, so they can leverage
> > the DeviceCreationPriority flag introduced by the first patch to DeviceClass.
> >
> > The second patch sorts the devices by their DeviceCreationPriority
> > before creating them.
> >
> > Finally, the last patch demonstrates how it can be used to ensure
> > the creation of host-bridges before the pci-bridges and pci-bridges before
> > the others.
> >
> > I preferred to combine all the priorities into a single enum
> > to better manage the creation order.
> >
> > This is an RFC because I only wanted to know if it seems like the right way to go.
> > Comments are appreciated,
> 
> Can you explain why requiring the user to specify -device in a sane
> order isn't good enough?

Because it requires knowledge about the hardware.
For example, how do users know that iommu must come before the devices?
They don't. They often don't know their devices are pci devices even.

-- 
MST

Re: [Qemu-devel] [PATCH RFC 0/3] qdev: order devices by priority before creating them

[Paolo Bonzini]

On 10/05/2016 17:36, Michael S. Tsirkin wrote:
>> > Can you explain why requiring the user to specify -device in a sane
>> > order isn't good enough?
> Because it requires knowledge about the hardware.
> For example, how do users know that iommu must come before the devices?
> They don't.

Could they "know" it by adding an iommu=foo argument to e.g. the PCI
bridge, pointing to the IOMMU device?

Paolo

Re: [Qemu-devel] [PATCH RFC 0/3] qdev: order devices by priority before creating them

[Markus Armbruster]

Marcel Apfelbaum <marcel@redhat.com> writes:

> On 05/10/2016 11:28 AM, Markus Armbruster wrote:
>> Marcel Apfelbaum <marcel@redhat.com> writes:
>>
>>> This series aims to allow more devices to be used with '-device'
>>> by sorting the devices based on a predefined creation order flag
>>> before creating them.
>>>
>>> Devices like IOMMU need to be created before others, so they can leverage
>>> the DeviceCreationPriority flag introduced by the first patch to DeviceClass.
>>>
>>> The second patch sorts the devices by their DeviceCreationPriority
>>> before creating them.
>>>
>>> Finally, the last patch demonstrates how it can be used to ensure
>>> the creation of host-bridges before the pci-bridges and pci-bridges before
>>> the others.
>>>
>>> I preferred to combine all the priorities into a single enum
>>> to better manage the creation order.
>>>
>>> This is an RFC because I only wanted to know if it seems like the right way to go.
>>> Comments are appreciated,
>>
>
> Hi Markus,
> Thanks for looking into this.
>
>
>> Can you explain why requiring the user to specify -device in a sane
>> order isn't good enough?
>>
>
> Point taken, the truth is I didn't like the 'order' restriction in the
> first place.
>
> If the device creation depends on the id of some other devices (e.g we
> need the bus id to plug a device into it), for IOMMU devices it gets a
> little tricky. You can add the IOMMU device before other PCI devices
> but it will not work (because some internal implementation). This is
> why we added using -machine pc,iommu=on.  I suppose we have other
> examples as well. This is not user friendly IMO.
>
> To solve the specific IOMMU problem we can check that there are no PCI
> devices created yet, but I am not sure is a better approach and is
> strictly related to this device.
>
> The goal is to be able to add more devices with -device and I thought
> this kind of creation in steps may help.

In my opinion, there are two sane ways to do command line options.

One is to make order relevant, and process them strictly left to right.

The other is to do the right thing regardless of order.  This requires
some kind of dependency tracking if there are any.

QEMU, of course, does neither: we process them in left to right unless
we don't, and users juggle them until the errors go away.

I'm afraid this patch adds to "unless we don't" without covering much
ground towards "do the right thing regardless of order".  Static
priorities are a rather crude approximation of dependencies.  Is it the
best we can do for user now?

Re: [Qemu-devel] [PATCH RFC 0/3] qdev: order devices by priority before creating them

[Michael S. Tsirkin]

On Tue, May 10, 2016 at 07:13:29PM +0200, Paolo Bonzini wrote:
> 
> 
> On 10/05/2016 17:36, Michael S. Tsirkin wrote:
> >> > Can you explain why requiring the user to specify -device in a sane
> >> > order isn't good enough?
> > Because it requires knowledge about the hardware.
> > For example, how do users know that iommu must come before the devices?
> > They don't.
> 
> Could they "know" it by adding an iommu=foo argument to e.g. the PCI
> bridge, pointing to the IOMMU device?
> 
> Paolo

People don't create the pci host bridge though, it's
part of the default machine.

-- 
MST

Re: [Qemu-devel] [PATCH RFC 0/3] qdev: order devices by priority before creating them

[Paolo Bonzini]

On 11/05/2016 15:55, Michael S. Tsirkin wrote:
> > Could they "know" it by adding an iommu=foo argument to e.g. the PCI
> > bridge, pointing to the IOMMU device?
> 
> People don't create the pci host bridge though, it's
> part of the default machine.

If there's just one of its class you can use -global (similar to how we
use it for floppies).

Thanks,

Paolo

Re: [Qemu-devel] [PATCH RFC 0/3] qdev: order devices by priority before creating them

[Michael S. Tsirkin]

On Wed, May 11, 2016 at 05:09:56PM +0200, Paolo Bonzini wrote:
> 
> 
> On 11/05/2016 15:55, Michael S. Tsirkin wrote:
> > > Could they "know" it by adding an iommu=foo argument to e.g. the PCI
> > > bridge, pointing to the IOMMU device?
> > 
> > People don't create the pci host bridge though, it's
> > part of the default machine.
> 
> If there's just one of its class you can use -global (similar to how we
> use it for floppies).
> 
> Thanks,
> 
> Paolo

I don't like that there's no documentation for these
at all. How are users supposed to know which type to use
for -global?

-- 
MST

Re: [Qemu-devel] [PATCH RFC 0/3] qdev: order devices by priority before creating them

[Paolo Bonzini]

On 11/05/2016 17:19, Michael S. Tsirkin wrote:
> > If there's just one of its class you can use -global (similar to how we
> > use it for floppies).
>
> I don't like that there's no documentation for these
> at all. How are users supposed to know which type to use
> for -global?

Documentation, of course.  But someone has to write it...

Paolo

Re: [Qemu-devel] [PATCH RFC 0/3] qdev: order devices by priority before creating them

[Marcel Apfelbaum]

On 05/11/2016 10:51 AM, Markus Armbruster wrote:
> Marcel Apfelbaum <marcel@redhat.com> writes:
>
>> On 05/10/2016 11:28 AM, Markus Armbruster wrote:
>>> Marcel Apfelbaum <marcel@redhat.com> writes:
>>>
>>>> This series aims to allow more devices to be used with '-device'
>>>> by sorting the devices based on a predefined creation order flag
>>>> before creating them.
>>>>
>>>> Devices like IOMMU need to be created before others, so they can leverage
>>>> the DeviceCreationPriority flag introduced by the first patch to DeviceClass.
>>>>
>>>> The second patch sorts the devices by their DeviceCreationPriority
>>>> before creating them.
>>>>
>>>> Finally, the last patch demonstrates how it can be used to ensure
>>>> the creation of host-bridges before the pci-bridges and pci-bridges before
>>>> the others.
>>>>
>>>> I preferred to combine all the priorities into a single enum
>>>> to better manage the creation order.
>>>>
>>>> This is an RFC because I only wanted to know if it seems like the right way to go.
>>>> Comments are appreciated,
>>>
>>
>> Hi Markus,
>> Thanks for looking into this.
>>
>>
>>> Can you explain why requiring the user to specify -device in a sane
>>> order isn't good enough?
>>>
>>
>> Point taken, the truth is I didn't like the 'order' restriction in the
>> first place.
>>
>> If the device creation depends on the id of some other devices (e.g we
>> need the bus id to plug a device into it), for IOMMU devices it gets a
>> little tricky. You can add the IOMMU device before other PCI devices
>> but it will not work (because some internal implementation). This is
>> why we added using -machine pc,iommu=on.  I suppose we have other
>> examples as well. This is not user friendly IMO.
>>
>> To solve the specific IOMMU problem we can check that there are no PCI
>> devices created yet, but I am not sure is a better approach and is
>> strictly related to this device.
>>
>> The goal is to be able to add more devices with -device and I thought
>> this kind of creation in steps may help.


Hi Markus,

>
> In my opinion, there are two sane ways to do command line options.
>
> One is to make order relevant, and process them strictly left to right.
>
> The other is to do the right thing regardless of order.  This requires
> some kind of dependency tracking if there are any.
>

I personally like this way more, however I confess I do not aim to solve this
globally, my scope is making more user friendly the stuff I work with.

> QEMU, of course, does neither: we process them in left to right unless
> we don't, and users juggle them until the errors go away.
>

And this is why I thought, we have some case the order is important,
some cases it is not. Adding one more case to "order not important" set
looks like a little win.

> I'm afraid this patch adds to "unless we don't" without covering much
> ground towards "do the right thing regardless of order".  Static
> priorities are a rather crude approximation of dependencies.  Is it the
> best we can do for user now?
>

As stated above, I am perfectly aware the static priorities angle does not
solve all the problems, but it helps and may be a good step in the right direction.

The static priorities can make sense, the host bridge needs to be created before
the other pci bridges, which should also be created before the devices itself.

IOMMU can also leverage static priorities, it needs to be created after the host bridge,
but before anything else.

I am sure we can think of other cases those will help too. To answer your question,
no, is not the best we can do, but maybe it worth it.

Thanks,
Marcel