On 4/9/2025 1:35 PM, Alexey Kardashevskiy wrote:
>
>
> On 7/4/25 17:49, Chenyi Qiang wrote:
>> Modify memory_region_set_ram_discard_manager() to return false 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
>> a MemoryRegion at present.
>>
>> Suggested-by: David Hildenbrand <da...@redhat.com>
>> Signed-off-by: Chenyi Qiang <chenyi.qi...@intel.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 | 29 ++++++++++++++++-------------
>> include/exec/memory.h | 6 +++---
>> system/memory.c | 10 +++++++---
>> 3 files changed, 26 insertions(+), 19 deletions(-)
>>
>> diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
>> index 21f16e4912..d0d3a0240f 100644
>> --- a/hw/virtio/virtio-mem.c
>> +++ b/hw/virtio/virtio-mem.c
>> @@ -1049,6 +1049,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
>
> Right after the end of this comment block, do not you want
> memory_region_set_generic_state_manager(..., NULL)?
Nice catch! Miss to add memory_region_set_generic_state_manager(NULL)
for the ram_block_discard_range() failure case.
>
>
>> @@ -1124,13 +1135,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)
>> @@ -1138,12 +1142,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));
>> @@ -1156,6 +1154,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/exec/memory.h b/include/exec/memory.h
>> index 3bebc43d59..390477b588 100644
>> --- a/include/exec/memory.h
>> +++ b/include/exec/memory.h
>> @@ -2487,13 +2487,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 b17b5538ff..62d6b410f0 100644
>> --- a/system/memory.c
>> +++ b/system/memory.c
>> @@ -2115,12 +2115,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;
>
> This is a change which can potentially break something, or currently
> there is no way to trigger -EBUSY? Thanks,
Before this series, virtio-mem is the only user to
set_ram_discard_manager(), there's no way to trigger -EBUSY. With this
series, guest_memfd-backed RAMBlocks become the second user. It can be
triggered if we try to use virtio-mem in confidential VMs.
>
>
>> }
>> uint64_t ram_discard_manager_get_min_granularity(const
>> RamDiscardManager *rdm,
>
