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 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
