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