[PATCH v3 0/3] user: Extract common MMAP API to 'user/mmap.h'

[PATCH v3 0/3] user: Extract common MMAP API to 'user/mmap.h'

[Philippe Mathieu-Daudé]

Since v1:
- Propagate alignment argument to BSD's mmap_find_vma()

Philippe Mathieu-Daudé (3):
  bsd-user: Always use mmap_find_vma_aligned() in target_mmap()
  bsd-user: Propagate alignment argument to mmap_find_vma()
  user: Extract common MMAP API to 'user/mmap.h'

 bsd-user/bsd-mem.h     |  2 +-
 bsd-user/qemu.h        | 12 +-----------
 include/user/mmap.h    | 32 ++++++++++++++++++++++++++++++++
 linux-user/user-mmap.h | 19 ++-----------------
 bsd-user/mmap.c        | 19 +++++++------------
 5 files changed, 43 insertions(+), 41 deletions(-)
 create mode 100644 include/user/mmap.h

-- 
2.47.1

[PATCH v3 1/3] bsd-user: Always use mmap_find_vma_aligned() in target_mmap()

[Philippe Mathieu-Daudé]

Massage target_mmap(): calculate alignment once, then
unconditionally call mmap_find_vma_aligned().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 bsd-user/mmap.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/bsd-user/mmap.c b/bsd-user/mmap.c
index 346f2cefd32..28d7e387a20 100644
--- a/bsd-user/mmap.c
+++ b/bsd-user/mmap.c
@@ -489,13 +489,14 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
      * before we truncate the length for mapping files below.
      */
     if (!(flags & MAP_FIXED)) {
+        abi_ulong alignment = 0;
+
         host_len = len + offset - host_offset;
         host_len = HOST_PAGE_ALIGN(host_len);
-        if ((flags & MAP_ALIGNMENT_MASK) != 0)
-            start = mmap_find_vma_aligned(real_start, host_len,
-                (flags & MAP_ALIGNMENT_MASK) >> MAP_ALIGNMENT_SHIFT);
-        else
-            start = mmap_find_vma(real_start, host_len);
+        if ((flags & MAP_ALIGNMENT_MASK) != 0) {
+            alignment = (flags & MAP_ALIGNMENT_MASK) >> MAP_ALIGNMENT_SHIFT;
+        }
+        start = mmap_find_vma_aligned(real_start, host_len, alignment);
         if (start == (abi_ulong)-1) {
             errno = ENOMEM;
             goto fail;
-- 
2.47.1

Re: [PATCH v3 1/3] bsd-user: Always use mmap_find_vma_aligned() in target_mmap()

[Richard Henderson]

On 3/7/25 05:09, Philippe Mathieu-Daudé wrote:
> Massage target_mmap(): calculate alignment once, then
> unconditionally call mmap_find_vma_aligned().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   bsd-user/mmap.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/bsd-user/mmap.c b/bsd-user/mmap.c
> index 346f2cefd32..28d7e387a20 100644
> --- a/bsd-user/mmap.c
> +++ b/bsd-user/mmap.c
> @@ -489,13 +489,14 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
>        * before we truncate the length for mapping files below.
>        */
>       if (!(flags & MAP_FIXED)) {
> +        abi_ulong alignment = 0;
> +
>           host_len = len + offset - host_offset;
>           host_len = HOST_PAGE_ALIGN(host_len);
> -        if ((flags & MAP_ALIGNMENT_MASK) != 0)
> -            start = mmap_find_vma_aligned(real_start, host_len,
> -                (flags & MAP_ALIGNMENT_MASK) >> MAP_ALIGNMENT_SHIFT);
> -        else
> -            start = mmap_find_vma(real_start, host_len);
> +        if ((flags & MAP_ALIGNMENT_MASK) != 0) {
> +            alignment = (flags & MAP_ALIGNMENT_MASK) >> MAP_ALIGNMENT_SHIFT;
> +        }

No need for the if -- (0 & mask) >> shift == 0.

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v3 1/3] bsd-user: Always use mmap_find_vma_aligned() in target_mmap()

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 403 bytes --]

On Fri, Mar 7, 2025 at 6:09 AM Philippe Mathieu-Daudé <philmd@linaro.org>
wrote:

> Massage target_mmap(): calculate alignment once, then
> unconditionally call mmap_find_vma_aligned().
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  bsd-user/mmap.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>

Reviewed-by: Warner Losh <imp@bsdimp.com>

[-- Attachment #2: Type: text/html, Size: 876 bytes --]

[PATCH v3 2/3] bsd-user: Propagate alignment argument to mmap_find_vma()

[Philippe Mathieu-Daudé]

Propagate the alignment to mmap_find_vma(), effectively
embedding mmap_find_vma_aligned() within mmap_find_vma().

Since we ignore the alignment in do_bsd_shmat(), leave a
FIXME comment.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 bsd-user/bsd-mem.h |  2 +-
 bsd-user/qemu.h    |  2 +-
 bsd-user/mmap.c    | 10 ++--------
 3 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/bsd-user/bsd-mem.h b/bsd-user/bsd-mem.h
index f5ec0de24ca..87219da2919 100644
--- a/bsd-user/bsd-mem.h
+++ b/bsd-user/bsd-mem.h
@@ -372,7 +372,7 @@ static inline abi_long do_bsd_shmat(int shmid, abi_ulong shmaddr, int shmflg)
         } else {
             abi_ulong mmap_start;
 
-            mmap_start = mmap_find_vma(0, shm_info.shm_segsz);
+            mmap_start = mmap_find_vma(0, shm_info.shm_segsz, 0 /* FIXME??? */);
 
             if (mmap_start == -1) {
                 return -TARGET_ENOMEM;
diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
index 4e97c796318..0b3bd65b180 100644
--- a/bsd-user/qemu.h
+++ b/bsd-user/qemu.h
@@ -242,7 +242,7 @@ abi_long target_mremap(abi_ulong old_addr, abi_ulong old_size,
                        abi_ulong new_addr);
 int target_msync(abi_ulong start, abi_ulong len, int flags);
 extern abi_ulong mmap_next_start;
-abi_ulong mmap_find_vma(abi_ulong start, abi_ulong size);
+abi_ulong mmap_find_vma(abi_ulong start, abi_ulong size, abi_ulong alignment);
 void mmap_reserve(abi_ulong start, abi_ulong size);
 void TSA_NO_TSA mmap_fork_start(void);
 void TSA_NO_TSA mmap_fork_end(int child);
diff --git a/bsd-user/mmap.c b/bsd-user/mmap.c
index 28d7e387a20..da22fcc7c41 100644
--- a/bsd-user/mmap.c
+++ b/bsd-user/mmap.c
@@ -275,8 +275,7 @@ static abi_ulong mmap_find_vma_reserved(abi_ulong start, abi_ulong size,
  * It must be called with mmap_lock() held.
  * Return -1 if error.
  */
-static abi_ulong mmap_find_vma_aligned(abi_ulong start, abi_ulong size,
-                                       abi_ulong alignment)
+abi_ulong mmap_find_vma(abi_ulong start, abi_ulong size, abi_ulong alignment)
 {
     void *ptr, *prev;
     abi_ulong addr;
@@ -395,11 +394,6 @@ static abi_ulong mmap_find_vma_aligned(abi_ulong start, abi_ulong size,
     }
 }
 
-abi_ulong mmap_find_vma(abi_ulong start, abi_ulong size)
-{
-    return mmap_find_vma_aligned(start, size, 0);
-}
-
 /* NOTE: all the constants are the HOST ones */
 abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
                      int flags, int fd, off_t offset)
@@ -496,7 +490,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
         if ((flags & MAP_ALIGNMENT_MASK) != 0) {
             alignment = (flags & MAP_ALIGNMENT_MASK) >> MAP_ALIGNMENT_SHIFT;
         }
-        start = mmap_find_vma_aligned(real_start, host_len, alignment);
+        start = mmap_find_vma(real_start, host_len, alignment);
         if (start == (abi_ulong)-1) {
             errno = ENOMEM;
             goto fail;
-- 
2.47.1

Re: [PATCH v3 2/3] bsd-user: Propagate alignment argument to mmap_find_vma()

[Richard Henderson]

On 3/7/25 05:09, Philippe Mathieu-Daudé wrote:
> Propagate the alignment to mmap_find_vma(), effectively
> embedding mmap_find_vma_aligned() within mmap_find_vma().
> 
> Since we ignore the alignment in do_bsd_shmat(), leave a
> FIXME comment.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   bsd-user/bsd-mem.h |  2 +-
>   bsd-user/qemu.h    |  2 +-
>   bsd-user/mmap.c    | 10 ++--------
>   3 files changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/bsd-user/bsd-mem.h b/bsd-user/bsd-mem.h
> index f5ec0de24ca..87219da2919 100644
> --- a/bsd-user/bsd-mem.h
> +++ b/bsd-user/bsd-mem.h
> @@ -372,7 +372,7 @@ static inline abi_long do_bsd_shmat(int shmid, abi_ulong shmaddr, int shmflg)
>           } else {
>               abi_ulong mmap_start;
>   
> -            mmap_start = mmap_find_vma(0, shm_info.shm_segsz);
> +            mmap_start = mmap_find_vma(0, shm_info.shm_segsz, 0 /* FIXME??? */);

It's not really ignoring the alignment, but not requiring alignment above page size.

Traditionally, the alignment for shmat should be SHMLBA.
But in current freebsd sources,

sys/sys/shm.h:#define SHMLBA      PAGE_SIZE /* Segment low boundary address multiple */

there are no crazy broken old architectures to worry about.


r~

Re: [PATCH v3 2/3] bsd-user: Propagate alignment argument to mmap_find_vma()

[Philippe Mathieu-Daudé]

On 7/3/25 17:08, Richard Henderson wrote:
> On 3/7/25 05:09, Philippe Mathieu-Daudé wrote:
>> Propagate the alignment to mmap_find_vma(), effectively
>> embedding mmap_find_vma_aligned() within mmap_find_vma().
>>
>> Since we ignore the alignment in do_bsd_shmat(), leave a
>> FIXME comment.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   bsd-user/bsd-mem.h |  2 +-
>>   bsd-user/qemu.h    |  2 +-
>>   bsd-user/mmap.c    | 10 ++--------
>>   3 files changed, 4 insertions(+), 10 deletions(-)
>>
>> diff --git a/bsd-user/bsd-mem.h b/bsd-user/bsd-mem.h
>> index f5ec0de24ca..87219da2919 100644
>> --- a/bsd-user/bsd-mem.h
>> +++ b/bsd-user/bsd-mem.h
>> @@ -372,7 +372,7 @@ static inline abi_long do_bsd_shmat(int shmid, 
>> abi_ulong shmaddr, int shmflg)
>>           } else {
>>               abi_ulong mmap_start;
>> -            mmap_start = mmap_find_vma(0, shm_info.shm_segsz);
>> +            mmap_start = mmap_find_vma(0, shm_info.shm_segsz, 0 /* 
>> FIXME??? */);
> 
> It's not really ignoring the alignment, but not requiring alignment 
> above page size.
> 
> Traditionally, the alignment for shmat should be SHMLBA.
> But in current freebsd sources,
> 
> sys/sys/shm.h:#define SHMLBA      PAGE_SIZE /* Segment low boundary 
> address multiple */
> 
> there are no crazy broken old architectures to worry about.

OK, thank you for checking!

Re: [PATCH v3 2/3] bsd-user: Propagate alignment argument to mmap_find_vma()

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 564 bytes --]

On Fri, Mar 7, 2025 at 6:10 AM Philippe Mathieu-Daudé <philmd@linaro.org>
wrote:

> Propagate the alignment to mmap_find_vma(), effectively
> embedding mmap_find_vma_aligned() within mmap_find_vma().
>
> Since we ignore the alignment in do_bsd_shmat(), leave a
> FIXME comment.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  bsd-user/bsd-mem.h |  2 +-
>  bsd-user/qemu.h    |  2 +-
>  bsd-user/mmap.c    | 10 ++--------
>  3 files changed, 4 insertions(+), 10 deletions(-)
>

Reviewed-by: Warner Losh <imp@bsdimp.com>

[-- Attachment #2: Type: text/html, Size: 1044 bytes --]

[PATCH v3 3/3] user: Extract common MMAP API to 'user/mmap.h'

[Philippe Mathieu-Daudé]

Keep common MMAP-related declarations in a single place.

Note, this disable ThreadSafetyAnalysis on Linux for:
- mmap_fork_start()
- mmap_fork_end().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Warner Losh <imp@bsdimp.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 bsd-user/qemu.h        | 12 +-----------
 include/user/mmap.h    | 32 ++++++++++++++++++++++++++++++++
 linux-user/user-mmap.h | 19 ++-----------------
 3 files changed, 35 insertions(+), 28 deletions(-)
 create mode 100644 include/user/mmap.h

diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
index 0b3bd65b180..c1c508281a8 100644
--- a/bsd-user/qemu.h
+++ b/bsd-user/qemu.h
@@ -32,6 +32,7 @@
 extern char **environ;
 
 #include "user/thunk.h"
+#include "user/mmap.h"
 #include "target_arch.h"
 #include "syscall_defs.h"
 #include "target_syscall.h"
@@ -233,19 +234,8 @@ void print_taken_signal(int target_signum, const target_siginfo_t *tinfo);
 extern int do_strace;
 
 /* mmap.c */
-int target_mprotect(abi_ulong start, abi_ulong len, int prot);
-abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
-                     int flags, int fd, off_t offset);
-int target_munmap(abi_ulong start, abi_ulong len);
-abi_long target_mremap(abi_ulong old_addr, abi_ulong old_size,
-                       abi_ulong new_size, unsigned long flags,
-                       abi_ulong new_addr);
 int target_msync(abi_ulong start, abi_ulong len, int flags);
-extern abi_ulong mmap_next_start;
-abi_ulong mmap_find_vma(abi_ulong start, abi_ulong size, abi_ulong alignment);
 void mmap_reserve(abi_ulong start, abi_ulong size);
-void TSA_NO_TSA mmap_fork_start(void);
-void TSA_NO_TSA mmap_fork_end(int child);
 
 /* main.c */
 extern char qemu_proc_pathname[];
diff --git a/include/user/mmap.h b/include/user/mmap.h
new file mode 100644
index 00000000000..4d5e9aac70a
--- /dev/null
+++ b/include/user/mmap.h
@@ -0,0 +1,32 @@
+/*
+ * MMAP declarations for QEMU user emulation
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#ifndef USER_MMAP_H
+#define USER_MMAP_H
+
+#include "user/abitypes.h"
+
+/*
+ * mmap_next_start: The base address for the next mmap without hint,
+ * increased after each successful map, starting at task_unmapped_base.
+ * This is an optimization within QEMU and not part of ADDR_COMPAT_LAYOUT.
+ */
+extern abi_ulong mmap_next_start;
+
+int target_mprotect(abi_ulong start, abi_ulong len, int prot);
+
+abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
+                     int flags, int fd, off_t offset);
+int target_munmap(abi_ulong start, abi_ulong len);
+abi_long target_mremap(abi_ulong old_addr, abi_ulong old_size,
+                       abi_ulong new_size, unsigned long flags,
+                       abi_ulong new_addr);
+
+abi_ulong mmap_find_vma(abi_ulong start, abi_ulong size, abi_ulong alignment);
+
+void TSA_NO_TSA mmap_fork_start(void);
+void TSA_NO_TSA mmap_fork_end(int child);
+
+#endif
diff --git a/linux-user/user-mmap.h b/linux-user/user-mmap.h
index b94bcdcf83c..dfc4477a720 100644
--- a/linux-user/user-mmap.h
+++ b/linux-user/user-mmap.h
@@ -18,6 +18,8 @@
 #ifndef LINUX_USER_USER_MMAP_H
 #define LINUX_USER_USER_MMAP_H
 
+#include "user/mmap.h"
+
 /*
  * Guest parameters for the ADDR_COMPAT_LAYOUT personality
  * (at present this is the only layout supported by QEMU).
@@ -39,24 +41,7 @@
 extern abi_ulong task_unmapped_base;
 extern abi_ulong elf_et_dyn_base;
 
-/*
- * mmap_next_start: The base address for the next mmap without hint,
- * increased after each successful map, starting at task_unmapped_base.
- * This is an optimization within QEMU and not part of ADDR_COMPAT_LAYOUT.
- */
-extern abi_ulong mmap_next_start;
-
-int target_mprotect(abi_ulong start, abi_ulong len, int prot);
-abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
-                     int flags, int fd, off_t offset);
-int target_munmap(abi_ulong start, abi_ulong len);
-abi_long target_mremap(abi_ulong old_addr, abi_ulong old_size,
-                       abi_ulong new_size, unsigned long flags,
-                       abi_ulong new_addr);
 abi_long target_madvise(abi_ulong start, abi_ulong len_in, int advice);
-abi_ulong mmap_find_vma(abi_ulong, abi_ulong, abi_ulong);
-void mmap_fork_start(void);
-void mmap_fork_end(int child);
 
 abi_ulong target_shmat(CPUArchState *cpu_env, int shmid,
                        abi_ulong shmaddr, int shmflg);
-- 
2.47.1