Re: [Qemu-devel] [PATCH 19/24] qdev: move reset handler list from vl.c to qdev.c

[Igor Mammedov]

On Thu, 15 Nov 2012 16:42:41 -0200
Eduardo Habkost <address@hidden> wrote:

> On Thu, Nov 15, 2012 at 02:54:57AM +0100, Andreas Färber wrote:
> > Am 09.11.2012 15:56, schrieb Eduardo Habkost:
> > > The core qdev code uses the reset handler list from vl.c, so move
> > > qemu_register_reset(), qemu_unregister_reset() and qemu_devices_reset()
> > > to qdev.c.
> > > 
> > > The function declarations were moved to a new qdev-reset.h file, that is
> > > included by hw.h to keep compatibility, so we don't need to change all
> > > files that use qemu_register_reset().
> > > 
> > > Signed-off-by: Eduardo Habkost <address@hidden>
> > > ---
> > >  hw/hw.h         |  6 +-----
> > >  hw/qdev-reset.h | 11 +++++++++++
> > >  hw/qdev.c       | 41 +++++++++++++++++++++++++++++++++++++++++
> > >  hw/qdev.h       |  1 +
> > >  sysemu.h        |  1 -
> > >  vl.c            | 40 ----------------------------------------
> > >  6 files changed, 54 insertions(+), 46 deletions(-)
> > >  create mode 100644 hw/qdev-reset.h
> > > 
> > > diff --git a/hw/hw.h b/hw/hw.h
> > > index f530f6f..622a157 100644
> > > --- a/hw/hw.h
> > > +++ b/hw/hw.h
> > > @@ -14,6 +14,7 @@
> > >  #include "qemu-file.h"
> > >  #include "vmstate.h"
> > >  #include "qemu-log.h"
> > > +#include "qdev-reset.h"
> > >  
> > >  #ifdef NEED_CPU_H
> > >  #if TARGET_LONG_BITS == 64
> > > @@ -37,11 +38,6 @@
> > >  #endif
> > >  #endif
> > >  
> > > -typedef void QEMUResetHandler(void *opaque);
> > > -
> > > -void qemu_register_reset(QEMUResetHandler *func, void *opaque);
> > > -void qemu_unregister_reset(QEMUResetHandler *func, void *opaque);
> > > -
> > >  /* handler to set the boot_device order for a specific type of
> > > QEMUMachine */ /* return 0 if success */
> > >  typedef int QEMUBootSetHandler(void *opaque, const char *boot_devices);
> > > diff --git a/hw/qdev-reset.h b/hw/qdev-reset.h
> > > new file mode 100644
> > > index 0000000..40ae9a5
> > > --- /dev/null
> > > +++ b/hw/qdev-reset.h
> > > @@ -0,0 +1,11 @@
> > > +/* Device reset handler function registration, used by qdev */
> > > +#ifndef QDEV_RESET_H
> > > +#define QDEV_RESET_H
> > > +
> > > +typedef void QEMUResetHandler(void *opaque);
> > > +
> > > +void qemu_register_reset(QEMUResetHandler *func, void *opaque);
> > > +void qemu_unregister_reset(QEMUResetHandler *func, void *opaque);
> > > +void qemu_devices_reset(void);
> > > +
> > > +#endif /* QDEV_RESET_H */
> > > diff --git a/hw/qdev.c b/hw/qdev.c
> > > index 2cc6434..c242097 100644
> > > --- a/hw/qdev.c
> > > +++ b/hw/qdev.c
> > > @@ -35,6 +35,47 @@ int qdev_hotplug = 0;
> > >  static bool qdev_hot_added = false;
> > >  static bool qdev_hot_removed = false;
> > >  
> > > +typedef struct QEMUResetEntry {
> > > +    QTAILQ_ENTRY(QEMUResetEntry) entry;
> > > +    QEMUResetHandler *func;
> > > +    void *opaque;
> > > +} QEMUResetEntry;
> > > +
> > > +static QTAILQ_HEAD(reset_handlers, QEMUResetEntry) reset_handlers =
> > > +    QTAILQ_HEAD_INITIALIZER(reset_handlers);
> > > +
> > > +void qemu_register_reset(QEMUResetHandler *func, void *opaque)
> > > +{
> > > +    QEMUResetEntry *re = g_malloc0(sizeof(QEMUResetEntry));
> > > +
> > > +    re->func = func;
> > > +    re->opaque = opaque;
> > > +    QTAILQ_INSERT_TAIL(&reset_handlers, re, entry);
> > > +}
> > > +
> > > +void qemu_unregister_reset(QEMUResetHandler *func, void *opaque)
> > > +{
> > > +    QEMUResetEntry *re;
> > > +
> > > +    QTAILQ_FOREACH(re, &reset_handlers, entry) {
> > > +        if (re->func == func && re->opaque == opaque) {
> > > +            QTAILQ_REMOVE(&reset_handlers, re, entry);
> > > +            g_free(re);
> > > +            return;
> > > +        }
> > > +    }
> > > +}
> > 
> > My tired mind does not like this move and the naming qdev-reset.h.
> > The reset handling infrastructure is not limited to DeviceState (qdev),
> > it takes an opaque and is limited to softmmu whereas qdev prefers its
> > own DeviceClass::reset hook.
> 
> True, it isn't qdev-specific. But it doesn't belong to vl.c either.
> Should we create a reset.o file just for those few lines of code, or is
> there any obvious place where this code can go?
> 
> DeviceState CPUs have to be reset too, and DeviceState uses the
> reset-handler system to make sure DeviceState objects are reset, so it
> won't be softmmu-specific.
> 
> An alternative is to make empty stubs for qemu_[un]register_reset(), and
> not move the reset-handler system to *-user. But somehow I feel better
> having a working qemu_register_reset() function (and then adding a
> qemu_devices_reset() call to *-user) than having a fake
> qemu_register_reset() function and having to use a different API to
> reset the CPUs on on *-user.
There is no real need to add anything, *-user uses pretty much well defined
cpu_reset() and doesn't need anything else to improve it.

