On Tue, Feb 27, 2024 at 07:03:32PM +0100, Cédric Le Goater wrote:
> Modify all log_global*() handlers to take an Error** parameter and
> return a bool. A new MEMORY_LISTENER_CALL_LOG_GLOBAL macro looping on
> the listeners is introduced to handle a possible error, which will
> would interrupt the loop if necessary.
>
> To be noted a change in memory_global_dirty_log_start() behavior as it
> will return as soon as an error is detected.
>
> Cc: Stefano Stabellini <sstabell...@kernel.org>
> Cc: Anthony Perard <anthony.per...@citrix.com>
> Cc: Paul Durrant <p...@xen.org>
> Cc: Michael S. Tsirkin <m...@redhat.com>
> Cc: Paolo Bonzini <pbonz...@redhat.com>
> Cc: David Hildenbrand <da...@redhat.com>
> Signed-off-by: Cédric Le Goater <c...@redhat.com>
> ---
> include/exec/memory.h | 15 ++++++--
> hw/i386/xen/xen-hvm.c | 6 ++--
> hw/vfio/common.c | 8 +++--
> hw/virtio/vhost.c | 6 ++--
> system/memory.c | 83 +++++++++++++++++++++++++++++++++++++------
> system/physmem.c | 5 +--
> 6 files changed, 101 insertions(+), 22 deletions(-)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index
> 8626a355b310ed7b1a1db7978ba4b394032c2f15..4bc146c5ebdd377cd14a4e462f32cc945db5a0a8
> 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -998,8 +998,11 @@ struct MemoryListener {
> * active at that time.
> *
> * @listener: The #MemoryListener.
> + * @errp: pointer to Error*, to store an error if it happens.
> + *
> + * Return: true on success, else false setting @errp with error.
> */
> - void (*log_global_start)(MemoryListener *listener);
> + bool (*log_global_start)(MemoryListener *listener, Error **errp);
>
> /**
> * @log_global_stop:
> @@ -1009,8 +1012,11 @@ struct MemoryListener {
> * the address space.
> *
> * @listener: The #MemoryListener.
> + * @errp: pointer to Error*, to store an error if it happens.
> + *
> + * Return: true on success, else false setting @errp with error.
> */
> - void (*log_global_stop)(MemoryListener *listener);
> + bool (*log_global_stop)(MemoryListener *listener, Error **errp);
>
> /**
> * @log_global_after_sync:
> @@ -1019,8 +1025,11 @@ struct MemoryListener {
> * for any #MemoryRegionSection.
> *
> * @listener: The #MemoryListener.
> + * @errp: pointer to Error*, to store an error if it happens.
> + *
> + * Return: true on success, else false setting @errp with error.
> */
> - void (*log_global_after_sync)(MemoryListener *listener);
> + bool (*log_global_after_sync)(MemoryListener *listener, Error **errp);
>
> /**
> * @eventfd_add:
> diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> index
> f42621e6742552035122ea58092c91c3458338ff..925a207b494b4eed52d5f360b554f18ac8a9806d
> 100644
> --- a/hw/i386/xen/xen-hvm.c
> +++ b/hw/i386/xen/xen-hvm.c
> @@ -446,16 +446,18 @@ static void xen_log_sync(MemoryListener *listener,
> MemoryRegionSection *section)
> int128_get64(section->size));
> }
>
> -static void xen_log_global_start(MemoryListener *listener)
> +static bool xen_log_global_start(MemoryListener *listener, Error **errp)
> {
> if (xen_enabled()) {
> xen_in_migration = true;
> }
> + return true;
> }
>
> -static void xen_log_global_stop(MemoryListener *listener)
> +static bool xen_log_global_stop(MemoryListener *listener, Error **errp)
> {
> xen_in_migration = false;
> + return true;
> }
>
> static const MemoryListener xen_memory_listener = {
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index
> 059bfdc07a85e2eb908df828c1f42104d683e911..8bba95ba6a2010b78cae54c6905857686bbb6309
> 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1075,7 +1075,8 @@ out:
> return ret;
> }
>
> -static void vfio_listener_log_global_start(MemoryListener *listener)
> +static bool vfio_listener_log_global_start(MemoryListener *listener,
> + Error **errp)
> {
> VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
> listener);
> @@ -1092,9 +1093,11 @@ static void
> vfio_listener_log_global_start(MemoryListener *listener)
> ret, strerror(-ret));
> vfio_set_migration_error(ret);
> }
> + return !!ret;
> }
>
> -static void vfio_listener_log_global_stop(MemoryListener *listener)
> +static bool vfio_listener_log_global_stop(MemoryListener *listener,
> + Error **errp)
> {
> VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
> listener);
> @@ -1111,6 +1114,7 @@ static void
> vfio_listener_log_global_stop(MemoryListener *listener)
> ret, strerror(-ret));
> vfio_set_migration_error(ret);
> }
> + return !!ret;
> }
Shouldn't both of these be "return !ret"?
Besides, maybe it'll be easier to go with either:
1) if to only add the new parameter, here we can constantly return true
to keep the old behavior, or,
2) allow failure within this patch, then error_report() should already do
error_setg() to errp.
>
> static int vfio_device_dma_logging_report(VFIODevice *vbasedev, hwaddr iova,
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index
> 2c9ac794680ea9b65eba6cc22e70cf141e90aa73..7a555f941934991a72a2817e5505fe0ce6d6fc64
> 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1044,7 +1044,7 @@ check_dev_state:
> return r;
> }
>
> -static void vhost_log_global_start(MemoryListener *listener)
> +static bool vhost_log_global_start(MemoryListener *listener, Error **errp)
> {
> int r;
>
> @@ -1052,9 +1052,10 @@ static void vhost_log_global_start(MemoryListener
> *listener)
> if (r < 0) {
> abort();
> }
> + return true;
> }
>
> -static void vhost_log_global_stop(MemoryListener *listener)
> +static bool vhost_log_global_stop(MemoryListener *listener, Error **errp)
> {
> int r;
>
> @@ -1062,6 +1063,7 @@ static void vhost_log_global_stop(MemoryListener
> *listener)
> if (r < 0) {
> abort();
> }
> + return true;
> }
>
> static void vhost_log_start(MemoryListener *listener,
> diff --git a/system/memory.c b/system/memory.c
> index
> a229a79988fce2aa3cb77e3a130db4c694e8cd49..af06157ead5b1272548e87f79ab9fb3036055328
> 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -127,6 +127,35 @@ enum ListenerDirection { Forward, Reverse };
> } \
> } while (0)
>
> +#define MEMORY_LISTENER_CALL_LOG_GLOBAL(_callback, _direction, _errp, \
> + _args...) \
> + do { \
> + MemoryListener *_listener; \
> + \
> + switch (_direction) { \
> + case Forward: \
> + QTAILQ_FOREACH(_listener, &memory_listeners, link) { \
> + if (_listener->_callback) { \
> + if (!_listener->_callback(_listener, _errp, ##_args)) { \
> + break; \
> + } \
> + } \
> + } \
> + break; \
> + case Reverse: \
> + QTAILQ_FOREACH_REVERSE(_listener, &memory_listeners, link) { \
> + if (_listener->_callback) { \
> + if (!_listener->_callback(_listener, _errp, ##_args)) { \
> + break; \
> + } \
> + } \
> + } \
> + break; \
> + default: \
> + abort(); \
> + }; \
> + } while (0)
> +
> #define MEMORY_LISTENER_CALL(_as, _callback, _direction, _section, _args...)
> \
> do { \
> MemoryListener *_listener; \
> @@ -2903,7 +2932,13 @@ void memory_global_dirty_log_sync(bool last_stage)
>
> void memory_global_after_dirty_log_sync(void)
> {
> - MEMORY_LISTENER_CALL_GLOBAL(log_global_after_sync, Forward);
> + Error *local_err = NULL;
> +
> + MEMORY_LISTENER_CALL_LOG_GLOBAL(log_global_after_sync, Forward,
> + &local_err);
> + if (local_err) {
> + error_report_err(local_err);
> + }
> }
>
> /*
> @@ -2912,18 +2947,22 @@ void memory_global_after_dirty_log_sync(void)
> */
> static unsigned int postponed_stop_flags;
> static VMChangeStateEntry *vmstate_change;
> -static void memory_global_dirty_log_stop_postponed_run(void);
> +static bool memory_global_dirty_log_stop_postponed_run(Error **errp);
>
> void memory_global_dirty_log_start(unsigned int flags)
> {
> unsigned int old_flags;
> + Error *local_err = NULL;
>
> assert(flags && !(flags & (~GLOBAL_DIRTY_MASK)));
>
> if (vmstate_change) {
> /* If there is postponed stop(), operate on it first */
> postponed_stop_flags &= ~flags;
> - memory_global_dirty_log_stop_postponed_run();
> + if (!memory_global_dirty_log_stop_postponed_run(&local_err)) {
> + error_report_err(local_err);
> + return;
> + }
> }
>
> flags &= ~global_dirty_tracking;
> @@ -2936,15 +2975,22 @@ 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);
> + MEMORY_LISTENER_CALL_LOG_GLOBAL(log_global_start, Forward,
> + &local_err);
> + if (local_err) {
> + error_report_err(local_err);
> + return;
> + }
> memory_region_transaction_begin();
> memory_region_update_pending = true;
> memory_region_transaction_commit();
> }
> }
>
> -static void memory_global_dirty_log_do_stop(unsigned int flags)
> +static bool memory_global_dirty_log_do_stop(unsigned int flags, Error **errp)
> {
> + ERRP_GUARD();
> +
> assert(flags && !(flags & (~GLOBAL_DIRTY_MASK)));
> assert((global_dirty_tracking & flags) == flags);
> global_dirty_tracking &= ~flags;
> @@ -2955,39 +3001,49 @@ static void memory_global_dirty_log_do_stop(unsigned
> int flags)
> memory_region_transaction_begin();
> memory_region_update_pending = true;
> memory_region_transaction_commit();
> - MEMORY_LISTENER_CALL_GLOBAL(log_global_stop, Reverse);
> + MEMORY_LISTENER_CALL_LOG_GLOBAL(log_global_stop, Reverse, errp);
> }
> + return !*errp;
> }
>
> /*
> * Execute the postponed dirty log stop operations if there is, then reset
> * everything (including the flags and the vmstate change hook).
> */
> -static void memory_global_dirty_log_stop_postponed_run(void)
> +static bool memory_global_dirty_log_stop_postponed_run(Error **errp)
> {
> + bool ret = true;
> +
> /* This must be called with the vmstate handler registered */
> assert(vmstate_change);
>
> /* Note: postponed_stop_flags can be cleared in log start routine */
> if (postponed_stop_flags) {
> - memory_global_dirty_log_do_stop(postponed_stop_flags);
> + ret = memory_global_dirty_log_do_stop(postponed_stop_flags, errp);
> postponed_stop_flags = 0;
> }
>
> qemu_del_vm_change_state_handler(vmstate_change);
> vmstate_change = NULL;
> + return ret;
> }
>
> static void memory_vm_change_state_handler(void *opaque, bool running,
> RunState state)
> {
> + Error *local_err = NULL;
> +
> if (running) {
> - memory_global_dirty_log_stop_postponed_run();
> + if (!memory_global_dirty_log_stop_postponed_run(&local_err)) {
> + error_report_err(local_err);
> + }
> }
> }
>
> void memory_global_dirty_log_stop(unsigned int flags)
> {
> + Error *local_err = NULL;
> +
> if (!runstate_is_running()) {
> /* Postpone the dirty log stop, e.g., to when VM starts again */
> if (vmstate_change) {
> @@ -3001,7 +3057,9 @@ void memory_global_dirty_log_stop(unsigned int flags)
> return;
> }
>
> - memory_global_dirty_log_do_stop(flags);
> + if (!memory_global_dirty_log_do_stop(flags, &local_err)) {
> + error_report_err(local_err);
> + }
> }
>
> static void listener_add_address_space(MemoryListener *listener,
> @@ -3009,13 +3067,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);
> + }
> }
> }
The stop process is indeed more involved due to the postponed mechanism.
Is it really a goal that we should also allow global stop() to be able to
fail? Or maybe we only care about global start() on the "VFIO doesn't have
enough dirty tracking unit for the current device" issue?
It makes more sense to me to allow a start() to fail rather than stop(),
because if start()s failed we can still fallback to stop() on whatever got
start()ed already, and fail the operation that triggered the start().
If stop() can also fail I don't know what we can do; consider if the
fallback stop()s failed above. It sounds just more challenging to allow
stop() to fail v.s. start() only from that regard to behave correctly.
Same question to after_sync(); that seems only to be used by tcg. If VFIO
has no plan to leverage that Error** maybe we can keep them as-is?
>
> diff --git a/system/physmem.c b/system/physmem.c
> index
> e3ebc19eefd8050a1dee16e3d1449f0c144f751f..9adbf9aea847cd80bdac6dca466fb476844ac048
> 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -148,7 +148,7 @@ typedef struct subpage_t {
>
> static void io_mem_init(void);
> static void memory_map_init(void);
> -static void tcg_log_global_after_sync(MemoryListener *listener);
> +static bool tcg_log_global_after_sync(MemoryListener *listener, Error
> **errp);
> static void tcg_commit(MemoryListener *listener);
>
> /**
> @@ -2475,7 +2475,7 @@ static void do_nothing(CPUState *cpu, run_on_cpu_data d)
> {
> }
>
> -static void tcg_log_global_after_sync(MemoryListener *listener)
> +static bool tcg_log_global_after_sync(MemoryListener *listener, Error **errp)
> {
> CPUAddressSpace *cpuas;
>
> @@ -2507,6 +2507,7 @@ static void tcg_log_global_after_sync(MemoryListener
> *listener)
> cpuas = container_of(listener, CPUAddressSpace, tcg_as_listener);
> run_on_cpu(cpuas->cpu, do_nothing, RUN_ON_CPU_NULL);
> }
> + return true;
> }
>
> static void tcg_commit_cpu(CPUState *cpu, run_on_cpu_data data)
> --
> 2.43.2
>
--
Peter Xu
