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

[Peter Xu <peterx@redhat.com>]

On Mon, Oct 07, 2024 at 11:49:25AM -0400, Peter Xu wrote:
> 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.

PS: If this is needed by CPR-transfer only because mmap() later can fail
due to a bigger max_length, I wonder whether it can be fixed by passing
truncate=true in the upcoming file_ram_alloc(), rather than overwritting
the max_length value itself.

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

-- 
Peter Xu