| Message ID | 20250217081833.21568-3-chenyi.qiang@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enable shared device assignment | expand |
On 17/2/25 19:18, 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 <david@redhat.com> > Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com> > --- > Changes in v2: > - newly added. > --- > hw/virtio/virtio-mem.c | 30 +++++++++++++++++------------- > include/exec/memory.h | 6 +++--- > system/memory.c | 11 ++++++++--- > 3 files changed, 28 insertions(+), 19 deletions(-) > > diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c > index 21f16e4912..ef818a2cdf 100644 > --- a/hw/virtio/virtio-mem.c > +++ b/hw/virtio/virtio-mem.c > @@ -1074,6 +1074,18 @@ static void virtio_mem_device_realize(DeviceState *dev, Error **errp) > vmem->block_size; > vmem->bitmap = bitmap_new(vmem->bitmap_size); > > + /* > + * 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"); > + g_free(vmem->bitmap); > + ram_block_coordinated_discard_require(false); > + return; > + } Looks like this can move before vmem->bitmap is allocated (or even before ram_block_coordinated_discard_require(true)?). Then you can drop g_free() and avoid having a stale pointer in vmem->bitmap (not that it matters here though). > + > virtio_init(vdev, VIRTIO_ID_MEM, sizeof(struct virtio_mem_config)); > vmem->vq = virtio_add_queue(vdev, 128, virtio_mem_handle_request); > vmem->bitmap > @@ -1124,13 +1136,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 +1143,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)); > > @@ -1155,6 +1154,11 @@ static void virtio_mem_device_unrealize(DeviceState *dev) > host_memory_backend_set_mapped(vmem->memdev, false); > virtio_del_queue(vdev, 0); > virtio_cleanup(vdev); > + /* > + * 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); > g_free(vmem->bitmap); > 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..297a3dbcd4 100644 > --- a/system/memory.c > +++ b/system/memory.c > @@ -2115,12 +2115,17 @@ 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 != NULL) { Drop "!= NULL". > + return -1; -EBUSY? > + } > + > + /* !rdm || !mr->rdm */ See, like here - no "!= NULL" :) (and the comment is useless). Thanks, > mr->rdm = rdm; > + return 0; > } > > uint64_t ram_discard_manager_get_min_granularity(const RamDiscardManager *rdm,
On 2/18/2025 5:19 PM, Alexey Kardashevskiy wrote: > > > On 17/2/25 19:18, 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 <david@redhat.com> >> Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com> >> --- >> Changes in v2: >> - newly added. >> --- >> hw/virtio/virtio-mem.c | 30 +++++++++++++++++------------- >> include/exec/memory.h | 6 +++--- >> system/memory.c | 11 ++++++++--- >> 3 files changed, 28 insertions(+), 19 deletions(-) >> >> diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c >> index 21f16e4912..ef818a2cdf 100644 >> --- a/hw/virtio/virtio-mem.c >> +++ b/hw/virtio/virtio-mem.c >> @@ -1074,6 +1074,18 @@ static void >> virtio_mem_device_realize(DeviceState *dev, Error **errp) >> vmem->block_size; >> vmem->bitmap = bitmap_new(vmem->bitmap_size); >> + /* >> + * 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"); >> + g_free(vmem->bitmap); >> + ram_block_coordinated_discard_require(false); >> + return; >> + } > > Looks like this can move before vmem->bitmap is allocated (or even > before ram_block_coordinated_discard_require(true)?). Then you can drop > g_free() and avoid having a stale pointer in vmem->bitmap (not that it > matters here though). I'm not sure if moving up the memory_region_set_ram_discard_manager() will have bring any side effect (seems no requirement for the operation order here). But Maybe it's better to put after ram_block_coordinated_discard_require(true) as ram_block_coordinated_discard_require(true) unlocks RDM users. > >> + >> virtio_init(vdev, VIRTIO_ID_MEM, sizeof(struct virtio_mem_config)); >> vmem->vq = virtio_add_queue(vdev, 128, virtio_mem_handle_request); >> vmem->bitmap >> @@ -1124,13 +1136,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 +1143,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)); >> @@ -1155,6 +1154,11 @@ static void >> virtio_mem_device_unrealize(DeviceState *dev) >> host_memory_backend_set_mapped(vmem->memdev, false); >> virtio_del_queue(vdev, 0); >> virtio_cleanup(vdev); >> + /* >> + * 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); >> g_free(vmem->bitmap); >> 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..297a3dbcd4 100644 >> --- a/system/memory.c >> +++ b/system/memory.c >> @@ -2115,12 +2115,17 @@ 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 != NULL) { > > Drop "!= NULL". > >> + return -1; > > -EBUSY? [..] > >> + } >> + >> + /* !rdm || !mr->rdm */ > > See, like here - no "!= NULL" :) (and the comment is useless). Thanks, LGTM, will change it. Thanks! > > >> mr->rdm = rdm; >> + return 0; >> } >> uint64_t ram_discard_manager_get_min_granularity(const >> RamDiscardManager *rdm, >
On 18.02.25 10:19, Alexey Kardashevskiy wrote: > > > On 17/2/25 19:18, 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 <david@redhat.com> >> Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com> >> --- >> Changes in v2: >> - newly added. >> --- >> hw/virtio/virtio-mem.c | 30 +++++++++++++++++------------- >> include/exec/memory.h | 6 +++--- >> system/memory.c | 11 ++++++++--- >> 3 files changed, 28 insertions(+), 19 deletions(-) >> >> diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c >> index 21f16e4912..ef818a2cdf 100644 >> --- a/hw/virtio/virtio-mem.c >> +++ b/hw/virtio/virtio-mem.c >> @@ -1074,6 +1074,18 @@ static void virtio_mem_device_realize(DeviceState *dev, Error **errp) >> vmem->block_size; >> vmem->bitmap = bitmap_new(vmem->bitmap_size); >> >> + /* >> + * 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"); >> + g_free(vmem->bitmap); >> + ram_block_coordinated_discard_require(false); >> + return; >> + } > > Looks like this can move before vmem->bitmap is allocated (or even > before ram_block_coordinated_discard_require(true)?). Then you can drop > g_free() and avoid having a stale pointer in vmem->bitmap (not that it > matters here though). > >> + >> virtio_init(vdev, VIRTIO_ID_MEM, sizeof(struct virtio_mem_config)); >> vmem->vq = virtio_add_queue(vdev, 128, virtio_mem_handle_request); >> vmem->bitmap >> @@ -1124,13 +1136,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 +1143,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)); >> >> @@ -1155,6 +1154,11 @@ static void virtio_mem_device_unrealize(DeviceState *dev) >> host_memory_backend_set_mapped(vmem->memdev, false); >> virtio_del_queue(vdev, 0); >> virtio_cleanup(vdev); >> + /* >> + * 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); >> g_free(vmem->bitmap); >> 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..297a3dbcd4 100644 >> --- a/system/memory.c >> +++ b/system/memory.c >> @@ -2115,12 +2115,17 @@ 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 != NULL) { > > Drop "!= NULL". > >> + return -1; > > -EBUSY? > >> + } >> + >> + /* !rdm || !mr->rdm */ > > See, like here - no "!= NULL" :) (and the comment is useless). Thanks, > > >> mr->rdm = rdm; >> + return 0; >> } >> >> uint64_t ram_discard_manager_get_min_granularity(const RamDiscardManager *rdm, > Agreed to all, with that it LGTM, thanks!
diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c index 21f16e4912..ef818a2cdf 100644 --- a/hw/virtio/virtio-mem.c +++ b/hw/virtio/virtio-mem.c @@ -1074,6 +1074,18 @@ static void virtio_mem_device_realize(DeviceState *dev, Error **errp) vmem->block_size; vmem->bitmap = bitmap_new(vmem->bitmap_size); + /* + * 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"); + g_free(vmem->bitmap); + ram_block_coordinated_discard_require(false); + return; + } + virtio_init(vdev, VIRTIO_ID_MEM, sizeof(struct virtio_mem_config)); vmem->vq = virtio_add_queue(vdev, 128, virtio_mem_handle_request); @@ -1124,13 +1136,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 +1143,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)); @@ -1155,6 +1154,11 @@ static void virtio_mem_device_unrealize(DeviceState *dev) host_memory_backend_set_mapped(vmem->memdev, false); virtio_del_queue(vdev, 0); virtio_cleanup(vdev); + /* + * 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); g_free(vmem->bitmap); 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..297a3dbcd4 100644 --- a/system/memory.c +++ b/system/memory.c @@ -2115,12 +2115,17 @@ 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 != NULL) { + return -1; + } + + /* !rdm || !mr->rdm */ 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 v2: - newly added. --- hw/virtio/virtio-mem.c | 30 +++++++++++++++++------------- include/exec/memory.h | 6 +++--- system/memory.c | 11 ++++++++--- 3 files changed, 28 insertions(+), 19 deletions(-)