Re: [Qemu-devel] [PATCH] linux-user: remove MAX_ARG_PAGES limit

[Peter Maydell]

On 27 August 2015 at 20:35, Stefan Brüns <address@hidden> wrote:
> Instead of creating a temporary copy for the whole environment and
> the arguments, directly copy everything to the target stack.

> For this to work, we have to change the order of stack creation and
> copying the arguments.
>
> v2: fixed scratch pointer type, fixed checkpatch issues.

This sort of 'v1 to v2 diffs' comment should go below the '---'
line, so it doesn't end up in the final git commit message.

>
> Signed-off-by: Stefan Brüns <address@hidden>
> ---
>  linux-user/elfload.c   | 110 
> ++++++++++++++++++++++++-------------------------
>  linux-user/linuxload.c |   7 +---
>  linux-user/qemu.h      |   7 ----
>  linux-user/syscall.c   |   6 ---
>  4 files changed, 56 insertions(+), 74 deletions(-)

Still doesn't compile:

/home/petmay01/linaro/qemu-from-laptop/qemu/linux-user/linuxload.c: In
function ‘loader_exec’:
/home/petmay01/linaro/qemu-from-laptop/qemu/linux-user/linuxload.c:138:9:
error: unused variable ‘i’ [-Werror=unused-variable]
     int i;
         ^

Mostly this looks good; I have a few review comments below.

> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 9c999ac..991dd35 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c

> +            int bytes_to_copy = (len > offset) ? offset : len;
> +            tmp -= bytes_to_copy;
> +            p -= bytes_to_copy;
> +            offset -= bytes_to_copy;
> +            len -= bytes_to_copy;
> +
> +            if (bytes_to_copy == 1) {
> +                *(scratch + offset) = *tmp;
> +            } else {
> +                memcpy_fromfs(scratch + offset, tmp, bytes_to_copy);

Why bother to special case the 1 byte case?

>              }
> -            else {
> -                int bytes_to_copy = (len > offset) ? offset : len;
> -                tmp -= bytes_to_copy;
> -                p -= bytes_to_copy;
> -                offset -= bytes_to_copy;
> -                len -= bytes_to_copy;
> -                memcpy_fromfs(pag + offset, tmp, bytes_to_copy + 1);
> +
> +            if (offset == 0) {
> +                memcpy_to_target(p, scratch, top - p);
> +                top = p;
> +                offset = TARGET_PAGE_SIZE;
>              }
>          }
>      }
> +    if (offset) {
> +        memcpy_to_target(p, scratch + offset, top - p);
> +    }
> +
>      return p;
>  }
>
> -static abi_ulong setup_arg_pages(abi_ulong p, struct linux_binprm *bprm,
> +static abi_ulong setup_arg_pages(struct linux_binprm *bprm,
>                                   struct image_info *info)
>  {
> -    abi_ulong stack_base, size, error, guard;
> -    int i;
> +    abi_ulong size, error, guard;
> +
> +    /* Linux kernel limits argument/environment space to 1/4th of stack size,
> +       but also has a floor of 32 pages. Mimic this behaviour.  */
> +    #define MAX_ARG_PAGES 32

This sounds like a minimum, not a maximum, so the name is very
misleading.

The comment says "limit to 1/4 of stack size" but the code doesn't
seem to do anything like that.

Please move the #define to top-level in the file, not hidden
inside a function definition.

>
> -    /* Create enough stack to hold everything.  If we don't use
> -       it for args, we'll use it for something else.  */
>      size = guest_stack_size;
> -    if (size < MAX_ARG_PAGES*TARGET_PAGE_SIZE) {
> -        size = MAX_ARG_PAGES*TARGET_PAGE_SIZE;
> +    if (size < MAX_ARG_PAGES * TARGET_PAGE_SIZE) {
> +        size = MAX_ARG_PAGES * TARGET_PAGE_SIZE;
>      }
>      guard = TARGET_PAGE_SIZE;
>      if (guard < qemu_real_host_page_size) {
> @@ -1442,19 +1446,8 @@ static abi_ulong setup_arg_pages(abi_ulong p, struct 
> linux_binprm *bprm,
>      target_mprotect(error, guard, PROT_NONE);
>
>      info->stack_limit = error + guard;
> -    stack_base = info->stack_limit + size - MAX_ARG_PAGES*TARGET_PAGE_SIZE;
> -    p += stack_base;
> -
> -    for (i = 0 ; i < MAX_ARG_PAGES ; i++) {
> -        if (bprm->page[i]) {
> -            info->rss++;

I think your patch has lost the calculation of info->rss.

(We don't actually use info->rss anywhere, though, so if you wanted
to add a patch in front of this one explicitly removing it as useless
that would be an OK way to handle this.)

> -            /* FIXME - check return value of memcpy_to_target() for failure 
> */
> -            memcpy_to_target(stack_base, bprm->page[i], TARGET_PAGE_SIZE);
> -            g_free(bprm->page[i]);
> -        }
> -        stack_base += TARGET_PAGE_SIZE;
> -    }
> -    return p;
> +
> +    return info->stack_limit + size - sizeof(void *);
>  }
>
>  /* Map and zero the bss.  We need to explicitly zero any fractional pages

> diff --git a/linux-user/linuxload.c b/linux-user/linuxload.c
> index 506e837..09df934 100644
> --- a/linux-user/linuxload.c
> +++ b/linux-user/linuxload.c
> @@ -137,8 +137,7 @@ int loader_exec(int fdexec, const char *filename, char 
> **argv, char **envp,
>      int retval;
>      int i;
>
> -    bprm->p = TARGET_PAGE_SIZE*MAX_ARG_PAGES-sizeof(unsigned int);
> -    memset(bprm->page, 0, sizeof(bprm->page));
> +    bprm->p = 0;

Nothing actually uses this value -- both the elfload and the flatload code
paths now either ignore bprm->p or set it themselves. It would be
better to delete this and also the dead assignment "p = bprm->p" at
the top of load_flt_binary().

>      bprm->fd = fdexec;
>      bprm->filename = (char *)filename;
>      bprm->argc = count(argv);

thanks
-- PMM