Patchwork [1/2] linux-user: Factor out guest space probing into a function

login
register
mail settings
Submitter Meador Inge
Date July 10, 2012, 3:57 p.m.
Message ID <1341935833-2655-2-git-send-email-meadori@codesourcery.com>
Download mbox | patch
Permalink /patch/170226/
State New
Headers show

Comments

Meador Inge - July 10, 2012, 3:57 p.m.
Signed-off-by: Meador Inge <meadori@codesourcery.com>
---
 linux-user/elfload.c |  111 +++++++++++++++++++++++++++++++++++---------------
 linux-user/qemu.h    |   11 +++++
 2 files changed, 89 insertions(+), 33 deletions(-)
Peter Maydell - July 10, 2012, 4:12 p.m.
On 10 July 2012 16:57, Meador Inge <meadori@codesourcery.com> wrote:
> Signed-off-by: Meador Inge <meadori@codesourcery.com>
> ---
>  linux-user/elfload.c |  111 +++++++++++++++++++++++++++++++++++---------------
>  linux-user/qemu.h    |   11 +++++
>  2 files changed, 89 insertions(+), 33 deletions(-)
>
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index f3b1552..44b4bdb 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -1385,6 +1385,74 @@ bool guest_validate_base(unsigned long guest_base)
>  }
>  #endif
>
> +unsigned long init_guest_space(unsigned long host_start,
> +                               unsigned long host_size,
> +                               bool fixed)
> +{
> +    unsigned long current_start, real_start;
> +    int flags;
> +
> +    /* Nothing to do here, move along.  */
> +    if (!host_start && !host_size) {
> +        return 0;
> +    }

This is a check that wasn't in the pre-refactoring code. Is it actually
a possible case, or should we be asserting() (perhaps checking for
bad ELF files and printing a suitable error message earlier)?

> +
> +    /* If just a starting address is given, then just verify that
> +     * address.  */
> +    if (host_start && !host_size) {
> +        if (guest_validate_base(host_start)) {
> +            return host_start;
> +        } else {
> +            return (unsigned long)-1;
> +        }
> +    }
> +
> +    /* Setup the initial flags and start address.  */
> +    current_start = host_start & qemu_host_page_mask;
> +    flags = MAP_ANONYMOUS | MAP_PRIVATE | MAP_NORESERVE;
> +    if (fixed) {
> +        flags |= MAP_FIXED;
> +    }
> +
> +    /* Otherwise, a non-zero size region of memory needs to be mapped
> +     * and validated.  */
> +    while (1) {
> +        /* Do not use mmap_find_vma here because that is limited to the
> +         * guest address space.  We are going to make the
> +         * guest address space fit whatever we're given.
> +         */
> +        real_start = (unsigned long)
> +            mmap((void *)current_start, host_size, PROT_NONE, flags, -1, 0);
> +        if (real_start == (unsigned long)-1) {
> +            return (unsigned long)-1;
> +        }
> +
> +        if ((real_start == current_start) && guest_validate_base(real_start)) {

This doesn't look like it's going to be calling guest_validate_base()
on the same value as the pre-refactoring code: before this commit
we called g_v_b() on real_start - loaddr.

> +            break;
> +        }
> +
> +        /* That address didn't work.  Unmap and try a different one.
> +         * The address the host picked because is typically right at
> +         * the top of the host address space and leaves the guest with
> +         * no usable address space.  Resort to a linear search.  We
> +         * already compensated for mmap_min_addr, so this should not
> +         * happen often.  Probably means we got unlucky and host
> +         * address space randomization put a shared library somewhere
> +         * inconvenient.
> +         */
> +        munmap((void *)real_start, host_size);
> +        current_start += qemu_host_page_size;
> +        if (host_start == current_start) {
> +            /* Theoretically possible if host doesn't have any suitably
> +             * aligned areas.  Normally the first mmap will fail.
> +             */
> +            return (unsigned long)-1;
> +        }
> +    }
> +
> +    return real_start;
> +}
> +
>  static void probe_guest_base(const char *image_name,
>                               abi_ulong loaddr, abi_ulong hiaddr)
>  {
> @@ -1411,46 +1479,23 @@ static void probe_guest_base(const char *image_name,
>              }
>          }
>          host_size = hiaddr - loaddr;
> -        while (1) {
> -            /* Do not use mmap_find_vma here because that is limited to the
> -               guest address space.  We are going to make the
> -               guest address space fit whatever we're given.  */
> -            real_start = (unsigned long)
> -                mmap((void *)host_start, host_size, PROT_NONE,
> -                     MAP_ANONYMOUS | MAP_PRIVATE | MAP_NORESERVE, -1, 0);
> -            if (real_start == (unsigned long)-1) {
> -                goto exit_perror;
> -            }
> -            guest_base = real_start - loaddr;
> -            if ((real_start == host_start) &&
> -                guest_validate_base(guest_base)) {
> -                break;
> -            }
> -            /* That address didn't work.  Unmap and try a different one.
> -               The address the host picked because is typically right at
> -               the top of the host address space and leaves the guest with
> -               no usable address space.  Resort to a linear search.  We
> -               already compensated for mmap_min_addr, so this should not
> -               happen often.  Probably means we got unlucky and host
> -               address space randomization put a shared library somewhere
> -               inconvenient.  */
> -            munmap((void *)real_start, host_size);
> -            host_start += qemu_host_page_size;
> -            if (host_start == loaddr) {
> -                /* Theoretically possible if host doesn't have any suitably
> -                   aligned areas.  Normally the first mmap will fail.  */
> -                errmsg = "Unable to find space for application";
> -                goto exit_errmsg;
> -            }
> +
> +        /* Setup the initial guest memory space with ranges gleaned from
> +         * the ELF image that is being loaded.
> +         */
> +        real_start = init_guest_space(host_start, host_size, 0);

If we're declaring the 'fixed' argument as 'bool' we should be passing
'false' rather than '0' here.

> +        if (real_start == (unsigned long)-1) {
> +            errmsg = "Unable to find space for application";
> +            goto exit_errmsg;
>          }
> +        guest_base = real_start - loaddr;
> +
>          qemu_log("Relocating guest address space from 0x"
>                   TARGET_ABI_FMT_lx " to 0x%lx\n",
>                   loaddr, real_start);
>      }
>      return;
>
> -exit_perror:
> -    errmsg = strerror(errno);
>  exit_errmsg:
>      fprintf(stderr, "%s: %s\n", image_name, errmsg);
>      exit(-1);
> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index 7b299b7..c23dd8a 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -210,6 +210,17 @@ void fork_end(int child);
>   */
>  bool guest_validate_base(unsigned long guest_base);
>
> +/* Creates the initial guest address space in the host memory space using the
> + * given host start address hint and size.  If fixed is specified, then the
> + * mapped address space must start at host_start.  If host_start and host_size
> + * are both zero then nothing is done and zero is returned. Otherwise, the
> + * guest address space is initialized and the real start address of the mapped
> + * memory space is returned or -1 if there is was error.

"was an error".

> + */
> +unsigned long init_guest_space(unsigned long host_start,
> +                               unsigned long host_size,
> +                               bool fixed);
> +
>  #include "qemu-log.h"
>
>  /* strace.c */
> --
> 1.7.7.6
>
Meador Inge - July 25, 2012, 10:26 p.m.
On 07/10/2012 11:12 AM, Peter Maydell wrote:

