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