On Fri, Nov 30, 2012 at 05:56:26PM +0100, Igor Mammedov wrote:
> On Thu, 15 Nov 2012 16:42:41 -0200
> Eduardo Habkost <ehabk...@redhat.com> 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 <ehabk...@redhat.com>
> > > > ---
> > > > 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
