Patchwork [1/8] qdev: Allow device addressing via 'driver.instance'

login
register
mail settings
Submitter Jan Kiszka
Date May 14, 2010, 1:20 p.m.
Message ID <913f4f6a67305a72ed3d994bd12d5b34de9b9bb9.1273843151.git.jan.kiszka@siemens.com>
Download mbox | patch
Permalink /patch/52608/
State New
Headers show

Comments

Jan Kiszka - May 14, 2010, 1:20 p.m.
Extend qbus_find_dev to allow addressing of devices without an unique id
via an optional instance number. The new formats are 'driver.instance'
and 'alias.instance'.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/qdev.c |   23 ++++++++++++++++++-----
 1 files changed, 18 insertions(+), 5 deletions(-)
Markus Armbruster - May 18, 2010, 12:15 p.m.
Jan Kiszka <jan.kiszka@siemens.com> writes:

> Extend qbus_find_dev to allow addressing of devices without an unique id
> via an optional instance number. The new formats are 'driver.instance'
> and 'alias.instance'.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

How's the instance number defined?  Should be documented.

Is an instance number a good user interface?  Gerd, what do you think?

[...]
Gerd Hoffmann - May 18, 2010, 12:31 p.m.
On 05/18/10 14:15, Markus Armbruster wrote:
> Jan Kiszka<jan.kiszka@siemens.com>  writes:
>
>> Extend qbus_find_dev to allow addressing of devices without an unique id
>> via an optional instance number. The new formats are 'driver.instance'
>> and 'alias.instance'.
>>
>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>
> How's the instance number defined?  Should be documented.

savevm instance id, used to identify multiple instances of some device 
on loadvm.  By default is just incrementing (0,1,2,...) for each new 
device instance I think.  Drivers can also specify one.  Most don't do 
that.  IIRC some ISA drivers use the base ioport as instance id, which 
sort-of makes sense as it makes sure the id identifies the correct 
device no matter what the initialization order is.

It probably makes sense to replace the instance id with the device path 
once all devices are converted over to qdev+vmstate, so we avoid the 
nasty ordering issues altogether.

cheers,
   Gerd
Juan Quintela - May 18, 2010, 12:38 p.m.
Gerd Hoffmann <kraxel@redhat.com> wrote:
> On 05/18/10 14:15, Markus Armbruster wrote:
>> Jan Kiszka<jan.kiszka@siemens.com>  writes:
>>
>>> Extend qbus_find_dev to allow addressing of devices without an unique id
>>> via an optional instance number. The new formats are 'driver.instance'
>>> and 'alias.instance'.
>>>
>>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>>
>> How's the instance number defined?  Should be documented.
>
> savevm instance id, used to identify multiple instances of some device
> on loadvm.  By default is just incrementing (0,1,2,...) for each new
> device instance I think.  Drivers can also specify one.  Most don't do
> that.  IIRC some ISA drivers use the base ioport as instance id, which
> sort-of makes sense as it makes sure the id identifies the correct
> device no matter what the initialization order is.
>
> It probably makes sense to replace the instance id with the device
> path once all devices are converted over to qdev+vmstate, so we avoid
> the nasty ordering issues altogether.

