Re: [PATCH v1 04/10] linux-user: completely re-write init_guest_space

[Thomas Huth]

On 13/05/2020 19.51, Alex Bennée wrote:
> First we ensure all guest space initialisation logic comes through
> probe_guest_base once we understand the nature of the binary we are
> loading. The convoluted init_guest_space routine is removed and
> replaced with a number of pgb_* helpers which are called depending on
> what requirements we have when loading the binary.
> 
> We first try to do what is requested by the host. Failing that we try
> and satisfy the guest requested base address. If all those options
> fail we fall back to finding a space in the memory map using our
> recently written read_self_maps() helper.
> 
> There are some additional complications we try and take into account
> when looking for holes in the address space. We try not to go directly
> after the system brk() space so there is space for a little growth. We
> also don't want to have to use negative offsets which would result in
> slightly less efficient code on x86 when it's unable to use the
> segment offset register.
> 
> Less mind-binding gotos and hopefully clearer logic throughout.
> 
> Signed-off-by: Alex Bennée <address@hidden>
> Acked-by: Laurent Vivier <address@hidden>
[...]
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 619c054cc48..01a9323a637 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -11,6 +11,7 @@
>  #include "qemu/queue.h"
>  #include "qemu/guest-random.h"
>  #include "qemu/units.h"
> +#include "qemu/selfmap.h"
>  
>  #ifdef _ARCH_PPC64
>  #undef ARCH_DLINFO
> @@ -382,68 +383,30 @@ enum {
>  
>  /* The commpage only exists for 32 bit kernels */
>  
> -/* Return 1 if the proposed guest space is suitable for the guest.
> - * Return 0 if the proposed guest space isn't suitable, but another
> - * address space should be tried.
> - * Return -1 if there is no way the proposed guest space can be
> - * valid regardless of the base.
> - * The guest code may leave a page mapped and populate it if the
> - * address is suitable.
> - */
> -static int init_guest_commpage(unsigned long guest_base,
> -                               unsigned long guest_size)
> -{
> -    unsigned long real_start, test_page_addr;
> -
> -    /* We need to check that we can force a fault on access to the
> -     * commpage at 0xffff0fxx
> -     */
> -    test_page_addr = guest_base + (0xffff0f00 & qemu_host_page_mask);
> -
> -    /* If the commpage lies within the already allocated guest space,
> -     * then there is no way we can allocate it.
> -     *
> -     * You may be thinking that that this check is redundant because
> -     * we already validated the guest size against MAX_RESERVED_VA;
> -     * but if qemu_host_page_mask is unusually large, then
> -     * test_page_addr may be lower.
> -     */
> -    if (test_page_addr >= guest_base
> -        && test_page_addr < (guest_base + guest_size)) {
> -        return -1;
> -    }
> +#define ARM_COMMPAGE (intptr_t)0xffff0f00u
>  
> -    /* Note it needs to be writeable to let us initialise it */
> -    real_start = (unsigned long)
> -                 mmap((void *)test_page_addr, qemu_host_page_size,
> -                     PROT_READ | PROT_WRITE,
> -                     MAP_ANONYMOUS | MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> +static bool init_guest_commpage(void)
> +{
> +    void *want = g2h(ARM_COMMPAGE & -qemu_host_page_size);
> +    void *addr = mmap(want, qemu_host_page_size, PROT_READ | PROT_WRITE,
> +                      MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
>  
> -    /* If we can't map it then try another address */
> -    if (real_start == -1ul) {
> -        return 0;
> +    if (addr == MAP_FAILED) {
> +        perror("Allocating guest commpage");
> +        exit(EXIT_FAILURE);
>      }
> -
> -    if (real_start != test_page_addr) {
> -        /* OS didn't put the page where we asked - unmap and reject */
> -        munmap((void *)real_start, qemu_host_page_size);
> -        return 0;
> +    if (addr != want) {
> +        return false;
>      }
>  
> -    /* Leave the page mapped
> -     * Populate it (mmap should have left it all 0'd)
> -     */
> -
> -    /* Kernel helper versions */
> -    __put_user(5, (uint32_t *)g2h(0xffff0ffcul));
> +    /* Set kernel helper versions; rest of page is 0.  */
> +    __put_user(5, (uint32_t *)g2h(0xffff0ffcu));
>  
> -    /* Now it's populated make it RO */
> -    if (mprotect((void *)test_page_addr, qemu_host_page_size, PROT_READ)) {
> +    if (mprotect(addr, qemu_host_page_size, PROT_READ)) {
>          perror("Protecting guest commpage");
> -        exit(-1);
> +        exit(EXIT_FAILURE);
>      }
> -
> -    return 1; /* All good */
> +    return true;
>  }
>  
>  #define ELF_HWCAP get_elf_hwcap()
> @@ -2075,239 +2038,267 @@ static abi_ulong create_elf_tables(abi_ulong p, int 
> argc, int envc,
>      return sp;
>  }
>  
> -unsigned long init_guest_space(unsigned long host_start,
> -                               unsigned long host_size,
> -                               unsigned long guest_start,
> -                               bool fixed)
> -{
> -    /* In order to use host shmat, we must be able to honor SHMLBA.  */
> -    unsigned long align = MAX(SHMLBA, qemu_host_page_size);
> -    unsigned long current_start, aligned_start;
> -    int flags;
> -
> -    assert(host_start || host_size);
> -
> -    /* If just a starting address is given, then just verify that
> -     * address.  */
> -    if (host_start && !host_size) {
> -#if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
> -        if (init_guest_commpage(host_start, host_size) != 1) {
> -            return (unsigned long)-1;
> -        }
> +#ifndef ARM_COMMPAGE
> +#define ARM_COMMPAGE 0
> +#define init_guest_commpage() true
>  #endif
> -        return host_start;
> -    }
>  
> -    /* Setup the initial flags and start address.  */
> -    current_start = host_start & -align;
> -    flags = MAP_ANONYMOUS | MAP_PRIVATE | MAP_NORESERVE;
> -    if (fixed) {
> -        flags |= MAP_FIXED;
> -    }
> +static void pgb_fail_in_use(const char *image_name)
> +{
> +    error_report("%s: requires virtual address space that is in use "
> +                 "(omit the -B option or choose a different value)",
> +                 image_name);
> +    exit(EXIT_FAILURE);
> +}
>  
> -    /* Otherwise, a non-zero size region of memory needs to be mapped
> -     * and validated.  */
> +static void pgb_have_guest_base(const char *image_name, abi_ulong 
> guest_loaddr,
> +                                abi_ulong guest_hiaddr, long align)
> +{
> +    const int flags = MAP_ANONYMOUS | MAP_PRIVATE | MAP_NORESERVE;
> +    void *addr, *test;
>  
> -#if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
> -    /* On 32-bit ARM, we need to map not just the usable memory, but
> -     * also the commpage.  Try to find a suitable place by allocating
> -     * a big chunk for all of it.  If host_start, then the naive
> -     * strategy probably does good enough.
> -     */
> -    if (!host_start) {
> -        unsigned long guest_full_size, host_full_size, real_start;
> -
> -        guest_full_size =
> -            (0xffff0f00 & qemu_host_page_mask) + qemu_host_page_size;
> -        host_full_size = guest_full_size - guest_start;
> -        real_start = (unsigned long)
> -            mmap(NULL, host_full_size, PROT_NONE, flags, -1, 0);
> -        if (real_start == (unsigned long)-1) {
> -            if (host_size < host_full_size - qemu_host_page_size) {
> -                /* We failed to map a continous segment, but we're
> -                 * allowed to have a gap between the usable memory and
> -                 * the commpage where other things can be mapped.
> -                 * This sparseness gives us more flexibility to find
> -                 * an address range.
> -                 */
> -                goto naive;
> -            }
> -            return (unsigned long)-1;
> +    if (!QEMU_IS_ALIGNED(guest_base, align)) {
> +        fprintf(stderr, "Requested guest base 0x%lx does not satisfy "
> +                "host minimum alignment (0x%lx)\n",
> +                guest_base, align);
> +        exit(EXIT_FAILURE);
> +    }
> +
> +    /* Sanity check the guest binary. */
> +    if (reserved_va) {
> +        if (guest_hiaddr > reserved_va) {
> +            error_report("%s: requires more than reserved virtual "
> +                         "address space (0x%" PRIx64 " > 0x%lx)",
> +                         image_name, (uint64_t)guest_hiaddr, reserved_va);
> +            exit(EXIT_FAILURE);
>          }
> -        munmap((void *)real_start, host_full_size);
> -        if (real_start & (align - 1)) {
> -            /* The same thing again, but with extra
> -             * so that we can shift around alignment.
> -             */
> -            unsigned long real_size = host_full_size + qemu_host_page_size;
> -            real_start = (unsigned long)
> -                mmap(NULL, real_size, PROT_NONE, flags, -1, 0);
> -            if (real_start == (unsigned long)-1) {
> -                if (host_size < host_full_size - qemu_host_page_size) {
> -                    goto naive;
> -                }
> -                return (unsigned long)-1;
> -            }
> -            munmap((void *)real_start, real_size);
> -            real_start = ROUND_UP(real_start, align);
> +    } else {
> +        if ((guest_hiaddr - guest_base) > ~(uintptr_t)0) {
> +            error_report("%s: requires more virtual address space "
> +                         "than the host can provide (0x%" PRIx64 ")",
> +                         image_name, (uint64_t)guest_hiaddr - guest_base);
> +            exit(EXIT_FAILURE);
>          }

 Hi Alex,

this causes an error with newer versions of Clang:

linux-user/elfload.c:2076:41: error: result of comparison 'unsigned
long' > 18446744073709551615 is always false
[-Werror,-Wtautological-type-limit-compare]
4685         if ((guest_hiaddr - guest_base) > ~(uintptr_t)0) {
4686             ~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~
4687 1 error generated.

Any ideas how to fix this?

 Thomas