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

[Peter Xu]

On Tue, Mar 09, 2021 at 09:27:10PM +0100, David Hildenbrand wrote:
> 
> > 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.

Yeh the READONLY flag would be special, it will need to be separated from the
rest flags.  I'd keep my own preference, but if you really like the current
way, maybe at least move it to qemu/osdep.h?  So at least when someone needs a
cross-platform flag they'll show up - while mmap-alloc.h looks still only for
the posix world, then it'll be odd to introduce these flags only for posix even
if posix definied most of them.

At the meantime, maybe rename QEMU_RAM_MMAP_* to QEMU_MMAP_* too?  All of them
look applicable to no-RAM-backends too.

Thanks,

-- 
Peter Xu