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

[David Hildenbrand]

On 09.03.21 21:58, Peter Xu wrote:
> 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.

I'll give it another thought today. I certainly want to avoid moving all 
that MAP_ flag and PROT_ logic to the callers. E.g., MAP_SHARED implies 
!MAP_PRIVATE. MAP_SYNC implies that we want MAP_SHARED_VALIDATE. fd < 0 
implies MAP_ANONYMOUS.

Maybe something like

/*
 * QEMU's MMAP abstraction to map guest RAM, taking care of alignment
 * requirements and guard pages.
 *
 * Supported flags: MAP_SHARED, MAP_SYNC
 *
 * Implicitly set flags:
 * - MAP PRIVATE: When !MAP_SHARED and !MAP_SYNC
 * - MAP_ANONYMOUS: When fd < 0
 * - MAP_SHARED_VALIDATE: When MAP_SYNC
 *
 * If mapping with MAP_SYNC|MAP_SHARED_VALIDATE fails, fallback to
 * !MAP_SYNC|MAP_SHARED and warn.
 */
 void *qemu_ram_mmap(int fd,
                     size_t size,
                     size_t align,
                     bool readonly,
                     uint32_t mmap_flags,
                     off_t map_offset);


I also thought about introducing
        QEMU_MAP_READONLY 0x100000000ul

and using "uint64_t mmap_flags" - thoughts?

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

Hm, I don't think this is a good idea unless we would have something 
like qemu_mmap() - which I don't think we'll have in the near future.

--
Thanks,

David / dhildenb