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 <david@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
---
Changes in v6:
- Add Reviewed-by from David.
Changes in v5:
- Nit in commit message (return false -> -EBUSY)
- Add set_ram_discard_manager(NULL) when ram_block_discard_range()
fails.
Changes in v3:
- Move set_ram_discard_manager() up to avoid a g_free()
- Clean up set_ram_discard_manager() definition
---
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,
--
2.43.5On 5/30/2025 4:32 PM, 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<david@redhat.com> > Reviewed-by: David Hildenbrand<david@redhat.com> > Signed-off-by: Chenyi Qiang<chenyi.qiang@intel.com> Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>
> 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 <david@redhat.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
Reviewed-by: Pankaj Gupta <pankaj.gupta@amd.com>
> ---
> Changes in v6:
> - Add Reviewed-by from David.
>
> Changes in v5:
> - Nit in commit message (return false -> -EBUSY)
> - Add set_ram_discard_manager(NULL) when ram_block_discard_range()
> fails.
>
> Changes in v3:
> - Move set_ram_discard_manager() up to avoid a g_free()
> - Clean up set_ram_discard_manager() definition
> ---
> 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,
© 2016 - 2025 Red Hat, Inc.