Re: [PATCH v3 09/12] util/mmap-alloc: Pass flags instead of separate bools to qemu_ram_mmap()

[David Hildenbrand]

> Am 09.03.2021 um 21:04 schrieb Peter Xu <peterx@redhat.com>:
> 
> On Mon, Mar 08, 2021 at 04:05:57PM +0100, David Hildenbrand wrote:
>> Let's introduce a new set of flags that abstract mmap logic and replace
>> our current set of bools, to prepare for another flag.
>> 
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>> include/qemu/mmap-alloc.h | 17 +++++++++++------
>> softmmu/physmem.c         |  8 +++++---
>> util/mmap-alloc.c         | 14 +++++++-------
>> util/oslib-posix.c        |  3 ++-
>> 4 files changed, 25 insertions(+), 17 deletions(-)
>> 
>> diff --git a/include/qemu/mmap-alloc.h b/include/qemu/mmap-alloc.h
>> index 456ff87df1..55664ea9f3 100644
>> --- a/include/qemu/mmap-alloc.h
>> +++ b/include/qemu/mmap-alloc.h
>> @@ -6,6 +6,15 @@ size_t qemu_fd_getpagesize(int fd);
>> 
>> size_t qemu_mempath_getpagesize(const char *mem_path);
>> 
>> +/* Map PROT_READ instead of PROT_READ|PROT_WRITE. */
>> +#define QEMU_RAM_MMAP_READONLY      (1 << 0)
>> +
>> +/* Map MAP_SHARED instead of MAP_PRIVATE. */
>> +#define QEMU_RAM_MMAP_SHARED        (1 << 1)
>> +
>> +/* Map MAP_SYNC|MAP_SHARED_VALIDATE if possible, fallback and warn 
>> otherwise. */
>> +#define QEMU_RAM_MMAP_PMEM          (1 << 2)
> 
> Sorry to speak late - I just noticed that is_pmem can actually be converted 
> too
> with "MAP_SYNC | MAP_SHARED_VALIDATE".  We can even define MAP_PMEM_EXTRA for
> use within qemu if we want.  Then we can avoid one layer of QEMU_RAM_* by
> directly using MAP_*, I think?
> 

No problem :) I don‘t think passing in random MAP_ flags is a good interface 
(we would at least need an allow list).

 I like the abstraction / explicit semenatics of QEMU_RAM_MMAP_PMEM as spelled 
out in the comment. Doing the fallback when passing in the mmap flags is a 
little ugly. We could do the fallback in the caller, I think I remember there 
is only a single call site.

PROT_READ won‘t be covered as well, not sure if passing in protections improves 
the interface.

Long story short, I like the abstraction provided by these flags, only 
exporting what we actually support/abstracting it, and setting some MAP_ flags 
automatically (MAP_PRIVATE, MAP_ANON) instead of having to spell that put in 
the caller.


> -- 
> Peter Xu
>