Re: [PATCH for-8.1 v10 01/14] linux-user: Adjust task_unmapped_base for reserved_va

["Alex Bennée" <alex.bennee@linaro.org>]

Richard Henderson <richard.henderson@linaro.org> writes:

> Ensure that the chosen values for mmap_next_start and
> task_unmapped_base are within the guest address space.
>
> Tested-by: Helge Deller <deller@gmx.de>
> Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/user-mmap.h | 18 +++++++++++++++++-
>  linux-user/main.c      | 28 ++++++++++++++++++++++++++++
>  linux-user/mmap.c      | 18 +++---------------
>  3 files changed, 48 insertions(+), 16 deletions(-)
>
> diff --git a/linux-user/user-mmap.h b/linux-user/user-mmap.h
> index 7265c2c116..fd456e024e 100644
> --- a/linux-user/user-mmap.h
> +++ b/linux-user/user-mmap.h
> @@ -18,6 +18,23 @@
>  #ifndef LINUX_USER_USER_MMAP_H
>  #define LINUX_USER_USER_MMAP_H
>  
> +#if HOST_LONG_BITS == 64 && TARGET_ABI_BITS == 64
> +#ifdef TARGET_AARCH64
> +# define TASK_UNMAPPED_BASE  0x5500000000
> +#else
> +# define TASK_UNMAPPED_BASE  (1ul << 38)
> +#endif
> +#else
> +#ifdef TARGET_HPPA
> +# define TASK_UNMAPPED_BASE  0xfa000000
> +#else
> +# define TASK_UNMAPPED_BASE  0x40000000
> +#endif
> +#endif
> +
> +extern abi_ulong task_unmapped_base;
> +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);
> @@ -26,7 +43,6 @@ 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);
> -extern abi_ulong mmap_next_start;
>  abi_ulong mmap_find_vma(abi_ulong, abi_ulong, abi_ulong);
>  void mmap_fork_start(void);
>  void mmap_fork_end(int child);
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 556956c363..be621dc792 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -821,6 +821,34 @@ int main(int argc, char **argv, char **envp)
>          reserved_va = max_reserved_va;
>      }
>  
> +    /*
> +     * Temporarily disable
> +     *   "comparison is always false due to limited range of data type"
> +     * due to comparison between (possible) uint64_t and uintptr_t.
> +     */
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wtype-limits"
> +
> +    /*
> +     * Select an initial value for task_unmapped_base that is in range.
> +     */
> +    if (reserved_va) {
> +        if (TASK_UNMAPPED_BASE < reserved_va) {
> +            task_unmapped_base = TASK_UNMAPPED_BASE;
> +        } else {
> +            /* The most common default formula is TASK_SIZE / 3. */
> +            task_unmapped_base = TARGET_PAGE_ALIGN(reserved_va / 3);
> +        }
> +    } else if (TASK_UNMAPPED_BASE < UINTPTR_MAX) {
> +        task_unmapped_base = TASK_UNMAPPED_BASE;
> +    } else {
> +        /* 32-bit host: pick something medium size. */
> +        task_unmapped_base = 0x10000000;
> +    }
> +    mmap_next_start = task_unmapped_base;
> +
> +#pragma GCC diagnostic pop
> +
>      {
>          Error *err = NULL;
>          if (seed_optarg != NULL) {
> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
> index eb04fab8ab..84436d45c8 100644
> --- a/linux-user/mmap.c
> +++ b/linux-user/mmap.c
> @@ -299,20 +299,8 @@ static bool mmap_frag(abi_ulong real_start, abi_ulong start, abi_ulong last,
>      return true;
>  }
>  
> -#if HOST_LONG_BITS == 64 && TARGET_ABI_BITS == 64
> -#ifdef TARGET_AARCH64
> -# define TASK_UNMAPPED_BASE  0x5500000000
> -#else
> -# define TASK_UNMAPPED_BASE  (1ul << 38)
> -#endif
> -#else
> -#ifdef TARGET_HPPA
> -# define TASK_UNMAPPED_BASE  0xfa000000
> -#else
> -# define TASK_UNMAPPED_BASE  0x40000000
> -#endif
> -#endif
> -abi_ulong mmap_next_start = TASK_UNMAPPED_BASE;
> +abi_ulong task_unmapped_base;
> +abi_ulong mmap_next_start;

I feel we could help ourselves a bit more by documenting these globals
and what they mean:

  task_unmapped_base represents the start of unmapped memory in the
  guests programs address space. It is generally a function of the size
  of the address space and it defined at the start of execution.
  mmap_next_start is the base address for the next anonymous mmap and is
  increased after each successful map, starting at task_unmapped_base.

One thing I'm slightly confused by is the ELF_ET_DYN_BASE can be above
this (or sometimes the same). Should the mapping of ELF segments be
handled with mmap_next_start? I assume once mmap_next_start meets the
mappings for the ELF segments we skip over until we get to more free
space after the program code?

>  
>  /*
>   * Subroutine of mmap_find_vma, used when we have pre-allocated
> @@ -391,7 +379,7 @@ abi_ulong mmap_find_vma(abi_ulong start, abi_ulong size, abi_ulong align)
>  
>              if ((addr & (align - 1)) == 0) {
>                  /* Success.  */
> -                if (start == mmap_next_start && addr >= TASK_UNMAPPED_BASE) {
> +                if (start == mmap_next_start && addr >= task_unmapped_base) {
>                      mmap_next_start = addr + size;
>                  }
>                  return addr;


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro