Re: [PATCH 2/4] reset: Allow multiple stages of system resets

[Eric Auger]

Hi Peter,
On 1/17/24 10:15, peterx@redhat.com wrote:
> From: Peter Xu <peterx@redhat.com>
>
> QEMU resets do not have a way to order reset hooks.  Add one coarse grained
> reset stage so that some devices can be reset later than some others.
I would precise that the lowest stage has the highest priority and is
handled first.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/sysemu/reset.h |  5 ++++
>  hw/core/reset.c        | 60 +++++++++++++++++++++++++++++++-----------
>  2 files changed, 49 insertions(+), 16 deletions(-)
>
> diff --git a/include/sysemu/reset.h b/include/sysemu/reset.h
> index 609e4d50c2..0de697ce9f 100644
> --- a/include/sysemu/reset.h
> +++ b/include/sysemu/reset.h
> @@ -5,9 +5,14 @@
>  
>  typedef void QEMUResetHandler(void *opaque);
>  
> +#define  QEMU_RESET_STAGES_N  2
> +
>  void qemu_register_reset(QEMUResetHandler *func, void *opaque);
> +void qemu_register_reset_one(QEMUResetHandler *func, void *opaque,
> +                             bool skip_snap, int stage);
>  void qemu_register_reset_nosnapshotload(QEMUResetHandler *func, void 
> *opaque);
>  void qemu_unregister_reset(QEMUResetHandler *func, void *opaque);
> +void qemu_unregister_reset_one(QEMUResetHandler *func, void *opaque, int 
> stage);
>  void qemu_devices_reset(ShutdownCause reason);
>  
>  #endif
> diff --git a/hw/core/reset.c b/hw/core/reset.c
> index 8cf60b2b09..a84c9bee84 100644
> --- a/hw/core/reset.c
> +++ b/hw/core/reset.c
> @@ -36,55 +36,83 @@ typedef struct QEMUResetEntry {
>      bool skip_on_snapshot_load;
>  } QEMUResetEntry;
>  
> -static QTAILQ_HEAD(, QEMUResetEntry) reset_handlers =
> -    QTAILQ_HEAD_INITIALIZER(reset_handlers);
> +typedef QTAILQ_HEAD(QEMUResetList, QEMUResetEntry) QEMUResetList;
> +static QEMUResetList reset_handlers[QEMU_RESET_STAGES_N];
>  
> -static void qemu_register_reset_one(QEMUResetHandler *func, void *opaque,
> -                                    bool skip_snap)
> +static void __attribute__((__constructor__)) qemu_reset_handlers_init(void)
> +{
> +    QEMUResetList *head;
> +    int i = 0;
nit: you may put the declarations within the block
> +
> +    for (i = 0; i < QEMU_RESET_STAGES_N; i++) {
> +        head = &reset_handlers[i];
> +        QTAILQ_INIT(head);
> +    }
> +}
> +
> +void qemu_register_reset_one(QEMUResetHandler *func, void *opaque,
> +                             bool skip_snap, int stage)
>  {
>      QEMUResetEntry *re = g_new0(QEMUResetEntry, 1);
> +    QEMUResetList *head;
> +
> +    assert(stage >= 0 && stage < QEMU_RESET_STAGES_N);
> +    head = &reset_handlers[stage];
>  
>      re->func = func;
>      re->opaque = opaque;
>      re->skip_on_snapshot_load = skip_snap;
> -    QTAILQ_INSERT_TAIL(&reset_handlers, re, entry);
> +    QTAILQ_INSERT_TAIL(head, re, entry);
>  }
>  
>  void qemu_register_reset(QEMUResetHandler *func, void *opaque)
>  {
> -    /* By default, do not skip during load of a snapshot */
Shouldn't the above comment stay since the statement is not affected by
this patch? Or remove it in previous patch?
> -    qemu_register_reset_one(func, opaque, false);
> +    qemu_register_reset_one(func, opaque, false, 0);
>  }
>  
>  void qemu_register_reset_nosnapshotload(QEMUResetHandler *func, void *opaque)
>  {
> -    qemu_register_reset_one(func, opaque, true);
> +    qemu_register_reset_one(func, opaque, true, 0);
>  }
>  
> -void qemu_unregister_reset(QEMUResetHandler *func, void *opaque)
> +void qemu_unregister_reset_one(QEMUResetHandler *func, void *opaque, int 
> stage)
>  {
> +    QEMUResetList *head;
>      QEMUResetEntry *re;
>  
> -    QTAILQ_FOREACH(re, &reset_handlers, entry) {
> +    assert(stage >= 0 && stage < QEMU_RESET_STAGES_N);
> +    head = &reset_handlers[stage];
> +
> +    QTAILQ_FOREACH(re, head, entry) {
>          if (re->func == func && re->opaque == opaque) {
> -            QTAILQ_REMOVE(&reset_handlers, re, entry);
> +            QTAILQ_REMOVE(head, re, entry);
>              g_free(re);
>              return;
>          }
>      }
>  }
>  
> +void qemu_unregister_reset(QEMUResetHandler *func, void *opaque)
> +{
> +    qemu_unregister_reset_one(func, opaque, 0);
> +}
> +
>  void qemu_devices_reset(ShutdownCause reason)
>  {
>      QEMUResetEntry *re, *nre;
> +    QEMUResetList *head;
> +    int stage;
>  
>      /* reset all devices */
> -    QTAILQ_FOREACH_SAFE(re, &reset_handlers, entry, nre) {
> -        if (reason == SHUTDOWN_CAUSE_SNAPSHOT_LOAD &&
> -            re->skip_on_snapshot_load) {
> -            continue;
> +    for (stage = 0; stage < QEMU_RESET_STAGES_N; stage++) {
> +        head = &reset_handlers[stage];
> +        QTAILQ_FOREACH_SAFE(re, head, entry, nre) {
> +            if (reason == SHUTDOWN_CAUSE_SNAPSHOT_LOAD &&
> +                re->skip_on_snapshot_load) {
> +                continue;
> +            }
> +            re->func(re->opaque);
>          }
> -        re->func(re->opaque);
>      }
>  }
>  
Thanks

Eric