Re: [PATCH v3 6/7] memory: Attach MemoryAttributeManager to guest_memfd-backed RAMBlocks

[Tony Lindgren <tony.lindgren@linux.intel.com>]

On Mon, Mar 17, 2025 at 06:21:13PM +0800, Chenyi Qiang wrote:
> 
> 
> On 3/17/2025 5:45 PM, Tony Lindgren wrote:
> > On Mon, Mar 17, 2025 at 03:32:16PM +0800, Chenyi Qiang wrote:
> >>
> >>
> >> On 3/17/2025 2:18 PM, Tony Lindgren wrote:
> >>> Hi,
> >>>
> >>> On Mon, Mar 10, 2025 at 04:18:34PM +0800, Chenyi Qiang wrote:
> >>>> --- a/system/physmem.c
> >>>> +++ b/system/physmem.c
> >>>> @@ -1885,6 +1886,16 @@ static void ram_block_add(RAMBlock *new_block, Error **errp)
> >>>>              qemu_mutex_unlock_ramlist();
> >>>>              goto out_free;
> >>>>          }
> >>>> +
> >>>> +        new_block->memory_attribute_manager = MEMORY_ATTRIBUTE_MANAGER(object_new(TYPE_MEMORY_ATTRIBUTE_MANAGER));
> >>>> +        if (memory_attribute_manager_realize(new_block->memory_attribute_manager, new_block->mr)) {
> >>>> +            error_setg(errp, "Failed to realize memory attribute manager");
> >>>> +            object_unref(OBJECT(new_block->memory_attribute_manager));
> >>>> +            close(new_block->guest_memfd);
> >>>> +            ram_block_discard_require(false);
> >>>> +            qemu_mutex_unlock_ramlist();
> >>>> +            goto out_free;
> >>>> +        }
> >>>>      }
> >>>>  
> >>>>      ram_size = (new_block->offset + new_block->max_length) >> TARGET_PAGE_BITS;
> >>>
> >>> Might as well put the above into a separate memory manager init function
> >>> to start with. It keeps the goto out_free error path unified, and makes
> >>> things more future proof if the rest of ram_block_add() ever develops a
> >>> need to check for errors.
> >>
> >> Which part to be defined in a separate function? The init function of
> >> object_new() + realize(), or the error handling operation
> >> (object_unref() + close() + ram_block_discard_require(false))?
> > 
> > I was thinking the whole thing, including freeing :) But maybe there's
> > something more to consider to keep calls paired.
> 
> If putting the whole thing separately, I think the rest part to do error
> handling still needs to add the same operation. Or I misunderstand
> something?

So maybe you suggestion of just a separate clean-up function would work:

new_block->memory_attribute_manager =
    MEMORY_ATTRIBUTE_MANAGER(object_new(TYPE_MEMORY_ATTRIBUTE_MANAGER));
if (memory_attribute_manager_realize(new_block->memory_attribute_manager,
    new_block->mr)) {
    memory_attribute_manager_cleanup(...);
    goto out_free;
}

> >> If need to check for errors in the rest of ram_block_add() in future,
> >> how about adding a new label before out_free and move the error handling
> >> there?
> > 
> > Yeah that would work too.
> 
> I'm not sure if we should add such change directly, or we can wait for
> the real error check introduced in future.

Right, not sure either.

Regards,

Tony