Re: [PATCH v7 10/21] multi-process: setup memory manager for remote device

[Stefan Hajnoczi]

On Sat, Jun 27, 2020 at 10:09:32AM -0700, elena.ufimtseva@oracle.com wrote:
> +void remote_sysmem_reconfig(MPQemuMsg *msg, Error **errp)
> +{
> +    SyncSysmemMsg *sysmem_info = &msg->data1.sync_sysmem;
> +    MemoryRegion *sysmem, *subregion, *next;
> +    static unsigned int suffix;
> +    Error *local_err = NULL;
> +    char *name;
> +    int region;
> +
> +    sysmem = get_system_memory();
> +
> +    memory_region_transaction_begin();
> +
> +    QTAILQ_FOREACH_SAFE(subregion, &sysmem->subregions, subregions_link, 
> next) {
> +        if (subregion->ram) {
> +            memory_region_del_subregion(sysmem, subregion);
> +            qemu_ram_free(subregion->ram_block);

Why is qemu_ram_free() called explicitly? Normally the only caller is
memory_region_destructor_ram().

MemoryRegions are reference counted. The qemu_ram_free() call should
probably be replaced with memory_region_unref(subregion). This also
solves the subregion memory leak.

That said, I haven't read the MemoryRegion lifecycle code so please
check that what I'm suggesting is correct.

> +typedef struct {
> +    hwaddr gpas[REMOTE_MAX_FDS];
> +    uint64_t sizes[REMOTE_MAX_FDS];
> +    off_t offsets[REMOTE_MAX_FDS];
> +} SyncSysmemMsg;

It's safer to use fixed-width types like uint64_t for external
communication, but in this case we expect the client and server to
match.

signature.asc
Description: PGP signature