[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 09/12] util/mmap-alloc: Pass flags instead of separate boo
From: |
Peter Xu |
Subject: |
Re: [PATCH v3 09/12] util/mmap-alloc: Pass flags instead of separate bools to qemu_ram_mmap() |
Date: |
Wed, 10 Mar 2021 11:27:03 -0500 |
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
- [PATCH v3 06/12] util/mmap-alloc: Factor out activating of memory to mmap_activate(), (continued)
- [PATCH v3 06/12] util/mmap-alloc: Factor out activating of memory to mmap_activate(), David Hildenbrand, 2021/03/08
- [PATCH v3 07/12] softmmu/memory: Pass ram_flags into qemu_ram_alloc_from_fd(), David Hildenbrand, 2021/03/08
- [PATCH v3 08/12] softmmu/memory: Pass ram_flags into memory_region_init_ram_shared_nomigrate(), David Hildenbrand, 2021/03/08
- [PATCH v3 09/12] util/mmap-alloc: Pass flags instead of separate bools to qemu_ram_mmap(), David Hildenbrand, 2021/03/08
- Re: [PATCH v3 09/12] util/mmap-alloc: Pass flags instead of separate bools to qemu_ram_mmap(), Peter Xu, 2021/03/09
- Re: [PATCH v3 09/12] util/mmap-alloc: Pass flags instead of separate bools to qemu_ram_mmap(), David Hildenbrand, 2021/03/09
- Re: [PATCH v3 09/12] util/mmap-alloc: Pass flags instead of separate bools to qemu_ram_mmap(), Peter Xu, 2021/03/09
- Re: [PATCH v3 09/12] util/mmap-alloc: Pass flags instead of separate bools to qemu_ram_mmap(), David Hildenbrand, 2021/03/10
- Re: [PATCH v3 09/12] util/mmap-alloc: Pass flags instead of separate bools to qemu_ram_mmap(), David Hildenbrand, 2021/03/10
- Re: [PATCH v3 09/12] util/mmap-alloc: Pass flags instead of separate bools to qemu_ram_mmap(), David Hildenbrand, 2021/03/10
- Re: [PATCH v3 09/12] util/mmap-alloc: Pass flags instead of separate bools to qemu_ram_mmap(),
Peter Xu <=
[PATCH v3 10/12] memory: introduce RAM_NORESERVE and wire it up in qemu_ram_mmap(), David Hildenbrand, 2021/03/08
[PATCH v3 11/12] util/mmap-alloc: Support RAM_NORESERVE via MAP_NORESERVE, David Hildenbrand, 2021/03/08
[PATCH v3 12/12] hostmem: Wire up RAM_NORESERVE via "reserve" property, David Hildenbrand, 2021/03/08