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

[Laszlo Ersek]

On 06/15/15 16:33, Markus Armbruster wrote:
> 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?

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 <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.
>      */

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("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.

Will do.

> Matter of
> taste, so
> 
> Reviewed-by: Markus Armbruster <address@hidden>

Awesome! :) Thank you!
Laszlo