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

[Eduardo Habkost]

On Fri, Nov 30, 2012 at 05:56:26PM +0100, Igor Mammedov wrote:
> 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.

cpu_reset() is not that well-defined, otherwise we wouldn't have this on
linux-user:

#if defined(TARGET_I386) || defined(TARGET_SPARC) || defined(TARGET_PPC)
    cpu_reset(ENV_GET_CPU(env));
#endif

(I have no idea why we have that #ifdef).

But that can be fixed, eventually, and:

> 
> 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.

I was considering this as well: even if cpu_reset() is not in great
shape, qemu_register_reset() is something we would like to kill as well.

I sent a v8 DeviceState series including reset.o on *-user, already, but
I will try removing it on a v9 series, next week.


> > 
> > > 
> > > 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
> > 
> 

-- 
Eduardo