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

[Brüns , Stefan]

On Tuesday, September 01, 2015 15:29:26 you wrote:
> 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?

Taken from the old code. Probably not worth the extra code, will remove it.
 
> >              }
> > 
> > -            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.

MAX_ARG_PAGES is the name used in old kernels (and current, when there is
no MMU), so it is useful to keep this name (maybe in a comment).

What about:
---
/* Older linux kernels provide up to MAX_ARG_PAGES (default: 32) of
 * argument/environment space. Newer kernels (>2.6.33) provide up to
 * 1/4th of stack size, but guarantee at least 32 pages for backwards
 * compatibility. */
#define STACK_LOWER_LIMIT (32 * TARGET_PAGE_SIZE)
---

The 1/4th is not enforced, as our stack has to be big enough to hold args/env 
coming from the kernel, but is no problem if we provide more space.

Thinking about it, maybe we should provide some extra space, as qemu-linux-
user puts some more stuff (e.g. auxv) between env/args and user stack.
 
> > -    /* 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.)

Will add an explicit removal patch. info->mmap is not used either, ok to 
remove both in one go?

> > -            /* 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().

OK to do this in a followup patch?

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

Kind regards, Stefan

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019
work: +49 2405 49936-424