Re: [Qemu-devel] [PATCH 1/2] linux-user: Factor out guest space probing into a function

[Meador Inge <meadori@codesourcery.com>]

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.

-- 
Meador Inge
CodeSourcery / Mentor Embedded
http://www.mentor.com/embedded-software