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

[Peter Maydell]

On 1 September 2015 at 18:27, Brüns, Stefan
<address@hidden> wrote:
> 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.

Oops, I thought I'd checked the diff to check it wasn't already
present, but I must have misread it.

>> > +    /* 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)
> ---

OK. (Incidentally I prefer the trailing "*/" of a multiline comment
on its own line.)

> 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.

That's OK, but then we should either (a) just not mention the 1/4
limit in the comment or (b) mention it and say we don't enforce
the limit in QEMU.

>> 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?

Yes.

>> > --- 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?

If you want to remove the dead assignment in its own patch
I would do that before this patch, rather than after.

thanks
-- PMM