On 1 September 2015 at 18:27, Brüns, Stefan
<stefan.bru...@rwth-aachen.de> wrote:
> On Tuesday, September 01, 2015 15:29:26 you wrote:
>> On 27 August 2015 at 20:35, Stefan Brüns <stefan.bru...@rwth-aachen.de>
> 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 <stefan.bru...@rwth-aachen.de>
>> > ---
>> >
>> > 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
