| Message ID | 5acc81b16d52949a47cbcbfcc2eacc0e4f3a5687.1645079934.git.jag.raman@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio-user server in QEMU | expand |
On Thu, Feb 17, 2022 at 02:48:50AM -0500, Jagannathan Raman wrote: > Add blocker to prevent hot-unplug of devices > > Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com> > Signed-off-by: John G Johnson <john.g.johnson@oracle.com> > Signed-off-by: Jagannathan Raman <jag.raman@oracle.com> > --- > include/hw/qdev-core.h | 35 +++++++++++++++++++++++++++++++++++ > softmmu/qdev-monitor.c | 26 ++++++++++++++++++++++++++ > 2 files changed, 61 insertions(+) > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > index 92c3d65208..4b1d77f44a 100644 > --- a/include/hw/qdev-core.h > +++ b/include/hw/qdev-core.h > @@ -193,6 +193,7 @@ struct DeviceState { > int instance_id_alias; > int alias_required_for_version; > ResettableState reset; > + GSList *unplug_blockers; > }; > > struct DeviceListener { > @@ -419,6 +420,40 @@ void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev, > void qdev_machine_creation_done(void); > bool qdev_machine_modified(void); > > +/** > + * Device Unplug blocker: prevents a device from being unplugged. It could ^^^^^^^^^^^^^^^^^^^^^ This looks strange. gtkdoc will probably treat it as the doc comment for qdev_add_unplug_blocker(), which is actually defined below. I suggest not trying to define a new section in the documentation and instead just focussing on doc comments for qdev_add_unplug_block() and other functions. The gtkdoc way of defining sections is covered here but it's almost never used in QEMU: https://developer-old.gnome.org/gtk-doc-manual/stable/documenting_sections.html.en > + * be used to indicate that another object depends on the device. > + * > + * qdev_add_unplug_blocker: Adds an unplug blocker to a device > + * > + * @dev: Device to be blocked from unplug > + * @reason: Reason for blocking > + * > + */ > +void qdev_add_unplug_blocker(DeviceState *dev, Error *reason); Does the caller have to call qdev_del_unplug_blocker() later? An assert(!dev->unplug_blockers) would be nice when DeviceState is destroyed. That way leaks will be caught. > + > +/** > + * qdev_del_unplug_blocker: Removes an unplug blocker from a device > + * > + * @dev: Device to be unblocked > + * @reason: Pointer to the Error used with qdev_add_unplug_blocker. > + * Used as a handle to lookup the blocker for deletion. > + * > + */ > +void qdev_del_unplug_blocker(DeviceState *dev, Error *reason); > + > +/** > + * qdev_unplug_blocked: Confirms if a device is blocked from unplug > + * > + * @dev: Device to be tested > + * @reason: Returns one of the reasons why the device is blocked, > + * if any > + * > + * Returns: true if device is blocked from unplug, false otherwise > + * > + */ > +bool qdev_unplug_blocked(DeviceState *dev, Error **errp); > + > /** > * GpioPolarity: Polarity of a GPIO line > * > diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c > index 01f3834db5..69d9cf3f25 100644 > --- a/softmmu/qdev-monitor.c > +++ b/softmmu/qdev-monitor.c > @@ -945,10 +945,36 @@ void qmp_device_del(const char *id, Error **errp) > return; > } > > + if (qdev_unplug_blocked(dev, errp)) { > + return; > + } > + > qdev_unplug(dev, errp); > } > } > > +void qdev_add_unplug_blocker(DeviceState *dev, Error *reason) These functions belong in hw/core/qdev.c because they are part of the DeviceState API, not qdev monitor commands? > +{ > + dev->unplug_blockers = g_slist_prepend(dev->unplug_blockers, reason); > +} > + > +void qdev_del_unplug_blocker(DeviceState *dev, Error *reason) > +{ > + dev->unplug_blockers = g_slist_remove(dev->unplug_blockers, reason); > +} > + > +bool qdev_unplug_blocked(DeviceState *dev, Error **errp) > +{ > + ERRP_GUARD(); > + > + if (dev->unplug_blockers) { > + error_propagate(errp, error_copy(dev->unplug_blockers->data)); > + return true; > + } > + > + return false; > +} > + > void hmp_device_add(Monitor *mon, const QDict *qdict) > { > Error *err = NULL; > -- > 2.20.1 >
On Thu, Feb 17, 2022 at 02:48:50AM -0500, Jagannathan Raman wrote: > diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c > index 01f3834db5..69d9cf3f25 100644 > --- a/softmmu/qdev-monitor.c > +++ b/softmmu/qdev-monitor.c > @@ -945,10 +945,36 @@ void qmp_device_del(const char *id, Error **errp) > return; > } > > + if (qdev_unplug_blocked(dev, errp)) { > + return; > + } > + > qdev_unplug(dev, errp); Can qdev_unplug() check this internally?
> On Feb 21, 2022, at 10:27 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote: > > On Thu, Feb 17, 2022 at 02:48:50AM -0500, Jagannathan Raman wrote: >> Add blocker to prevent hot-unplug of devices >> >> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com> >> Signed-off-by: John G Johnson <john.g.johnson@oracle.com> >> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com> >> --- >> include/hw/qdev-core.h | 35 +++++++++++++++++++++++++++++++++++ >> softmmu/qdev-monitor.c | 26 ++++++++++++++++++++++++++ >> 2 files changed, 61 insertions(+) >> >> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h >> index 92c3d65208..4b1d77f44a 100644 >> --- a/include/hw/qdev-core.h >> +++ b/include/hw/qdev-core.h >> @@ -193,6 +193,7 @@ struct DeviceState { >> int instance_id_alias; >> int alias_required_for_version; >> ResettableState reset; >> + GSList *unplug_blockers; >> }; >> >> struct DeviceListener { >> @@ -419,6 +420,40 @@ void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev, >> void qdev_machine_creation_done(void); >> bool qdev_machine_modified(void); >> >> +/** >> + * Device Unplug blocker: prevents a device from being unplugged. It could > ^^^^^^^^^^^^^^^^^^^^^ > > This looks strange. gtkdoc will probably treat it as the doc comment for > qdev_add_unplug_blocker(), which is actually defined below. I suggest > not trying to define a new section in the documentation and instead just > focussing on doc comments for qdev_add_unplug_block() and other > functions. Sorry I assumed that we needed an extra ‘*’ at the beginning of the comment. I got this idea when checking out block.c and blockdev.c while working on the migration patch. I’ll follow the “Comment style” section in “docs/devel/style.rst" > > The gtkdoc way of defining sections is covered here but it's almost > never used in QEMU: > https://developer-old.gnome.org/gtk-doc-manual/stable/documenting_sections.html.en > >> + * be used to indicate that another object depends on the device. >> + * >> + * qdev_add_unplug_blocker: Adds an unplug blocker to a device >> + * >> + * @dev: Device to be blocked from unplug >> + * @reason: Reason for blocking >> + * >> + */ >> +void qdev_add_unplug_blocker(DeviceState *dev, Error *reason); > > Does the caller have to call qdev_del_unplug_blocker() later? > > An assert(!dev->unplug_blockers) would be nice when DeviceState is > destroyed. That way leaks will be caught. Makes sense, will add. > >> + >> +/** >> + * qdev_del_unplug_blocker: Removes an unplug blocker from a device >> + * >> + * @dev: Device to be unblocked >> + * @reason: Pointer to the Error used with qdev_add_unplug_blocker. >> + * Used as a handle to lookup the blocker for deletion. >> + * >> + */ >> +void qdev_del_unplug_blocker(DeviceState *dev, Error *reason); >> + >> +/** >> + * qdev_unplug_blocked: Confirms if a device is blocked from unplug >> + * >> + * @dev: Device to be tested >> + * @reason: Returns one of the reasons why the device is blocked, >> + * if any >> + * >> + * Returns: true if device is blocked from unplug, false otherwise >> + * >> + */ >> +bool qdev_unplug_blocked(DeviceState *dev, Error **errp); >> + >> /** >> * GpioPolarity: Polarity of a GPIO line >> * >> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c >> index 01f3834db5..69d9cf3f25 100644 >> --- a/softmmu/qdev-monitor.c >> +++ b/softmmu/qdev-monitor.c >> @@ -945,10 +945,36 @@ void qmp_device_del(const char *id, Error **errp) >> return; >> } >> >> + if (qdev_unplug_blocked(dev, errp)) { >> + return; >> + } >> + >> qdev_unplug(dev, errp); >> } >> } >> >> +void qdev_add_unplug_blocker(DeviceState *dev, Error *reason) > > These functions belong in hw/core/qdev.c because they are part of the > DeviceState API, not qdev monitor commands? Both hw/core/qdev.c and softmmu/qdev-monitor.c seem to manage the DeviceState. softmmu/qdev-monitor.c seems to manage device addition and removal using qdev_device_add() and qdev_unplug(). I noticed that some functions in this file change DeviceState. For example, qdev_device_add() sets DeviceState->opts, qdev_set_id() sets DeviceState->id. Given the above two reasons, I thought it the unplug blockers could be better place here. Since hw/core/qdev.c makes the majority of changes to the DeviceState, moving the unplug blockers over there makes sense to me. Thank you! -- Jag > >> +{ >> + dev->unplug_blockers = g_slist_prepend(dev->unplug_blockers, reason); >> +} >> + >> +void qdev_del_unplug_blocker(DeviceState *dev, Error *reason) >> +{ >> + dev->unplug_blockers = g_slist_remove(dev->unplug_blockers, reason); >> +} >> + >> +bool qdev_unplug_blocked(DeviceState *dev, Error **errp) >> +{ >> + ERRP_GUARD(); >> + >> + if (dev->unplug_blockers) { >> + error_propagate(errp, error_copy(dev->unplug_blockers->data)); >> + return true; >> + } >> + >> + return false; >> +} >> + >> void hmp_device_add(Monitor *mon, const QDict *qdict) >> { >> Error *err = NULL; >> -- >> 2.20.1 >>
> On Feb 21, 2022, at 10:30 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote: > > On Thu, Feb 17, 2022 at 02:48:50AM -0500, Jagannathan Raman wrote: >> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c >> index 01f3834db5..69d9cf3f25 100644 >> --- a/softmmu/qdev-monitor.c >> +++ b/softmmu/qdev-monitor.c >> @@ -945,10 +945,36 @@ void qmp_device_del(const char *id, Error **errp) >> return; >> } >> >> + if (qdev_unplug_blocked(dev, errp)) { >> + return; >> + } >> + >> qdev_unplug(dev, errp); > > Can qdev_unplug() check this internally? Yes, I think qdev_unplug() could check this internally. Will move it there. Thank you! -- Jag
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index 92c3d65208..4b1d77f44a 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -193,6 +193,7 @@ struct DeviceState { int instance_id_alias; int alias_required_for_version; ResettableState reset; + GSList *unplug_blockers; }; struct DeviceListener { @@ -419,6 +420,40 @@ void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev, void qdev_machine_creation_done(void); bool qdev_machine_modified(void); +/** + * Device Unplug blocker: prevents a device from being unplugged. It could + * be used to indicate that another object depends on the device. + * + * qdev_add_unplug_blocker: Adds an unplug blocker to a device + * + * @dev: Device to be blocked from unplug + * @reason: Reason for blocking + * + */ +void qdev_add_unplug_blocker(DeviceState *dev, Error *reason); + +/** + * qdev_del_unplug_blocker: Removes an unplug blocker from a device + * + * @dev: Device to be unblocked + * @reason: Pointer to the Error used with qdev_add_unplug_blocker. + * Used as a handle to lookup the blocker for deletion. + * + */ +void qdev_del_unplug_blocker(DeviceState *dev, Error *reason); + +/** + * qdev_unplug_blocked: Confirms if a device is blocked from unplug + * + * @dev: Device to be tested + * @reason: Returns one of the reasons why the device is blocked, + * if any + * + * Returns: true if device is blocked from unplug, false otherwise + * + */ +bool qdev_unplug_blocked(DeviceState *dev, Error **errp); + /** * GpioPolarity: Polarity of a GPIO line * diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c index 01f3834db5..69d9cf3f25 100644 --- a/softmmu/qdev-monitor.c +++ b/softmmu/qdev-monitor.c @@ -945,10 +945,36 @@ void qmp_device_del(const char *id, Error **errp) return; } + if (qdev_unplug_blocked(dev, errp)) { + return; + } + qdev_unplug(dev, errp); } } +void qdev_add_unplug_blocker(DeviceState *dev, Error *reason) +{ + dev->unplug_blockers = g_slist_prepend(dev->unplug_blockers, reason); +} + +void qdev_del_unplug_blocker(DeviceState *dev, Error *reason) +{ + dev->unplug_blockers = g_slist_remove(dev->unplug_blockers, reason); +} + +bool qdev_unplug_blocked(DeviceState *dev, Error **errp) +{ + ERRP_GUARD(); + + if (dev->unplug_blockers) { + error_propagate(errp, error_copy(dev->unplug_blockers->data)); + return true; + } + + return false; +} + void hmp_device_add(Monitor *mon, const QDict *qdict) { Error *err = NULL;