| Message ID | 20230716081541.27900-3-avihaih@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | vfio/migration: Add P2P support for VFIO migration | expand |
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); > }
On 27/07/2023 19:23, Cédric Le Goater wrote: > External email: Use caution opening links or attachments > > > On 7/16/23 10:15, Avihai Horon wrote: >> Add pre VM state change callback to struct VMChangeStateEntry. > > This sentence could be the subject. Sure. > >> >> 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' Sure, I will change it. > > >> 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. I followed the GLib naming convention. For example, g_tree_new and g_tree_new_full, or g_hash_table_new and g_hash_table_new_full. I tend to go with GLib naming as it might be more familiar to people. What do you think? Thanks! > > 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); >> } >
[ ... ] >>> + * 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. > > I followed the GLib naming convention. > For example, g_tree_new and g_tree_new_full, or g_hash_table_new and g_hash_table_new_full. > I tend to go with GLib naming as it might be more familiar to people. > What do you think? Let's keep it then. Thanks, C.
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; 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: + * @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); }
Add pre VM state change callback to struct VMChangeStateEntry. 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(+)