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 <alex.ben...@linaro.org>
> Acked-by: Laurent Vivier <laur...@vivier.eu>
[...]
> 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