> On 10 July 2012 16:57, Meador Inge <meadori@codesourcery.com> wrote:
>> Signed-off-by: Meador Inge <meadori@codesourcery.com>
>> ---
>>  linux-user/elfload.c |  111 +++++++++++++++++++++++++++++++++++---------------
>>  linux-user/qemu.h    |   11 +++++
>>  2 files changed, 89 insertions(+), 33 deletions(-)
>>
>> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
>> index f3b1552..44b4bdb 100644
>> --- a/linux-user/elfload.c
>> +++ b/linux-user/elfload.c
>> @@ -1385,6 +1385,74 @@ bool guest_validate_base(unsigned long guest_base)
>>  }
>>  #endif
>>
>> +unsigned long init_guest_space(unsigned long host_start,
>> +                               unsigned long host_size,
>> +                               bool fixed)
>> +{
>> +    unsigned long current_start, real_start;
>> +    int flags;
>> +
>> +    /* Nothing to do here, move along.  */
>> +    if (!host_start && !host_size) {
>> +        return 0;
>> +    }
> 
> This is a check that wasn't in the pre-refactoring code. Is it actually
> a possible case, or should we be asserting() (perhaps checking for
> bad ELF files and printing a suitable error message earlier)?

Yeah, that shouldn't happen.  An assert should be sufficient.

>> +
>> +    /* If just a starting address is given, then just verify that
>> +     * address.  */
>> +    if (host_start && !host_size) {
>> +        if (guest_validate_base(host_start)) {
>> +            return host_start;
>> +        } else {
>> +            return (unsigned long)-1;
>> +        }
>> +    }
>> +
>> +    /* Setup the initial flags and start address.  */
>> +    current_start = host_start & qemu_host_page_mask;
>> +    flags = MAP_ANONYMOUS | MAP_PRIVATE | MAP_NORESERVE;
>> +    if (fixed) {
>> +        flags |= MAP_FIXED;
>> +    }
>> +
>> +    /* Otherwise, a non-zero size region of memory needs to be mapped
>> +     * and validated.  */
>> +    while (1) {
>> +        /* Do not use mmap_find_vma here because that is limited to the
>> +         * guest address space.  We are going to make the
>> +         * guest address space fit whatever we're given.
>> +         */
>> +        real_start = (unsigned long)
>> +            mmap((void *)current_start, host_size, PROT_NONE, flags, -1, 0);
>> +        if (real_start == (unsigned long)-1) {
>> +            return (unsigned long)-1;
>> +        }
>> +
>> +        if ((real_start == current_start) && guest_validate_base(real_start)) {
> 
> This doesn't look like it's going to be calling guest_validate_base()
> on the same value as the pre-refactoring code: before this commit
> we called g_v_b() on real_start - loaddr.

Gah, fixed.  Thanks for finding that.

>> +            break;
>> +        }
>> +
>> +        /* That address didn't work.  Unmap and try a different one.
>> +         * The address the host picked because is typically right at
>> +         * the top of the host address space and leaves the guest with
>> +         * no usable address space.  Resort to a linear search.  We
>> +         * already compensated for mmap_min_addr, so this should not
>> +         * happen often.  Probably means we got unlucky and host
>> +         * address space randomization put a shared library somewhere
>> +         * inconvenient.
>> +         */
>> +        munmap((void *)real_start, host_size);
>> +        current_start += qemu_host_page_size;
>> +        if (host_start == current_start) {
>> +            /* Theoretically possible if host doesn't have any suitably
>> +             * aligned areas.  Normally the first mmap will fail.
>> +             */
>> +            return (unsigned long)-1;
>> +        }
>> +    }
>> +
>> +    return real_start;
>> +}
>> +
>>  static void probe_guest_base(const char *image_name,
>>                               abi_ulong loaddr, abi_ulong hiaddr)
>>  {
>> @@ -1411,46 +1479,23 @@ static void probe_guest_base(const char *image_name,
>>              }
>>          }
>>          host_size = hiaddr - loaddr;
>> -        while (1) {
>> -            /* Do not use mmap_find_vma here because that is limited to the
>> -               guest address space.  We are going to make the
>> -               guest address space fit whatever we're given.  */
>> -            real_start = (unsigned long)
>> -                mmap((void *)host_start, host_size, PROT_NONE,
>> -                     MAP_ANONYMOUS | MAP_PRIVATE | MAP_NORESERVE, -1, 0);
>> -            if (real_start == (unsigned long)-1) {
>> -                goto exit_perror;
>> -            }
>> -            guest_base = real_start - loaddr;
>> -            if ((real_start == host_start) &&
>> -                guest_validate_base(guest_base)) {
>> -                break;
>> -            }
>> -            /* That address didn't work.  Unmap and try a different one.
>> -               The address the host picked because is typically right at
>> -               the top of the host address space and leaves the guest with
>> -               no usable address space.  Resort to a linear search.  We
>> -               already compensated for mmap_min_addr, so this should not
>> -               happen often.  Probably means we got unlucky and host
>> -               address space randomization put a shared library somewhere
>> -               inconvenient.  */
>> -            munmap((void *)real_start, host_size);
>> -            host_start += qemu_host_page_size;
>> -            if (host_start == loaddr) {
>> -                /* Theoretically possible if host doesn't have any suitably
>> -                   aligned areas.  Normally the first mmap will fail.  */
>> -                errmsg = "Unable to find space for application";
>> -                goto exit_errmsg;
>> -            }
>> +
>> +        /* Setup the initial guest memory space with ranges gleaned from
>> +         * the ELF image that is being loaded.
>> +         */
>> +        real_start = init_guest_space(host_start, host_size, 0);
> 
> If we're declaring the 'fixed' argument as 'bool' we should be passing
> 'false' rather than '0' here.

Fixed.

>> +        if (real_start == (unsigned long)-1) {
>> +            errmsg = "Unable to find space for application";
>> +            goto exit_errmsg;
>>          }
>> +        guest_base = real_start - loaddr;
>> +
>>          qemu_log("Relocating guest address space from 0x"
>>                   TARGET_ABI_FMT_lx " to 0x%lx\n",
>>                   loaddr, real_start);
>>      }
>>      return;
>>
>> -exit_perror:
>> -    errmsg = strerror(errno);
>>  exit_errmsg:
>>      fprintf(stderr, "%s: %s\n", image_name, errmsg);
>>      exit(-1);
>> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
>> index 7b299b7..c23dd8a 100644
>> --- a/linux-user/qemu.h
>> +++ b/linux-user/qemu.h
>> @@ -210,6 +210,17 @@ void fork_end(int child);
>>   */
>>  bool guest_validate_base(unsigned long guest_base);
>>
>> +/* Creates the initial guest address space in the host memory space using the
>> + * given host start address hint and size.  If fixed is specified, then the
>> + * mapped address space must start at host_start.  If host_start and host_size
>> + * are both zero then nothing is done and zero is returned. Otherwise, the
>> + * guest address space is initialized and the real start address of the mapped
>> + * memory space is returned or -1 if there is was error.
> 
> "was an error".

Fixed.

>> + */
>> +unsigned long init_guest_space(unsigned long host_start,
>> +                               unsigned long host_size,
>> +                               bool fixed);
>> +
>>  #include "qemu-log.h"
>>
>>  /* strace.c */
>> --
>> 1.7.7.6
>>

