| Message ID | 20210308150600.14440-10-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | RAM_NORESERVE, MAP_NORESERVE and hostmem "reserve" property | expand |
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?
> 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. > -- > 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,
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.
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: From 13a59d404bb3edaed9e42c94432be28fb9a65c26 Mon Sep 17 00:00:00 2001 From: David Hildenbrand <david@redhat.com> Date: Fri, 5 Mar 2021 17:20:37 +0100 Subject: [PATCH] util/mmap-alloc: Pass MAP_ flags instead of separate bools to qemu_ram_mmap() Let's pass MAP_ flags instead of bools to prepare for passing other MAP_ flags and update the documentation of qemu_ram_mmap(). Only allow selected MAP_ flags (MAP_SHARED, MAP_SYNC) to be passed and keep setting other flags implicitly. Keep the "readonly" flag, as it cannot be expressed via MAP_ flags. Signed-off-by: David Hildenbrand <david@redhat.com> --- include/qemu/mmap-alloc.h | 19 ++++++++++++++----- softmmu/physmem.c | 6 ++++-- util/mmap-alloc.c | 13 ++++++++----- util/oslib-posix.c | 3 ++- 4 files changed, 28 insertions(+), 13 deletions(-) diff --git a/include/qemu/mmap-alloc.h b/include/qemu/mmap-alloc.h index 456ff87df1..27ef374810 100644 --- a/include/qemu/mmap-alloc.h +++ b/include/qemu/mmap-alloc.h @@ -7,7 +7,10 @@ size_t qemu_fd_getpagesize(int fd); size_t qemu_mempath_getpagesize(const char *mem_path); /** - * qemu_ram_mmap: mmap the specified file or device. + * qemu_ram_mmap: mmap anonymous memory, the specified file or device. + * + * QEMU's MMAP abstraction to map guest RAM, simplifying flag handling, + * taking care of alignment requirements and installing guard pages. * * Parameters: * @fd: the file or the device to mmap @@ -15,10 +18,17 @@ size_t qemu_mempath_getpagesize(const char *mem_path); * @align: if not zero, specify the alignment of the starting mapping address; * otherwise, the alignment in use will be determined by QEMU. * @readonly: true for a read-only mapping, false for read/write. - * @shared: map has RAM_SHARED flag. - * @is_pmem: map has RAM_PMEM flag. + * @map_flags: supported MAP_* flags: MAP_SHARED, MAP_SYNC * @map_offset: map starts at offset of map_offset from the start of fd * + * Implicitly handled map_flags: + * - MAP PRIVATE: With !MAP_SHARED + * - MAP_ANONYMOUS: With fd < 0 + * - MAP_SHARED_VALIDATE: With MAP_SYNC && MAP_SHARED + * + * MAP_SYNC is ignored without MAP_SHARED. If mapping via MAP_SYNC fails, + * warn and fallback to mapping without MAP_SYNC. + * * Return: * On success, return a pointer to the mapped area. * On failure, return MAP_FAILED. @@ -27,8 +37,7 @@ void *qemu_ram_mmap(int fd, size_t size, size_t align, bool readonly, - bool shared, - bool is_pmem, + uint32_t map_flags, off_t map_offset); void qemu_ram_munmap(int fd, void *ptr, size_t size); diff --git a/softmmu/physmem.c b/softmmu/physmem.c index 8f3d286e12..1336884b51 100644 --- a/softmmu/physmem.c +++ b/softmmu/physmem.c @@ -1533,6 +1533,7 @@ static void *file_ram_alloc(RAMBlock *block, off_t offset, Error **errp) { + uint32_t map_flags; void *area; block->page_size = qemu_fd_getpagesize(fd); @@ -1580,9 +1581,10 @@ static void *file_ram_alloc(RAMBlock *block, perror("ftruncate"); } + map_flags = (block->flags & RAM_SHARED) ? MAP_SHARED : 0; + map_flags |= (block->flags & RAM_PMEM) ? MAP_SYNC : 0; area = qemu_ram_mmap(fd, memory, block->mr->align, readonly, - block->flags & RAM_SHARED, block->flags & RAM_PMEM, - offset); + map_flags, offset); if (area == MAP_FAILED) { error_setg_errno(errp, errno, "unable to map backing store for guest RAM"); diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c index 0e2bd7bc0e..b558f1675a 100644 --- a/util/mmap-alloc.c +++ b/util/mmap-alloc.c @@ -119,16 +119,20 @@ static void *mmap_reserve(size_t size, int fd) * it accessible. */ static void *mmap_activate(void *ptr, size_t size, int fd, bool readonly, - bool shared, bool is_pmem, off_t map_offset) + uint32_t map_flags, off_t map_offset) { + const bool shared = map_flags & MAP_SHARED; + const bool sync = map_flags & MAP_SYNC; const int prot = PROT_READ | (readonly ? 0 : PROT_WRITE); int map_sync_flags = 0; int flags = MAP_FIXED; void *activated_ptr; + g_assert(!(map_flags & ~(MAP_SHARED | MAP_SYNC))); + flags |= fd == -1 ? MAP_ANONYMOUS : 0; flags |= shared ? MAP_SHARED : MAP_PRIVATE; - if (shared && is_pmem) { + if (shared && sync) { map_sync_flags = MAP_SYNC | MAP_SHARED_VALIDATE; } @@ -174,8 +178,7 @@ void *qemu_ram_mmap(int fd, size_t size, size_t align, bool readonly, - bool shared, - bool is_pmem, + uint32_t map_flags, off_t map_offset) { const size_t guard_pagesize = mmap_guard_pagesize(fd); @@ -199,7 +202,7 @@ void *qemu_ram_mmap(int fd, offset = QEMU_ALIGN_UP((uintptr_t)guardptr, align) - (uintptr_t)guardptr; - ptr = mmap_activate(guardptr + offset, size, fd, readonly, shared, is_pmem, + ptr = mmap_activate(guardptr + offset, size, fd, readonly, map_flags, map_offset); if (ptr == MAP_FAILED) { munmap(guardptr, total); diff --git a/util/oslib-posix.c b/util/oslib-posix.c index 36820fec16..95e2b85279 100644 --- a/util/oslib-posix.c +++ b/util/oslib-posix.c @@ -229,8 +229,9 @@ void *qemu_memalign(size_t alignment, size_t size) /* alloc shared memory pages */ void *qemu_anon_ram_alloc(size_t size, uint64_t *alignment, bool shared) { + const uint32_t map_flags = shared ? MAP_SHARED : 0; size_t align = QEMU_VMALLOC_ALIGN; - void *ptr = qemu_ram_mmap(-1, size, align, false, shared, false, 0); + void *ptr = qemu_ram_mmap(-1, size, align, false, map_flags, 0); if (ptr == MAP_FAILED) { return NULL;
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 ... > > From 13a59d404bb3edaed9e42c94432be28fb9a65c26 Mon Sep 17 00:00:00 2001 > From: David Hildenbrand <david@redhat.com> > Date: Fri, 5 Mar 2021 17:20:37 +0100 > Subject: [PATCH] util/mmap-alloc: Pass MAP_ flags instead of separate bools to > qemu_ram_mmap() > > Let's pass MAP_ flags instead of bools to prepare for passing other MAP_ > flags and update the documentation of qemu_ram_mmap(). Only allow selected > MAP_ flags (MAP_SHARED, MAP_SYNC) to be passed and keep setting other > flags implicitly. > > Keep the "readonly" flag, as it cannot be expressed via MAP_ flags. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > include/qemu/mmap-alloc.h | 19 ++++++++++++++----- > softmmu/physmem.c | 6 ++++-- > util/mmap-alloc.c | 13 ++++++++----- > util/oslib-posix.c | 3 ++- > 4 files changed, 28 insertions(+), 13 deletions(-) > > diff --git a/include/qemu/mmap-alloc.h b/include/qemu/mmap-alloc.h > index 456ff87df1..27ef374810 100644 > --- a/include/qemu/mmap-alloc.h > +++ b/include/qemu/mmap-alloc.h > @@ -7,7 +7,10 @@ size_t qemu_fd_getpagesize(int fd); > size_t qemu_mempath_getpagesize(const char *mem_path); > > /** > - * qemu_ram_mmap: mmap the specified file or device. > + * qemu_ram_mmap: mmap anonymous memory, the specified file or device. > + * > + * QEMU's MMAP abstraction to map guest RAM, simplifying flag handling, > + * taking care of alignment requirements and installing guard pages. > * > * Parameters: > * @fd: the file or the device to mmap > @@ -15,10 +18,17 @@ size_t qemu_mempath_getpagesize(const char *mem_path); > * @align: if not zero, specify the alignment of the starting mapping address; > * otherwise, the alignment in use will be determined by QEMU. > * @readonly: true for a read-only mapping, false for read/write. > - * @shared: map has RAM_SHARED flag. > - * @is_pmem: map has RAM_PMEM flag. > + * @map_flags: supported MAP_* flags: MAP_SHARED, MAP_SYNC > * @map_offset: map starts at offset of map_offset from the start of fd > * > + * Implicitly handled map_flags: > + * - MAP PRIVATE: With !MAP_SHARED > + * - MAP_ANONYMOUS: With fd < 0 > + * - MAP_SHARED_VALIDATE: With MAP_SYNC && MAP_SHARED > + * > + * MAP_SYNC is ignored without MAP_SHARED. If mapping via MAP_SYNC fails, > + * warn and fallback to mapping without MAP_SYNC. > + * > * Return: > * On success, return a pointer to the mapped area. > * On failure, return MAP_FAILED. > @@ -27,8 +37,7 @@ void *qemu_ram_mmap(int fd, > size_t size, > size_t align, > bool readonly, > - bool shared, > - bool is_pmem, > + uint32_t map_flags, > off_t map_offset); > > void qemu_ram_munmap(int fd, void *ptr, size_t size); > diff --git a/softmmu/physmem.c b/softmmu/physmem.c > index 8f3d286e12..1336884b51 100644 > --- a/softmmu/physmem.c > +++ b/softmmu/physmem.c > @@ -1533,6 +1533,7 @@ static void *file_ram_alloc(RAMBlock *block, > off_t offset, > Error **errp) > { > + uint32_t map_flags; > void *area; > > block->page_size = qemu_fd_getpagesize(fd); > @@ -1580,9 +1581,10 @@ static void *file_ram_alloc(RAMBlock *block, > perror("ftruncate"); > } > > + map_flags = (block->flags & RAM_SHARED) ? MAP_SHARED : 0; > + map_flags |= (block->flags & RAM_PMEM) ? MAP_SYNC : 0; > area = qemu_ram_mmap(fd, memory, block->mr->align, readonly, > - block->flags & RAM_SHARED, block->flags & RAM_PMEM, > - offset); > + map_flags, offset); > if (area == MAP_FAILED) { > error_setg_errno(errp, errno, > "unable to map backing store for guest RAM"); > diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c > index 0e2bd7bc0e..b558f1675a 100644 > --- a/util/mmap-alloc.c > +++ b/util/mmap-alloc.c > @@ -119,16 +119,20 @@ static void *mmap_reserve(size_t size, int fd) > * it accessible. > */ > static void *mmap_activate(void *ptr, size_t size, int fd, bool readonly, > - bool shared, bool is_pmem, off_t map_offset) > + uint32_t map_flags, off_t map_offset) > { > + const bool shared = map_flags & MAP_SHARED; > + const bool sync = map_flags & MAP_SYNC; > const int prot = PROT_READ | (readonly ? 0 : PROT_WRITE); > int map_sync_flags = 0; > int flags = MAP_FIXED; > void *activated_ptr; > > + g_assert(!(map_flags & ~(MAP_SHARED | MAP_SYNC))); > + > flags |= fd == -1 ? MAP_ANONYMOUS : 0; > flags |= shared ? MAP_SHARED : MAP_PRIVATE; > - if (shared && is_pmem) { > + if (shared && sync) { > map_sync_flags = MAP_SYNC | MAP_SHARED_VALIDATE; > } > > @@ -174,8 +178,7 @@ void *qemu_ram_mmap(int fd, > size_t size, > size_t align, > bool readonly, > - bool shared, > - bool is_pmem, > + uint32_t map_flags, > off_t map_offset) > { > const size_t guard_pagesize = mmap_guard_pagesize(fd); > @@ -199,7 +202,7 @@ void *qemu_ram_mmap(int fd, > > offset = QEMU_ALIGN_UP((uintptr_t)guardptr, align) - (uintptr_t)guardptr; > > - ptr = mmap_activate(guardptr + offset, size, fd, readonly, shared, is_pmem, > + ptr = mmap_activate(guardptr + offset, size, fd, readonly, map_flags, > map_offset); > if (ptr == MAP_FAILED) { > munmap(guardptr, total); > diff --git a/util/oslib-posix.c b/util/oslib-posix.c > index 36820fec16..95e2b85279 100644 > --- a/util/oslib-posix.c > +++ b/util/oslib-posix.c > @@ -229,8 +229,9 @@ void *qemu_memalign(size_t alignment, size_t size) > /* alloc shared memory pages */ > void *qemu_anon_ram_alloc(size_t size, uint64_t *alignment, bool shared) > { > + const uint32_t map_flags = shared ? MAP_SHARED : 0; > size_t align = QEMU_VMALLOC_ALIGN; > - void *ptr = qemu_ram_mmap(-1, size, align, false, shared, false, 0); > + void *ptr = qemu_ram_mmap(-1, size, align, false, map_flags, 0); > > if (ptr == MAP_FAILED) { > return NULL; >
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,
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) + /** * qemu_ram_mmap: mmap the specified file or device. * @@ -14,9 +23,7 @@ size_t qemu_mempath_getpagesize(const char *mem_path); * @size: the number of bytes to be mmaped * @align: if not zero, specify the alignment of the starting mapping address; * otherwise, the alignment in use will be determined by QEMU. - * @readonly: true for a read-only mapping, false for read/write. - * @shared: map has RAM_SHARED flag. - * @is_pmem: map has RAM_PMEM flag. + * @mmap_flags: QEMU_RAM_MMAP_* flags * @map_offset: map starts at offset of map_offset from the start of fd * * Return: @@ -26,9 +33,7 @@ size_t qemu_mempath_getpagesize(const char *mem_path); void *qemu_ram_mmap(int fd, size_t size, size_t align, - bool readonly, - bool shared, - bool is_pmem, + uint32_t mmap_flags, off_t map_offset); void qemu_ram_munmap(int fd, void *ptr, size_t size); diff --git a/softmmu/physmem.c b/softmmu/physmem.c index 8f90cb4cd2..ec7a382ccd 100644 --- a/softmmu/physmem.c +++ b/softmmu/physmem.c @@ -1533,6 +1533,7 @@ static void *file_ram_alloc(RAMBlock *block, off_t offset, Error **errp) { + uint32_t mmap_flags; void *area; block->page_size = qemu_fd_getpagesize(fd); @@ -1580,9 +1581,10 @@ static void *file_ram_alloc(RAMBlock *block, perror("ftruncate"); } - area = qemu_ram_mmap(fd, memory, block->mr->align, readonly, - block->flags & RAM_SHARED, block->flags & RAM_PMEM, - offset); + mmap_flags = readonly ? QEMU_RAM_MMAP_READONLY : 0; + mmap_flags |= (block->flags & RAM_SHARED) ? QEMU_RAM_MMAP_SHARED : 0; + mmap_flags |= (block->flags & RAM_PMEM) ? QEMU_RAM_MMAP_PMEM : 0; + area = qemu_ram_mmap(fd, memory, block->mr->align, mmap_flags, offset); if (area == MAP_FAILED) { error_setg_errno(errp, errno, "unable to map backing store for guest RAM"); diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c index 0e2bd7bc0e..bd8f7ab547 100644 --- a/util/mmap-alloc.c +++ b/util/mmap-alloc.c @@ -118,9 +118,12 @@ static void *mmap_reserve(size_t size, int fd) * Activate memory in a reserved region from the given fd (if any), to make * it accessible. */ -static void *mmap_activate(void *ptr, size_t size, int fd, bool readonly, - bool shared, bool is_pmem, off_t map_offset) +static void *mmap_activate(void *ptr, size_t size, int fd, uint32_t mmap_flags, + off_t map_offset) { + const bool readonly = mmap_flags & QEMU_RAM_MMAP_READONLY; + const bool shared = mmap_flags & QEMU_RAM_MMAP_SHARED; + const bool is_pmem = mmap_flags & QEMU_RAM_MMAP_PMEM; const int prot = PROT_READ | (readonly ? 0 : PROT_WRITE); int map_sync_flags = 0; int flags = MAP_FIXED; @@ -173,9 +176,7 @@ static inline size_t mmap_guard_pagesize(int fd) void *qemu_ram_mmap(int fd, size_t size, size_t align, - bool readonly, - bool shared, - bool is_pmem, + uint32_t mmap_flags, off_t map_offset) { const size_t guard_pagesize = mmap_guard_pagesize(fd); @@ -199,8 +200,7 @@ void *qemu_ram_mmap(int fd, offset = QEMU_ALIGN_UP((uintptr_t)guardptr, align) - (uintptr_t)guardptr; - ptr = mmap_activate(guardptr + offset, size, fd, readonly, shared, is_pmem, - map_offset); + ptr = mmap_activate(guardptr + offset, size, fd, mmap_flags, map_offset); if (ptr == MAP_FAILED) { munmap(guardptr, total); return MAP_FAILED; diff --git a/util/oslib-posix.c b/util/oslib-posix.c index 36820fec16..1d250416f1 100644 --- a/util/oslib-posix.c +++ b/util/oslib-posix.c @@ -229,8 +229,9 @@ void *qemu_memalign(size_t alignment, size_t size) /* alloc shared memory pages */ void *qemu_anon_ram_alloc(size_t size, uint64_t *alignment, bool shared) { + const uint32_t mmap_flags = shared ? QEMU_RAM_MMAP_SHARED : 0; size_t align = QEMU_VMALLOC_ALIGN; - void *ptr = qemu_ram_mmap(-1, size, align, false, shared, false, 0); + void *ptr = qemu_ram_mmap(-1, size, align, mmap_flags, 0); if (ptr == MAP_FAILED) { return NULL;
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(-)