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

["Cédric Le Goater" <clg@redhat.com>]

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