Patchwork [v2,03/15] qdev: Allow device addressing via 'driver.instance'

login
register
mail settings
Submitter Jan Kiszka
Date May 22, 2010, 8:18 a.m.
Message ID <fa4946308c87d4ca2dd3d53b00da1df0f3541e30.1274516288.git.jan.kiszka@web.de>
Download mbox | patch
Permalink /patch/53251/
State New
Headers show

Comments

Jan Kiszka - May 22, 2010, 8:18 a.m.
From: Jan Kiszka <jan.kiszka@siemens.com>

Extend qbus_find_dev to allow addressing of devices without an unique id
via an optional per-bus instance number. The new formats are
'driver.instance' and 'alias.instance'.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 docs/qdev-device-use.txt |   12 +++++++++++-
 hw/qdev.c                |   23 ++++++++++++++++++-----
 2 files changed, 29 insertions(+), 6 deletions(-)
Markus Armbruster - May 29, 2010, 7:50 a.m.
Jan Kiszka <jan.kiszka@web.de> writes:

> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> Extend qbus_find_dev to allow addressing of devices without an unique id
> via an optional per-bus instance number. The new formats are
> 'driver.instance' and 'alias.instance'.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  docs/qdev-device-use.txt |   12 +++++++++++-
>  hw/qdev.c                |   23 ++++++++++++++++++-----
>  2 files changed, 29 insertions(+), 6 deletions(-)
>
> diff --git a/docs/qdev-device-use.txt b/docs/qdev-device-use.txt
> index 9ac1fa1..5939481 100644
> --- a/docs/qdev-device-use.txt
> +++ b/docs/qdev-device-use.txt
> @@ -1,6 +1,6 @@
>  = How to convert to -device & friends =
>  
> -=== Specifying Bus and Address on Bus ===
> +=== Specifying Bus, Address on Bus, and Devices ===
>  
>  In qdev, each device has a parent bus.  Some devices provide one or
>  more buses for children.  You can specify a device's parent bus with
> @@ -24,6 +24,16 @@ Furthermore, if a device only hosts a single bus, the bus name can be
>  omitted in the path.  Example: /i440FX-pcihost/PIIX3 abbreviates
>  /i440FX-pcihost/pci.0/PIIX3/isa.0 as none of the buses has siblings.
>  
> +Existing devices can be addressed either via a unique ID if it was
> +assigned during creation or via the device tree path:
> +
> +/full_bus_address/driver_name[.instance_number]
> +    or
> +abbreviated_bus_address/driver_name[.instance_number]
> +
> +Example: /i440FX-pcihost/pci.0/e1000.2 addresses the second e1000
> +adapter on the bus 'pci.0'.
> +
>  Note: the USB device address can't be controlled at this time.

"instance number" isn't defined in this document.

I understand the problem you're trying to solve; I've had it myself many
times.  But is inventing an instance number the right solution?

The two e1000 devices already have a perfectly fine unique identifier on
their bus: their bus address.  What about recognizing bus addresses in
paths?  Requires a suitable restriction on device names and IDs to avoid
ambiguity, say "start with letter".

qdev currently doesn't abstract bus addresses, but that's fixable.

[...]
Jan Kiszka - May 29, 2010, 8:09 a.m.
Markus Armbruster wrote:
> Jan Kiszka <jan.kiszka@web.de> writes:
> 
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> Extend qbus_find_dev to allow addressing of devices without an unique id
>> via an optional per-bus instance number. The new formats are
>> 'driver.instance' and 'alias.instance'.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  docs/qdev-device-use.txt |   12 +++++++++++-
>>  hw/qdev.c                |   23 ++++++++++++++++++-----
>>  2 files changed, 29 insertions(+), 6 deletions(-)
>>
>> diff --git a/docs/qdev-device-use.txt b/docs/qdev-device-use.txt
>> index 9ac1fa1..5939481 100644
>> --- a/docs/qdev-device-use.txt
>> +++ b/docs/qdev-device-use.txt
>> @@ -1,6 +1,6 @@
>>  = How to convert to -device & friends =
>>  
>> -=== Specifying Bus and Address on Bus ===
>> +=== Specifying Bus, Address on Bus, and Devices ===
>>  
>>  In qdev, each device has a parent bus.  Some devices provide one or
>>  more buses for children.  You can specify a device's parent bus with
>> @@ -24,6 +24,16 @@ Furthermore, if a device only hosts a single bus, the bus name can be
>>  omitted in the path.  Example: /i440FX-pcihost/PIIX3 abbreviates
>>  /i440FX-pcihost/pci.0/PIIX3/isa.0 as none of the buses has siblings.
>>  
>> +Existing devices can be addressed either via a unique ID if it was
>> +assigned during creation or via the device tree path:
>> +
>> +/full_bus_address/driver_name[.instance_number]
>> +    or
>> +abbreviated_bus_address/driver_name[.instance_number]
>> +
>> +Example: /i440FX-pcihost/pci.0/e1000.2 addresses the second e1000
>> +adapter on the bus 'pci.0'.
>> +
>>  Note: the USB device address can't be controlled at this time.
> 
> "instance number" isn't defined in this document.

