On Wed, Mar 06, 2024 at 02:34:28PM +0100, Cédric Le Goater wrote:
> diff --git a/system/memory.c b/system/memory.c
> index
> a229a79988fce2aa3cb77e3a130db4c694e8cd49..3600e716149407c10a1f6bf8f0a81c2611cf15ba
> 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -2914,9 +2914,27 @@ static unsigned int postponed_stop_flags;
> static VMChangeStateEntry *vmstate_change;
> static void memory_global_dirty_log_stop_postponed_run(void);
>
> +/*
> + * Stop dirty logging on all listeners where it was previously enabled.
> + */
> +static void memory_global_dirty_log_rollback(MemoryListener *listener,
> + unsigned int flags)
> +{
> + global_dirty_tracking &= ~flags;
Having a hook rollback function to touch the global_dirty_tracking flag is
IMHO tricky.
Can we instead provide a helper to call all log_global_start() hooks, but
allow a gracefully fail (so rollback will be called if it fails)?
bool memory_global_dirty_log_start_hooks(...)
Or any better names.. Leaving global_dirty_tracking rollback to
memory_global_dirty_log_start() when it returns false.
Would this be cleaner?
> + trace_global_dirty_changed(global_dirty_tracking);
> +
> + while (listener) {
> + if (listener->log_global_stop) {
> + listener->log_global_stop(listener);
> + }
> + listener = QTAILQ_PREV(listener, link);
> + }
> +}
> +
> void memory_global_dirty_log_start(unsigned int flags)
> {
> unsigned int old_flags;
> + Error *local_err = NULL;
>
> assert(flags && !(flags & (~GLOBAL_DIRTY_MASK)));
>
> @@ -2936,7 +2954,25 @@ void memory_global_dirty_log_start(unsigned int flags)
> trace_global_dirty_changed(global_dirty_tracking);
>
> if (!old_flags) {
> - MEMORY_LISTENER_CALL_GLOBAL(log_global_start, Forward);
> + MemoryListener *listener;
> + bool ret = true;
> +
> + QTAILQ_FOREACH(listener, &memory_listeners, link) {
> + if (listener->log_global_start) {
> + ret = listener->log_global_start(listener, &local_err);
> + if (!ret) {
> + break;
> + }
> + }
> + }
> +
> + if (!ret) {
> + memory_global_dirty_log_rollback(QTAILQ_PREV(listener, link),
> + flags);
> + error_report_err(local_err);
> + return;
> + }
> +
> memory_region_transaction_begin();
> memory_region_update_pending = true;
> memory_region_transaction_commit();
> @@ -3009,13 +3045,16 @@ static void listener_add_address_space(MemoryListener
> *listener,
> {
> FlatView *view;
> FlatRange *fr;
> + Error *local_err = NULL;
>
> if (listener->begin) {
> listener->begin(listener);
> }
> if (global_dirty_tracking) {
> if (listener->log_global_start) {
> - listener->log_global_start(listener);
> + if (!listener->log_global_start(listener, &local_err)) {
> + error_report_err(local_err);
> + }
IMHO we should assert here instead of error report. We have this to guard
hot-plug during migration so I think the assert is justified:
qdev_device_add_from_qdict():
if (!migration_is_idle()) {
error_setg(errp, "device_add not allowed while migrating");
return NULL;
}
If it really happens it's a bug, as listener_add_address_space() will still
keep the rest things around even if the hook failed. It'll start to be a
total mess..
Thanks,
> }
> }
>
> --
> 2.44.0
>
--
Peter Xu
