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 <pet...@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 <da...@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
