| Message ID | 1352473012-20500-20-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <ehabkost@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. 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) { >
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 <ehabkost@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. > > 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
On 15 November 2012 18:42, Eduardo Habkost <ehabkost@redhat.com> wrote: > 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. cf the pretty nasty stuff in linux-user/main.c which currently calls cpu_reset() but only for some targets. -- PMM
On Thu, 15 Nov 2012 16:42:41 -0200 Eduardo Habkost <ehabkost@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 <ehabkost@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. 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 >
On Fri, Nov 30, 2012 at 05:56:26PM +0100, Igor Mammedov wrote: > On Thu, 15 Nov 2012 16:42:41 -0200 > Eduardo Habkost <ehabkost@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 <ehabkost@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 > > >
On 30 November 2012 21:38, Eduardo Habkost <ehabkost@redhat.com> wrote: > 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). I think this is because the different targets disagree about whether the CPU should be reset on initial construction or whether it needs a specific reset call. (The current setup with #ifdefs is among other things a historical effect as a result of various refactorings in the past; you can trace the git history if you're interested.) -- PMM
Am 01.12.2012 12:26, schrieb Peter Maydell: > On 30 November 2012 21:38, Eduardo Habkost <ehabkost@redhat.com> wrote: >> 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). > > I think this is because the different targets disagree about whether > the CPU should be reset on initial construction or whether it needs > a specific reset call. (The current setup with #ifdefs is among other > things a historical effect as a result of various refactorings in > the past; you can trace the git history if you're interested.) Peter and me had long IRC discussions about how to fix this in the past: * On my qom-cpu-copy branch I have a patch queued that drops the #ifdef above, accepting that CPUs may get reset twice then. => Dispels doubt for target authors; doubts about correctness though. * PMM suggested to move cpu_clone_regs() from target-*/cpu.h to *-user/. => Would lead to duplication between linux-user and bsd-user; ABI? * PMM suggested to replace cpu_copy() with ABI-specific code in *-user/. Unfortunately I don't quite remember the details of how... ;) The x86 APIC refactorings that Iguardo have done do bring us very close to sane cpu_reset() semantics (ignoring the two hands full of hard/soft/... reset variants that ppc and other architectures feature). Declaring cpu_reset() inferior to reset handlers due to the linux-user mess is going into the wrong direction - some targets seem to ignore reset or fork/clone completely at the moment, so the state we see cannot be considered fully correct. In particular the above reset is being performed *after* cpu_copy() memcpy()'ed random memory contents (which for some targets may contain pointers), undoing the copying in large parts. Therefore, when all targets reset as part of cpu_init() (or by moving the cpu_reset() call into early cpu_copy()?) we could get rid of it in do_fork() and of its weird conditions. Andreas
On 2 December 2012 05:44, Andreas Färber <afaerber@suse.de> wrote: > Am 01.12.2012 12:26, schrieb Peter Maydell: >> On 30 November 2012 21:38, Eduardo Habkost <ehabkost@redhat.com> wrote: >>> 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). >> >> I think this is because the different targets disagree about whether >> the CPU should be reset on initial construction or whether it needs >> a specific reset call. (The current setup with #ifdefs is among other >> things a historical effect as a result of various refactorings in >> the past; you can trace the git history if you're interested.) > > Peter and me had long IRC discussions about how to fix this in the past: > > * On my qom-cpu-copy branch I have a patch queued that drops the > #ifdef above, accepting that CPUs may get reset twice then. > => Dispels doubt for target authors; doubts about correctness though. So on its own, this will break things, because the cpu_reset() happens after the cpu_copy, so we'll end up not in fact copying most of the registers across from the original thread. In fact this looks broken for i386/sparc/ppc at the moment, and is probably only working to the extent that user programs don't actually care about the register state across the clone() syscall. [and i386 threads are busted anyhow.] If we want to retain cpu_copy for the moment, the correct place for the cpu_reset() is in cpu_copy() itself, after the cpu_init but before the memcpy. [I see actually you mention this later in your email.] > * PMM suggested to move cpu_clone_regs() from target-*/cpu.h to *-user/. > => Would lead to duplication between linux-user and bsd-user; ABI? clone_regs should definitely be in *-user/, because it is in fact ABI dependent -- which register the 0 return value from the clone() syscall ends up in is up to the ABI. Also, there won't in fact be any duplication problem here because bsd-user doesn't implement threads and doesn't call cpu_clone_regs(). > * PMM suggested to replace cpu_copy() with ABI-specific code in *-user/. > Unfortunately I don't quite remember the details of how... ;) I don't know that I got as far as the details, but the general idea is that the CPU state that needs to be propagated to the CPU in the new thread is only the user-space-facing bits, which *-user/ already needs to know about (and fish directly in CPUState for) to get things like signal handlers right. So rather than doing a huge memcpy of all the CPU's internal state, we just create and reset a CPU as normal, and then have the *-user/ code copy across those bits of register state which matter to userspace. I think this is much cleaner because it means the CPU emulation itself has a simple interface ("create cpu" and "reset cpu") and the details of copying thread state across are restricted to the user-mode code. -- PMM
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; + } + } +} + +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) {
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 <ehabkost@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