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

[Peter Xu]

On Wed, Mar 10, 2021 at 11:55:58AM +0100, David Hildenbrand wrote:
> On 10.03.21 11:11, David Hildenbrand wrote:
> > On 10.03.21 09:41, David Hildenbrand wrote:
> > > 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);
> > 
> > What about this:
> > 
> 
> 
> The only ugly thing is that e.g., MAP_SYNC is only defined for Linux and
> MAP_NORESERVE is mostly only defined on Linux.
> 
> So we need something like we already have in mmap-alloc.c:
> 
> #ifdef CONFIG_LINUX
> #include <linux/mman.h>
> #else  /* !CONFIG_LINUX */
> #define MAP_SYNC              0x0
> #define MAP_SHARED_VALIDATE   0x0
> #endif /* CONFIG_LINUX */
> 
> 
> and for the noreserve part
> 
> 
> #ifndef MAP_NORESERVE
> #define MAP_NORESERVE 0x0
> #endif
> 
> 
> But then, I can no longer bail out if someone specifies a flag although it
> is unsupported/not effective. hmmmm ...

I see, indeed that'll be awkward too.  How about keep your original proposal,
but just move it to osdep.h?  That seems to be the simplest and cleanest
approach so far.  Thanks,

-- 
Peter Xu