[v4,3/4] hw/core: explicit OFW unit address callback for SysBusDeviceClass

The sysbus_get_fw_dev_path() function formats OpenFirmware device path
nodes ("driver-name@unit-address") for sysbus devices. The first choice
for "unit-address" is the base address of the device's first MMIO region.
The second choice is its first IO port.

However, if two sysbus devices with the same "driver-name" lack both MMIO
and PIO resources, then there is no good way to distinguish them based on
their OFW nodes, because in this case unit-address is omitted completely
for both devices.

For the sake of such devices, introduce the explicit_ofw_unit_address()
"virtual member function". With this function, each sysbus device in the
same SysBusDeviceClass can state its own address.

Cc: Markus Armbruster <armbru@redhat.com>
Cc: Marcel Apfelbaum <marcel@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    v4:
    - Yet another approach. Instead of allowing the creator of the device to
      set a string property statically, introduce a class level callback.
    
    v3:
    - new in v3
    - new approach

 include/hw/sysbus.h |  9 +++++++++
 hw/core/sysbus.c    | 13 +++++++++++++
 2 files changed, 22 insertions(+)

Laszlo Ersek <lersek@redhat.com> writes:

> The sysbus_get_fw_dev_path() function formats OpenFirmware device path
> nodes ("driver-name@unit-address") for sysbus devices. The first choice
> for "unit-address" is the base address of the device's first MMIO region.
> The second choice is its first IO port.
>
> However, if two sysbus devices with the same "driver-name" lack both MMIO
> and PIO resources, then there is no good way to distinguish them based on
> their OFW nodes, because in this case unit-address is omitted completely
> for both devices.

Got an example for such a device?  Mind adding it to the commit message?

