Re: [PATCH v4 13/25] memory: Add Error** argument to .log_global_start() handler

[Cédric Le Goater]

On 3/15/24 12:18, Peter Xu wrote:

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?

I will introduce a memory_global_dirty_log_do_start() helper to call
the log_global_start() handlers and to do the rollback in case of
error. Modification of the global_dirty_tracking flag will stay local
to memory_global_dirty_log_start() to avoid any futur errors.

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

OK. I will change the Error parameter to error_abort in that case.

However, It would be useful to catch errors of the .region_add() handler
for VFIO. Let's address that later.

Thanks,

C.