On 3/13/2026 2:50 AM, BALATON Zoltan wrote:
On Thu, 12 Mar 2026, Peter Xu wrote:
On Thu, Mar 12, 2026 at 07:04:06PM +0100, BALATON Zoltan wrote:
On Thu, 12 Mar 2026, Peter Xu wrote:
On Thu, Mar 12, 2026 at 02:34:20PM +0800, Xiaoyao Li wrote:
Commit 2fb627ef2f48 ("memory: Factor out common ram region
initialization")
introduced a helper function memory_region_set_ram_block(), which
causes
mr->ram to be set to true after the RAM Block allocation by
qemu_ram_alloc_*().
It leads to the assertion
g_assert(memory_region_is_ram(mr));
in memory_region_set_ram_discard_manager() being triggered when
creating
RAM Block with the RAM_GUEST_MEMFD flag.
Fix this by restoring the original behavior of setting mr->ram before
RAM Block allocation.
Closes: https://gitlab.com/qemu-project/qemu/-/work_items/3330
Reported-by: Farrah Chen <[email protected]>
Fixes: 2fb627ef2f48 ("memory: Factor out common ram region
initialization")
Signed-off-by: Xiaoyao Li <[email protected]>
Thanks for the report. This is fast..
Almost agreed with the fix, except that it duplicates the lines all
over
the places. Would it be better to introduce memory_region_init_ram()?
First sorry for breaking this, I checked that qemu_alloc_ram did not
refer
to these fields but missed this use much deeper in the call stack. The
memory_region_init_ram name is already taken so can't call it like that.
Since all the different memory_region_init variants call different
qemu_ram_alloc variants there does not seem to be a way to set this
without
addining an additional line. Previous versions of the series had
memory_region_setup_ram() that set these mr fields then
qemu_alloc_ram and
then error_propagate was called last. We could bring back
memory_region_setup_ram but it's the same additional line everywhere
so this
seems to be a simpler fix. I don't see a way to avoid this
duplication other
than maybe changing qemu_ram_alloc to put it somehow within that but
that
does not seem to be simple. I can try to think about it some more but
so far
I could not find a simpler fix.
We can call it memory_region_init_ram_internal(), or something else.
The point is to avoid duplicating the same line all over again.. with
these
call sites s/memory_region_init/$NEW_NAME/ where $NEW_NAME set ram=true.
The point of refactoring was to make memory_region_init on it's own line
so it can be replaced with memory_region_new in the follow up series.
You can introduce a new function that does init+setup some fields and do
the same for new+setup some fields but not sure that's worth it.
I don't think it would save much lines in the end. This patch adds 6
lines. Your proposed function would take at least 5 (or 6 with empty
line after)
and bring back the mr->ram=false in the romd case
For romd case, it's not affected since it uses memory_region_init_io().
However, the ram device is affected that is also uses
memory_region_init_io() which will not be covered by the helper.
Diff attached below. I doubt it is better/simpler than my original fix
because memory_region_init_ram_device_ptr() still requires the separate
mr->ram = true;
diff --git a/system/memory.c b/system/memory.c
index 17a7bcd9af7c..1878d4aa843a 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1576,9 +1576,15 @@ void memory_region_init_io(MemoryRegion *mr,
Object *owner,
memory_region_set_ops(mr, ops, opaque);
}
+static void memory_region_init_ram_internal(MemoryRegion *mr, Object
*owner,
+ const char *name, uint64_t
size)
+{
+ memory_region_init(mr, owner, name, size);
+ mr->ram = true;
+}
+
static bool memory_region_set_ram_block(MemoryRegion *mr, RAMBlock *rb)
{
- mr->ram = true;
mr->terminates = true;
mr->destructor = memory_region_destructor_ram;
mr->ram_block = rb;
@@ -1596,7 +1602,7 @@ bool
memory_region_init_ram_flags_nomigrate(MemoryRegion *mr, Object *owner,
{
RAMBlock *rb;
- memory_region_init(mr, owner, name, size);
+ memory_region_init_ram_internal(mr, owner, name, size);
rb = qemu_ram_alloc(size, ram_flags, mr, errp);
return memory_region_set_ram_block(mr, rb);
}
@@ -1613,7 +1619,7 @@ bool
memory_region_init_resizeable_ram(MemoryRegion *mr,
{
RAMBlock *rb;
- memory_region_init(mr, owner, name, size);
+ memory_region_init_ram_internal(mr, owner, name, size);
rb = qemu_ram_alloc_resizeable(size, max_size, resized, mr, errp);
return memory_region_set_ram_block(mr, rb);
}
@@ -1627,7 +1633,7 @@ bool memory_region_init_ram_from_file(MemoryRegion
*mr, Object *owner,
{
RAMBlock *rb;
- memory_region_init(mr, owner, name, size);
+ memory_region_init_ram_internal(mr, owner, name, size);
mr->readonly = !!(ram_flags & RAM_READONLY);
mr->align = align;
rb = qemu_ram_alloc_from_file(size, mr, ram_flags, path, offset,
errp);
@@ -1641,7 +1647,7 @@ bool memory_region_init_ram_from_fd(MemoryRegion
*mr, Object *owner,
{
RAMBlock *rb;
- memory_region_init(mr, owner, name, size);
+ memory_region_init_ram_internal(mr, owner, name, size);
mr->readonly = !!(ram_flags & RAM_READONLY);
rb = qemu_ram_alloc_from_fd(size, size, NULL, mr, ram_flags, fd,
offset,
false, errp);
@@ -1662,7 +1668,7 @@ void memory_region_init_ram_ptr(MemoryRegion *mr,
Object *owner,
const char *name, uint64_t size,
void *ptr)
{
- memory_region_init(mr, owner, name, size);
+ memory_region_init_ram_internal(mr, owner, name, size);
memory_region_set_ram_ptr(mr, size, ptr);
}
@@ -1671,6 +1677,7 @@ void
memory_region_init_ram_device_ptr(MemoryRegion *mr, Object *owner,
void *ptr)
{
memory_region_init_io(mr, owner, &ram_device_mem_ops, mr, name, size);
+ mr->ram = true;
mr->ram_device = true;
memory_region_set_ram_ptr(mr, size, ptr);
}
@@ -3699,7 +3706,6 @@ bool memory_region_init_rom_device(MemoryRegion
*mr, Object *owner,
memory_region_init_io(mr, owner, ops, opaque, name, size);
rb = qemu_ram_alloc(size, 0, mr, errp);
if (memory_region_set_ram_block(mr, rb)) {
- mr->ram = false;
mr->rom_device = true;
memory_region_register_ram(mr, owner);
return true;
so at the
end it's about 1 line difference and may even be longer that way. I
don't like the duplication either but the alternatives don't seem to be
less lines or simpler either.
But at the end it's your decision so do what you think is better.
Regards,
BALATON Zoltan
