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

[Peter Xu <peterx@redhat.com>]

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