path: root/results/scraper/launchpad/1880225

Emulation of some arm programs fail with "Assertion `have_guest_base' failed."

This issue is observer with QEMU ToT, checked out around May 15th (but I believe it is present in current master too), and wasn't present in QEMU v5.0.0.

I am using 32-bit Intel(R) Pentium(R) M processor 1.73GHz host.

Arm cross-compiler is a standard cross-compiler that comes with Debian-based distributions, and gcc version is:

$ arm-linux-gnueabi-gcc --version
arm-linux-gnueabi-gcc (Debian 8.3.0-2) 8.3.0

Compile this program with cross compiler:

$ arm-linux-gnueabi-gcc -O2 -static toupper_string.c -o toupper_string-arm

Emulation with QEMU v5.0.0 is correct, and gives expected output:

$ ~/Build/qemu-5.0.0/build-gcc/arm-linux-user/qemu-arm ./toupper_string-arm
CONTROL RESULT: (toupper_string)
 nwlrbbmqbhcdarz owkkyhiddqscdxr jmowfrxsjybldbe fsarcbynecdyggx xpklorellnmpapq
 NWLRBBMQBHCDARZ OWKKYHIDDQSCDXR JMOWFRXSJYBLDBE FSARCBYNECDYGGX XPKLORELLNMPAPQ

While, in case of QEMU master it fails:

$ ~/Build/qemu-master/build-gcc/arm-linux-user/qemu-arm ./toupper_string-arm
qemu-arm: /home/rtrk/Build/qemu-master/linux-user/elfload.c:2294: probe_guest_base: Assertion `have_guest_base' failed.
Aborted

There are many other programs that exibit the same behavior. The failure is arm-sprecific.


-----------------------------------------------------

source code: (let's call this file toupper_string.c) (similar file is also in attachment)


#include <stdlib.h>
#include <string.h>
#include <stdio.h>
#include <unistd.h>


#define MAX_STRING_LENGHT              15
#define NUMBER_OF_RANDOM_STRINGS       100
#define DEFAULT_NUMBER_OF_REPETITIONS  30000
#define MAX_NUMBER_OF_REPETITIONS      1000000000
#define NUMBER_OF_CONTROL_PRINT_ITEMS  5

/* Structure for keeping an array of strings */
struct StringStruct {
    char chars[MAX_STRING_LENGHT + 1];
};

/**
 * Sets characters of the given string to random small letters a-z.
 * @param s String to get random characters.
 * @len Length of the input string.
 */
static void gen_random_string(char *chars, const int len)
{
    static const char letters[] = "abcdefghijklmnopqrstuvwxyz";

    for (size_t i = 0; i < len; i++) {
        chars[i] = letters[rand() % (sizeof(letters) - 1)];
    }
    chars[len] = 0;
}

void main (int argc, char* argv[])
{
    struct StringStruct random_strings[NUMBER_OF_RANDOM_STRINGS];
    struct StringStruct strings_to_be_uppercased[NUMBER_OF_RANDOM_STRINGS];
    int32_t number_of_repetitions = DEFAULT_NUMBER_OF_REPETITIONS;
    int32_t option;

    /* Parse command line options */
    while ((option = getopt(argc, argv, "n:")) != -1) {
        if (option == 'n') {
            int32_t user_number_of_repetitions = atoi(optarg);
            /* Check if the value is a negative number */
            if (user_number_of_repetitions < 1) {
                fprintf(stderr, "Error ... Value for option '-n' cannot be a "
                                "negative number.\n");
                exit(EXIT_FAILURE);
            }
            /* Check if the value is a string or zero */
            if (user_number_of_repetitions == 0) {
                fprintf(stderr, "Error ... Invalid value for option '-n'.\n");
                exit(EXIT_FAILURE);
            }
            /* Check if the value is too large */
            if (user_number_of_repetitions > MAX_NUMBER_OF_REPETITIONS) {
                fprintf(stderr, "Error ... Value for option '-n' cannot be "
                                "more than %d.\n", MAX_NUMBER_OF_REPETITIONS);
                exit(EXIT_FAILURE);
            }
            number_of_repetitions = user_number_of_repetitions;
        } else {
            exit(EXIT_FAILURE);
        }
    }

    /* Create an array of strings with random content */
    srand(1);
    for (size_t i = 0; i < NUMBER_OF_RANDOM_STRINGS; i++) {
        gen_random_string(random_strings[i].chars, MAX_STRING_LENGHT);
    }

    /* Perform uppercasing of a set of random strings multiple times */
    for (size_t j = 0; j < number_of_repetitions; j++) {
        /* Copy initial set of random strings to the set to be uppercased */
        memcpy(strings_to_be_uppercased, random_strings,
               NUMBER_OF_RANDOM_STRINGS * (MAX_STRING_LENGHT + 1));
        /* Do actual changing case to uppercase */
        for (size_t i = 0; i < NUMBER_OF_RANDOM_STRINGS; i++) {
            int k = 0;
  
            while (strings_to_be_uppercased[i].chars[k]) { 
                char ch = strings_to_be_uppercased[i].chars[k] - 32; 
                memcpy((void *)strings_to_be_uppercased[i].chars + k,
                       &ch, 1);
                k++; 
            } 
        }
    }

    /* Control printing */
    printf("CONTROL RESULT: (toupper_string)\n");
    for (size_t i = 0; i < NUMBER_OF_CONTROL_PRINT_ITEMS; i++) {
        printf(" %s", random_strings[i].chars);
    }
    printf("\n");
    for (size_t i = 0; i < NUMBER_OF_CONTROL_PRINT_ITEMS; i++) {
        printf(" %s", strings_to_be_uppercased[i].chars);
    }
    printf("\n");
}




Aleksandar Markovic <email address hidden> writes:

> Public bug reported:
>
> This issue is observer with QEMU ToT, checked out around May 15th (but I
> believe it is present in current master too), and wasn't present in QEMU
> v5.0.0.
>
> I am using 32-bit Intel(R) Pentium(R) M processor 1.73GHz host.
>
> Arm cross-compiler is a standard cross-compiler that comes with Debian-
> based distributions, and gcc version is:
>
> $ arm-linux-gnueabi-gcc --version
> arm-linux-gnueabi-gcc (Debian 8.3.0-2) 8.3.0
>
> Compile this program with cross compiler:
>
> $ arm-linux-gnueabi-gcc -O2 -static toupper_string.c -o toupper_string-
> arm
>
> Emulation with QEMU v5.0.0 is correct, and gives expected output:
>
> $ ~/Build/qemu-5.0.0/build-gcc/arm-linux-user/qemu-arm ./toupper_string-arm
> CONTROL RESULT: (toupper_string)
>  nwlrbbmqbhcdarz owkkyhiddqscdxr jmowfrxsjybldbe fsarcbynecdyggx xpklorellnmpapq
>  NWLRBBMQBHCDARZ OWKKYHIDDQSCDXR JMOWFRXSJYBLDBE FSARCBYNECDYGGX XPKLORELLNMPAPQ
>
> While, in case of QEMU master it fails:
>
> $ ~/Build/qemu-master/build-gcc/arm-linux-user/qemu-arm ./toupper_string-arm
> qemu-arm: /home/rtrk/Build/qemu-master/linux-user/elfload.c:2294: probe_guest_base: Assertion `have_guest_base' failed.
> Aborted
<snip>

