Re: [Qemu-trivial] [Qemu-devel] [PATCH v3] linux-user: Remove extra mapping

[Laurent Vivier]

Le 29/05/2018 à 20:15, Steve Mcpolin a écrit :
> When a guest mmap()'d a file, an anonymous mapping was created to
> align different host and client page granularity and provide for
> beyond EOF mappings.  The file was then mapped ontop of this mapping,
> releasing the memory reserved by it.  A file based mmap does not reserve
> memory, but the anonymous map does; thus this anonymous mapping could
> could cause spurious failures.
> 
> This patch minimizes the size of the anonymous mapping to prevent spurious
> failures.
> 
> Signed-off-by: Steve Mcpolin <address@hidden>
> ---

v3:
  add missing flags in the second mmap()

v2:
  use REAL_HOST_PAGE_ALIGN(len) instead of HOST_PAGE_ALIGN(len)


>  linux-user/mmap.c | 32 ++++++++++++++++++++------------
>  1 file changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
> index 9168a20..7a975c8 100644
> --- a/linux-user/mmap.c
> +++ b/linux-user/mmap.c
> @@ -453,21 +453,29 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, 
> int prot,
>          /* Note: we prefer to control the mapping address. It is
>             especially important if qemu_host_page_size >
>             qemu_real_host_page_size */
> -        p = mmap(g2h(start), host_len, prot,
> -                 flags | MAP_FIXED | MAP_ANONYMOUS, -1, 0);
> -        if (p == MAP_FAILED)
> -            goto fail;
> -        /* update start so that it points to the file position at 'offset' */
> -        host_start = (unsigned long)p;
> -        if (!(flags & MAP_ANONYMOUS)) {
> -            p = mmap(g2h(start), len, prot,
> +        if (flags & MAP_ANONYMOUS) {
> +            offset = 0;
> +            host_offset = 0;
> +            fd = -1;
> +        }
> +        p = mmap(g2h(start), len, prot,
>                       flags | MAP_FIXED, fd, host_offset);
> -            if (p == MAP_FAILED) {
> -                munmap(g2h(start), host_len);
> -                goto fail;
> +        host_start = (uintptr_t)p;
> +        if (p != MAP_FAILED && host_len > REAL_HOST_PAGE_ALIGN(len)) {
> +            void *q;
> +            q = mmap(g2h(start + len), host_len - REAL_HOST_PAGE_ALIGN(len),
> +                     prot, MAP_FIXED | (flags & MAP_TYPE) | MAP_ANONYMOUS,

Why do you restrict the flags with MAP_TYPE? Original code was not doing
that. Is this needed?

Thanks,
Laurent