diff mbox series

[v5,04/14] softmmu/memory: Pass ram_flags to qemu_ram_alloc_from_fd()

Message ID 20210413091421.7707-5-david@redhat.com (mailing list archive)
State New
Headers show
Series RAM_NORESERVE, MAP_NORESERVE and hostmem "reserve" property | expand

Commit Message

David Hildenbrand April 13, 2021, 9:14 a.m. UTC
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(-)

Comments

Philippe Mathieu-Daudé April 20, 2021, 9:52 a.m. UTC | #1
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>
Philippe Mathieu-Daudé April 20, 2021, 10:18 a.m. UTC | #2
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)
David Hildenbrand April 20, 2021, 10:36 a.m. UTC | #3
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!
Philippe Mathieu-Daudé April 20, 2021, 10:45 a.m. UTC | #4
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.
David Hildenbrand April 20, 2021, 11:03 a.m. UTC | #5
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.
diff mbox series

Patch

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));