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

[Cédric Le Goater]

On 1/17/24 11:28, Eric Auger wrote:
> 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.

yes. May be add an enum like we have for migration :

typedef enum {
    MIG_PRI_DEFAULT = 0,
    MIG_PRI_IOMMU,              /* Must happen before PCI devices */
    MIG_PRI_PCI_BUS,            /* Must happen before IOMMU */
    MIG_PRI_VIRTIO_MEM,         /* Must happen before IOMMU */
    MIG_PRI_GICV3_ITS,          /* Must happen before PCI devices */
    MIG_PRI_GICV3,              /* Must happen before the ITS */
    MIG_PRI_MAX,
} MigrationPriority;

I think it would help understand the reset ordering and maintenance
when grepping qemu_register_reset_one().

Thanks,

C.



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