| Message ID | 8a19b27e4e9eed971bdd4e0d5e5789a923f8c8b6.1397527916.git.peter.crosthwaite@xilinx.com |
|---|---|
| State | New |
| Headers | show |
Am 15.04.2014 04:21, schrieb Peter Crosthwaite: > So clients can set the top level id string. > > Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> 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. 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. Having it as a getter only seem more secure and predictable. Another issue is bus naming. If supplied NULL, the bus will be based on ID. If the ID changes, bus naming will be inconsistent. Why would clients set the ID after having chosen a different ID or anonymity? Regards, Andreas
On Wed, Apr 16, 2014 at 2:16 AM, Andreas Färber <afaerber@suse.de> wrote: > Am 15.04.2014 04:21, schrieb Peter Crosthwaite: >> So clients can set the top level id string. >> >> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > > 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 :) 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? > 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". > 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? Regards, Peter or > anonymity? > > 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 >
Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes: > On Wed, Apr 16, 2014 at 2:16 AM, Andreas Färber <afaerber@suse.de> wrote: >> Am 15.04.2014 04:21, schrieb Peter Crosthwaite: >>> So clients can set the top level id string. >>> >>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >> >> 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. Let me try to paraphrase to make sure I got you: The canonical QOM path is fixed at creation time. Setting an ID dynamically would destroy the relationship between ID and canonical QOM path. Correct? > 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 > :) The qdev ID is for users. Corollary: there is none unless the user sets one. The qdev ID is fixed at creation time, and never changes. If it could change, we'd need machinery to notify clients such as libvirt, and we'd still suffer race conditions in multiple monitor situations. The need for a way to uniquely identify device instances by some name is real. But qdev IDs as presently designed are not a solution. What about the canonical QOM path? [...]
Am 16.04.2014 08:42, schrieb Markus Armbruster: > Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes: > >> On Wed, Apr 16, 2014 at 2:16 AM, Andreas Färber <afaerber@suse.de> wrote: >>> Am 15.04.2014 04:21, schrieb Peter Crosthwaite: >>>> So clients can set the top level id string. >>>> >>>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >>> >>> 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. > > Let me try to paraphrase to make sure I got you: The canonical QOM path > is fixed at creation time. Setting an ID dynamically would destroy the > relationship between ID and canonical QOM path. Correct? Kind of. If we use, say, qemu-system-x86_64 -device nec-usb-xhci, then it ends up as /machine/peripheral-anon/device[0] currently. No problem. If we instead use qemu-system-x86_64 -device nec-usb-xhci,id=foo, then it becomes /machine/peripheral/foo, with "foo" being the name of the child<> property named after the id= value. Not only can there be just one canonical path, each object can only have just one parent. So either /machine/peripheral/foo remains when renamed to ID bar, or we need to destroy .../foo and then re-create .../bar. I'm not aware of any precedent for the latter, so it would require some care to not let the object die for lack of references in the middle of the operation, for instance, or to survive unparenting at all due to bad programmer assumptions. While at QMP level link<> properties are treated as string paths, at QOM level they are represented as C pointers and the canonical path is reconstructed in the property getter. I.e., link<>s would not break if the canonical path changes, but a QMP user may get confused. Cheers, Andreas
Am 15.04.2014 23:39, schrieb Peter Crosthwaite: > On Wed, Apr 16, 2014 at 2:16 AM, Andreas Färber <afaerber@suse.de> wrote: >> Am 15.04.2014 04:21, schrieb Peter Crosthwaite: >>> So clients can set the top level id string. >>> >>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >> >> 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
On Thu, Apr 17, 2014 at 3:20 AM, Andreas Färber <afaerber@suse.de> wrote: > Am 15.04.2014 23:39, schrieb Peter Crosthwaite: >> On Wed, Apr 16, 2014 at 2:16 AM, Andreas Färber <afaerber@suse.de> wrote: >>> Am 15.04.2014 04:21, schrieb Peter Crosthwaite: >>>> So clients can set the top level id string. >>>> >>>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >>> >>> 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 >
Andreas Färber <afaerber@suse.de> writes: > Am 16.04.2014 08:42, schrieb Markus Armbruster: >> Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes: >> >>> On Wed, Apr 16, 2014 at 2:16 AM, Andreas Färber <afaerber@suse.de> wrote: >>>> Am 15.04.2014 04:21, schrieb Peter Crosthwaite: >>>>> So clients can set the top level id string. >>>>> >>>>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >>>> >>>> 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. >> >> Let me try to paraphrase to make sure I got you: The canonical QOM path >> is fixed at creation time. Setting an ID dynamically would destroy the >> relationship between ID and canonical QOM path. Correct? > > Kind of. > > If we use, say, qemu-system-x86_64 -device nec-usb-xhci, then it ends up > as /machine/peripheral-anon/device[0] currently. No problem. > > If we instead use qemu-system-x86_64 -device nec-usb-xhci,id=foo, then > it becomes /machine/peripheral/foo, with "foo" being the name of the > child<> property named after the id= value. > Not only can there be just one canonical path, each object can only have > just one parent. So either /machine/peripheral/foo remains when renamed > to ID bar, or we need to destroy .../foo and then re-create .../bar. I'm > not aware of any precedent for the latter, so it would require some care > to not let the object die for lack of references in the middle of the > operation, for instance, or to survive unparenting at all due to bad > programmer assumptions. Moving a live device around doesn't strike me as a good idea. > While at QMP level link<> properties are treated as string paths, at QOM > level they are represented as C pointers and the canonical path is > reconstructed in the property getter. I.e., link<>s would not break if > the canonical path changes, but a QMP user may get confused. Indeed. Thanks!
Il 16/04/2014 19:20, Andreas Färber ha scritto: > 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. Or even better, do not use the instance_id. It is prone to break when you have hotplug (because instance_ids on the source and destination might be different). instance_ids work for sysbus, but for everything else they'd better be avoided. In order to migrate hotpluggable busless devices, we will need an alternative, device-based implementation of qdev_get_dev_path. DIMM will be the first example of a hotpluggable busless device, but I think it is stateless so the problem will not be there. Paolo
diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 60f9df1..a32e39b 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -833,13 +833,16 @@ static void device_initfn(Object *obj) device_get_hotpluggable, NULL, NULL); class = object_get_class(OBJECT(dev)); - do { + for (;;) { for (prop = DEVICE_CLASS(class)->props; prop && prop->name; prop++) { qdev_property_add_legacy(dev, prop, &error_abort); qdev_property_add_static(dev, prop, &error_abort); } + if (class == object_class_by_name(TYPE_DEVICE)) { + break; + } class = object_class_get_parent(class); - } while (class != object_class_by_name(TYPE_DEVICE)); + } object_property_add_link(OBJECT(dev), "parent_bus", TYPE_BUS, (Object **)&dev->parent_bus, NULL, 0, @@ -906,6 +909,11 @@ static void device_unparent(Object *obj) } } +static Property device_props[] = { + DEFINE_PROP_STRING("device-id", DeviceState, id), + DEFINE_PROP_END_OF_LIST(), +}; + static void device_class_init(ObjectClass *class, void *data) { DeviceClass *dc = DEVICE_CLASS(class); @@ -913,6 +921,7 @@ static void device_class_init(ObjectClass *class, void *data) class->unparent = device_unparent; dc->realize = device_realize; dc->unrealize = device_unrealize; + dc->props = device_props; /* by default all devices were considered as hotpluggable, * so with intent to check it in generic qdev_unplug() / diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index dbe473c..38557d3 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -143,7 +143,7 @@ struct DeviceState { Object parent_obj; /*< public >*/ - const char *id; + char *id; bool realized; QemuOpts *opts; int hotplugged; diff --git a/qdev-monitor.c b/qdev-monitor.c index 9268c87..f0713a9 100644 --- a/qdev-monitor.c +++ b/qdev-monitor.c @@ -531,7 +531,8 @@ DeviceState *qdev_device_add(QemuOpts *opts) id = qemu_opts_id(opts); if (id) { - dev->id = id; + /* FIXME: Qdev String props cant handle consts */ + dev->id = (char *)id; } if (dev->id) {
So clients can set the top level id string. Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> --- hw/core/qdev.c | 13 +++++++++++-- include/hw/qdev-core.h | 2 +- qdev-monitor.c | 3 ++- 3 files changed, 14 insertions(+), 4 deletions(-)