[PATCH] memory: Set mr->ram before RAM Block allocation

[PATCH] memory: Set mr->ram before RAM Block allocation

[Xiaoyao Li]

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

Re: [PATCH] memory: Set mr->ram before RAM Block allocation

[Peter Xu]

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

Re: [PATCH] memory: Set mr->ram before RAM Block allocation

[BALATON Zoltan]

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

Re: [PATCH] memory: Set mr->ram before RAM Block allocation

[Peter Xu]

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

Re: [PATCH] memory: Set mr->ram before RAM Block allocation

[BALATON Zoltan]

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

Re: [PATCH] memory: Set mr->ram before RAM Block allocation

[BALATON Zoltan]

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

Re: [PATCH] memory: Set mr->ram before RAM Block allocation

[Xiaoyao Li]

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

Re: [PATCH] memory: Set mr->ram before RAM Block allocation

[Peter Xu]

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

Re: [PATCH] memory: Set mr->ram before RAM Block allocation

[Kim Phillips]

On 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