Works for me in our TCG tests on master:

20:15:43 [alex@zen:~/l/q/b/user.static] review/aarch64-vms-v7|… + ./arm-linux-user/qemu-arm ./tests/tcg/arm-linux-user/toupper
CONTROL RESULT: (toupper_string)
 nwlrbbmqbhcdarz owkkyhiddqscdxr jmowfrxsjybldbe fsarcbynecdyggx xpklorellnmpapq
 NWLRBBMQBHCDARZ OWKKYHIDDQSCDXR JMOWFRXSJYBLDBE FSARCBYNECDYGGX XPKLORELLNMPAPQ

I have submitted a fix to the list that affected programs that couldn't
see /proc/self/maps but I guess that isn't the case here.

-- 
Alex Bennée


Using bisection, it can be deduced that this behavior appears to be caused by this commit:


commit ee94743034bfb443cf246eda4971bdc15d8ee066 (HEAD)
Author: Alex Bennée <email address hidden>
Date:   Wed May 13 18:51:28 2020 +0100

    linux-user: completely re-write init_guest_space
    
    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 <email address hidden>
    Acked-by: Laurent Vivier <email address hidden>
    
    Message-Id: <email address hidden>

I just want to stress once again that the test was performed on a 32-bit Intel host.

It appear that there is no problem on Intel 64-bit hosts.

Perhaps the problem is manifested on all 32-bit hosts. I currently don't have access to any other 320bit host due to remote work.

The arm is the only target were I noticed this happens. I checked hppa, mips, mipsel, m68k, ppc, and sh4, they ae all fine,  with the same program/example, on the same 32-bit Intel host. I did not checked other target except those I just mentioned.


Aleksandar Markovic <email address hidden> writes:

> I just want to stress once again that the test was performed on a 32-bit
> Intel host.

Ahh - OK that makes sense. I'll see if I can replicate.


-- 
Alex Bennée


The problem may be in int_guest_commpage() - it returns false.

From gdb debugging session:

