Re: [PATCH V2 05/13] physmem: preserve ram blocks for cpr

[Peter Xu <peterx@redhat.com>]

On Mon, Sep 30, 2024 at 12:40:36PM -0700, Steve Sistare wrote:
> Save the memfd for anonymous ramblocks in CPR state, along with a name
> that uniquely identifies it.  The block's idstr is not yet set, so it
> cannot be used for this purpose.  Find the saved memfd in new QEMU when
> creating a block.  QEMU hard-codes the length of some internally-created
> blocks, so to guard against that length changing, use lseek to get the
> actual length of an incoming memfd.
> 
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>  system/physmem.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/system/physmem.c b/system/physmem.c
> index 174f7e0..ddbeec9 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -72,6 +72,7 @@
>  
>  #include "qapi/qapi-types-migration.h"
>  #include "migration/options.h"
> +#include "migration/cpr.h"
>  #include "migration/vmstate.h"
>  
>  #include "qemu/range.h"
> @@ -1663,6 +1664,19 @@ void qemu_ram_unset_idstr(RAMBlock *block)
>      }
>  }
>  
> +static char *cpr_name(RAMBlock *block)
> +{
> +    MemoryRegion *mr = block->mr;
> +    const char *mr_name = memory_region_name(mr);
> +    g_autofree char *id = mr->dev ? qdev_get_dev_path(mr->dev) : NULL;
> +
> +    if (id) {
> +        return g_strdup_printf("%s/%s", id, mr_name);
> +    } else {
> +        return g_strdup(mr_name);
> +    }
> +}
> +
>  size_t qemu_ram_pagesize(RAMBlock *rb)
>  {
>      return rb->page_size;
> @@ -1858,14 +1872,18 @@ static void ram_block_add(RAMBlock *new_block, Error **errp)
>                                          TYPE_MEMORY_BACKEND)) {
>              size_t max_length = new_block->max_length;
>              MemoryRegion *mr = new_block->mr;
> -            const char *name = memory_region_name(mr);
> +            g_autofree char *name = cpr_name(new_block);
>  
>              new_block->mr->align = QEMU_VMALLOC_ALIGN;
>              new_block->flags |= RAM_SHARED;
> +            new_block->fd = cpr_find_fd(name, 0);
>  
>              if (new_block->fd == -1) {
>                  new_block->fd = qemu_memfd_create(name, max_length + mr->align,
>                                                    0, 0, 0, errp);
> +                cpr_save_fd(name, 0, new_block->fd);
> +            } else {
> +                new_block->max_length = lseek(new_block->fd, 0, SEEK_END);

So this can overwrite the max_length that the caller specified..

I remember we used to have some tricks on specifying different max_length
for ROMs on dest QEMU (on which, qemu firmwares also upgraded on the dest
host so the size can be bigger than src qemu's old ramblocks), so that the
MR is always large enough to reload even the new firmwares, while migration
only migrates the smaller size (used_length) so it's fine as we keep the
extra sizes empty. I think that can relevant to the qemu_ram_resize() call
of parse_ramblock().

The reload will not happen until some point, perhaps system resets.  I
wonder whether that is an issue in this case.

+Igor +Mst for this.

>              }
>  
>              if (new_block->fd >= 0) {
> @@ -1875,6 +1893,7 @@ static void ram_block_add(RAMBlock *new_block, Error **errp)
>                                                   false, 0, errp);
>              }
>              if (!new_block->host) {
> +                cpr_delete_fd(name, 0);
>                  qemu_mutex_unlock_ramlist();
>                  return;
>              }
> @@ -2182,6 +2201,8 @@ static void reclaim_ramblock(RAMBlock *block)
>  
>  void qemu_ram_free(RAMBlock *block)
>  {
> +    g_autofree char *name = NULL;
> +
>      if (!block) {
>          return;
>      }
> @@ -2192,6 +2213,8 @@ void qemu_ram_free(RAMBlock *block)
>      }
>  
>      qemu_mutex_lock_ramlist();
> +    name = cpr_name(block);
> +    cpr_delete_fd(name, 0);
>      QLIST_REMOVE_RCU(block, next);
>      ram_list.mru_block = NULL;
>      /* Write list before version */
> -- 
> 1.8.3.1
> 

-- 
Peter Xu