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

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> 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?

With ramblock_file_map() it's not a 'just in case' any more though is
it?  This value always goes through the block-> now?

Dave

> > 
> > Other than that,
> > 
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> Thanks,
> 
> -- 
> Peter Xu
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK