Re: [Qemu-devel] [PATCH v1 1/3] qdev: Expose the qdev id string as a prop

[Peter Crosthwaite]

On Thu, Apr 17, 2014 at 3:20 AM, Andreas Färber <address@hidden> wrote:
> Am 15.04.2014 23:39, schrieb Peter Crosthwaite:
>> On Wed, Apr 16, 2014 at 2:16 AM, Andreas Färber <address@hidden> wrote:
>>> Am 15.04.2014 04:21, schrieb Peter Crosthwaite:
>>>> So clients can set the top level id string.
>>>>
>>>> Signed-off-by: Peter Crosthwaite <address@hidden>
>>>
>>> Anthony had nack'ed Paolo's attempt to generalize the qdev id to QOM, so
>>> I'm not sure if we should really do this even if just on device level.
>>> The id= is used to construct the canonical path, and we can't change
>>> that through a qdev setter.
>>
>> How can we change it? The problem I am trying to solve, is getting
>> meaningful device instance names instead of the anonymous defaults.
>> This includes in the canonical path. I am completely open to proposals
>> :)
>
> Possibly this is just a mixup, see below. On yesterday's KVM call
> "instance names" referred to VMState names and instance_ids and
> qdev/qbus paths. Then there's the device IDs that this patch was
> touching. And there's the canonical QOM composition tree paths, that for
> user-created devices may include the device ID.
>
>>  Using a dynamic property might allow us to
>>> unparent while keeping a reference and then add it as a child<> again,
>>> but that still feels awkward.
>>
>> Eww.
>>
>>  Having it as a getter only seem more
>>> secure and predictable.
>>>
>>
>> Sure, once it's committed to the canonical path it definitely needs a
>> read-only semantic.
>
>> Shouldn't be hugely different to the
>> writable-until-realize semantic of qdev props though?
>
> It is to some degree. The canonical paths get set up as part of or right
> after instance_init. Only legacy devices without canonical path get a
> path as part of realize, but that needs to be fixed for recursive
> realization: Devices need to be accessible somewhere in the tree to be
> user-modified via qom-set, and they will need to be in the tree to be
> realized.
>
>>> Another issue is bus naming. If supplied NULL, the bus will be based on
>>> ID. If the ID changes, bus naming will be inconsistent.
>>>
>>
>> Or rather - what the user intends. If the board has 2 USB busses named
>> "foo" and "bar", then those should be both the canonical path and bus
>> names. Rather than the homogenised default names "usb0", "usb1".
>
> Let's not discuss this here again, it's being discussed between ppc and
> libvirt already. If the device supplies a hardcoded bus name then (while
> it may not be unique) we don't have to care in the context of IDs here.
> The interesting case is if we create a device with id=foo, and the bus
> uses NULL as indicated above, then it will get foo.0, foo.1, etc. unless
> I've missed something. IDs are unique, so if you have only one bus per
> device you get foo.0 and bar.0; if a device author chooses not to use
> NULL as bus name and rolls their own then we can't help them on the
> DeviceState level.
>
>>> Why would clients set the ID after having chosen a different ID
>>
>> I'm lost here - what is the mechanism by which you can validly set
>> per-instance IDs?
>
> Sorry, maybe I don't fully get your question...
>
> Either the user specifies ,id=foo on command line or HMP, or she
> doesn't. The corresponding mechanism in config files is [device "foo"],
> see docs/q35-chipset.cfg for an example.
>
> qdev paths used for VMState currently still depend on a bus, which Alex
> or Alexey wanted to look into fixing. By default -1 is supplied by
> DeviceState for registering dc->vmsd, which can be overridden by
> inlining the corresponding qdev.c code or more conveniently - Alex'
> suggestion yesterday - by moving that default value of -1 into an
> overrideable instance field.
>
> As for canonical QOM paths, it's the job of machines and devices to set
> up those - unless a peripheral device is added by the user, in which
> case see previous explanations by Markus and me.
>
> You might remember my big MPCore refactoring? That was supposed to be
> the building block for you to create a proper SoC container device for
> Zynq and thereby constructing meaningful canonical paths. I.e., a
> ZedBoard or MicroZed or Parallella board instantiates the SoC as
> /machine/zynq, the Zynq SoC makes available /machine/zynq/cortex or
> whatever (-> PMM; recursively ./scu etc.) plus memory-controller (this
> series), uart and whatever Zynq peripherals.

I am aware of this and has been on my radar for quite some time.

You can't expect me to
> dictate paths for each model, that's no generic task; you need to take
> care of naming your favorite device models yourself so that *no*
> /machine/unassigned/device[n] remains. This may involve, such as in
> Anthony's case of i440fx or in case of function-driven Exynos/Zynq/...
> code, refactoring parts of devices into child<> devices for aggregation
> rather than just assigning names to existing devices on /machine.
>
> BTW that's also why - e.g. in the Xilinx context where you discussed
> inlining with Edgar - having qdev-style void create-foo wrappers hiding
> the object creation is undesirable since, however wrapped into helpers,
> we need to access the Object for adding as child<> property from its
> caller for uniqueness, usually.
>

My FWIW my motivations were different there but I see the point.

> Hope that explains all device naming.
>
> Now, the unanswered question is what use case do you need meaningful
> names for? Command line? QOM tree? Migration? Something else?
>

I guess the main one (in the context of this series) is propagation of
a meaningful name to the memory region instantiator. It will be
something of a regression if instead of the MR names being "zynq.ram"
and "zynq.ocm" they become homogenized to "ram", "ram".

I could just work around this by adding a memory-region-name property
specific to my new device (see P2), but this soln. seemed more concise
as the one name is now shared and consistent between qtree and mtree.

Regards,
Peter

> Regards,
> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>