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

[David Hildenbrand <david@redhat.com>]

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.

-- 
Thanks,

David / dhildenb