v2 patch coming soon.  Thanks for the review.

Patch

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index f3b1552..44b4bdb 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -1385,6 +1385,74 @@  bool guest_validate_base(unsigned long guest_base)
 }
 #endif
 
+unsigned long init_guest_space(unsigned long host_start,
+                               unsigned long host_size,
+                               bool fixed)
+{
+    unsigned long current_start, real_start;
+    int flags;
+
+    /* Nothing to do here, move along.  */
+    if (!host_start && !host_size) {
+        return 0;
+    }
+
+    /* If just a starting address is given, then just verify that
+     * address.  */
+    if (host_start && !host_size) {
+        if (guest_validate_base(host_start)) {
+            return host_start;
+        } else {
+            return (unsigned long)-1;
+        }
+    }
+
+    /* Setup the initial flags and start address.  */
+    current_start = host_start & qemu_host_page_mask;
+    flags = MAP_ANONYMOUS | MAP_PRIVATE | MAP_NORESERVE;
+    if (fixed) {
+        flags |= MAP_FIXED;
+    }
+
+    /* Otherwise, a non-zero size region of memory needs to be mapped
+     * and validated.  */
+    while (1) {
+        /* Do not use mmap_find_vma here because that is limited to the
+         * guest address space.  We are going to make the
+         * guest address space fit whatever we're given.
+         */
+        real_start = (unsigned long)
+            mmap((void *)current_start, host_size, PROT_NONE, flags, -1, 0);
+        if (real_start == (unsigned long)-1) {
+            return (unsigned long)-1;
+        }
+
+        if ((real_start == current_start) && guest_validate_base(real_start)) {
+            break;
+        }
+
+        /* That address didn't work.  Unmap and try a different one.
+         * The address the host picked because is typically right at
+         * the top of the host address space and leaves the guest with
+         * no usable address space.  Resort to a linear search.  We
+         * already compensated for mmap_min_addr, so this should not
+         * happen often.  Probably means we got unlucky and host
+         * address space randomization put a shared library somewhere
+         * inconvenient.
+         */
+        munmap((void *)real_start, host_size);
+        current_start += qemu_host_page_size;
+        if (host_start == current_start) {
+            /* Theoretically possible if host doesn't have any suitably
+             * aligned areas.  Normally the first mmap will fail.
+             */
+            return (unsigned long)-1;
+        }
+    }
+
+    return real_start;
+}
+
 static void probe_guest_base(const char *image_name,
                              abi_ulong loaddr, abi_ulong hiaddr)
 {
@@ -1411,46 +1479,23 @@  static void probe_guest_base(const char *image_name,
             }
         }
         host_size = hiaddr - loaddr;