PS:
reset-handler callbacks is not much loved thing in qemu, and people would
like to get rid of it eventually. If one checks target-i386/cpu.c:

/* TODO: remove me, when reset over QOM tree is implemented */
static void x86_cpu_machine_reset_cb(void *opaque)

some day in far far (or maybe not far) future there won't be reset cb for
x86-cpu.
Could you just leave reset-handler be a *-softmmu only part.
> 
> > 
> > Andreas
> > 
> > > +
> > > +void qemu_devices_reset(void)
> > > +{
> > > +    QEMUResetEntry *re, *nre;
> > > +
> > > +    /* reset all devices */
> > > +    QTAILQ_FOREACH_SAFE(re, &reset_handlers, entry, nre) {
> > > +        re->func(re->opaque);
> > > +    }
> > > +}
> > > +
> > >  const VMStateDescription *qdev_get_vmsd(DeviceState *dev)
> > >  {
> > >      DeviceClass *dc = DEVICE_GET_CLASS(dev);
> > > diff --git a/hw/qdev.h b/hw/qdev.h
> > > index 365b8d6..2487b3b 100644
> > > --- a/hw/qdev.h
> > > +++ b/hw/qdev.h
> > > @@ -5,5 +5,6 @@
> > >  #include "qdev-core.h"
> > >  #include "qdev-properties.h"
> > >  #include "qdev-monitor.h"
> > > +#include "qdev-reset.h"
> > >  
> > >  #endif
> > > diff --git a/sysemu.h b/sysemu.h
> > > index ab1ef8b..51f19cc 100644
> > > --- a/sysemu.h
> > > +++ b/sysemu.h
> > > @@ -57,7 +57,6 @@ void qemu_system_vmstop_request(RunState reason);
> > >  int qemu_shutdown_requested_get(void);
> > >  int qemu_reset_requested_get(void);
> > >  void qemu_system_killed(int signal, pid_t pid);
> > > -void qemu_devices_reset(void);
> > >  void qemu_system_reset(bool report);
> > >  
> > >  void qemu_add_exit_notifier(Notifier *notify);
> > > diff --git a/vl.c b/vl.c
> > > index 4f03a72..c7448a2 100644
> > > --- a/vl.c
> > > +++ b/vl.c
> > > @@ -1456,14 +1456,6 @@ void vm_start(void)
> > >  
> > >  /* reset/shutdown handler */
> > >  
> > > -typedef struct QEMUResetEntry {
> > > -    QTAILQ_ENTRY(QEMUResetEntry) entry;
> > > -    QEMUResetHandler *func;
> > > -    void *opaque;
> > > -} QEMUResetEntry;
> > > -
> > > -static QTAILQ_HEAD(reset_handlers, QEMUResetEntry) reset_handlers =
> > > -    QTAILQ_HEAD_INITIALIZER(reset_handlers);
> > >  static int reset_requested;
> > >  static int shutdown_requested, shutdown_signal = -1;
> > >  static pid_t shutdown_pid;
> > > @@ -1560,38 +1552,6 @@ static bool qemu_vmstop_requested(RunState *r)
> > >      return false;
> > >  }
> > >  
> > > -void qemu_register_reset(QEMUResetHandler *func, void *opaque)
> > > -{
> > > -    QEMUResetEntry *re = g_malloc0(sizeof(QEMUResetEntry));
> > > -
> > > -    re->func = func;
> > > -    re->opaque = opaque;
> > > -    QTAILQ_INSERT_TAIL(&reset_handlers, re, entry);
> > > -}
> > > -
> > > -void qemu_unregister_reset(QEMUResetHandler *func, void *opaque)
> > > -{
> > > -    QEMUResetEntry *re;
> > > -
> > > -    QTAILQ_FOREACH(re, &reset_handlers, entry) {
> > > -        if (re->func == func && re->opaque == opaque) {
> > > -            QTAILQ_REMOVE(&reset_handlers, re, entry);
> > > -            g_free(re);
> > > -            return;
> > > -        }
> > > -    }
> > > -}
> > > -
> > > -void qemu_devices_reset(void)
> > > -{
> > > -    QEMUResetEntry *re, *nre;
> > > -
> > > -    /* reset all devices */
> > > -    QTAILQ_FOREACH_SAFE(re, &reset_handlers, entry, nre) {
> > > -        re->func(re->opaque);
> > > -    }
> > > -}
> > > -
> > >  void qemu_system_reset(bool report)
> > >  {
> > >      if (current_machine && current_machine->reset) {
> > > 
> > 
> > 
> > -- 
> > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>