True, only implicitly via the example.

> 
> I understand the problem you're trying to solve; I've had it myself many
> times.  But is inventing an instance number the right solution?
> 
> The two e1000 devices already have a perfectly fine unique identifier on
> their bus: their bus address.  What about recognizing bus addresses in
> paths?  Requires a suitable restriction on device names and IDs to avoid
> ambiguity, say "start with letter".
> 
> qdev currently doesn't abstract bus addresses, but that's fixable.

You would also have to specify unique addressing scheme to those buses
that do not have it yet. E.g. what should be the address of a ISA bus
device? The base of its first ioport range? But if it does not have any
as it only injects ISA IRQs?

Jan
Markus Armbruster - May 31, 2010, 8:43 a.m.
Jan Kiszka <jan.kiszka@web.de> writes:

> Markus Armbruster wrote:
>> Jan Kiszka <jan.kiszka@web.de> writes:
>> 
>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> Extend qbus_find_dev to allow addressing of devices without an unique id
>>> via an optional per-bus instance number. The new formats are
>>> 'driver.instance' and 'alias.instance'.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> ---
>>>  docs/qdev-device-use.txt |   12 +++++++++++-
>>>  hw/qdev.c                |   23 ++++++++++++++++++-----
>>>  2 files changed, 29 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/docs/qdev-device-use.txt b/docs/qdev-device-use.txt
>>> index 9ac1fa1..5939481 100644
>>> --- a/docs/qdev-device-use.txt
>>> +++ b/docs/qdev-device-use.txt
>>> @@ -1,6 +1,6 @@
>>>  = How to convert to -device & friends =
>>>  
>>> -=== Specifying Bus and Address on Bus ===
>>> +=== Specifying Bus, Address on Bus, and Devices ===
>>>  
>>>  In qdev, each device has a parent bus.  Some devices provide one or
>>>  more buses for children.  You can specify a device's parent bus with
>>> @@ -24,6 +24,16 @@ Furthermore, if a device only hosts a single bus, the bus name can be
>>>  omitted in the path.  Example: /i440FX-pcihost/PIIX3 abbreviates
>>>  /i440FX-pcihost/pci.0/PIIX3/isa.0 as none of the buses has siblings.
>>>  
>>> +Existing devices can be addressed either via a unique ID if it was
>>> +assigned during creation or via the device tree path:
>>> +
>>> +/full_bus_address/driver_name[.instance_number]
>>> +    or
>>> +abbreviated_bus_address/driver_name[.instance_number]
>>> +
>>> +Example: /i440FX-pcihost/pci.0/e1000.2 addresses the second e1000
>>> +adapter on the bus 'pci.0'.
>>> +
>>>  Note: the USB device address can't be controlled at this time.
>> 
>> "instance number" isn't defined in this document.
>
> True, only implicitly via the example.
>
>> 
>> I understand the problem you're trying to solve; I've had it myself many
>> times.  But is inventing an instance number the right solution?
>> 
>> The two e1000 devices already have a perfectly fine unique identifier on
>> their bus: their bus address.  What about recognizing bus addresses in
>> paths?  Requires a suitable restriction on device names and IDs to avoid
>> ambiguity, say "start with letter".
>> 
>> qdev currently doesn't abstract bus addresses, but that's fixable.
>
> You would also have to specify unique addressing scheme to those buses
> that do not have it yet. E.g. what should be the address of a ISA bus
> device? The base of its first ioport range? But if it does not have any
> as it only injects ISA IRQs?

Hmm, instance numbers are the lesser evil in this case.

Define them properly, and make info qtree show them, and we'll be okay,
I guess.

Patch

diff --git a/docs/qdev-device-use.txt b/docs/qdev-device-use.txt
index 9ac1fa1..5939481 100644
--- a/docs/qdev-device-use.txt
+++ b/docs/qdev-device-use.txt
@@ -1,6 +1,6 @@ 
 = How to convert to -device & friends =
 
-=== Specifying Bus and Address on Bus ===
+=== Specifying Bus, Address on Bus, and Devices ===
 
 In qdev, each device has a parent bus.  Some devices provide one or
 more buses for children.  You can specify a device's parent bus with
@@ -24,6 +24,16 @@  Furthermore, if a device only hosts a single bus, the bus name can be
 omitted in the path.  Example: /i440FX-pcihost/PIIX3 abbreviates
 /i440FX-pcihost/pci.0/PIIX3/isa.0 as none of the buses has siblings.
 
+Existing devices can be addressed either via a unique ID if it was
+assigned during creation or via the device tree path:
+
+/full_bus_address/driver_name[.instance_number]
+    or
+abbreviated_bus_address/driver_name[.instance_number]
+
+Example: /i440FX-pcihost/pci.0/e1000.2 addresses the second e1000
+adapter on the bus 'pci.0'.
+
 Note: the USB device address can't be controlled at this time.
 
 === Block Devices ===
diff --git a/hw/qdev.c b/hw/qdev.c
index 2e50531..6b4a629 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -527,28 +527,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;
         }
     }