-        while (1) {
-            /* Do not use mmap_find_vma here because that is limited to the
-               guest address space.  We are going to make the
-               guest address space fit whatever we're given.  */
-            real_start = (unsigned long)
-                mmap((void *)host_start, host_size, PROT_NONE,
-                     MAP_ANONYMOUS | MAP_PRIVATE | MAP_NORESERVE, -1, 0);
-            if (real_start == (unsigned long)-1) {
-                goto exit_perror;
-            }
-            guest_base = real_start - loaddr;
-            if ((real_start == host_start) &&
-                guest_validate_base(guest_base)) {
-                break;
-            }
-            /* That address didn't work.  Unmap and try a different one.
-               The address the host picked because is typically right at
-               the top of the host address space and leaves the guest with
-               no usable address space.  Resort to a linear search.  We
-               already compensated for mmap_min_addr, so this should not
-               happen often.  Probably means we got unlucky and host
-               address space randomization put a shared library somewhere
-               inconvenient.  */
-            munmap((void *)real_start, host_size);
-            host_start += qemu_host_page_size;
-            if (host_start == loaddr) {
-                /* Theoretically possible if host doesn't have any suitably
-                   aligned areas.  Normally the first mmap will fail.  */
-                errmsg = "Unable to find space for application";
-                goto exit_errmsg;
-            }
+
+        /* Setup the initial guest memory space with ranges gleaned from
+         * the ELF image that is being loaded.
+         */
+        real_start = init_guest_space(host_start, host_size, 0);
+        if (real_start == (unsigned long)-1) {
+            errmsg = "Unable to find space for application";
+            goto exit_errmsg;
         }
+        guest_base = real_start - loaddr;
+
         qemu_log("Relocating guest address space from 0x"
                  TARGET_ABI_FMT_lx " to 0x%lx\n",
                  loaddr, real_start);
     }
     return;
 
-exit_perror:
-    errmsg = strerror(errno);
 exit_errmsg:
     fprintf(stderr, "%s: %s\n", image_name, errmsg);
     exit(-1);
diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index 7b299b7..c23dd8a 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -210,6 +210,17 @@  void fork_end(int child);
  */
 bool guest_validate_base(unsigned long guest_base);
 
+/* Creates the initial guest address space in the host memory space using the
+ * given host start address hint and size.  If fixed is specified, then the
+ * mapped address space must start at host_start.  If host_start and host_size
+ * are both zero then nothing is done and zero is returned. Otherwise, the
+ * guest address space is initialized and the real start address of the mapped
+ * memory space is returned or -1 if there is was error.
+ */
+unsigned long init_guest_space(unsigned long host_start,
+                               unsigned long host_size,
+                               bool fixed);
+
 #include "qemu-log.h"
 
 /* strace.c */