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.