Let's pass in ram flags just like we do with qemu_ram_alloc_from_file(),
to clean up and prepare for more flags.
Simplify the documentation of passed ram flags: Looking at our
documentation of RAM_SHARED and RAM_PMEM is sufficient, no need to be
repetitive.
Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
backends/hostmem-memfd.c | 7 ++++---
hw/misc/ivshmem.c | 5 ++---
include/exec/memory.h | 9 +++------
include/exec/ram_addr.h | 6 +-----
softmmu/memory.c | 7 +++----
5 files changed, 13 insertions(+), 21 deletions(-)
diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c
index 69b0ae30bb..93b5d1a4cf 100644
--- a/backends/hostmem-memfd.c
+++ b/backends/hostmem-memfd.c
@@ -36,6 +36,7 @@ static void
memfd_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
{
HostMemoryBackendMemfd *m = MEMORY_BACKEND_MEMFD(backend);
+ uint32_t ram_flags;
char *name;
int fd;
@@ -53,9 +54,9 @@ memfd_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
}
name = host_memory_backend_get_name(backend);
- memory_region_init_ram_from_fd(&backend->mr, OBJECT(backend),
- name, backend->size,
- backend->share, fd, 0, errp);
+ ram_flags = backend->share ? RAM_SHARED : 0;
+ memory_region_init_ram_from_fd(&backend->mr, OBJECT(backend), name,
+ backend->size, ram_flags, fd, 0, errp);
g_free(name);
}
diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index a1fa4878be..1ba4a98377 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -493,9 +493,8 @@ static void process_msg_shmem(IVShmemState *s, int fd, Error **errp)
size = buf.st_size;
/* mmap the region and map into the BAR2 */
- memory_region_init_ram_from_fd(&s->server_bar2, OBJECT(s),
- "ivshmem.bar2", size, true, fd, 0,
- &local_err);
+ memory_region_init_ram_from_fd(&s->server_bar2, OBJECT(s), "ivshmem.bar2",
+ size, RAM_SHARED, fd, 0, &local_err);
if (local_err) {
error_propagate(errp, local_err);
return;
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 5728a681b2..8ad280e532 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -991,10 +991,7 @@ void memory_region_init_resizeable_ram(MemoryRegion *mr,
* @size: size of the region.
* @align: alignment of the region base address; if 0, the default alignment
* (getpagesize()) will be used.
- * @ram_flags: Memory region features:
- * - RAM_SHARED: memory must be mmaped with the MAP_SHARED flag
- * - RAM_PMEM: the memory is persistent memory
- * Other bits are ignored now.
+ * @ram_flags: RamBlock flags. Supported flags: RAM_SHARED, RAM_PMEM.
* @path: the path in which to allocate the RAM.
* @readonly: true to open @path for reading, false for read/write.
* @errp: pointer to Error*, to store an error if it happens.
@@ -1020,7 +1017,7 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
* @owner: the object that tracks the region's reference count
* @name: the name of the region.
* @size: size of the region.
- * @share: %true if memory must be mmaped with the MAP_SHARED flag
+ * @ram_flags: RamBlock flags. Supported flags: RAM_SHARED, RAM_PMEM.
* @fd: the fd to mmap.
* @offset: offset within the file referenced by fd
* @errp: pointer to Error*, to store an error if it happens.
@@ -1032,7 +1029,7 @@ void memory_region_init_ram_from_fd(MemoryRegion *mr,
Object *owner,
const char *name,
uint64_t size,
- bool share,
+ uint32_t ram_flags,
int fd,
ram_addr_t offset,
Error **errp);
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 3cb9791df3..a7e3378340 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -104,11 +104,7 @@ long qemu_maxrampagesize(void);
* Parameters:
* @size: the size in bytes of the ram block
* @mr: the memory region where the ram block is
- * @ram_flags: specify the properties of the ram block, which can be one
- * or bit-or of following values
- * - RAM_SHARED: mmap the backing file or device with MAP_SHARED
- * - RAM_PMEM: the backend @mem_path or @fd is persistent memory
- * Other bits are ignored.
+ * @ram_flags: RamBlock flags. Supported flags: RAM_SHARED, RAM_PMEM.
* @mem_path or @fd: specify the backing file or device
* @readonly: true to open @path for reading, false for read/write.
* @errp: pointer to Error*, to store an error if it happens
diff --git a/softmmu/memory.c b/softmmu/memory.c
index d4493ef9e4..8c3acd839e 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1611,7 +1611,7 @@ void memory_region_init_ram_from_fd(MemoryRegion *mr,
Object *owner,
const char *name,
uint64_t size,
- bool share,
+ uint32_t ram_flags,
int fd,
ram_addr_t offset,
Error **errp)
@@ -1621,9 +1621,8 @@ void memory_region_init_ram_from_fd(MemoryRegion *mr,
mr->ram = true;
mr->terminates = true;
mr->destructor = memory_region_destructor_ram;
- mr->ram_block = qemu_ram_alloc_from_fd(size, mr,
- share ? RAM_SHARED : 0,
- fd, offset, false, &err);
+ mr->ram_block = qemu_ram_alloc_from_fd(size, mr, ram_flags, fd, offset,
+ false, &err);
if (err) {
mr->size = int128_zero();
object_unparent(OBJECT(mr));
--
2.30.2
On 4/13/21 11:14 AM, David Hildenbrand wrote: > Let's pass in ram flags just like we do with qemu_ram_alloc_from_file(), > to clean up and prepare for more flags. > > Simplify the documentation of passed ram flags: Looking at our > documentation of RAM_SHARED and RAM_PMEM is sufficient, no need to be > repetitive. > > Reviewed-by: Peter Xu <peterx@redhat.com> > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > backends/hostmem-memfd.c | 7 ++++--- > hw/misc/ivshmem.c | 5 ++--- > include/exec/memory.h | 9 +++------ > include/exec/ram_addr.h | 6 +----- > softmmu/memory.c | 7 +++---- > 5 files changed, 13 insertions(+), 21 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Hi David, On 4/20/21 11:52 AM, Philippe Mathieu-Daudé wrote: > On 4/13/21 11:14 AM, David Hildenbrand wrote: >> Let's pass in ram flags just like we do with qemu_ram_alloc_from_file(), >> to clean up and prepare for more flags. >> >> Simplify the documentation of passed ram flags: Looking at our >> documentation of RAM_SHARED and RAM_PMEM is sufficient, no need to be >> repetitive. >> >> Reviewed-by: Peter Xu <peterx@redhat.com> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> backends/hostmem-memfd.c | 7 ++++--- >> hw/misc/ivshmem.c | 5 ++--- >> include/exec/memory.h | 9 +++------ >> include/exec/ram_addr.h | 6 +----- >> softmmu/memory.c | 7 +++---- >> 5 files changed, 13 insertions(+), 21 deletions(-) > > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > Actually it would be clearer to define the 0 value, maybe: #define RAM_NOFLAG (0 << 0)
On 20.04.21 12:18, Philippe Mathieu-Daudé wrote: > Hi David, > > On 4/20/21 11:52 AM, Philippe Mathieu-Daudé wrote: >> On 4/13/21 11:14 AM, David Hildenbrand wrote: >>> Let's pass in ram flags just like we do with qemu_ram_alloc_from_file(), >>> to clean up and prepare for more flags. >>> >>> Simplify the documentation of passed ram flags: Looking at our >>> documentation of RAM_SHARED and RAM_PMEM is sufficient, no need to be >>> repetitive. >>> >>> Reviewed-by: Peter Xu <peterx@redhat.com> >>> Signed-off-by: David Hildenbrand <david@redhat.com> >>> --- >>> backends/hostmem-memfd.c | 7 ++++--- >>> hw/misc/ivshmem.c | 5 ++--- >>> include/exec/memory.h | 9 +++------ >>> include/exec/ram_addr.h | 6 +----- >>> softmmu/memory.c | 7 +++---- >>> 5 files changed, 13 insertions(+), 21 deletions(-) >> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> > > Actually it would be clearer to define the 0 value, maybe: > > #define RAM_NOFLAG (0 << 0) > This will also turn some code into ram_flags = backend->share ? RAM_SHARED : RAM_NOFLAG; ram_flags |= backend->reserve ? RAM_NOFLAG : RAM_NORESERVE; Looking at other flag users, I barely see any such usage. XKB_CONTEXT_NO_FLAGS, ALLOC_NO_FLAGS, and MEM_AFFINITY_NOFLAGS seem to be the only ones. That's why I tend to not do it unless there are strong opinions. Thanks! -- Thanks, David / dhildenb
On 4/20/21 12:36 PM, David Hildenbrand wrote: > On 20.04.21 12:18, Philippe Mathieu-Daudé wrote: >> Hi David, >> >> On 4/20/21 11:52 AM, Philippe Mathieu-Daudé wrote: >>> On 4/13/21 11:14 AM, David Hildenbrand wrote: >>>> Let's pass in ram flags just like we do with >>>> qemu_ram_alloc_from_file(), >>>> to clean up and prepare for more flags. >>>> >>>> Simplify the documentation of passed ram flags: Looking at our >>>> documentation of RAM_SHARED and RAM_PMEM is sufficient, no need to be >>>> repetitive. >>>> >>>> Reviewed-by: Peter Xu <peterx@redhat.com> >>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>> --- >>>> backends/hostmem-memfd.c | 7 ++++--- >>>> hw/misc/ivshmem.c | 5 ++--- >>>> include/exec/memory.h | 9 +++------ >>>> include/exec/ram_addr.h | 6 +----- >>>> softmmu/memory.c | 7 +++---- >>>> 5 files changed, 13 insertions(+), 21 deletions(-) >>> >>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> >> >> Actually it would be clearer to define the 0 value, maybe: >> >> #define RAM_NOFLAG (0 << 0) >> > > This will also turn some code into > > ram_flags = backend->share ? RAM_SHARED : RAM_NOFLAG; > ram_flags |= backend->reserve ? RAM_NOFLAG : RAM_NORESERVE; This is the callee view, withing the API, where you have all the context. > Looking at other flag users, I barely see any such usage. > XKB_CONTEXT_NO_FLAGS, ALLOC_NO_FLAGS, and MEM_AFFINITY_NOFLAGS seem to > be the only ones. That's why I tend to not do it unless there are strong > opinions. I'm more concerned about the caller perspective. What means this magic '0' in the arguments? Then I have to check the prototype. If the caller uses RAM_NO_FLAGS, I directly understand what is passed. Anyway my comment fits the usual "can be cleaned later" case. Thanks, Phil.
On 20.04.21 12:45, Philippe Mathieu-Daudé wrote: > On 4/20/21 12:36 PM, David Hildenbrand wrote: >> On 20.04.21 12:18, Philippe Mathieu-Daudé wrote: >>> Hi David, >>> >>> On 4/20/21 11:52 AM, Philippe Mathieu-Daudé wrote: >>>> On 4/13/21 11:14 AM, David Hildenbrand wrote: >>>>> Let's pass in ram flags just like we do with >>>>> qemu_ram_alloc_from_file(), >>>>> to clean up and prepare for more flags. >>>>> >>>>> Simplify the documentation of passed ram flags: Looking at our >>>>> documentation of RAM_SHARED and RAM_PMEM is sufficient, no need to be >>>>> repetitive. >>>>> >>>>> Reviewed-by: Peter Xu <peterx@redhat.com> >>>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>>> --- >>>>> backends/hostmem-memfd.c | 7 ++++--- >>>>> hw/misc/ivshmem.c | 5 ++--- >>>>> include/exec/memory.h | 9 +++------ >>>>> include/exec/ram_addr.h | 6 +----- >>>>> softmmu/memory.c | 7 +++---- >>>>> 5 files changed, 13 insertions(+), 21 deletions(-) >>>> >>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>>> >>> >>> Actually it would be clearer to define the 0 value, maybe: >>> >>> #define RAM_NOFLAG (0 << 0) >>> >> >> This will also turn some code into >> >> ram_flags = backend->share ? RAM_SHARED : RAM_NOFLAG; >> ram_flags |= backend->reserve ? RAM_NOFLAG : RAM_NORESERVE; > > This is the callee view, withing the API, where you have all > the context. > >> Looking at other flag users, I barely see any such usage. >> XKB_CONTEXT_NO_FLAGS, ALLOC_NO_FLAGS, and MEM_AFFINITY_NOFLAGS seem to >> be the only ones. That's why I tend to not do it unless there are strong >> opinions. > > I'm more concerned about the caller perspective. What means this > magic '0' in the arguments? Then I have to check the prototype. > If the caller uses RAM_NO_FLAGS, I directly understand what is passed. > Yeah, that makes sense. Even cleaner would be using the type system as we do in the kernel, e.g., for GFP flags typedef unsigned int __bitwise gfp_t; #define ___GFP_DMA 0x01u #define __GFP_DMA ((__force gfp_t)___GFP_DMA) And passing around gfp_t. Then even using "0" will bail out. > Anyway my comment fits the usual "can be cleaned later" case. Make sense, thanks. -- Thanks, David / dhildenb
© 2016 - 2025 Red Hat, Inc.