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

[Warner Losh <imp@bsdimp.com>]

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

On Sat, Mar 8, 2025 at 5:28 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().
>
> Add a comment in do_bsd_shmat() to clarify alignment above
> page size is not required.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  bsd-user/bsd-mem.h |  4 +++-
>  bsd-user/qemu.h    |  2 +-
>  bsd-user/mmap.c    | 10 ++--------
>  3 files changed, 6 insertions(+), 10 deletions(-)
>

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


> diff --git a/bsd-user/bsd-mem.h b/bsd-user/bsd-mem.h
> index f5ec0de24ca..90ca0e33775 100644
> --- a/bsd-user/bsd-mem.h
> +++ b/bsd-user/bsd-mem.h
> @@ -370,9 +370,11 @@ static inline abi_long do_bsd_shmat(int shmid,
> abi_ulong shmaddr, int shmflg)
>          if (shmaddr) {
>              host_raddr = shmat(shmid, (void *)g2h_untagged(shmaddr),
> shmflg);
>          } else {
> +            abi_ulong alignment;
>              abi_ulong mmap_start;
>
> -            mmap_start = mmap_find_vma(0, shm_info.shm_segsz);
> +            alignment = 0; /* alignment above page size not required */
> +            mmap_start = mmap_find_vma(0, shm_info.shm_segsz, alignment);
>

Why create a write-only variable that you just pass to this function? Is
this
a pattern used elsewhere and if so when?

It's fine, imho, like this, but it is a little odd so I wonder why you did
it like this.
There may be something I can learn.

Warner


>              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 dfa6e728ab5..3f0df79c375 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)
> @@ -494,7 +488,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len,
> int prot,
>          host_len = len + offset - host_offset;
>          host_len = HOST_PAGE_ALIGN(host_len);
>          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
>
>

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