Re: [PATCH v5 02/10] memory: Change memory_region_set_ram_discard_manager() to return the result

[Alexey Kardashevskiy]

On 20/5/25 20:28, Chenyi Qiang wrote:

Modify memory_region_set_ram_discard_manager() to return -EBUSY if a
RamDiscardManager is already set in the MemoryRegion. The caller must
handle this failure, such as having virtio-mem undo its actions and fail
the realize() process. Opportunistically move the call earlier to avoid
complex error handling.

This change is beneficial when introducing a new RamDiscardManager
instance besides virtio-mem. After
ram_block_coordinated_discard_require(true) unlocks all
RamDiscardManager instances, only one instance is allowed to be set for
one MemoryRegion at present.

Suggested-by: David Hildenbrand <da...@redhat.com>
Signed-off-by: Chenyi Qiang <chenyi.qi...@intel.com>
---
Changes in v5:
     - Nit in commit message (return false -> -EBUSY)
     - Add set_ram_discard_manager(NULL) when ram_block_discard_range()
       fails.

Reviewed-by: Alexey Kardashevskiy <a...@amd.com>

Changes in v4:
     - No change.

Changes in v3:
     - Move set_ram_discard_manager() up to avoid a g_free()
     - Clean up set_ram_discard_manager() definition

Changes in v2:
     - newly added.
---
  hw/virtio/virtio-mem.c  | 30 +++++++++++++++++-------------
  include/system/memory.h |  6 +++---
  system/memory.c         | 10 +++++++---
  3 files changed, 27 insertions(+), 19 deletions(-)

diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index b3c126ea1e..2e491e8c44 100644
--- a/hw/virtio/virtio-mem.c
+++ b/hw/virtio/virtio-mem.c
@@ -1047,6 +1047,17 @@ static void virtio_mem_device_realize(DeviceState *dev, 
Error **errp)
          return;
      }

+ /*

+     * Set ourselves as RamDiscardManager before the plug handler maps the
+     * memory region and exposes it via an address space.
+     */
+    if (memory_region_set_ram_discard_manager(&vmem->memdev->mr,
+                                              RAM_DISCARD_MANAGER(vmem))) {
+        error_setg(errp, "Failed to set RamDiscardManager");
+        ram_block_coordinated_discard_require(false);
+        return;
+    }
+
      /*
       * We don't know at this point whether shared RAM is migrated using
       * QEMU or migrated using the file content. "x-ignore-shared" will be
@@ -1061,6 +1072,7 @@ static void virtio_mem_device_realize(DeviceState *dev, 
Error **errp)
          ret = ram_block_discard_range(rb, 0, qemu_ram_get_used_length(rb));
          if (ret) {
              error_setg_errno(errp, -ret, "Unexpected error discarding RAM");
+            memory_region_set_ram_discard_manager(&vmem->memdev->mr, NULL);
              ram_block_coordinated_discard_require(false);
              return;
          }
@@ -1122,13 +1134,6 @@ static void virtio_mem_device_realize(DeviceState *dev, 
Error **errp)
      vmem->system_reset = VIRTIO_MEM_SYSTEM_RESET(obj);
      vmem->system_reset->vmem = vmem;
      qemu_register_resettable(obj);
-
-    /*
-     * Set ourselves as RamDiscardManager before the plug handler maps the
-     * memory region and exposes it via an address space.
-     */
-    memory_region_set_ram_discard_manager(&vmem->memdev->mr,
-                                          RAM_DISCARD_MANAGER(vmem));
  }

static void virtio_mem_device_unrealize(DeviceState *dev)

@@ -1136,12 +1141,6 @@ static void virtio_mem_device_unrealize(DeviceState *dev)
      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
      VirtIOMEM *vmem = VIRTIO_MEM(dev);

- /*

-     * The unplug handler unmapped the memory region, it cannot be
-     * found via an address space anymore. Unset ourselves.
-     */
-    memory_region_set_ram_discard_manager(&vmem->memdev->mr, NULL);
-
      qemu_unregister_resettable(OBJECT(vmem->system_reset));
      object_unref(OBJECT(vmem->system_reset));

@@ -1154,6 +1153,11 @@ static void virtio_mem_device_unrealize(DeviceState *dev)

      virtio_del_queue(vdev, 0);
      virtio_cleanup(vdev);
      g_free(vmem->bitmap);
+    /*
+     * The unplug handler unmapped the memory region, it cannot be
+     * found via an address space anymore. Unset ourselves.
+     */
+    memory_region_set_ram_discard_manager(&vmem->memdev->mr, NULL);
      ram_block_coordinated_discard_require(false);
  }

diff --git a/include/system/memory.h b/include/system/memory.h

index b961c4076a..896948deb1 100644
--- a/include/system/memory.h
+++ b/include/system/memory.h
@@ -2499,13 +2499,13 @@ static inline bool 
memory_region_has_ram_discard_manager(MemoryRegion *mr)
   *
   * This function must not be called for a mapped #MemoryRegion, a 
#MemoryRegion
   * that does not cover RAM, or a #MemoryRegion that already has a
- * #RamDiscardManager assigned.
+ * #RamDiscardManager assigned. Return 0 if the rdm is set successfully.
   *
   * @mr: the #MemoryRegion
   * @rdm: #RamDiscardManager to set
   */
-void memory_region_set_ram_discard_manager(MemoryRegion *mr,
-                                           RamDiscardManager *rdm);
+int memory_region_set_ram_discard_manager(MemoryRegion *mr,
+                                          RamDiscardManager *rdm);

/**

   * memory_region_find: translate an address/size relative to a
diff --git a/system/memory.c b/system/memory.c
index 63b983efcd..b45b508dce 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -2106,12 +2106,16 @@ RamDiscardManager 
*memory_region_get_ram_discard_manager(MemoryRegion *mr)
      return mr->rdm;
  }

-void memory_region_set_ram_discard_manager(MemoryRegion *mr,

-                                           RamDiscardManager *rdm)
+int memory_region_set_ram_discard_manager(MemoryRegion *mr,
+                                          RamDiscardManager *rdm)
  {
      g_assert(memory_region_is_ram(mr));
-    g_assert(!rdm || !mr->rdm);
+    if (mr->rdm && rdm) {
+        return -EBUSY;
+    }
+
      mr->rdm = rdm;
+    return 0;
  }

uint64_t ram_discard_manager_get_min_granularity(const RamDiscardManager *rdm,

--
Alexey