| Message ID | 20250407074939.18657-3-chenyi.qiang@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enable shared device assignment | expand |
On 4/7/2025 3:49 PM, Chenyi Qiang wrote: > Modify memory_region_set_ram_discard_manager() to return false if a > RamDiscardManager is already set in the MemoryRegion. It doesn't return false, but -EBUSY. > 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 <david@redhat.com> > Signed-off-by: Chenyi Qiang <chenyi.qiang@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 > @@ -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; > } > > uint64_t ram_discard_manager_get_min_granularity(const RamDiscardManager *rdm,
On 4/7/2025 5:53 PM, Xiaoyao Li wrote: > On 4/7/2025 3:49 PM, Chenyi Qiang wrote: >> Modify memory_region_set_ram_discard_manager() to return false if a >> RamDiscardManager is already set in the MemoryRegion. > > It doesn't return false, but -EBUSY. Nice catch! Forgot to modify this commit message. > >> 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 <david@redhat.com> >> Signed-off-by: Chenyi Qiang <chenyi.qiang@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 >> @@ -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; >> } >> uint64_t ram_discard_manager_get_min_granularity(const >> RamDiscardManager *rdm, >
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 @@ -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; } uint64_t ram_discard_manager_get_min_granularity(const RamDiscardManager *rdm,
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 <david@redhat.com> Signed-off-by: Chenyi Qiang <chenyi.qiang@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(-)