Agreed.  The problem here is that we sent the instance_id on the wire,
so for "legacy" devices that used an instance_id != -1, we are stuck
with it :(

Guess why a friend calls "backwards compatibility": A Curse of Bible
proportions :(

Later, Juan.
Gerd Hoffmann - May 18, 2010, 1:06 p.m.
Hi,

> Agreed.  The problem here is that we sent the instance_id on the wire,
> so for "legacy" devices that used an instance_id != -1, we are stuck
> with it :(

Another way would be to fill the instance id with something useful, say 
encode the pci address there for pci devices.  That should be good 
enough to reliably identify devices even after hot-plugging them in and out.

Of course this has transition isses too.  But maybe it is easier to 
handle than replacing instance_id with something entirely different.

And it also makes the instance id less useful to address devices in a 
human-friendly way.

cheers,
   Gerd
Jan Kiszka - May 18, 2010, 4:54 p.m.
Gerd Hoffmann wrote:
> On 05/18/10 14:15, Markus Armbruster wrote:
>> Jan Kiszka<jan.kiszka@siemens.com>  writes:
>>
>>> Extend qbus_find_dev to allow addressing of devices without an unique id
>>> via an optional instance number. The new formats are 'driver.instance'
>>> and 'alias.instance'.
>>>
>>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>>
>> How's the instance number defined?  Should be documented.

OK, will do. For now it is the per-bus instance ID. I think that makes
most sense and is easily handleable. Still, we should probably print it
also via "info qtree" (and a future "query-qtree").

> 
> savevm instance id, used to identify multiple instances of some device
> on loadvm.  By default is just incrementing (0,1,2,...) for each new
> device instance I think.  Drivers can also specify one.  Most don't do
> that.  IIRC some ISA drivers use the base ioport as instance id, which
> sort-of makes sense as it makes sure the id identifies the correct
> device no matter what the initialization order is.

That io-address-based instance numbers have just been deprecated, see
4d2ffa08b601bdd40d9ccf225480c0a7e90ca078. ISA devices are already
converted, there are just a few non-PC devices remaining that don't use
the (auto-generated) vmstate instance number. But that is actually a
different, user-invisible numbering scheme.

> 
> It probably makes sense to replace the instance id with the device path
> once all devices are converted over to qdev+vmstate, so we avoid the
> nasty ordering issues altogether.

You are always free to address devices via a unique user-defined ID.

Jan
Avi Kivity - May 19, 2010, 8:29 a.m.
On 05/18/2010 03:31 PM, Gerd Hoffmann wrote:
> On 05/18/10 14:15, Markus Armbruster wrote:
>> Jan Kiszka<jan.kiszka@siemens.com>  writes:
>>
>>> Extend qbus_find_dev to allow addressing of devices without an 
>>> unique id
>>> via an optional instance number. The new formats are 'driver.instance'
>>> and 'alias.instance'.
>>>
>>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>>
>> How's the instance number defined?  Should be documented.
>
> savevm instance id, used to identify multiple instances of some device 
> on loadvm.  By default is just incrementing (0,1,2,...) for each new 
> device instance I think.

That's an implementation detail that's being exposed in an external 
interface.  Granted, users shouldn't expect this to remain stable across 
invocations, yet it makes me uncomfortable.

Why not always use topology to locate devices?


>   Drivers can also specify one.  Most don't do that.  IIRC some ISA 
> drivers use the base ioport as instance id, which sort-of makes sense 
> as it makes sure the id identifies the correct device no matter what 
> the initialization order is.
>
> It probably makes sense to replace the instance id with the device 
> path once all devices are converted over to qdev+vmstate, so we avoid 
> the nasty ordering issues altogether.

Oh, that's what you're suggesting.  So we agree.

Patch

diff --git a/hw/qdev.c b/hw/qdev.c
index d3bf0fa..fe49e71 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -515,28 +515,41 @@  static BusState *qbus_find_bus(DeviceState *dev, char *elem)
     return NULL;
 }
 
-static DeviceState *qbus_find_dev(BusState *bus, char *elem)
+static DeviceState *qbus_find_dev(BusState *bus, const char *elem)
 {
     DeviceState *dev;
+    int instance, n;
+    char buf[128];
 
     /*
      * try to match in order:
      *   (1) instance id, if present
-     *   (2) driver name
-     *   (3) driver alias, if present
+     *   (2) driver name [.instance]
+     *   (3) driver alias [.instance], if present
      */
     QLIST_FOREACH(dev, &bus->children, sibling) {
         if (dev->id  &&  strcmp(dev->id, elem) == 0) {
             return dev;
         }
     }
+
+    if (sscanf(elem, "%127[^.].%u", buf, &instance) == 2) {
+        elem = buf;
+    } else {
+        instance = 0;
+    }
+
+    n = 0;
     QLIST_FOREACH(dev, &bus->children, sibling) {
-        if (strcmp(dev->info->name, elem) == 0) {
+        if (strcmp(dev->info->name, elem) == 0 && n++ == instance) {
             return dev;
         }
     }
+
+    n = 0;
     QLIST_FOREACH(dev, &bus->children, sibling) {
-        if (dev->info->alias && strcmp(dev->info->alias, elem) == 0) {
+        if (dev->info->alias && strcmp(dev->info->alias, elem) == 0 &&
+            n++ == instance) {
             return dev;
         }
     }