| Message ID | 20230117220914.2062125-11-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: Support hugetlb doublemaps | expand |
* 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. Other than that, Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > block->file_offset = offset; > + block->fd = fd; > block->page_size = qemu_fd_getpagesize(fd); > if (block->mr->align % block->page_size) { > error_setg(errp, "alignment 0x%" PRIx64 > @@ -1588,19 +1602,14 @@ static void *file_ram_alloc(RAMBlock *block, > perror("ftruncate"); > } > > - 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; > - area = qemu_ram_mmap(fd, block->mmap_length, block->mr->align, > - qemu_map_flags, offset); > + area = ramblock_file_map(block); > + > if (area == MAP_FAILED) { > error_setg_errno(errp, errno, > "unable to map backing store for guest RAM"); > return NULL; > } > > - block->fd = fd; > return area; > } > #endif > -- > 2.37.3 >
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 (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 >
On Wed, Jan 25, 2023 at 09:24:24AM +0000, Dr. David Alan Gilbert wrote: > > > > 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? Ah yes.. Since the comment is not extremely informative, instead of changing it, I can drop it in the previous patch when introduced.
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> Reviewed-by: Juan Quintela <quintela@redhat.com> > +void *ramblock_file_map(RAMBlock *block); I would have called it: void *qemu_ram_mmap_file(RAMBlock *block); To make clear that it is 'like' qemu_ram_mmap(), but for a file. But that is just a suggestion. Whoever does the patch, get the right to name the functions.
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 */ block->file_offset = offset; + block->fd = fd; block->page_size = qemu_fd_getpagesize(fd); if (block->mr->align % block->page_size) { error_setg(errp, "alignment 0x%" PRIx64 @@ -1588,19 +1602,14 @@ static void *file_ram_alloc(RAMBlock *block, perror("ftruncate"); } - 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; - area = qemu_ram_mmap(fd, block->mmap_length, block->mr->align, - qemu_map_flags, offset); + area = ramblock_file_map(block); + if (area == MAP_FAILED) { error_setg_errno(errp, errno, "unable to map backing store for guest RAM"); return NULL; } - block->fd = fd; return area; } #endif
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(-)