| Message ID | 20250128142152.9889-5-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | accel: Only include qdev-realized vCPUs in global &cpus_queue | expand |
On 1/28/25 06:21, Philippe Mathieu-Daudé wrote: > We are trying to understand what means "a qdev is realized". > One explanation was "the device is guest visible"; however > many devices are realized before being mapped, thus are not > "guest visible". Some devices map / wire their IRQs before > being realized (such ISA devices). There is a need for devices > to be "automatically" mapped/wired (see [2]) such CLI-created > devices, but this apply generically to dynamic machines. > > Currently the device creation steps are expected to roughly be: > > (external use) (QDev core) (Device Impl) > ~~~~~~~~~~~~ ~~~~~~~~~ ~~~~~~~~~~~ > > INIT enter > -----> > +----------------------+ > | Allocate state | > +----------------------+ > -----> > +---------------------+ > | INIT children | > | | > | Alias children properties > | | > | Expose properties | > INIT exit +---------------------+ > <----------------------------------- > +----------------+ > | set properties | > | | > | set ClkIn | > +----------------+ REALIZE enter > ----------------------------------> > +----------------------+ > | Use config properties| > | | > | Realize children | > | | > | Init GPIOs/IRQs | > | | > | Init MemoryRegions | > +----------------------+ > REALIZE exit > <----------------------------------- ---- "realized" / "guest visible" > +-----------------+ > | Explicit wiring:| > | IRQs | > | I/O / Mem | > | ClkOut | > +-----------------+ RESET enter > ---------------------------------> > +----------------------+ > | Reset default values | > +----------------------+ > > But as mentioned, various devices "wire" parts before they exit > the "realize" step. > In order to clarify, I'm trying to enforce what can be done > *before* and *after* realization. > > *after* a device is expected to be stable (no more configurable) > and fully usable. > > To be able to use internal/auto wiring (such ISA devices) and > keep the current external/explicit wiring, I propose to add an > extra "internal wiring" step, happening after the REALIZE step > as: > > (external use) (QDev core) (Device Impl) > ~~~~~~~~~~~~ ~~~~~~~~~ ~~~~~~~~~~~ > > INIT enter > -----> > +----------------------+ > | Allocate state | > +----------------------+ > -----> > +---------------------+ > | INIT children | > | | > | Alias children properties > | | > | Expose properties | > INIT exit +---------------------+ > <----------------------------------- > +----------------+ > | set properties | > | | > | set ClkIn | > +----------------+ REALIZE enter > ----------------------------------> > +----------------------+ > | Use config properties| > | | > | Realize children | > | | > | Init GPIOs/IRQs | > | | > | Init MemoryRegions | > +----------------------+ > REALIZE exit <--- > +----------------------+ > | Internal auto wiring | > | IRQs | (i.e. ISA bus) > | I/O / Mem | > | ClkOut | > +----------------------+ > <--- ---- "realized" > +-----------------+ > | External wiring:| > | IRQs | > | I/O / Mem | > | ClkOut | > +-----------------+ RESET enter ---- "guest visible" > ---------------------------------> > +----------------------+ > | Reset default values | > +----------------------+ > > The "realized" point is not changed. "guest visible" concept only > occurs *after* wiring, just before the reset phase. > > This change introduces the DeviceClass::wire handler within qdev > core realization code. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > include/hw/qdev-core.h | 7 +++++++ > hw/core/qdev.c | 20 +++++++++++++++++++- > 2 files changed, 26 insertions(+), 1 deletion(-) > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > index 530f3da7021..021bb7afdc0 100644 > --- a/include/hw/qdev-core.h > +++ b/include/hw/qdev-core.h > @@ -102,7 +102,12 @@ typedef int (*DeviceSyncConfig)(DeviceState *dev, Error **errp); > * @props: Properties accessing state fields. > * @realize: Callback function invoked when the #DeviceState:realized > * property is changed to %true. > + * @wire: Callback function called after @realize to connect IRQs, > + * clocks and map memories. Can not fail. > + * @unwire: Callback function to undo @wire. Called before @unrealize. > + * Can not fail. > * @unrealize: Callback function invoked when the #DeviceState:realized > + * property is changed to %false. Can not fail. > * property is changed to %false. > * @sync_config: Callback function invoked when QMP command device-sync-config > * is called. Should synchronize device configuration from host to guest part > @@ -171,6 +176,8 @@ struct DeviceClass { > */ > DeviceReset legacy_reset; > DeviceRealize realize; > + void (*wire)(DeviceState *dev); > + void (*unwire)(DeviceState *dev); > DeviceUnrealize unrealize; > DeviceSyncConfig sync_config; > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index 82bbdcb654e..38449255365 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -554,6 +554,15 @@ static void device_set_realized(Object *obj, bool value, Error **errp) > } > } > > + if (dc->wire) { > + if (!dc->unwire) { > + warn_report_once("wire() without unwire() for type '%s'", > + object_get_typename(OBJECT(dev))); > + } > + dc->wire(dev); > + } > + > + /* At this point the device is "guest visible". */ > qatomic_store_release(&dev->realized, value); > > } else if (!value && dev->realized) { > @@ -573,6 +582,15 @@ static void device_set_realized(Object *obj, bool value, Error **errp) > */ > smp_wmb(); > > + if (dc->unwire) { > + if (!dc->wire) { > + error_report("disconnect() without connect() for type '%s'", > + object_get_typename(OBJECT(dev))); > + abort(); > + } > + dc->unwire(dev); > + } Mismatched error messages (wire vs connect). But, really, just check both directions properly at startup. There's probably lots of places where devices are never unrealized. Otherwise, this seems sane, having a kind of post_init on the realize path, running after all superclass realization is done. r~
On Tue, 28 Jan 2025 12:52:39 -0800 Richard Henderson <richard.henderson@linaro.org> wrote: > On 1/28/25 06:21, Philippe Mathieu-Daudé wrote: > > We are trying to understand what means "a qdev is realized". > > One explanation was "the device is guest visible"; however > > many devices are realized before being mapped, thus are not > > "guest visible". Some devices map / wire their IRQs before > > being realized (such ISA devices). There is a need for devices > > to be "automatically" mapped/wired (see [2]) such CLI-created > > devices, but this apply generically to dynamic machines. > > > > Currently the device creation steps are expected to roughly be: > > > > (external use) (QDev core) (Device Impl) > > ~~~~~~~~~~~~ ~~~~~~~~~ ~~~~~~~~~~~ > > > > INIT enter > > -----> > > +----------------------+ > > | Allocate state | > > +----------------------+ > > -----> > > +---------------------+ > > | INIT children | > > | | > > | Alias children properties > > | | > > | Expose properties | > > INIT exit +---------------------+ > > <----------------------------------- > > +----------------+ > > | set properties | > > | | > > | set ClkIn | > > +----------------+ REALIZE enter > > ----------------------------------> > > +----------------------+ > > | Use config properties| > > | | > > | Realize children | > > | | > > | Init GPIOs/IRQs | > > | | > > | Init MemoryRegions | > > +----------------------+ > > REALIZE exit > > <----------------------------------- ---- "realized" / "guest visible" > > +-----------------+ > > | Explicit wiring:| > > | IRQs | > > | I/O / Mem | > > | ClkOut | > > +-----------------+ RESET enter > > ---------------------------------> > > +----------------------+ > > | Reset default values | > > +----------------------+ > > > > But as mentioned, various devices "wire" parts before they exit > > the "realize" step. > > In order to clarify, I'm trying to enforce what can be done > > *before* and *after* realization. > > > > *after* a device is expected to be stable (no more configurable) > > and fully usable. > > > > To be able to use internal/auto wiring (such ISA devices) and > > keep the current external/explicit wiring, I propose to add an > > extra "internal wiring" step, happening after the REALIZE step we partially have such wiring in place hotplug_handler_plug 'hotplug_' prefix is legacy remnant to confuse every, it really should be renamed to just plug_handler or 'wire_foo'. For bus-less CPUs it's used both on hot- and cold-plug paths. It does take care of wiring after realize part is completed (basically exposing device to the external users). And I it also handles 'bus' based plug workflow see qdev_get_hotplug_handler() assuming bus owner has provided a callback. It's likely no-one had cared about ISA, as it was out of hotplug scope when the interface was introduced. Unplug part is however is not wired directly into unrealize() stage, but rather to an external caller (mgmt or guest), which essentially unwinds into the same hotplug_handler chain as plug, to unwire device and then the same external caller does call unrealize on device. (see:hotplug_handler_unplug() ) As for more direct hack, there is also DEVICE_LISTENER_CALL() callback, which is more close to this approach (modulo users are hidden as there is not explicit wiring). Unrealize part is called too late (which probably is not right), and realize part is called before subtree is realized (might work for current users, but likely should be placed at the same site as wire() call. Hence we should think if already existing methods could be reused/adapted to, before adding similar wire/unwire mechanism. > > as: > > > > (external use) (QDev core) (Device Impl) > > ~~~~~~~~~~~~ ~~~~~~~~~ ~~~~~~~~~~~ > > > > INIT enter > > -----> > > +----------------------+ > > | Allocate state | > > +----------------------+ > > -----> > > +---------------------+ > > | INIT children | > > | | > > | Alias children properties > > | | > > | Expose properties | > > INIT exit +---------------------+ > > <----------------------------------- > > +----------------+ > > | set properties | > > | | > > | set ClkIn | > > +----------------+ REALIZE enter > > ----------------------------------> > > +----------------------+ > > | Use config properties| > > | | > > | Realize children | > > | | > > | Init GPIOs/IRQs | > > | | > > | Init MemoryRegions | > > +----------------------+ > > REALIZE exit <--- > > +----------------------+ > > | Internal auto wiring | > > | IRQs | (i.e. ISA bus) > > | I/O / Mem | > > | ClkOut | > > +----------------------+ > > <--- ---- "realized" > > +-----------------+ > > | External wiring:| > > | IRQs | > > | I/O / Mem | > > | ClkOut | > > +-----------------+ RESET enter ---- "guest visible" > > ---------------------------------> > > +----------------------+ > > | Reset default values | > > +----------------------+ > > > > The "realized" point is not changed. "guest visible" concept only > > occurs *after* wiring, just before the reset phase. > > > > This change introduces the DeviceClass::wire handler within qdev > > core realization code. > > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > --- > > include/hw/qdev-core.h | 7 +++++++ > > hw/core/qdev.c | 20 +++++++++++++++++++- > > 2 files changed, 26 insertions(+), 1 deletion(-) > > > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > > index 530f3da7021..021bb7afdc0 100644 > > --- a/include/hw/qdev-core.h > > +++ b/include/hw/qdev-core.h > > @@ -102,7 +102,12 @@ typedef int (*DeviceSyncConfig)(DeviceState *dev, Error **errp); > > * @props: Properties accessing state fields. > > * @realize: Callback function invoked when the #DeviceState:realized > > * property is changed to %true. > > + * @wire: Callback function called after @realize to connect IRQs, > > + * clocks and map memories. Can not fail. > > + * @unwire: Callback function to undo @wire. Called before @unrealize. > > + * Can not fail. > > * @unrealize: Callback function invoked when the #DeviceState:realized > > + * property is changed to %false. Can not fail. > > * property is changed to %false. > > * @sync_config: Callback function invoked when QMP command device-sync-config > > * is called. Should synchronize device configuration from host to guest part > > @@ -171,6 +176,8 @@ struct DeviceClass { > > */ > > DeviceReset legacy_reset; > > DeviceRealize realize; > > + void (*wire)(DeviceState *dev); > > + void (*unwire)(DeviceState *dev); > > DeviceUnrealize unrealize; > > DeviceSyncConfig sync_config; > > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > > index 82bbdcb654e..38449255365 100644 > > --- a/hw/core/qdev.c > > +++ b/hw/core/qdev.c > > @@ -554,6 +554,15 @@ static void device_set_realized(Object *obj, bool value, Error **errp) > > } > > } > > > > + if (dc->wire) { > > + if (!dc->unwire) { > > + warn_report_once("wire() without unwire() for type '%s'", > > + object_get_typename(OBJECT(dev))); > > + } > > + dc->wire(dev); > > + } > > + > > + /* At this point the device is "guest visible". */ > > qatomic_store_release(&dev->realized, value); > > > > } else if (!value && dev->realized) { > > @@ -573,6 +582,15 @@ static void device_set_realized(Object *obj, bool value, Error **errp) > > */ > > smp_wmb(); > > > > + if (dc->unwire) { > > + if (!dc->wire) { > > + error_report("disconnect() without connect() for type '%s'", > > + object_get_typename(OBJECT(dev))); > > + abort(); > > + } > > + dc->unwire(dev); > > + } > > Mismatched error messages (wire vs connect). > But, really, just check both directions properly at startup. > There's probably lots of places where devices are never unrealized. > > Otherwise, this seems sane, having a kind of post_init on the realize path, running after > all superclass realization is done. > > > r~ >
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index 530f3da7021..021bb7afdc0 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -102,7 +102,12 @@ typedef int (*DeviceSyncConfig)(DeviceState *dev, Error **errp); * @props: Properties accessing state fields. * @realize: Callback function invoked when the #DeviceState:realized * property is changed to %true. + * @wire: Callback function called after @realize to connect IRQs, + * clocks and map memories. Can not fail. + * @unwire: Callback function to undo @wire. Called before @unrealize. + * Can not fail. * @unrealize: Callback function invoked when the #DeviceState:realized + * property is changed to %false. Can not fail. * property is changed to %false. * @sync_config: Callback function invoked when QMP command device-sync-config * is called. Should synchronize device configuration from host to guest part @@ -171,6 +176,8 @@ struct DeviceClass { */ DeviceReset legacy_reset; DeviceRealize realize; + void (*wire)(DeviceState *dev); + void (*unwire)(DeviceState *dev); DeviceUnrealize unrealize; DeviceSyncConfig sync_config; diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 82bbdcb654e..38449255365 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -554,6 +554,15 @@ static void device_set_realized(Object *obj, bool value, Error **errp) } } + if (dc->wire) { + if (!dc->unwire) { + warn_report_once("wire() without unwire() for type '%s'", + object_get_typename(OBJECT(dev))); + } + dc->wire(dev); + } + + /* At this point the device is "guest visible". */ qatomic_store_release(&dev->realized, value); } else if (!value && dev->realized) { @@ -573,6 +582,15 @@ static void device_set_realized(Object *obj, bool value, Error **errp) */ smp_wmb(); + if (dc->unwire) { + if (!dc->wire) { + error_report("disconnect() without connect() for type '%s'", + object_get_typename(OBJECT(dev))); + abort(); + } + dc->unwire(dev); + } + QLIST_FOREACH(bus, &dev->child_bus, sibling) { qbus_unrealize(bus); } @@ -585,8 +603,8 @@ static void device_set_realized(Object *obj, bool value, Error **errp) dev->pending_deleted_event = true; DEVICE_LISTENER_CALL(unrealize, Reverse, dev); } - assert(local_err == NULL); + return; child_realize_fail:
We are trying to understand what means "a qdev is realized". One explanation was "the device is guest visible"; however many devices are realized before being mapped, thus are not "guest visible". Some devices map / wire their IRQs before being realized (such ISA devices). There is a need for devices to be "automatically" mapped/wired (see [2]) such CLI-created devices, but this apply generically to dynamic machines. Currently the device creation steps are expected to roughly be: (external use) (QDev core) (Device Impl) ~~~~~~~~~~~~ ~~~~~~~~~ ~~~~~~~~~~~ INIT enter -----> +----------------------+ | Allocate state | +----------------------+ -----> +---------------------+ | INIT children | | | | Alias children properties | | | Expose properties | INIT exit +---------------------+ <----------------------------------- +----------------+ | set properties | | | | set ClkIn | +----------------+ REALIZE enter ----------------------------------> +----------------------+ | Use config properties| | | | Realize children | | | | Init GPIOs/IRQs | | | | Init MemoryRegions | +----------------------+ REALIZE exit <----------------------------------- ---- "realized" / "guest visible" +-----------------+ | Explicit wiring:| | IRQs | | I/O / Mem | | ClkOut | +-----------------+ RESET enter ---------------------------------> +----------------------+ | Reset default values | +----------------------+ But as mentioned, various devices "wire" parts before they exit the "realize" step. In order to clarify, I'm trying to enforce what can be done *before* and *after* realization. *after* a device is expected to be stable (no more configurable) and fully usable. To be able to use internal/auto wiring (such ISA devices) and keep the current external/explicit wiring, I propose to add an extra "internal wiring" step, happening after the REALIZE step as: (external use) (QDev core) (Device Impl) ~~~~~~~~~~~~ ~~~~~~~~~ ~~~~~~~~~~~ INIT enter -----> +----------------------+ | Allocate state | +----------------------+ -----> +---------------------+ | INIT children | | | | Alias children properties | | | Expose properties | INIT exit +---------------------+ <----------------------------------- +----------------+ | set properties | | | | set ClkIn | +----------------+ REALIZE enter ----------------------------------> +----------------------+ | Use config properties| | | | Realize children | | | | Init GPIOs/IRQs | | | | Init MemoryRegions | +----------------------+ REALIZE exit <--- +----------------------+ | Internal auto wiring | | IRQs | (i.e. ISA bus) | I/O / Mem | | ClkOut | +----------------------+ <--- ---- "realized" +-----------------+ | External wiring:| | IRQs | | I/O / Mem | | ClkOut | +-----------------+ RESET enter ---- "guest visible" ---------------------------------> +----------------------+ | Reset default values | +----------------------+ The "realized" point is not changed. "guest visible" concept only occurs *after* wiring, just before the reset phase. This change introduces the DeviceClass::wire handler within qdev core realization code. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- include/hw/qdev-core.h | 7 +++++++ hw/core/qdev.c | 20 +++++++++++++++++++- 2 files changed, 26 insertions(+), 1 deletion(-)