(gdb) p addr
$1 = (void *) 0xb7ffd000
(gdb) p want
$2 = (void *) 0xffff0000
(gdb) n
398	    if (addr != want) {
(gdb) p qemu_host_page_size
$3 = 4096
(gdb) l
393	
394	    if (addr == MAP_FAILED) {
395	        perror("Allocating guest commpage");
396	        exit(EXIT_FAILURE);
397	    }
398	    if (addr != want) {
399	        return false;
400	    }
401	
402	    /* Set kernel helper versions; rest of page is 0.  */
(gdb) 


Aleksandar Markovic <email address hidden> writes:

> The problem may be in int_guest_commpage() - it returns false.
>
>>From gdb debugging session:
>
> (gdb) p addr
> $1 = (void *) 0xb7ffd000
> (gdb) p want
> $2 = (void *) 0xffff0000
> (gdb) n
> 398	    if (addr != want) {
> (gdb) p qemu_host_page_size
> $3 = 4096
> (gdb) l
> 393	
> 394	    if (addr == MAP_FAILED) {
> 395	        perror("Allocating guest commpage");
> 396	        exit(EXIT_FAILURE);
> 397	    }
> 398	    if (addr != want) {
> 399	        return false;
> 400	    }
> 401	
> 402	    /* Set kernel helper versions; rest of page is 0.  */
> (gdb)

I'm not totally convinced the calculation that we do to work out the
extended size of the guest space in 32 bit:

  11:10 alex@debian-buster-i386/i686  [arm/bugs/add-mmap-fallback@github] >./arm-linux-user/qemu-arm tests/tcg/arm-linux-user/sha1
  pgb_static: loaddr: 10000
  pgb_static: loaddr: ffff0000
  pgb_find_hole: ffff0000:10809a8 (1000)
  pgb_find_hole: 0:10809a8
  init_guest_commpage: 0xffff0000 -> 0xb7f48000 (4096)
  qemu-arm: /home/alex/lsrc/qemu.git/linux-user/elfload.c:2350: probe_guest_base: Assertion `have_guest_base' failed.
  Aborted (core dumped)

Or in fact why we don't do a MAP_FIXED ass we should have ensured we
have enough space allocated for the guest. Richard any ideas?

-- 
Alex Bennée


We rely on the pointer to wrap when accessing the high address of the
COMMPAGE so it lands somewhere reasonable. However on 32 bit hosts we
cannot afford just to map the entire 4gb address range. The old mmap
trial and error code handled this by just checking we could map both
the guest_base and the computed COMMPAGE address.

We can't just manipulate loadaddr to get what we want so we introduce
an offset which pgb_find_hole can apply when looking for a gap for
guest_base that ensures there is space left to map the COMMPAGE
afterwards.

This is arguably a little inefficient for the one 32 bit
value (kuser_helper_version) we need to keep there given all the
actual code entries are picked up during the translation phase.

Fixes: ee94743034b
Bug: https://bugs.launchpad.net/qemu/+bug/1880225
Cc: Bug 1880225 <email address hidden>
Signed-off-by: Alex Bennée <email address hidden>
Cc: Richard Henderson <email address hidden>
Cc: Peter Maydell <email address hidden>
---
 linux-user/elfload.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index d6027867a1a..31defce95b5 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2145,7 +2145,7 @@ static uintptr_t pgd_find_hole_fallback(uintptr_t guest_size, uintptr_t brk, lon
 
 /* Return value for guest_base, or -1 if no hole found. */
 static uintptr_t pgb_find_hole(uintptr_t guest_loaddr, uintptr_t guest_size,
-                               long align)
+                               long align, uintptr_t offset)
 {
     GSList *maps, *iter;
     uintptr_t this_start, this_end, next_start, brk;
@@ -2171,7 +2171,7 @@ static uintptr_t pgb_find_hole(uintptr_t guest_loaddr, uintptr_t guest_size,
 
         this_end = ((MapInfo *)iter->data)->start;
         next_start = ((MapInfo *)iter->data)->end;
-        align_start = ROUND_UP(this_start, align);
+        align_start = ROUND_UP(this_start + offset, align);
 
         /* Skip holes that are too small. */
         if (align_start >= this_end) {
@@ -2221,6 +2221,7 @@ static void pgb_static(const char *image_name, abi_ulong orig_loaddr,
 {
     uintptr_t loaddr = orig_loaddr;
     uintptr_t hiaddr = orig_hiaddr;
+    uintptr_t offset = 0;
     uintptr_t addr;
 
     if (hiaddr != orig_hiaddr) {
@@ -2234,18 +2235,19 @@ static void pgb_static(const char *image_name, abi_ulong orig_loaddr,
     if (ARM_COMMPAGE) {
         /*
          * Extend the allocation to include the commpage.
-         * For a 64-bit host, this is just 4GiB; for a 32-bit host,
-         * the address arithmetic will wrap around, but the difference
-         * will produce the correct allocation size.
+         * For a 64-bit host, this is just 4GiB; for a 32-bit host we
+         * need to ensure there is space bellow the guest_base so we
+         * can map the commpage in the place needed when the address
+         * arithmetic wraps around.
          */
         if (sizeof(uintptr_t) == 8 || loaddr >= 0x80000000u) {
             hiaddr = (uintptr_t)4 << 30;
         } else {
-            loaddr = ARM_COMMPAGE & -align;
+            offset = (128 * KiB);
         }
     }
 
-    addr = pgb_find_hole(loaddr, hiaddr - loaddr, align);
+    addr = pgb_find_hole(loaddr, hiaddr - loaddr, align, offset);
     if (addr == -1) {
         /*
          * If ARM_COMMPAGE, there *might* be a non-consecutive allocation
@@ -2280,7 +2282,7 @@ static void pgb_dynamic(const char *image_name, long align)
          * just above that, and maximises the positive guest addresses.
          */
         commpage = ARM_COMMPAGE & -align;
-        addr = pgb_find_hole(commpage, -commpage, align);
+        addr = pgb_find_hole(commpage, -commpage, align, 0);
         assert(addr != -1);
         guest_base = addr;
     }
-- 
2.20.1



сре, 27. мај 2020. у 12:07 Alex Bennée <email address hidden> је написао/ла:
>
> We rely on the pointer to wrap when accessing the high address of the
> COMMPAGE so it lands somewhere reasonable. However on 32 bit hosts we
> cannot afford just to map the entire 4gb address range. The old mmap
> trial and error code handled this by just checking we could map both
> the guest_base and the computed COMMPAGE address.
>
> We can't just manipulate loadaddr to get what we want so we introduce
> an offset which pgb_find_hole can apply when looking for a gap for
> guest_base that ensures there is space left to map the COMMPAGE
> afterwards.
>
> This is arguably a little inefficient for the one 32 bit
> value (kuser_helper_version) we need to keep there given all the
> actual code entries are picked up during the translation phase.
>
> Fixes: ee94743034b
> Bug: https://bugs.launchpad.net/qemu/+bug/1880225

For the scenario in this bug report, for today's master, before and after
applying this patch:

BEFORE:

$ ~/Build/qemu-master/build-gcc/arm-linux-user/qemu-arm ./toupper_string-arm
qemu-arm: /home/rtrk/Build/qemu-master/linux-user/elfload.c:2294:
probe_guest_base: Assertion `have_guest_base' failed.
Aborted

AFTER:

$ ~/Build/qemu-master/build-gcc/arm-linux-user/qemu-arm ./toupper_string-arm
CONTROL RESULT: (toupper_string)
 nwlrbbmqbhcdarz owkkyhiddqscdxr jmowfrxsjybldbe fsarcbynecdyggx xpklorellnmpapq
 NWLRBBMQBHCDARZ OWKKYHIDDQSCDXR JMOWFRXSJYBLDBE FSARCBYNECDYGGX XPKLORELLNMPAPQ

So, it works in my test bed.

Tested-by: Aleksandar Markovic <email address hidden>

> Cc: Bug 1880225 <email address hidden>
> Signed-off-by: Alex Bennée <email address hidden>
> Cc: Richard Henderson <email address hidden>
> Cc: Peter Maydell <email address hidden>
> ---
>  linux-user/elfload.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index d6027867a1a..31defce95b5 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -2145,7 +2145,7 @@ static uintptr_t pgd_find_hole_fallback(uintptr_t guest_size, uintptr_t brk, lon
>
>  /* Return value for guest_base, or -1 if no hole found. */
>  static uintptr_t pgb_find_hole(uintptr_t guest_loaddr, uintptr_t guest_size,
> -                               long align)
> +                               long align, uintptr_t offset)
>  {
>      GSList *maps, *iter;
>      uintptr_t this_start, this_end, next_start, brk;
> @@ -2171,7 +2171,7 @@ static uintptr_t pgb_find_hole(uintptr_t guest_loaddr, uintptr_t guest_size,
>
>          this_end = ((MapInfo *)iter->data)->start;
>          next_start = ((MapInfo *)iter->data)->end;
> -        align_start = ROUND_UP(this_start, align);
> +        align_start = ROUND_UP(this_start + offset, align);
>
>          /* Skip holes that are too small. */
>          if (align_start >= this_end) {
> @@ -2221,6 +2221,7 @@ static void pgb_static(const char *image_name, abi_ulong orig_loaddr,
>  {
>      uintptr_t loaddr = orig_loaddr;
>      uintptr_t hiaddr = orig_hiaddr;
> +    uintptr_t offset = 0;
>      uintptr_t addr;
>
>      if (hiaddr != orig_hiaddr) {
> @@ -2234,18 +2235,19 @@ static void pgb_static(const char *image_name, abi_ulong orig_loaddr,
>      if (ARM_COMMPAGE) {
>          /*
>           * Extend the allocation to include the commpage.
> -         * For a 64-bit host, this is just 4GiB; for a 32-bit host,
> -         * the address arithmetic will wrap around, but the difference
> -         * will produce the correct allocation size.
> +         * For a 64-bit host, this is just 4GiB; for a 32-bit host we
> +         * need to ensure there is space bellow the guest_base so we
> +         * can map the commpage in the place needed when the address
> +         * arithmetic wraps around.
>           */
>          if (sizeof(uintptr_t) == 8 || loaddr >= 0x80000000u) {
>              hiaddr = (uintptr_t)4 << 30;
>          } else {
> -            loaddr = ARM_COMMPAGE & -align;
> +            offset = (128 * KiB);
>          }
>      }
>
> -    addr = pgb_find_hole(loaddr, hiaddr - loaddr, align);
> +    addr = pgb_find_hole(loaddr, hiaddr - loaddr, align, offset);
>      if (addr == -1) {
>          /*
>           * If ARM_COMMPAGE, there *might* be a non-consecutive allocation
> @@ -2280,7 +2282,7 @@ static void pgb_dynamic(const char *image_name, long align)
>           * just above that, and maximises the positive guest addresses.
>           */
>          commpage = ARM_COMMPAGE & -align;
> -        addr = pgb_find_hole(commpage, -commpage, align);
> +        addr = pgb_find_hole(commpage, -commpage, align, 0);
>          assert(addr != -1);
>          guest_base = addr;
>      }
> --
> 2.20.1
>
>


сре, 27. мај 2020. у 14:05 Aleksandar Markovic
<email address hidden> је написао/ла:
>
> сре, 27. мај 2020. у 12:07 Alex Bennée <email address hidden> је написао/ла:
> >
> > We rely on the pointer to wrap when accessing the high address of the
> > COMMPAGE so it lands somewhere reasonable. However on 32 bit hosts we
> > cannot afford just to map the entire 4gb address range. The old mmap
> > trial and error code handled this by just checking we could map both
> > the guest_base and the computed COMMPAGE address.
> >
> > We can't just manipulate loadaddr to get what we want so we introduce
> > an offset which pgb_find_hole can apply when looking for a gap for
> > guest_base that ensures there is space left to map the COMMPAGE
> > afterwards.
> >
> > This is arguably a little inefficient for the one 32 bit
> > value (kuser_helper_version) we need to keep there given all the
> > actual code entries are picked up during the translation phase.
> >
> > Fixes: ee94743034b
> > Bug: https://bugs.launchpad.net/qemu/+bug/1880225
>
> For the scenario in this bug report, for today's master, before and after
> applying this patch:
>

I am not sure how significant is this info, but I executed the test
without applying patch 1/3, so only 2/3 was applied in the case
"AFTER".

Aleksandar

> BEFORE:
>
> $ ~/Build/qemu-master/build-gcc/arm-linux-user/qemu-arm ./toupper_string-arm
> qemu-arm: /home/rtrk/Build/qemu-master/linux-user/elfload.c:2294:
> probe_guest_base: Assertion `have_guest_base' failed.
> Aborted
>
> AFTER:
>
> $ ~/Build/qemu-master/build-gcc/arm-linux-user/qemu-arm ./toupper_string-arm
> CONTROL RESULT: (toupper_string)
>  nwlrbbmqbhcdarz owkkyhiddqscdxr jmowfrxsjybldbe fsarcbynecdyggx xpklorellnmpapq
>  NWLRBBMQBHCDARZ OWKKYHIDDQSCDXR JMOWFRXSJYBLDBE FSARCBYNECDYGGX XPKLORELLNMPAPQ
>
> So, it works in my test bed.
>
> Tested-by: Aleksandar Markovic <email address hidden>
>
> > Cc: Bug 1880225 <email address hidden>
> > Signed-off-by: Alex Bennée <email address hidden>
> > Cc: Richard Henderson <email address hidden>
> > Cc: Peter Maydell <email address hidden>
> > ---
> >  linux-user/elfload.c | 18 ++++++++++--------
> >  1 file changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> > index d6027867a1a..31defce95b5 100644
> > --- a/linux-user/elfload.c
> > +++ b/linux-user/elfload.c
> > @@ -2145,7 +2145,7 @@ static uintptr_t pgd_find_hole_fallback(uintptr_t guest_size, uintptr_t brk, lon
> >
> >  /* Return value for guest_base, or -1 if no hole found. */
> >  static uintptr_t pgb_find_hole(uintptr_t guest_loaddr, uintptr_t guest_size,
> > -                               long align)
> > +                               long align, uintptr_t offset)
> >  {
> >      GSList *maps, *iter;
> >      uintptr_t this_start, this_end, next_start, brk;
> > @@ -2171,7 +2171,7 @@ static uintptr_t pgb_find_hole(uintptr_t guest_loaddr, uintptr_t guest_size,
> >
> >          this_end = ((MapInfo *)iter->data)->start;
> >          next_start = ((MapInfo *)iter->data)->end;
> > -        align_start = ROUND_UP(this_start, align);
> > +        align_start = ROUND_UP(this_start + offset, align);
> >
> >          /* Skip holes that are too small. */
> >          if (align_start >= this_end) {
> > @@ -2221,6 +2221,7 @@ static void pgb_static(const char *image_name, abi_ulong orig_loaddr,
> >  {
> >      uintptr_t loaddr = orig_loaddr;
> >      uintptr_t hiaddr = orig_hiaddr;
> > +    uintptr_t offset = 0;
> >      uintptr_t addr;
> >
> >      if (hiaddr != orig_hiaddr) {
> > @@ -2234,18 +2235,19 @@ static void pgb_static(const char *image_name, abi_ulong orig_loaddr,
> >      if (ARM_COMMPAGE) {
> >          /*
> >           * Extend the allocation to include the commpage.
> > -         * For a 64-bit host, this is just 4GiB; for a 32-bit host,
> > -         * the address arithmetic will wrap around, but the difference
> > -         * will produce the correct allocation size.
> > +         * For a 64-bit host, this is just 4GiB; for a 32-bit host we
> > +         * need to ensure there is space bellow the guest_base so we
> > +         * can map the commpage in the place needed when the address
> > +         * arithmetic wraps around.
> >           */
> >          if (sizeof(uintptr_t) == 8 || loaddr >= 0x80000000u) {
> >              hiaddr = (uintptr_t)4 << 30;
> >          } else {
> > -            loaddr = ARM_COMMPAGE & -align;
> > +            offset = (128 * KiB);
> >          }
> >      }
> >
> > -    addr = pgb_find_hole(loaddr, hiaddr - loaddr, align);
> > +    addr = pgb_find_hole(loaddr, hiaddr - loaddr, align, offset);
> >      if (addr == -1) {
> >          /*
> >           * If ARM_COMMPAGE, there *might* be a non-consecutive allocation
> > @@ -2280,7 +2282,7 @@ static void pgb_dynamic(const char *image_name, long align)
> >           * just above that, and maximises the positive guest addresses.
> >           */
> >          commpage = ARM_COMMPAGE & -align;
> > -        addr = pgb_find_hole(commpage, -commpage, align);
> > +        addr = pgb_find_hole(commpage, -commpage, align, 0);
> >          assert(addr != -1);
> >          guest_base = addr;
> >      }
> > --
> > 2.20.1
> >
> >



Aleksandar Markovic <email address hidden> writes:

> сре, 27. мај 2020. у 14:05 Aleksandar Markovic
> <email address hidden> је написао/ла:
>>
>> сре, 27. мај 2020. у 12:07 Alex Bennée <email address hidden> је написао/ла:
>> >
>> > We rely on the pointer to wrap when accessing the high address of the
>> > COMMPAGE so it lands somewhere reasonable. However on 32 bit hosts we
>> > cannot afford just to map the entire 4gb address range. The old mmap
>> > trial and error code handled this by just checking we could map both
>> > the guest_base and the computed COMMPAGE address.
>> >
>> > We can't just manipulate loadaddr to get what we want so we introduce
>> > an offset which pgb_find_hole can apply when looking for a gap for
>> > guest_base that ensures there is space left to map the COMMPAGE
>> > afterwards.
>> >
>> > This is arguably a little inefficient for the one 32 bit
>> > value (kuser_helper_version) we need to keep there given all the
>> > actual code entries are picked up during the translation phase.
>> >
>> > Fixes: ee94743034b
>> > Bug: https://bugs.launchpad.net/qemu/+bug/1880225
>>
>> For the scenario in this bug report, for today's master, before and after
>> applying this patch:
>>
>
> I am not sure how significant is this info, but I executed the test
> without applying patch 1/3, so only 2/3 was applied in the case
> "AFTER".

That is expected. The other fix only affects binaries run inside a
/proc-less chroot and the final patch is a test case for COMMPAGE
support.

-- 
Alex Bennée



Richard Henderson <email address hidden> writes:

> On 5/27/20 3:05 AM, Alex Bennée wrote:
>> @@ -2145,7 +2145,7 @@ static uintptr_t pgd_find_hole_fallback(uintptr_t guest_size, uintptr_t brk, lon
>>  
>>  /* Return value for guest_base, or -1 if no hole found. */
>>  static uintptr_t pgb_find_hole(uintptr_t guest_loaddr, uintptr_t guest_size,
>> -                               long align)
>> +                               long align, uintptr_t offset)
>>  {
>>      GSList *maps, *iter;
>>      uintptr_t this_start, this_end, next_start, brk;
>> @@ -2171,7 +2171,7 @@ static uintptr_t pgb_find_hole(uintptr_t guest_loaddr, uintptr_t guest_size,
>>  
>>          this_end = ((MapInfo *)iter->data)->start;
>>          next_start = ((MapInfo *)iter->data)->end;
>> -        align_start = ROUND_UP(this_start, align);
>> +        align_start = ROUND_UP(this_start + offset, align);
>>  
>>          /* Skip holes that are too small. */
>
> I suppose offset is supposed to mean we start from -offset?

Well guest_base will start higher meaning we have space for the
commpage beneath it.

> You didn't update
> pgb_find_hole_fallback.

Fixed.

>
>> -            loaddr = ARM_COMMPAGE & -align;
>> +            offset = (128 * KiB);
>
> Why 128K?  Surely this should be an expression against ARM_COMMPAGE.

In theory:

            offset = -(ARM_COMMPAGE & -align);

should do the trick but I found it failed every now and again.
Frustratingly putting printfs in made it go away so in frustration I
just upped the offset until it stopped happening.

I do kinda wish rr worked on i386 :-/


>
>
> r~


-- 
Alex Bennée



Alex Bennée <email address hidden> writes:

> Richard Henderson <email address hidden> writes:
>
>> On 5/27/20 3:05 AM, Alex Bennée wrote:
>>> @@ -2145,7 +2145,7 @@ static uintptr_t pgd_find_hole_fallback(uintptr_t guest_size, uintptr_t brk, lon
>>>  
>>>  /* Return value for guest_base, or -1 if no hole found. */
>>>  static uintptr_t pgb_find_hole(uintptr_t guest_loaddr, uintptr_t guest_size,
>>> -                               long align)
>>> +                               long align, uintptr_t offset)
>>>  {
>>>      GSList *maps, *iter;
>>>      uintptr_t this_start, this_end, next_start, brk;
>>> @@ -2171,7 +2171,7 @@ static uintptr_t pgb_find_hole(uintptr_t guest_loaddr, uintptr_t guest_size,
>>>  
>>>          this_end = ((MapInfo *)iter->data)->start;
>>>          next_start = ((MapInfo *)iter->data)->end;
>>> -        align_start = ROUND_UP(this_start, align);
>>> +        align_start = ROUND_UP(this_start + offset, align);
>>>  
>>>          /* Skip holes that are too small. */
>>
>> I suppose offset is supposed to mean we start from -offset?
>
> Well guest_base will start higher meaning we have space for the
> commpage beneath it.
>
>> You didn't update
>> pgb_find_hole_fallback.
>
> Fixed.
>
>>
>>> -            loaddr = ARM_COMMPAGE & -align;
>>> +            offset = (128 * KiB);
>>
>> Why 128K?  Surely this should be an expression against ARM_COMMPAGE.
>
> In theory:
>
>             offset = -(ARM_COMMPAGE & -align);
>
> should do the trick but I found it failed every now and again.
> Frustratingly putting printfs in made it go away so in frustration I
> just upped the offset until it stopped happening.
>
> I do kinda wish rr worked on i386 :-/

Ahh all I needed was a MAP_FIXED for init_commpage

-- 
Alex Bennée


We rely on the pointer to wrap when accessing the high address of the
COMMPAGE so it lands somewhere reasonable. However on 32 bit hosts we
cannot afford just to map the entire 4gb address range. The old mmap
trial and error code handled this by just checking we could map both
the guest_base and the computed COMMPAGE address.

We can't just manipulate loadaddr to get what we want so we introduce
an offset which pgb_find_hole can apply when looking for a gap for
guest_base that ensures there is space left to map the COMMPAGE
afterwards.

This is arguably a little inefficient for the one 32 bit
value (kuser_helper_version) we need to keep there given all the
actual code entries are picked up during the translation phase.

Fixes: ee94743034b
Bug: https://bugs.launchpad.net/qemu/+bug/1880225
Cc: Bug 1880225 <email address hidden>
Signed-off-by: Alex Bennée <email address hidden>
Tested-by: Aleksandar Markovic <email address hidden>
Cc: Richard Henderson <email address hidden>
Cc: Peter Maydell <email address hidden>
Message-Id: <email address hidden>

---
v3
  - base offset on ARM_COMMPAGE
  - ensure we MAP_FIXED the commpage
  - support offset in pgd_find_hole_fallback
---
 linux-user/elfload.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 475d243f3bd..b5cb21384a1 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -389,7 +389,7 @@ 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);
+                      MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);
 
     if (addr == MAP_FAILED) {
         perror("Allocating guest commpage");
@@ -2113,7 +2113,8 @@ static void pgb_have_guest_base(const char *image_name, abi_ulong guest_loaddr,
  * only dumbly iterate up the host address space seeing if the
  * allocation would work.
  */
-static uintptr_t pgd_find_hole_fallback(uintptr_t guest_size, uintptr_t brk, long align)
+static uintptr_t pgd_find_hole_fallback(uintptr_t guest_size, uintptr_t brk,
+                                        long align, uintptr_t offset)
 {
     uintptr_t base;
 
@@ -2123,7 +2124,7 @@ static uintptr_t pgd_find_hole_fallback(uintptr_t guest_size, uintptr_t brk, lon
     while (true) {
         uintptr_t align_start, end;
         align_start = ROUND_UP(base, align);
-        end = align_start + guest_size;
+        end = align_start + guest_size + offset;
 
         /* if brk is anywhere in the range give ourselves some room to grow. */
         if (align_start <= brk && brk < end) {
@@ -2138,7 +2139,7 @@ static uintptr_t pgd_find_hole_fallback(uintptr_t guest_size, uintptr_t brk, lon
                                      PROT_NONE, flags, -1, 0);
             if (mmap_start != MAP_FAILED) {
                 munmap((void *) align_start, guest_size);
-                return (uintptr_t) mmap_start;
+                return (uintptr_t) mmap_start + offset;
             }
             base += qemu_host_page_size;
         }
@@ -2147,7 +2148,7 @@ static uintptr_t pgd_find_hole_fallback(uintptr_t guest_size, uintptr_t brk, lon
 
 /* Return value for guest_base, or -1 if no hole found. */
 static uintptr_t pgb_find_hole(uintptr_t guest_loaddr, uintptr_t guest_size,
-                               long align)
+                               long align, uintptr_t offset)
 {
     GSList *maps, *iter;
     uintptr_t this_start, this_end, next_start, brk;
@@ -2161,7 +2162,7 @@ static uintptr_t pgb_find_hole(uintptr_t guest_loaddr, uintptr_t guest_size,
     brk = (uintptr_t)sbrk(0);
 
     if (!maps) {
-        return pgd_find_hole_fallback(guest_size, brk, align);
+        return pgd_find_hole_fallback(guest_size, brk, align, offset);
     }
 
     /* The first hole is before the first map entry. */
@@ -2173,7 +2174,7 @@ static uintptr_t pgb_find_hole(uintptr_t guest_loaddr, uintptr_t guest_size,
 
         this_end = ((MapInfo *)iter->data)->start;
         next_start = ((MapInfo *)iter->data)->end;
-        align_start = ROUND_UP(this_start, align);
+        align_start = ROUND_UP(this_start + offset, align);
 
         /* Skip holes that are too small. */
         if (align_start >= this_end) {
@@ -2223,6 +2224,7 @@ static void pgb_static(const char *image_name, abi_ulong orig_loaddr,
 {
     uintptr_t loaddr = orig_loaddr;
     uintptr_t hiaddr = orig_hiaddr;
+    uintptr_t offset = 0;
     uintptr_t addr;
 
     if (hiaddr != orig_hiaddr) {
@@ -2236,18 +2238,19 @@ static void pgb_static(const char *image_name, abi_ulong orig_loaddr,
     if (ARM_COMMPAGE) {
         /*
          * Extend the allocation to include the commpage.
-         * For a 64-bit host, this is just 4GiB; for a 32-bit host,
-         * the address arithmetic will wrap around, but the difference
-         * will produce the correct allocation size.
+         * For a 64-bit host, this is just 4GiB; for a 32-bit host we
+         * need to ensure there is space bellow the guest_base so we
+         * can map the commpage in the place needed when the address
+         * arithmetic wraps around.
          */
         if (sizeof(uintptr_t) == 8 || loaddr >= 0x80000000u) {
-            hiaddr = (uintptr_t)4 << 30;
+            hiaddr = (uintptr_t) 4 << 30;
         } else {
-            loaddr = ARM_COMMPAGE & -align;
+            offset = -(ARM_COMMPAGE & -align);
         }
     }
 
-    addr = pgb_find_hole(loaddr, hiaddr - loaddr, align);
+    addr = pgb_find_hole(loaddr, hiaddr - loaddr, align, offset);
     if (addr == -1) {
         /*
          * If ARM_COMMPAGE, there *might* be a non-consecutive allocation
@@ -2282,7 +2285,7 @@ static void pgb_dynamic(const char *image_name, long align)
          * just above that, and maximises the positive guest addresses.
          */
         commpage = ARM_COMMPAGE & -align;
-        addr = pgb_find_hole(commpage, -commpage, align);
+        addr = pgb_find_hole(commpage, -commpage, align, 0);
         assert(addr != -1);
         guest_base = addr;
     }
-- 
2.20.1



We rely on the pointer to wrap when accessing the high address of the
COMMPAGE so it lands somewhere reasonable. However on 32 bit hosts we
cannot afford just to map the entire 4gb address range. The old mmap
trial and error code handled this by just checking we could map both
the guest_base and the computed COMMPAGE address.

We can't just manipulate loadaddr to get what we want so we introduce
an offset which pgb_find_hole can apply when looking for a gap for
guest_base that ensures there is space left to map the COMMPAGE
afterwards.

This is arguably a little inefficient for the one 32 bit
value (kuser_helper_version) we need to keep there given all the
actual code entries are picked up during the translation phase.

Fixes: ee94743034b
Bug: https://bugs.launchpad.net/qemu/+bug/1880225
Cc: Bug 1880225 <email address hidden>
Signed-off-by: Alex Bennée <email address hidden>
Tested-by: Aleksandar Markovic <email address hidden>
Cc: Richard Henderson <email address hidden>
Cc: Peter Maydell <email address hidden>
Message-Id: <email address hidden>

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 475d243f3bd..b5cb21384a1 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -389,7 +389,7 @@ 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);
+                      MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);
 
     if (addr == MAP_FAILED) {
         perror("Allocating guest commpage");
@@ -2113,7 +2113,8 @@ static void pgb_have_guest_base(const char *image_name, abi_ulong guest_loaddr,
  * only dumbly iterate up the host address space seeing if the
  * allocation would work.
  */
-static uintptr_t pgd_find_hole_fallback(uintptr_t guest_size, uintptr_t brk, long align)
+static uintptr_t pgd_find_hole_fallback(uintptr_t guest_size, uintptr_t brk,
+                                        long align, uintptr_t offset)
 {
     uintptr_t base;
 
@@ -2123,7 +2124,7 @@ static uintptr_t pgd_find_hole_fallback(uintptr_t guest_size, uintptr_t brk, lon
     while (true) {
         uintptr_t align_start, end;
         align_start = ROUND_UP(base, align);
-        end = align_start + guest_size;
+        end = align_start + guest_size + offset;
 
         /* if brk is anywhere in the range give ourselves some room to grow. */
         if (align_start <= brk && brk < end) {
@@ -2138,7 +2139,7 @@ static uintptr_t pgd_find_hole_fallback(uintptr_t guest_size, uintptr_t brk, lon
                                      PROT_NONE, flags, -1, 0);
             if (mmap_start != MAP_FAILED) {
                 munmap((void *) align_start, guest_size);
-                return (uintptr_t) mmap_start;
+                return (uintptr_t) mmap_start + offset;
             }
             base += qemu_host_page_size;
         }
@@ -2147,7 +2148,7 @@ static uintptr_t pgd_find_hole_fallback(uintptr_t guest_size, uintptr_t brk, lon
 
 /* Return value for guest_base, or -1 if no hole found. */
 static uintptr_t pgb_find_hole(uintptr_t guest_loaddr, uintptr_t guest_size,
-                               long align)
+                               long align, uintptr_t offset)
 {
     GSList *maps, *iter;
     uintptr_t this_start, this_end, next_start, brk;
@@ -2161,7 +2162,7 @@ static uintptr_t pgb_find_hole(uintptr_t guest_loaddr, uintptr_t guest_size,
     brk = (uintptr_t)sbrk(0);
 
     if (!maps) {
-        return pgd_find_hole_fallback(guest_size, brk, align);
+        return pgd_find_hole_fallback(guest_size, brk, align, offset);
     }
 
     /* The first hole is before the first map entry. */
@@ -2173,7 +2174,7 @@ static uintptr_t pgb_find_hole(uintptr_t guest_loaddr, uintptr_t guest_size,
 
         this_end = ((MapInfo *)iter->data)->start;
         next_start = ((MapInfo *)iter->data)->end;
-        align_start = ROUND_UP(this_start, align);
+        align_start = ROUND_UP(this_start + offset, align);
 
         /* Skip holes that are too small. */
         if (align_start >= this_end) {
@@ -2223,6 +2224,7 @@ static void pgb_static(const char *image_name, abi_ulong orig_loaddr,
 {
     uintptr_t loaddr = orig_loaddr;
     uintptr_t hiaddr = orig_hiaddr;
+    uintptr_t offset = 0;
     uintptr_t addr;
 
     if (hiaddr != orig_hiaddr) {
@@ -2236,18 +2238,19 @@ static void pgb_static(const char *image_name, abi_ulong orig_loaddr,
     if (ARM_COMMPAGE) {
         /*
          * Extend the allocation to include the commpage.
-         * For a 64-bit host, this is just 4GiB; for a 32-bit host,
-         * the address arithmetic will wrap around, but the difference
-         * will produce the correct allocation size.
+         * For a 64-bit host, this is just 4GiB; for a 32-bit host we
+         * need to ensure there is space bellow the guest_base so we
+         * can map the commpage in the place needed when the address
+         * arithmetic wraps around.
          */
         if (sizeof(uintptr_t) == 8 || loaddr >= 0x80000000u) {
-            hiaddr = (uintptr_t)4 << 30;
+            hiaddr = (uintptr_t) 4 << 30;
         } else {
-            loaddr = ARM_COMMPAGE & -align;
+            offset = -(ARM_COMMPAGE & -align);
         }
     }
 
-    addr = pgb_find_hole(loaddr, hiaddr - loaddr, align);
+    addr = pgb_find_hole(loaddr, hiaddr - loaddr, align, offset);
     if (addr == -1) {
         /*
          * If ARM_COMMPAGE, there *might* be a non-consecutive allocation
@@ -2282,7 +2285,7 @@ static void pgb_dynamic(const char *image_name, long align)
          * just above that, and maximises the positive guest addresses.
          */
         commpage = ARM_COMMPAGE & -align;
-        addr = pgb_find_hole(commpage, -commpage, align);
+        addr = pgb_find_hole(commpage, -commpage, align, 0);
         assert(addr != -1);
         guest_base = addr;
     }
-- 
2.20.1



Fixed here:
https://git.qemu.org/?p=qemu.git;a=commitdiff;h=5c3e87f345ac93de9260f