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

[Andreas Färber]

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

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?

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