Re: [Qemu-devel] [PATCH v4 3/4] hw/core: explicit OFW unit address callback for SysBusDeviceClass

[Markus Armbruster]

Laszlo Ersek <address@hidden> writes:

> The sysbus_get_fw_dev_path() function formats OpenFirmware device path
> nodes ("address@hidden") 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 <address@hidden>
> Cc: Marcel Apfelbaum <address@hidden>
> Cc: Michael S. Tsirkin <address@hidden>
> Signed-off-by: Laszlo Ersek <address@hidden>
> ---
>
> 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("address@hidden", 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("address@hidden", 
> 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 <address@hidden>