> For the sake of such devices, introduce the explicit_ofw_unit_address()
> "virtual member function". With this function, each sysbus device in the
> same SysBusDeviceClass can state its own address.
>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Marcel Apfelbaum <marcel@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>
> Notes:
>     v4:
>     - Yet another approach. Instead of allowing the creator of the device to
>       set a string property statically, introduce a class level callback.
>     
>     v3:
>     - new in v3
>     - new approach
>
>  include/hw/sysbus.h |  9 +++++++++
>  hw/core/sysbus.c    | 13 +++++++++++++
>  2 files changed, 22 insertions(+)
>
> diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
> index d1f3f00..63b036b 100644
> --- a/include/hw/sysbus.h
> +++ b/include/hw/sysbus.h
> @@ -41,6 +41,15 @@ typedef struct SysBusDeviceClass {
>      /*< public >*/
>  
>      int (*init)(SysBusDevice *dev);
> +
> +    /*
> +     * Sometimes a class of SysBusDevices has neither MMIO nor PIO resources,
> +     * yet instances of it would like to distinguish themselves, in
> +     * OpenFirmware device paths, from other instances of the same class on the
> +     * same sysbus. For that end we expose this callback. It returns a
> +     * dynamically allocated string.
> +     */
> +    char *(*explicit_ofw_unit_address)(SysBusDevice *dev);

I prefer function comments to follow a strict pattern:

    /*
     * Headline explaining the function's purpose[*]
     * Zero or more paragraphs explaining preconditions, side effects,
     * return values, error conditions.
     */

[*] If you can't come up with a headline fitting into a single line,
chances are the function does too many things.

>  } SysBusDeviceClass;
>  
>  struct SysBusDevice {
> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> index 0ebb4e2..a0ec814 100644
> --- a/hw/core/sysbus.c
> +++ b/hw/core/sysbus.c
> @@ -281,6 +281,7 @@ static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent)
>  static char *sysbus_get_fw_dev_path(DeviceState *dev)
>  {
>      SysBusDevice *s = SYS_BUS_DEVICE(dev);
> +    SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(s);
>  
>      if (s->num_mmio) {
>          return g_strdup_printf("%s@"TARGET_FMT_plx, qdev_fw_name(dev),
> @@ -289,6 +290,18 @@ static char *sysbus_get_fw_dev_path(DeviceState *dev)
>      if (s->num_pio) {
>          return g_strdup_printf("%s@i%04x", qdev_fw_name(dev), s->pio[0]);
>      }
> +    if (sbc->explicit_ofw_unit_address) {
> +        char *addr;
> +
> +        addr = sbc->explicit_ofw_unit_address(s);
> +        if (addr) {
> +            char *fw_dev_path;
> +
> +            fw_dev_path = g_strdup_printf("%s@%s", qdev_fw_name(dev), addr);
> +            g_free(addr);
> +            return fw_dev_path;
> +        }
> +    }
>      return g_strdup(qdev_fw_name(dev));
>  }

In short functions like this one, I prefer to have declarations out of
the way in one place rather than cluttering inner blocks.  Matter of
taste, so

Reviewed-by: Markus Armbruster <armbru@redhat.com>

On 06/15/15 16:33, Markus Armbruster wrote:
> Laszlo Ersek <lersek@redhat.com> writes:
> 
>> The sysbus_get_fw_dev_path() function formats OpenFirmware device path
>> nodes ("driver-name@unit-address") for sysbus devices. The first choice
>> for "unit-address" is the base address of the device's first MMIO region.
>> The second choice is its first IO port.
>>
>> However, if two sysbus devices with the same "driver-name" lack both MMIO
>> and PIO resources, then there is no good way to distinguish them based on
>> their OFW nodes, because in this case unit-address is omitted completely
>> for both devices.
> 
> Got an example for such a device?  Mind adding it to the commit message?

That's the right next patch in the series (on which I didn't Cc you,
apologies). If you'd like I can hint at the next patch / the device in
question (PXB) in the commit message.

> 
>> For the sake of such devices, introduce the explicit_ofw_unit_address()
>> "virtual member function". With this function, each sysbus device in the
>> same SysBusDeviceClass can state its own address.
>>
>> Cc: Markus Armbruster <armbru@redhat.com>
>> Cc: Marcel Apfelbaum <marcel@redhat.com>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>
>> Notes:
>>     v4:
>>     - Yet another approach. Instead of allowing the creator of the device to
>>       set a string property statically, introduce a class level callback.
>>     
>>     v3:
>>     - new in v3
>>     - new approach
>>
>>  include/hw/sysbus.h |  9 +++++++++
>>  hw/core/sysbus.c    | 13 +++++++++++++
>>  2 files changed, 22 insertions(+)
>>
>> diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
>> index d1f3f00..63b036b 100644
>> --- a/include/hw/sysbus.h
>> +++ b/include/hw/sysbus.h
>> @@ -41,6 +41,15 @@ typedef struct SysBusDeviceClass {
>>      /*< public >*/
>>  
>>      int (*init)(SysBusDevice *dev);
>> +
>> +    /*
>> +     * Sometimes a class of SysBusDevices has neither MMIO nor PIO resources,
>> +     * yet instances of it would like to distinguish themselves, in
>> +     * OpenFirmware device paths, from other instances of the same class on the
>> +     * same sysbus. For that end we expose this callback. It returns a
>> +     * dynamically allocated string.
>> +     */
>> +    char *(*explicit_ofw_unit_address)(SysBusDevice *dev);
> 
> I prefer function comments to follow a strict pattern:
> 
>     /*
>      * Headline explaining the function's purpose[*]
>      * Zero or more paragraphs explaining preconditions, side effects,
>      * return values, error conditions.
>      */

I follow a very similar requirement in all my edk2 code closely -- but
in edk2 that's actually a *requirement*. :) I wasn't aware of any such
requirement in QEMU, and I thought "any function comment will be seen as
a bonus". :)

I'll rewrite the comment like this, thanks.

> [*] If you can't come up with a headline fitting into a single line,
> chances are the function does too many things.

"Delegate formatting of non-IO, non-MMIO address of sysbus device, due
to bus not knowing."

> 
>>  } SysBusDeviceClass;
>>  
>>  struct SysBusDevice {
>> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
>> index 0ebb4e2..a0ec814 100644
>> --- a/hw/core/sysbus.c
>> +++ b/hw/core/sysbus.c
>> @@ -281,6 +281,7 @@ static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent)
>>  static char *sysbus_get_fw_dev_path(DeviceState *dev)
>>  {
>>      SysBusDevice *s = SYS_BUS_DEVICE(dev);
>> +    SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(s);
>>  
>>      if (s->num_mmio) {
>>          return g_strdup_printf("%s@"TARGET_FMT_plx, qdev_fw_name(dev),
>> @@ -289,6 +290,18 @@ static char *sysbus_get_fw_dev_path(DeviceState *dev)
>>      if (s->num_pio) {
>>          return g_strdup_printf("%s@i%04x", qdev_fw_name(dev), s->pio[0]);
>>      }
>> +    if (sbc->explicit_ofw_unit_address) {
>> +        char *addr;
>> +
>> +        addr = sbc->explicit_ofw_unit_address(s);
>> +        if (addr) {
>> +            char *fw_dev_path;
>> +
>> +            fw_dev_path = g_strdup_printf("%s@%s", qdev_fw_name(dev), addr);
>> +            g_free(addr);
>> +            return fw_dev_path;
>> +        }
>> +    }
>>      return g_strdup(qdev_fw_name(dev));
>>  }
> 
> In short functions like this one, I prefer to have declarations out of
> the way in one place rather than cluttering inner blocks.

Will do.

> Matter of
> taste, so
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Awesome! :) Thank you!
Laszlo