Re: [PATCH RFC 10/21] ramblock: Add ramblock_file_map()

[Peter Xu]

On Tue, Jan 24, 2023 at 10:06:48AM +0000, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > Add a helper to do mmap() for a ramblock based on the cached informations.
> > 
> > A trivial thing to mention is we need to move ramblock->fd setup to be
> > earlier, before the ramblock_file_map() call, because it'll need to
> > reference the fd being mapped.  However that should not be a problem at
> > all, majorly because the fd won't be freed if successful, and if it failed
> > the fd will be freeed (or to be explicit, close()ed) by the caller.
> > 
> > Export it - prepare to be used outside this file.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  include/exec/ram_addr.h |  1 +
> >  softmmu/physmem.c       | 25 +++++++++++++++++--------
> >  2 files changed, 18 insertions(+), 8 deletions(-)
> > 
> > diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> > index 0bf9cfc659..56db25009a 100644
> > --- a/include/exec/ram_addr.h
> > +++ b/include/exec/ram_addr.h
> > @@ -98,6 +98,7 @@ bool ramblock_is_pmem(RAMBlock *rb);
> >  
> >  long qemu_minrampagesize(void);
> >  long qemu_maxrampagesize(void);
> > +void *ramblock_file_map(RAMBlock *block);
> >  
> >  /**
> >   * qemu_ram_alloc_from_file,
> > diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> > index 6096eac286..cdda7eaea5 100644
> > --- a/softmmu/physmem.c
> > +++ b/softmmu/physmem.c
> > @@ -1532,17 +1532,31 @@ static int file_ram_open(const char *path,
> >      return fd;
> >  }
> >  
> > +/* Do the mmap() for a ramblock based on information already setup */
> > +void *ramblock_file_map(RAMBlock *block)
> > +{
> > +    uint32_t qemu_map_flags;
> > +
> > +    qemu_map_flags = (block->flags & RAM_READONLY) ? QEMU_MAP_READONLY : 0;
> > +    qemu_map_flags |= (block->flags & RAM_SHARED) ? QEMU_MAP_SHARED : 0;
> > +    qemu_map_flags |= (block->flags & RAM_PMEM) ? QEMU_MAP_SYNC : 0;
> > +    qemu_map_flags |= (block->flags & RAM_NORESERVE) ? QEMU_MAP_NORESERVE 
> > : 0;
> > +
> > +    return qemu_ram_mmap(block->fd, block->mmap_length, block->mr->align,
> > +                         qemu_map_flags, block->file_offset);
> > +}
> > +
> >  static void *file_ram_alloc(RAMBlock *block,
> >                              int fd,
> >                              bool truncate,
> >                              off_t offset,
> >                              Error **errp)
> >  {
> > -    uint32_t qemu_map_flags;
> >      void *area;
> >  
> >      /* Remember the offset just in case we'll need to map the range again 
> > */
> 
> Note that this comment is now wrong; you need to always set that for the
> map call.

This line is added in patch 7.  After this patch, a ramblock should always
be mapped with ramblock_file_map(), so it keeps being true?

> 
> Other than that,
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Thanks,

-- 
Peter Xu