system/memory.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
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 <farrah.chen@intel.com>
Fixes: 2fb627ef2f48 ("memory: Factor out common ram region initialization")
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
system/memory.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/system/memory.c b/system/memory.c
index 17a7bcd9af7c..56f3225b21ad 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1578,7 +1578,6 @@ void memory_region_init_io(MemoryRegion *mr, Object *owner,
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;
@@ -1597,6 +1596,7 @@ bool memory_region_init_ram_flags_nomigrate(MemoryRegion *mr, Object *owner,
RAMBlock *rb;
memory_region_init(mr, owner, name, size);
+ mr->ram = true;
rb = qemu_ram_alloc(size, ram_flags, mr, errp);
return memory_region_set_ram_block(mr, rb);
}
@@ -1614,6 +1614,7 @@ bool memory_region_init_resizeable_ram(MemoryRegion *mr,
RAMBlock *rb;
memory_region_init(mr, owner, name, size);
+ mr->ram = true;
rb = qemu_ram_alloc_resizeable(size, max_size, resized, mr, errp);
return memory_region_set_ram_block(mr, rb);
}
@@ -1628,6 +1629,7 @@ bool memory_region_init_ram_from_file(MemoryRegion *mr, Object *owner,
RAMBlock *rb;
memory_region_init(mr, owner, name, size);
+ mr->ram = true;
mr->readonly = !!(ram_flags & RAM_READONLY);
mr->align = align;
rb = qemu_ram_alloc_from_file(size, mr, ram_flags, path, offset, errp);
@@ -1642,6 +1644,7 @@ bool memory_region_init_ram_from_fd(MemoryRegion *mr, Object *owner,
RAMBlock *rb;
memory_region_init(mr, owner, name, size);
+ mr->ram = true;
mr->readonly = !!(ram_flags & RAM_READONLY);
rb = qemu_ram_alloc_from_fd(size, size, NULL, mr, ram_flags, fd, offset,
false, errp);
@@ -1663,6 +1666,7 @@ void memory_region_init_ram_ptr(MemoryRegion *mr, Object *owner,
void *ptr)
{
memory_region_init(mr, owner, name, size);
+ mr->ram = true;
memory_region_set_ram_ptr(mr, size, ptr);
}
@@ -1671,6 +1675,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 +3704,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;
--
2.43.0On 3/12/26 1:34 AM, 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 <farrah.chen@intel.com>
> Fixes: 2fb627ef2f48 ("memory: Factor out common ram region initialization")
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
Tested-by: Kim Phillips <kim.phillips@amd.com>
Thanks,
Kim
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 <farrah.chen@intel.com>
> Fixes: 2fb627ef2f48 ("memory: Factor out common ram region initialization")
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
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()?
> ---
> system/memory.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/system/memory.c b/system/memory.c
> index 17a7bcd9af7c..56f3225b21ad 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -1578,7 +1578,6 @@ void memory_region_init_io(MemoryRegion *mr, Object *owner,
>
> 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;
> @@ -1597,6 +1596,7 @@ bool memory_region_init_ram_flags_nomigrate(MemoryRegion *mr, Object *owner,
> RAMBlock *rb;
>
> memory_region_init(mr, owner, name, size);
> + mr->ram = true;
> rb = qemu_ram_alloc(size, ram_flags, mr, errp);
> return memory_region_set_ram_block(mr, rb);
> }
> @@ -1614,6 +1614,7 @@ bool memory_region_init_resizeable_ram(MemoryRegion *mr,
> RAMBlock *rb;
>
> memory_region_init(mr, owner, name, size);
> + mr->ram = true;
> rb = qemu_ram_alloc_resizeable(size, max_size, resized, mr, errp);
> return memory_region_set_ram_block(mr, rb);
> }
> @@ -1628,6 +1629,7 @@ bool memory_region_init_ram_from_file(MemoryRegion *mr, Object *owner,
> RAMBlock *rb;
>
> memory_region_init(mr, owner, name, size);
> + mr->ram = true;
> mr->readonly = !!(ram_flags & RAM_READONLY);
> mr->align = align;
> rb = qemu_ram_alloc_from_file(size, mr, ram_flags, path, offset, errp);
> @@ -1642,6 +1644,7 @@ bool memory_region_init_ram_from_fd(MemoryRegion *mr, Object *owner,
> RAMBlock *rb;
>
> memory_region_init(mr, owner, name, size);
> + mr->ram = true;
> mr->readonly = !!(ram_flags & RAM_READONLY);
> rb = qemu_ram_alloc_from_fd(size, size, NULL, mr, ram_flags, fd, offset,
> false, errp);
> @@ -1663,6 +1666,7 @@ void memory_region_init_ram_ptr(MemoryRegion *mr, Object *owner,
> void *ptr)
> {
> memory_region_init(mr, owner, name, size);
> + mr->ram = true;
> memory_region_set_ram_ptr(mr, size, ptr);
> }
>
> @@ -1671,6 +1675,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 +3704,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;
> --
> 2.43.0
>
--
Peter Xu
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 <farrah.chen@intel.com>
>> Fixes: 2fb627ef2f48 ("memory: Factor out common ram region initialization")
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>
> 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.
Regards,
BALATON Zoltan
>> ---
>> system/memory.c | 8 ++++++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/system/memory.c b/system/memory.c
>> index 17a7bcd9af7c..56f3225b21ad 100644
>> --- a/system/memory.c
>> +++ b/system/memory.c
>> @@ -1578,7 +1578,6 @@ void memory_region_init_io(MemoryRegion *mr, Object *owner,
>>
>> 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;
>> @@ -1597,6 +1596,7 @@ bool memory_region_init_ram_flags_nomigrate(MemoryRegion *mr, Object *owner,
>> RAMBlock *rb;
>>
>> memory_region_init(mr, owner, name, size);
>> + mr->ram = true;
>> rb = qemu_ram_alloc(size, ram_flags, mr, errp);
>> return memory_region_set_ram_block(mr, rb);
>> }
>> @@ -1614,6 +1614,7 @@ bool memory_region_init_resizeable_ram(MemoryRegion *mr,
>> RAMBlock *rb;
>>
>> memory_region_init(mr, owner, name, size);
>> + mr->ram = true;
>> rb = qemu_ram_alloc_resizeable(size, max_size, resized, mr, errp);
>> return memory_region_set_ram_block(mr, rb);
>> }
>> @@ -1628,6 +1629,7 @@ bool memory_region_init_ram_from_file(MemoryRegion *mr, Object *owner,
>> RAMBlock *rb;
>>
>> memory_region_init(mr, owner, name, size);
>> + mr->ram = true;
>> mr->readonly = !!(ram_flags & RAM_READONLY);
>> mr->align = align;
>> rb = qemu_ram_alloc_from_file(size, mr, ram_flags, path, offset, errp);
>> @@ -1642,6 +1644,7 @@ bool memory_region_init_ram_from_fd(MemoryRegion *mr, Object *owner,
>> RAMBlock *rb;
>>
>> memory_region_init(mr, owner, name, size);
>> + mr->ram = true;
>> mr->readonly = !!(ram_flags & RAM_READONLY);
>> rb = qemu_ram_alloc_from_fd(size, size, NULL, mr, ram_flags, fd, offset,
>> false, errp);
>> @@ -1663,6 +1666,7 @@ void memory_region_init_ram_ptr(MemoryRegion *mr, Object *owner,
>> void *ptr)
>> {
>> memory_region_init(mr, owner, name, size);
>> + mr->ram = true;
>> memory_region_set_ram_ptr(mr, size, ptr);
>> }
>>
>> @@ -1671,6 +1675,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 +3704,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;
>> --
>> 2.43.0
>>
>
>
On Thu, 12 Mar 2026, 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 <farrah.chen@intel.com>
>>> Fixes: 2fb627ef2f48 ("memory: Factor out common ram region
>>> initialization")
>>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>>
>> 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.
It seems the assert happens when called with RAM_GUEST_MEMFD flag so maybe
some of these that can't pass this flag would not need setting it before
the qemu_ram_alloc call but it may be hard to tell which ones need it so
doing consistently seems better. If we allow qemu_ram_alloc to set this
when needed we could move it there but that's too much change for bugfix.
The qemu_alloc_ram* functions from physmem.c seem to be closely entangled
with memory.c and nothing seems to call these functions outside of
memory.c so some refactoring may allow to unexport this API and make it
static or simplify it further but that seems to be a bigger refactoring
and I don't know why these were separate in the first place. Maybe memory
regions were added as an additional layer on top of physmem?
For now as a bugfix this seems to be the simplest way.
Regards,
BALATON Zoltan
>>> ---
>>> system/memory.c | 8 ++++++--
>>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/system/memory.c b/system/memory.c
>>> index 17a7bcd9af7c..56f3225b21ad 100644
>>> --- a/system/memory.c
>>> +++ b/system/memory.c
>>> @@ -1578,7 +1578,6 @@ void memory_region_init_io(MemoryRegion *mr, Object
>>> *owner,
>>>
>>> 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;
>>> @@ -1597,6 +1596,7 @@ bool
>>> memory_region_init_ram_flags_nomigrate(MemoryRegion *mr, Object *owner,
>>> RAMBlock *rb;
>>>
>>> memory_region_init(mr, owner, name, size);
>>> + mr->ram = true;
>>> rb = qemu_ram_alloc(size, ram_flags, mr, errp);
>>> return memory_region_set_ram_block(mr, rb);
>>> }
>>> @@ -1614,6 +1614,7 @@ bool memory_region_init_resizeable_ram(MemoryRegion
>>> *mr,
>>> RAMBlock *rb;
>>>
>>> memory_region_init(mr, owner, name, size);
>>> + mr->ram = true;
>>> rb = qemu_ram_alloc_resizeable(size, max_size, resized, mr, errp);
>>> return memory_region_set_ram_block(mr, rb);
>>> }
>>> @@ -1628,6 +1629,7 @@ bool memory_region_init_ram_from_file(MemoryRegion
>>> *mr, Object *owner,
>>> RAMBlock *rb;
>>>
>>> memory_region_init(mr, owner, name, size);
>>> + mr->ram = true;
>>> mr->readonly = !!(ram_flags & RAM_READONLY);
>>> mr->align = align;
>>> rb = qemu_ram_alloc_from_file(size, mr, ram_flags, path, offset,
>>> errp);
>>> @@ -1642,6 +1644,7 @@ bool memory_region_init_ram_from_fd(MemoryRegion
>>> *mr, Object *owner,
>>> RAMBlock *rb;
>>>
>>> memory_region_init(mr, owner, name, size);
>>> + mr->ram = true;
>>> mr->readonly = !!(ram_flags & RAM_READONLY);
>>> rb = qemu_ram_alloc_from_fd(size, size, NULL, mr, ram_flags, fd,
>>> offset,
>>> false, errp);
>>> @@ -1663,6 +1666,7 @@ void memory_region_init_ram_ptr(MemoryRegion *mr,
>>> Object *owner,
>>> void *ptr)
>>> {
>>> memory_region_init(mr, owner, name, size);
>>> + mr->ram = true;
>>> memory_region_set_ram_ptr(mr, size, ptr);
>>> }
>>>
>>> @@ -1671,6 +1675,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 +3704,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;
>>> --
>>> 2.43.0
>>>
>>
>>
>
>
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 <farrah.chen@intel.com>
> > > Fixes: 2fb627ef2f48 ("memory: Factor out common ram region initialization")
> > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> >
> > 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.
Thanks,
--
Peter Xu
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 <farrah.chen@intel.com>
>>>> Fixes: 2fb627ef2f48 ("memory: Factor out common ram region initialization")
>>>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>>>
>>> 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
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 <farrah.chen@intel.com>
>>>>> Fixes: 2fb627ef2f48 ("memory: Factor out common ram region
>>>>> initialization")
>>>>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>>>>
>>>> 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
On Fri, Mar 13, 2026 at 11:36:45AM +0800, Xiaoyao Li wrote: > 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; OK, thanks for the fixup proposal, let me queue the original version for now. -- Peter Xu
© 2016 - 2026 Red Hat, Inc.