Re: [PATCH for-8.2 2/6] sysemu: Add pre VM state change callback

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

On 7/16/23 10:15, Avihai Horon wrote:
> Add pre VM state change callback to struct VMChangeStateEntry.

This sentence could be the subject.

> 
> The pre VM state change callback is optional and can be set by the new
> function qemu_add_vm_change_state_handler_prio_full() that allows
> setting this callback in addition to the main callback.
> 
> The pre VM state change callbacks and main callbacks are called in two
> separate phases: First all pre VM state change callbacks are called and
> only then all main callbacks are called.
> 
> The purpose of the new pre VM state change callback is to allow all
> devices to run a preliminary task before calling the devices' main
> callbacks.
> 
> This will facilitate adding P2P support for VFIO migration where all
> VFIO devices need to be put in an intermediate P2P quiescent state
> before being stopped or started by the main VM state change callback.
> 
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> ---
>   include/sysemu/runstate.h |  4 ++++
>   softmmu/runstate.c        | 39 +++++++++++++++++++++++++++++++++++++++
>   2 files changed, 43 insertions(+)
> 
> diff --git a/include/sysemu/runstate.h b/include/sysemu/runstate.h
> index 7beb29c2e2..bb38a4b4bd 100644
> --- a/include/sysemu/runstate.h
> +++ b/include/sysemu/runstate.h
> @@ -16,6 +16,10 @@ VMChangeStateEntry *qemu_add_vm_change_state_handler(VMChangeStateHandler *cb,
>                                                        void *opaque);
>   VMChangeStateEntry *qemu_add_vm_change_state_handler_prio(
>           VMChangeStateHandler *cb, void *opaque, int priority);
> +VMChangeStateEntry *
> +qemu_add_vm_change_state_handler_prio_full(VMChangeStateHandler *cb,
> +                                           VMChangeStateHandler *pre_change_cb,
> +                                           void *opaque, int priority);
>   VMChangeStateEntry *qdev_add_vm_change_state_handler(DeviceState *dev,
>                                                        VMChangeStateHandler *cb,
>                                                        void *opaque);
> diff --git a/softmmu/runstate.c b/softmmu/runstate.c
> index f3bd862818..a1f0653899 100644
> --- a/softmmu/runstate.c
> +++ b/softmmu/runstate.c
> @@ -271,6 +271,7 @@ void qemu_system_vmstop_request(RunState state)
>   }
>   struct VMChangeStateEntry {
>       VMChangeStateHandler *cb;
> +    VMChangeStateHandler *pre_change_cb;

I propose to use 'prepare' instead of 'pre_change'


>       void *opaque;
>       QTAILQ_ENTRY(VMChangeStateEntry) entries;
>       int priority;
> @@ -293,12 +294,38 @@ static QTAILQ_HEAD(, VMChangeStateEntry) vm_change_state_head =
>    */
>   VMChangeStateEntry *qemu_add_vm_change_state_handler_prio(
>           VMChangeStateHandler *cb, void *opaque, int priority)
> +{
> +    return qemu_add_vm_change_state_handler_prio_full(cb, NULL, opaque,
> +                                                      priority);
> +}
> +
> +/**
> + * qemu_add_vm_change_state_handler_prio_full:

qemu_add_vm_change_state_handler_prio_all() may be. I don't have much better
but 'full' doesn't sound right. minor.

The rest looks fine to me.

Thanks,

C.

> + * @cb: the main callback to invoke
> + * @pre_change_cb: a callback to invoke before the main callback
> + * @opaque: user data passed to the callbacks
> + * @priority: low priorities execute first when the vm runs and the reverse is
> + *            true when the vm stops
> + *
> + * Register a main callback function and an optional pre VM state change
> + * callback function that are invoked when the vm starts or stops running. The
> + * main callback and the pre VM state change callback are called in two
> + * separate phases: First all pre VM state change callbacks are called and only
> + * then all main callbacks are called.
> + *
> + * Returns: an entry to be freed using qemu_del_vm_change_state_handler()
> + */
> +VMChangeStateEntry *
> +qemu_add_vm_change_state_handler_prio_full(VMChangeStateHandler *cb,
> +                                           VMChangeStateHandler *pre_change_cb,
> +                                           void *opaque, int priority)
>   {
>       VMChangeStateEntry *e;
>       VMChangeStateEntry *other;
>   
>       e = g_malloc0(sizeof(*e));
>       e->cb = cb;
> +    e->pre_change_cb = pre_change_cb;
>       e->opaque = opaque;
>       e->priority = priority;
>   
> @@ -333,10 +360,22 @@ void vm_state_notify(bool running, RunState state)
>       trace_vm_state_notify(running, state, RunState_str(state));
>   
>       if (running) {
> +        QTAILQ_FOREACH_SAFE(e, &vm_change_state_head, entries, next) {
> +            if (e->pre_change_cb) {
> +                e->pre_change_cb(e->opaque, running, state);
> +            }
> +        }
> +
>           QTAILQ_FOREACH_SAFE(e, &vm_change_state_head, entries, next) {
>               e->cb(e->opaque, running, state);
>           }
>       } else {
> +        QTAILQ_FOREACH_REVERSE_SAFE(e, &vm_change_state_head, entries, next) {
> +            if (e->pre_change_cb) {
> +                e->pre_change_cb(e->opaque, running, state);
> +            }
> +        }
> +
>           QTAILQ_FOREACH_REVERSE_SAFE(e, &vm_change_state_head, entries, next) {
>               e->cb(e->opaque, running, state);
>           }