| Message ID | 1395327676-29753-5-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
Il 20/03/2014 16:01, Igor Mammedov ha scritto: > + /* > + * Returns path to link<> that should be set/unset on dev hotplug. > + * Used for link based bussless devices hotplug. > + */ > + char* (*hotplug_path)(DeviceState *dev); > + What about just looking up on the QOM tree until you find a HotplugHandler, if the device doesn't have a bus or the bus doesn't have a hotplug handler link itself? This is similar to how FWPathProvider works. Paolo
On Thu, 20 Mar 2014 17:12:17 +0100 Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 20/03/2014 16:01, Igor Mammedov ha scritto: > > + /* > > + * Returns path to link<> that should be set/unset on dev hotplug. > > + * Used for link based bussless devices hotplug. > > + */ > > + char* (*hotplug_path)(DeviceState *dev); > > + > > What about just looking up on the QOM tree until you find a > HotplugHandler, if the device doesn't have a bus or the bus doesn't have > a hotplug handler link itself? This is similar to how FWPathProvider works. it does so "hotplug_handler_get_from_path()", above just provides option to specify lookup path. See 6/8 where PC board allocates links and sets its own board specific path for generic DimmDevice. > > Paolo
Il 20/03/2014 17:20, Igor Mammedov ha scritto: >> > >> > What about just looking up on the QOM tree until you find a >> > HotplugHandler, if the device doesn't have a bus or the bus doesn't have >> > a hotplug handler link itself? This is similar to how FWPathProvider works. > it does so "hotplug_handler_get_from_path()", > above just provides option to specify lookup path. See 6/8 where PC board > allocates links and sets its own board specific path for generic DimmDevice. Yeah, but I'm not sure why you need the links. Why can't you just start from the canonical path, such as /machine/peripheral/dimm-0 for -device dimm,id=dimm-0, and walk up from there? Paolo
On Thu, 20 Mar 2014 19:03:57 +0100 Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 20/03/2014 17:20, Igor Mammedov ha scritto: > >> > > >> > What about just looking up on the QOM tree until you find a > >> > HotplugHandler, if the device doesn't have a bus or the bus doesn't have > >> > a hotplug handler link itself? This is similar to how FWPathProvider works. > > it does so "hotplug_handler_get_from_path()", > > above just provides option to specify lookup path. See 6/8 where PC board > > allocates links and sets its own board specific path for generic DimmDevice. > > Yeah, but I'm not sure why you need the links. Why can't you just start > from the canonical path, such as /machine/peripheral/dimm-0 for -device > dimm,id=dimm-0, and walk up from there? That would work in this particular case since /machine is hotplug handler, but in generic case hotplug handler can be located somewhere else in QOM tree i.e. it isn't a parent of hotplugged device. Allowing to customize lookup path via DeviceClass.hotplug_path() makes wiring hotplug handler flexible. For example: * A target that decides to use DimmDevice cloud have hotplug handler located at /machine/peripheral/ec_foo, walking down from /machine/peripheral/dimm-0 or /machine/unassigned/dimm-0 (in hotplug case) won't allow to find hotplug handler. But overriding DeviceClass.hotplug_path() the target machine could provide means to locate necessary hotplug handler Maybe setting link<> is over-engineered and should be dropped for now, while still keeping DeviceClass.hotplug_path() as a means to provide custom hotplug hander path. BTW not related to hotplug but why I used link<>s: I've added link<>s as an attempt to visualize Andreas' idea to use them for hotplug and mgmt. It has it's own benefits if we try to provide more or less uniform QOM interface view for management. What I have in mind is that we could have tree like this: /machine/node[...]/dimm[...] /cpu[...]/core[...]/thread[...] where leaves are link<>s which are prebuilt at startup and set when device is added. It provides an easy to enumerate interface for mgmt and also gives us a quite informative path that encodes topology and later we could use it instead of custom properties. For example: device_add x86cpu,path=/machine/node[1]/cpu[0]/core[3]/thread[2] vs device_add x86cpu,apic-id=[who knows how it's calculated] or device_add dimm,path=/machine/node[0]/dimm[5] vs device_add dimm,node=0,slot=5 i.e. being added device could decode all needed information from above provided path instead of creating a bunch of custom properties. > > Paolo
Il 21/03/2014 11:35, Igor Mammedov ha scritto: > On Thu, 20 Mar 2014 19:03:57 +0100 > Paolo Bonzini <pbonzini@redhat.com> wrote: > >> Il 20/03/2014 17:20, Igor Mammedov ha scritto: >>>>> >>>>> What about just looking up on the QOM tree until you find a >>>>> HotplugHandler, if the device doesn't have a bus or the bus doesn't have >>>>> a hotplug handler link itself? This is similar to how FWPathProvider works. >>> it does so "hotplug_handler_get_from_path()", >>> above just provides option to specify lookup path. See 6/8 where PC board >>> allocates links and sets its own board specific path for generic DimmDevice. >> >> Yeah, but I'm not sure why you need the links. Why can't you just start >> from the canonical path, such as /machine/peripheral/dimm-0 for -device >> dimm,id=dimm-0, and walk up from there? > That would work in this particular case since /machine is hotplug handler, > but in generic case hotplug handler can be located somewhere else in QOM tree > i.e. it isn't a parent of hotplugged device. Yes, it removes more flexibility than I thought. Hotplugged devices are by definition at /machine/peripheral, so there's not much "walking up" that we can do. However... > Allowing to customize lookup path via DeviceClass.hotplug_path() makes > wiring hotplug handler flexible. For example: > * A target that decides to use DimmDevice cloud have hotplug handler > located at /machine/peripheral/ec_foo, walking down from > /machine/peripheral/dimm-0 or /machine/unassigned/dimm-0 (in hotplug case) > won't allow to find hotplug handler. But overriding DeviceClass.hotplug_path() > the target machine could provide means to locate necessary hotplug handler ... since this is meant for "monkeypatching" in the target machine (which we don't really do elsewhere, do we?), perhaps /machine itself could grow a get_hotplug_handler() or get_hotplug_path() method? The machine object then can return itself if you want to implement HotplugHandler in /machine, or it could return the PM device, or some other controller. Or even simpler (perhaps too simple) you could just check if /machine implements HotplugHandler if the hotplug device is busless. > I've added link<>s as an attempt to visualize Andreas' idea to use them for > hotplug and mgmt. Yes, links for hotplug/management are a nice idea. I think however we're still in an early phase of that, and we've already made memory hotplug depend on a lot of infrastructure! > leaves are link<>s which are prebuilt at startup and set when device > is added. It provides an easy to enumerate interface for mgmt and also > gives us a quite informative path that encodes topology and later > we could use it instead of custom properties. For example: > > device_add x86cpu,path=/machine/node[1]/cpu[0]/core[3]/thread[2] > vs > device_add x86cpu,apic-id=[who knows how it's calculated] > > or > device_add dimm,path=/machine/node[0]/dimm[5] > vs > device_add dimm,node=0,slot=5 > > i.e. being added device could decode all needed information from above > provided path instead of creating a bunch of custom properties. So "path" would mean "look for a link there and set it to myself". In turn the link setter would take care of setting the device's address based on the property name, as well as (if applicable) adding the device on the bus. It's a nice alternative to the bus+addr, and one we could consider to move device creation to -object. Anthony in the past had mentioned something like -object dimm,id=foo -set machine/node[0].dimm[5]=foo -set dimm.realize=true I think I like your proposal better. We now have moved towards UserCreatable instead of setting magic properties manually, and I think "path" fits in the "UserCreatable" scheme better than "-set". It could be extended to PCI, as basically a version of bus+addr with QOM paths: -object e1000,id=e1000,path=/machine/pci[0]/slot[03.0] Here is an example of configuring a PCIe switch with relative paths: -object ioh3420,id=port4,path=pci[0]/slot[1c.0] -object x3130-upstream,id=up,path=port4/pci[0]/slot[00.0],chassis=1 -object xio3130-downstream,id=down0,multifunction=on,path=up/pci[0]/slot[00.0],chassis=2 -object xio3130-downstream,id=down1,multifunction=on,path=up/pci[0]/slot[00.1],chassis=3 -object e1000,id=e1000,path=down0/pci[0]/slot[00.0] It's a bit verbose. It doesn't allow for automatically attributing slots within a bus, which is the main drawback compared to bus+addr. Quite frankly I'm not sure I would like it as a user, even though it's likely superior for management and for complicated configurations such as the above PCIe example. In the past we stalled on how to create the properties, since there is the problem of requiring pre-creation of links. On SCSI you would have thousands of links. Since interfaces are fancy now, perhaps we could use them here too (DynamicPropertyCreator?). object_property_find would pass missing property names to the interface if implemented, and the object would use it to validate dynamic property names and create them dynamically. Thanks for throwing up these ideas. Even if we end up with a vastly simplified mechanism for memory hotplug, it's good to get a fresh view on old concepts! Paolo
On Fri, 21 Mar 2014 12:45:01 +0100 Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 21/03/2014 11:35, Igor Mammedov ha scritto: > > On Thu, 20 Mar 2014 19:03:57 +0100 > > Paolo Bonzini <pbonzini@redhat.com> wrote: > > > >> Il 20/03/2014 17:20, Igor Mammedov ha scritto: > >>>>> > >>>>> What about just looking up on the QOM tree until you find a > >>>>> HotplugHandler, if the device doesn't have a bus or the bus doesn't have > >>>>> a hotplug handler link itself? This is similar to how FWPathProvider works. > >>> it does so "hotplug_handler_get_from_path()", > >>> above just provides option to specify lookup path. See 6/8 where PC board > >>> allocates links and sets its own board specific path for generic DimmDevice. > >> > >> Yeah, but I'm not sure why you need the links. Why can't you just start > >> from the canonical path, such as /machine/peripheral/dimm-0 for -device > >> dimm,id=dimm-0, and walk up from there? > > That would work in this particular case since /machine is hotplug handler, > > but in generic case hotplug handler can be located somewhere else in QOM tree > > i.e. it isn't a parent of hotplugged device. > > Yes, it removes more flexibility than I thought. Hotplugged devices > are by definition at /machine/peripheral, so there's not much "walking > up" that we can do. > > However... > > > Allowing to customize lookup path via DeviceClass.hotplug_path() makes > > wiring hotplug handler flexible. For example: > > * A target that decides to use DimmDevice cloud have hotplug handler > > located at /machine/peripheral/ec_foo, walking down from > > /machine/peripheral/dimm-0 or /machine/unassigned/dimm-0 (in hotplug case) > > won't allow to find hotplug handler. But overriding DeviceClass.hotplug_path() > > the target machine could provide means to locate necessary hotplug handler > > ... since this is meant for "monkeypatching" in the target machine > (which we don't really do elsewhere, do we?), perhaps /machine itself > could grow a get_hotplug_handler() or get_hotplug_path() method? Ok for memory hotplug, I'll try to add and use get_hotplug_handler() method to /machine as a minimal solution to the problem. > > The machine object then can return itself if you want to implement > HotplugHandler in /machine, or it could return the PM device, or some > other controller. > > Or even simpler (perhaps too simple) you could just check if /machine > implements HotplugHandler if the hotplug device is busless. > > > I've added link<>s as an attempt to visualize Andreas' idea to use them for > > hotplug and mgmt. > > Yes, links for hotplug/management are a nice idea. I think however > we're still in an early phase of that, and we've already made memory > hotplug depend on a lot of infrastructure! > > > leaves are link<>s which are prebuilt at startup and set when device > > is added. It provides an easy to enumerate interface for mgmt and also > > gives us a quite informative path that encodes topology and later > > we could use it instead of custom properties. For example: > > > > device_add x86cpu,path=/machine/node[1]/cpu[0]/core[3]/thread[2] > > vs > > device_add x86cpu,apic-id=[who knows how it's calculated] > > > > or > > device_add dimm,path=/machine/node[0]/dimm[5] > > vs > > device_add dimm,node=0,slot=5 > > > > i.e. being added device could decode all needed information from above > > provided path instead of creating a bunch of custom properties. > > So "path" would mean "look for a link there and set it to myself". > In turn the link setter would take care of setting the device's > address based on the property name, as well as (if applicable) > adding the device on the bus. > > It's a nice alternative to the bus+addr, and one we could consider to > move device creation to -object. Anthony in the past had mentioned > something like > > -object dimm,id=foo > -set machine/node[0].dimm[5]=foo > -set dimm.realize=true > > I think I like your proposal better. > > We now have moved towards UserCreatable instead of setting magic > properties manually, and I think "path" fits in the "UserCreatable" > scheme better than "-set". > > It could be extended to PCI, as basically a version of bus+addr > with QOM paths: > > -object e1000,id=e1000,path=/machine/pci[0]/slot[03.0] > > Here is an example of configuring a PCIe switch with relative paths: > > -object ioh3420,id=port4,path=pci[0]/slot[1c.0] > -object x3130-upstream,id=up,path=port4/pci[0]/slot[00.0],chassis=1 > -object xio3130-downstream,id=down0,multifunction=on,path=up/pci[0]/slot[00.0],chassis=2 > -object xio3130-downstream,id=down1,multifunction=on,path=up/pci[0]/slot[00.1],chassis=3 > -object e1000,id=e1000,path=down0/pci[0]/slot[00.0] > > It's a bit verbose. It doesn't allow for automatically attributing > slots within a bus, which is the main drawback compared to bus+addr. > Quite frankly I'm not sure I would like it as a user, even though it's > likely superior for management and for complicated configurations > such as the above PCIe example. > > In the past we stalled on how to create the properties, since there is > the problem of requiring pre-creation of links. On SCSI you would have > thousands of links. > > Since interfaces are fancy now, perhaps we could use them here too > (DynamicPropertyCreator?). object_property_find would pass missing > property names to the interface if implemented, and the object would > use it to validate dynamic property names and create them dynamically. > > Thanks for throwing up these ideas. Even if we end up with a vastly > simplified mechanism for memory hotplug, it's good to get a fresh > view on old concepts! > > Paolo
Am 21.03.2014 11:35, schrieb Igor Mammedov: > BTW not related to hotplug but why I used link<>s: > > I've added link<>s as an attempt to visualize Andreas' idea to use them for Anthony's :) > hotplug and mgmt. It has it's own benefits if we try to provide more or > less uniform QOM interface view for management. What I have in mind is that > we could have tree like this: > /machine/node[...]/dimm[...] > /cpu[...]/core[...]/thread[...] > > where leaves are link<>s which are prebuilt at startup and set when device > is added. It provides an easy to enumerate interface for mgmt and also > gives us a quite informative path that encodes topology and later > we could use it instead of custom properties. For example: > > device_add x86cpu,path=/machine/node[1]/cpu[0]/core[3]/thread[2] > vs > device_add x86cpu,apic-id=[who knows how it's calculated] This still collides with what Anthony and me have been saying about CPU hot-add: It should not happen on thread level. cpu-add covers the current approach, but device_add should add a full socket and definitely not in that /machine/node[n]/... path. You or someone replied on Paolo's NUMA thread that this was only as an internal convenience for lookups, now you're pushing this as user ABI. NACK to the latter. I was hoping that we could let device_add auto-discover free link<> slots like it does for buses today; having two places to link the same object would complicate that, so we need to be careful in designing our link<>s: Having /machine/node[0]/cpu[0] be a link<> would mean no second link<> directly on /machine/cpu[0], i.e. the NUMA node acting as a sub-container of the board; on the other hand having a link<> to the thread CPUState from some NUMA-specific path outside of the composition model would still be possible due to the different link<> type but having cpu[0] be a link to the actual socket object rules out using the same name for a NUMA-only container object as proposed. > or > device_add dimm,path=/machine/node[0]/dimm[5] > vs > device_add dimm,node=0,slot=5 > > i.e. being added device could decode all needed information from above > provided path instead of creating a bunch of custom properties. Hm, the advantage of having properties there would be better error checking in terms of restricting paths. I'd be open to having both. I do wonder in both cases if we should use paths relative to /machine to avoid them cluttering the command line? Regards, Andreas
On Mon, Mar 24, 2014 at 01:20:46PM +0100, Andreas Färber wrote: > Am 21.03.2014 11:35, schrieb Igor Mammedov: > > BTW not related to hotplug but why I used link<>s: > > > > I've added link<>s as an attempt to visualize Andreas' idea to use them for > > Anthony's :) > > > hotplug and mgmt. It has it's own benefits if we try to provide more or > > less uniform QOM interface view for management. What I have in mind is that > > we could have tree like this: > > /machine/node[...]/dimm[...] > > /cpu[...]/core[...]/thread[...] > > > > where leaves are link<>s which are prebuilt at startup and set when device > > is added. It provides an easy to enumerate interface for mgmt and also > > gives us a quite informative path that encodes topology and later > > we could use it instead of custom properties. For example: > > > > device_add x86cpu,path=/machine/node[1]/cpu[0]/core[3]/thread[2] > > vs > > device_add x86cpu,apic-id=[who knows how it's calculated] > > This still collides with what Anthony and me have been saying about CPU > hot-add: It should not happen on thread level. cpu-add covers the > current approach, but device_add should add a full socket and definitely > not in that /machine/node[n]/... path. You or someone replied on Paolo's > NUMA thread that this was only as an internal convenience for lookups, > now you're pushing this as user ABI. NACK to the latter. > > I was hoping that we could let device_add auto-discover free link<> > slots like it does for buses today; That's friendly for HMP but a huge maintainance pain. In the end hardly anyone uses HMP for hotplug so I don't think there's a lot of value in auto-discovery, make management as explicit as possible. > having two places to link the same > object would complicate that, so we need to be careful in designing our > link<>s: Having /machine/node[0]/cpu[0] be a link<> would mean no second > link<> directly on /machine/cpu[0], i.e. the NUMA node acting as a > sub-container of the board; on the other hand having a link<> to the > thread CPUState from some NUMA-specific path outside of the composition > model would still be possible due to the different link<> type but > having cpu[0] be a link to the actual socket object rules out using the > same name for a NUMA-only container object as proposed. > > > or > > device_add dimm,path=/machine/node[0]/dimm[5] > > vs > > device_add dimm,node=0,slot=5 > > > > i.e. being added device could decode all needed information from above > > provided path instead of creating a bunch of custom properties. > > Hm, the advantage of having properties there would be better error > checking in terms of restricting paths. I'd be open to having both. > > I do wonder in both cases if we should use paths relative to /machine to > avoid them cluttering the command line? > > 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
diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c index 5573d9d..1a13ca3 100644 --- a/hw/core/hotplug.c +++ b/hw/core/hotplug.c @@ -11,6 +11,36 @@ */ #include "hw/hotplug.h" #include "qemu/module.h" +#include <string.h> + +HotplugHandler *hotplug_handler_get_from_path(const char *path) +{ + Object *obj; + const char *sep; + bool ambiguous = false; + HotplugHandler *hh = NULL; + char *current_path = g_strdup(path); + + while ((sep = strrchr(current_path, '/'))) { + char *old_path = current_path; + if (!sep || sep == current_path) { + break; + } + + current_path = g_strndup(current_path, sep - current_path); + g_free(old_path); + + obj = object_resolve_path(current_path, &ambiguous); + g_assert(!ambiguous); + hh = (HotplugHandler *)object_dynamic_cast(obj, TYPE_HOTPLUG_HANDLER); + if (hh != NULL) { + break; + } + } + + g_free(current_path); + return hh; +} void hotplug_handler_plug(HotplugHandler *plug_handler, DeviceState *plugged_dev, diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 9f0a522..a8d6eea 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -743,6 +743,21 @@ static void device_set_realized(Object *obj, bool value, Error **err) local_err == NULL) { hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err); + } else if (dc->hotplug_path != NULL && local_err == NULL) { + HotplugHandler *hotplug_ctrl; + char *target_name; + char *path = dc->hotplug_path(dev); + + hotplug_ctrl = hotplug_handler_get_from_path(path); + g_assert(hotplug_ctrl); + + target_name = g_strdup(strrchr(path, '/') + 1); + object_property_set_link(OBJECT(hotplug_ctrl), OBJECT(dev), + target_name, &local_err); + g_free(target_name); + g_free(path); + + hotplug_handler_plug(hotplug_ctrl, dev, &local_err); } if (qdev_get_vmsd(dev) && local_err == NULL) { diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h index a6533cb..7a3815e 100644 --- a/include/hw/hotplug.h +++ b/include/hw/hotplug.h @@ -75,4 +75,6 @@ void hotplug_handler_plug(HotplugHandler *plug_handler, void hotplug_handler_unplug(HotplugHandler *plug_handler, DeviceState *plugged_dev, Error **errp); + +HotplugHandler *hotplug_handler_get_from_path(const char *path); #endif diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index dbe473c..6cd936d 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -116,6 +116,12 @@ typedef struct DeviceClass { bool cannot_instantiate_with_device_add_yet; bool hotpluggable; + /* + * Returns path to link<> that should be set/unset on dev hotplug. + * Used for link based bussless devices hotplug. + */ + char* (*hotplug_path)(DeviceState *dev); + /* callbacks */ void (*reset)(DeviceState *dev); DeviceRealize realize;
Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- hw/core/hotplug.c | 30 ++++++++++++++++++++++++++++++ hw/core/qdev.c | 15 +++++++++++++++ include/hw/hotplug.h | 2 ++ include/hw/qdev-core.h | 6 ++++++ 4 files changed, 53 insertions(+), 0 deletions(-)