[Qemu-devel] [PATCH 00/10] linux-user: Speed up guest space initialization on 32-bit ARM target

[Qemu-devel] [PATCH 00/10] linux-user: Speed up guest space initialization on 32-bit ARM target

[Luke Shumaker]

From: Luke Shumaker <lukeshu@parabola.nu>

The goal of this patchset is to fix
https://bugs.launchpad.net/qemu/+bug/1740219

The gist is that the current linear search for an acceptable address
range is a bad strategy when the reason we didn't get a good address
on the first try is that we've having problems mapping the commpage
for the 32-bit ARM target; especially if ASLR is disabled.

I think that only the final patch in this patchset is actually
necessary to fix the issue; but I didn't feel comfortable writing it
without also makeing the preceding (small) changes.

Luke Shumaker (10):
  linux-user: Use #if to only call validate_guest_space for 32-bit ARM
    target
  linux-user: Rename validate_guest_space => init_guest_commpage
  linux-user: init_guest_space: Clean up if we can't initialize the
    commpage
  linux-user: init_guest_space: Correctly handle guest_start in commpage
    initialization
  linux-user: init_guest_space: Clarify page alignment logic
  linux-user: init_guest_commpage: Add a comment about size check
  linux-user: init_guest_space: Clean up control flow a bit
  linux-user: init_guest_space: Don't try to align if we'll reject it
  linux-user: init_guest_space: Add a comment about search strategy
  linux-user: init_guest_space: Try to make ARM space+commpage
    continuous

 linux-user/elfload.c | 145 ++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 114 insertions(+), 31 deletions(-)

-- 
2.15.1

Happy hacking,
~ Luke Shumaker

[Qemu-devel] [PATCH 01/10] linux-user: Use #if to only call validate_guest_space for 32-bit ARM target

[Luke Shumaker]

From: Luke Shumaker <lukeshu@parabola.nu>

Instead of defining a bogus validate_guest_space that always returns 1 on
targets other than 32-bit ARM, use #if blocks to only call it on 32-bit ARM
targets.  This makes the "normal" flow control clearer.

Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
---
 linux-user/elfload.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 20f3d8c2c3..cac991159c 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -354,7 +354,6 @@ enum {
 
 /* The commpage only exists for 32 bit kernels */
 
-#define TARGET_HAS_VALIDATE_GUEST_SPACE
 /* 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.
@@ -1823,15 +1822,6 @@ static abi_ulong create_elf_tables(abi_ulong p, int argc, int envc,
     return sp;
 }
 
-#ifndef TARGET_HAS_VALIDATE_GUEST_SPACE
-/* If the guest doesn't have a validation function just agree */
-static int validate_guest_space(unsigned long guest_base,
-                                unsigned long guest_size)
-{
-    return 1;
-}
-#endif
-
 unsigned long init_guest_space(unsigned long host_start,
                                unsigned long host_size,
                                unsigned long guest_start,
@@ -1845,11 +1835,12 @@ unsigned long init_guest_space(unsigned long host_start,
     /* 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 (validate_guest_space(host_start, host_size) == 1) {
-            return host_start;
-        } else {
             return (unsigned long)-1;
         }
+#endif
+        return host_start;
     }
 
     /* Setup the initial flags and start address.  */
@@ -1888,6 +1879,8 @@ unsigned long init_guest_space(unsigned long host_start,
 
         /* Check to see if the address is valid.  */
         if (!host_start || real_start == current_start) {
+#if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
+            /* On 32-bit ARM, we need to also be able to map the commpage.  */
             int valid = validate_guest_space(real_start - guest_start,
                                              real_size);
             if (valid == 1) {
@@ -1896,6 +1889,10 @@ unsigned long init_guest_space(unsigned long host_start,
                 return (unsigned long)-1;
             }
             /* valid == 0, so try again. */
+#else
+            /* On other architectures, whatever we have here is fine.  */
+            break;
+#endif
         }
 
         /* That address didn't work.  Unmap and try a different one.
-- 
2.15.1

[Qemu-devel] [PATCH 02/10] linux-user: Rename validate_guest_space => init_guest_commpage

[Luke Shumaker]

From: Luke Shumaker <lukeshu@parabola.nu>

init_guest_commpage is a much more honest description of what the function
does.  validate_guest_space not only suggests that the function has no
side-effects, but also introduces confusion as to why it is only needed on
32-bit ARM targets.

Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
---
 linux-user/elfload.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index cac991159c..453394239c 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -362,8 +362,8 @@ enum {
  * The guest code may leave a page mapped and populate it if the
  * address is suitable.
  */
-static int validate_guest_space(unsigned long guest_base,
-                                unsigned long guest_size)
+static int init_guest_commpage(unsigned long guest_base,
+                               unsigned long guest_size)
 {
     unsigned long real_start, test_page_addr;
 
@@ -1836,7 +1836,7 @@ unsigned long init_guest_space(unsigned long host_start,
      * address.  */
     if (host_start && !host_size) {
 #if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
-        if (validate_guest_space(host_start, host_size) == 1) {
+        if (init_guest_commpage(host_start, host_size) != 1) {
             return (unsigned long)-1;
         }
 #endif
@@ -1881,8 +1881,8 @@ unsigned long init_guest_space(unsigned long host_start,
         if (!host_start || real_start == current_start) {
 #if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
             /* On 32-bit ARM, we need to also be able to map the commpage.  */
-            int valid = validate_guest_space(real_start - guest_start,
-                                             real_size);
+            int valid = init_guest_commpage(real_start - guest_start,
+                                            real_size);
             if (valid == 1) {
                 break;
             } else if (valid == -1) {
-- 
2.15.1

[Qemu-devel] [PATCH 03/10] linux-user: init_guest_space: Clean up if we can't initialize the commpage

[Luke Shumaker]

From: Luke Shumaker <lukeshu@parabola.nu>

We'll just exit with an error anyway, so it doesn't really matter, but it
is cleaned up in all of the other places were we error out.

Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
---
 linux-user/elfload.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 453394239c..1a6b660b25 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -1886,6 +1886,7 @@ unsigned long init_guest_space(unsigned long host_start,
             if (valid == 1) {
                 break;
             } else if (valid == -1) {
+                munmap((void *)real_start, host_size);
                 return (unsigned long)-1;
             }
             /* valid == 0, so try again. */
-- 
2.15.1

[Qemu-devel] [PATCH 04/10] linux-user: init_guest_space: Correctly handle guest_start in commpage initialization

[Luke Shumaker]

From: Luke Shumaker <lukeshu@parabola.nu>

init_guest_commpage  needs to check if the mapped space, which ends at
real_start+real_size overlaps with where it needs to put the commpage,
which is (assuming sane qemu_host_page_size) guest_base + 0xffff000, where
guest_base is real_start - guest_start.

    [guest_base][       0xffff0000      ][commpage]
    [guest_base][guest_start][real_size] [commpage]
    [       real_start      ][real_size] [commpage]
                                        ^
                                 fail if this gap < 0

Since init_guest_commpage wants to do everything relative to guest_base
(rather than real_start), it obviously needs to be comparing 0xffff0000
against guest_start+real_size, not just real_size.

This bug has been present since 806d102141b99d4f1e55a97d68b7ea8c8ba3129f in
2012, but guest_start is usually 0, and prior to v2.11 real_size was
usually much smaller than 0xffff0000, so it was uncommon for it to have
made a difference.

Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
---
 linux-user/elfload.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 1a6b660b25..f41cecc3cb 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -1882,7 +1882,7 @@ unsigned long init_guest_space(unsigned long host_start,
 #if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
             /* On 32-bit ARM, we need to also be able to map the commpage.  */
             int valid = init_guest_commpage(real_start - guest_start,
-                                            real_size);
+                                            real_size + guest_start);
             if (valid == 1) {
                 break;
             } else if (valid == -1) {
-- 
2.15.1

[Qemu-devel] [PATCH 05/10] linux-user: init_guest_space: Clarify page alignment logic

[Luke Shumaker]

From: Luke Shumaker <lukeshu@parabola.nu>

There are 3 parts to this change:
 - Add a comment showing the relative sizes and positions of the blocks of
   memory
 - introduce and use new aligned_{start,size} instead of adjusting
   real_{start_size}
 - When we clean up (on failure), munmap(real_start, real_size) instead of
   munmap(aligned_start, aligned_size).  It *shouldn't* make any
   difference, but I will admit that this does mean we are making the
   syscall with different values, so this isn't quite a no-op patch.

Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
---
 linux-user/elfload.c | 43 +++++++++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index f41cecc3cb..22f2632dfa 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -1827,7 +1827,7 @@ unsigned long init_guest_space(unsigned long host_start,
                                unsigned long guest_start,
                                bool fixed)
 {
-    unsigned long current_start, real_start;
+    unsigned long current_start, aligned_start;
     int flags;
 
     assert(host_start || host_size);
@@ -1853,7 +1853,8 @@ unsigned long init_guest_space(unsigned long host_start,
     /* Otherwise, a non-zero size region of memory needs to be mapped
      * and validated.  */
     while (1) {
-        unsigned long real_size = host_size;
+        unsigned long real_start, real_size, aligned_size;
+        aligned_size = real_size = host_size;
 
         /* Do not use mmap_find_vma here because that is limited to the
          * guest address space.  We are going to make the
@@ -1867,26 +1868,48 @@ unsigned long init_guest_space(unsigned long host_start,
 
         /* Ensure the address is properly aligned.  */
         if (real_start & ~qemu_host_page_mask) {
+            /* Ideally, we adjust like
+             *
+             *    pages: [  ][  ][  ][  ][  ]
+             *      old:   [   real   ]
+             *             [ aligned  ]
+             *      new:   [     real     ]
+             *               [ aligned  ]
+             *
+             * But if there is something else mapped right after it,
+             * then obviously it won't have room to grow, and the
+             * kernel will put the new larger real someplace else with
+             * unknown alignment (if we made it to here, then
+             * fixed=false).  Which is why we grow real by a full page
+             * size, instead of by part of one; so that even if we get
+             * moved, we can still guarantee alignment.  But this does
+             * mean that there is a padding of < 1 page both before
+             * and after the aligned range; the "after" could could
+             * cause problems for ARM emulation where it could butt in
+             * to where we need to put the commpage.
+             */
             munmap((void *)real_start, host_size);
-            real_size = host_size + qemu_host_page_size;
+            real_size = aligned_size + qemu_host_page_size;
             real_start = (unsigned long)
                 mmap((void *)real_start, real_size, PROT_NONE, flags, -1, 0);
             if (real_start == (unsigned long)-1) {
                 return (unsigned long)-1;
             }
-            real_start = HOST_PAGE_ALIGN(real_start);
+            aligned_start = HOST_PAGE_ALIGN(real_start);
+        } else {
+            aligned_start = real_start;
         }
 
         /* Check to see if the address is valid.  */
-        if (!host_start || real_start == current_start) {
+        if (!host_start || aligned_start == current_start) {
 #if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
             /* On 32-bit ARM, we need to also be able to map the commpage.  */
-            int valid = init_guest_commpage(real_start - guest_start,
-                                            real_size + guest_start);
+            int valid = init_guest_commpage(aligned_start - guest_start,
+                                            aligned_size + guest_start);
             if (valid == 1) {
                 break;
             } else if (valid == -1) {
-                munmap((void *)real_start, host_size);
+                munmap((void *)real_start, real_size);
                 return (unsigned long)-1;
             }
             /* valid == 0, so try again. */
@@ -1905,7 +1928,7 @@ unsigned long init_guest_space(unsigned long host_start,
          * address space randomization put a shared library somewhere
          * inconvenient.
          */
-        munmap((void *)real_start, host_size);
+        munmap((void *)real_start, real_size);
         current_start += qemu_host_page_size;
         if (host_start == current_start) {
             /* Theoretically possible if host doesn't have any suitably
@@ -1917,7 +1940,7 @@ unsigned long init_guest_space(unsigned long host_start,
 
     qemu_log_mask(CPU_LOG_PAGE, "Reserved 0x%lx bytes of guest address space\n", host_size);
 
-    return real_start;
+    return aligned_start;
 }
 
 static void probe_guest_base(const char *image_name,
-- 
2.15.1

[Qemu-devel] [PATCH 06/10] linux-user: init_guest_commpage: Add a comment about size check

[Luke Shumaker]

From: Luke Shumaker <lukeshu@parabola.nu>

Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
---
 linux-user/elfload.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 22f2632dfa..b560f5d6fe 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -374,6 +374,11 @@ static int init_guest_commpage(unsigned long guest_base,
 
     /* 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)) {
-- 
2.15.1

[Qemu-devel] [PATCH 07/10] linux-user: init_guest_space: Clean up control flow a bit

[Luke Shumaker]

From: Luke Shumaker <lukeshu@parabola.nu>

Instead of doing

        if (check1) {
            if (check2) {
               success;
            }
        }

        retry;

Do a clearer

        if (!check1) {
           goto try_again;
        }

        if (!check2) {
           goto try_again;
        }

        success;

    try_again:
        retry;

Besides being clearer, this makes it easier to insert more checks that
need to trigger a retry on check failure, or rearrange them, or anything
like that.

Because some indentation is changing, "ignore space change" may be useful
for viewing this patch.

Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
---
 linux-user/elfload.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index b560f5d6fe..5c0ad65611 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -1906,24 +1906,28 @@ unsigned long init_guest_space(unsigned long host_start,
         }
 
         /* Check to see if the address is valid.  */
-        if (!host_start || aligned_start == current_start) {
+        if (host_start && aligned_start != current_start) {
+            goto try_again;
+        }
+
 #if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
-            /* On 32-bit ARM, we need to also be able to map the commpage.  */
-            int valid = init_guest_commpage(aligned_start - guest_start,
-                                            aligned_size + guest_start);
-            if (valid == 1) {
-                break;
-            } else if (valid == -1) {
-                munmap((void *)real_start, real_size);
-                return (unsigned long)-1;
-            }
-            /* valid == 0, so try again. */
-#else
-            /* On other architectures, whatever we have here is fine.  */
-            break;
-#endif
+        /* On 32-bit ARM, we need to also be able to map the commpage.  */
+        int valid = init_guest_commpage(aligned_start - guest_start,
+                                        aligned_size + guest_start);
+        if (valid == -1) {
+            munmap((void *)real_start, real_size);
+            return (unsigned long)-1;
+        } else if (valid == -1) {
+            goto try_again;
         }
+#endif
+
+        /* If nothing has said `return -1` or `goto try_again` yet,
+         * then the address we have is good.
+         */
+        break;
 
+    try_again:
         /* 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
-- 
2.15.1

[Qemu-devel] [PATCH 08/10] linux-user: init_guest_space: Don't try to align if we'll reject it

[Luke Shumaker]

From: Luke Shumaker <lukeshu@parabola.nu>

If the ensure-alignment code gets triggered, then the
"if (host_start && real_start != current_start)" check will always trigger,
so save 2 syscalls and put that check first.

Note that we can't just switch to using MAP_FIXED for that check, because
then we couldn't differentiate between a failure because "there isn't
enough space" and "there isn't enough space *here*".

Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
---
 linux-user/elfload.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 5c0ad65611..1b7583d659 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -1871,6 +1871,11 @@ unsigned long init_guest_space(unsigned long host_start,
             return (unsigned long)-1;
         }
 
+        /* Check to see if the address is valid.  */
+        if (host_start && real_start != current_start) {
+            goto try_again;
+        }
+
         /* Ensure the address is properly aligned.  */
         if (real_start & ~qemu_host_page_mask) {
             /* Ideally, we adjust like
@@ -1905,11 +1910,6 @@ unsigned long init_guest_space(unsigned long host_start,
             aligned_start = real_start;
         }
 
-        /* Check to see if the address is valid.  */
-        if (host_start && aligned_start != current_start) {
-            goto try_again;
-        }
-
 #if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
         /* On 32-bit ARM, we need to also be able to map the commpage.  */
         int valid = init_guest_commpage(aligned_start - guest_start,
-- 
2.15.1

[Qemu-devel] [PATCH 09/10] linux-user: init_guest_space: Add a comment about search strategy

[Luke Shumaker]

From: Luke Shumaker <lukeshu@parabola.nu>

Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
---
 linux-user/elfload.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 1b7583d659..7736ea2c3a 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -1936,6 +1936,10 @@ unsigned long init_guest_space(unsigned long host_start,
          * happen often.  Probably means we got unlucky and host
          * address space randomization put a shared library somewhere
          * inconvenient.
+         *
+         * This is probably a good strategy if host_start, but is
+         * probably a bad strategy if not, which means we got here
+         * because of trouble with ARM commpage setup.
          */
         munmap((void *)real_start, real_size);
         current_start += qemu_host_page_size;
-- 
2.15.1

[Qemu-devel] [PATCH 10/10] linux-user: init_guest_space: Try to make ARM space+commpage continuous

[Luke Shumaker]

From: Luke Shumaker <lukeshu@parabola.nu>

At a fixed distance after the usable memory that init_guest_space maps, for
32-bit ARM targets we also need to map a commpage.  The normal
init_guest_space logic doesn't keep this in mind when searching for an
address range.

If !host_start, then try to find a big continuous segment where we can put
both the usable memory and the commpage; we then munmap that segment and
set current_start to that address; and let the normal code mmap the usable
memory and the commpage separately.  That is: if we don't have hint of
where to start looking for memory, come up with one that is better than
NULL.  Depending on host_size and guest_start, there may or may not be a
gap between the usable memory and the commpage, so this is slightly more
restrictive than it needs to be; but it's only a hint, so that's OK.

We only do that for !host start, because if host_start, then either:
 - we got an address passed in with -B, in which case we don't want to
   interfere with what the user said;
 - or host_start is based off of the ELF image's loaddr.  The check "if
   (host_start && real_start != current_start)" suggests that we really
   want lowest available address that is >= loaddr.  I don't know why that
   is, but I'm trusting that Paul Brook knew what he was doing when he
   wrote the original version of that check in
   c581deda322080e8beb88b2e468d4af54454e4b3 way back in 2010.

Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
---
 linux-user/elfload.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 7736ea2c3a..cd3a7d877d 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -1857,6 +1857,55 @@ unsigned long init_guest_space(unsigned long host_start,
 
     /* Otherwise, a non-zero size region of memory needs to be mapped
      * and validated.  */
+
+#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;
+        }
+        munmap((void *)real_start, host_full_size);
+        if (real_start & ~qemu_host_page_mask) {
+            /* The same thing again, but with an extra qemu_host_page_size
+             * 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 = HOST_PAGE_ALIGN(real_start);
+        }
+        current_start = real_start;
+    }
+ naive:
+#endif
+
     while (1) {
         unsigned long real_start, real_size, aligned_size;
         aligned_size = real_size = host_size;
-- 
2.15.1

Re: [Qemu-devel] [PATCH 00/10] linux-user: Speed up guest space initialization on 32-bit ARM target

[Luke Shumaker]

Ping for code review?

-- 
Happy hacking,
~ Luke Shumaker

Re: [Qemu-devel] [PATCH 00/10] linux-user: Speed up guest space initialization on 32-bit ARM target

[Luke Shumaker]

Ping.

On Thu, 28 Dec 2017 13:08:03 -0500,
Luke Shumaker wrote:
> 
> From: Luke Shumaker <lukeshu@parabola.nu>
> 
> The goal of this patchset is to fix
> https://bugs.launchpad.net/qemu/+bug/1740219
> 
> The gist is that the current linear search for an acceptable address
> range is a bad strategy when the reason we didn't get a good address
> on the first try is that we've having problems mapping the commpage
> for the 32-bit ARM target; especially if ASLR is disabled.
> 
> I think that only the final patch in this patchset is actually
> necessary to fix the issue; but I didn't feel comfortable writing it
> without also makeing the preceding (small) changes.
> 
> Luke Shumaker (10):
>   linux-user: Use #if to only call validate_guest_space for 32-bit ARM
>     target
>   linux-user: Rename validate_guest_space => init_guest_commpage
>   linux-user: init_guest_space: Clean up if we can't initialize the
>     commpage
>   linux-user: init_guest_space: Correctly handle guest_start in commpage
>     initialization
>   linux-user: init_guest_space: Clarify page alignment logic
>   linux-user: init_guest_commpage: Add a comment about size check
>   linux-user: init_guest_space: Clean up control flow a bit
>   linux-user: init_guest_space: Don't try to align if we'll reject it
>   linux-user: init_guest_space: Add a comment about search strategy
>   linux-user: init_guest_space: Try to make ARM space+commpage
>     continuous
> 
>  linux-user/elfload.c | 145 ++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 114 insertions(+), 31 deletions(-)
> 
> -- 
> 2.15.1
> 
> Happy hacking,
> ~ Luke Shumaker
> 

Re: [Qemu-devel] [PATCH 01/10] linux-user: Use #if to only call validate_guest_space for 32-bit ARM target

[Peter Maydell]

On 28 December 2017 at 18:08, Luke Shumaker <lukeshu@lukeshu.com> wrote:
> From: Luke Shumaker <lukeshu@parabola.nu>
>
> Instead of defining a bogus validate_guest_space that always returns 1 on
> targets other than 32-bit ARM, use #if blocks to only call it on 32-bit ARM
> targets.  This makes the "normal" flow control clearer.
>
> Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
> ---
>  linux-user/elfload.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 20f3d8c2c3..cac991159c 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -354,7 +354,6 @@ enum {
>
>  /* The commpage only exists for 32 bit kernels */
>
> -#define TARGET_HAS_VALIDATE_GUEST_SPACE
>  /* 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.
> @@ -1823,15 +1822,6 @@ static abi_ulong create_elf_tables(abi_ulong p, int argc, int envc,
>      return sp;
>  }
>
> -#ifndef TARGET_HAS_VALIDATE_GUEST_SPACE
> -/* If the guest doesn't have a validation function just agree */
> -static int validate_guest_space(unsigned long guest_base,
> -                                unsigned long guest_size)
> -{
> -    return 1;
> -}
> -#endif
> -
>  unsigned long init_guest_space(unsigned long host_start,
>                                 unsigned long host_size,
>                                 unsigned long guest_start,
> @@ -1845,11 +1835,12 @@ unsigned long init_guest_space(unsigned long host_start,
>      /* If just a starting address is given, then just verify that
>       * address.  */
>      if (host_start && !host_size) {
> +#if defined(TARGET_ARM) && !defined(TARGET_AARCH64)

I would strongly prefer us not to add new "these targets do
this" ifdefs, please. The current approach means that any
target can say it needs an implementation of this hook by
providing one and defining the TARGET_HAS_VALIDATE_GUEST_SPACE
macro to say so. I think that's a better approach.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 03/10] linux-user: init_guest_space: Clean up if we can't initialize the commpage

[Peter Maydell]

On 28 December 2017 at 18:08, Luke Shumaker <lukeshu@lukeshu.com> wrote:
> From: Luke Shumaker <lukeshu@parabola.nu>
>
> We'll just exit with an error anyway, so it doesn't really matter, but it
> is cleaned up in all of the other places were we error out.
>
> Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
> ---
>  linux-user/elfload.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 453394239c..1a6b660b25 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -1886,6 +1886,7 @@ unsigned long init_guest_space(unsigned long host_start,
>              if (valid == 1) {
>                  break;
>              } else if (valid == -1) {
> +                munmap((void *)real_start, host_size);
>                  return (unsigned long)-1;
>              }
>              /* valid == 0, so try again. */
> --
> 2.15.1

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 01/10] linux-user: Use #if to only call validate_guest_space for 32-bit ARM target

[Peter Maydell]

On 23 February 2018 at 18:35, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 28 December 2017 at 18:08, Luke Shumaker <lukeshu@lukeshu.com> wrote:
>> From: Luke Shumaker <lukeshu@parabola.nu>
>>
>> Instead of defining a bogus validate_guest_space that always returns 1 on
>> targets other than 32-bit ARM, use #if blocks to only call it on 32-bit ARM
>> targets.  This makes the "normal" flow control clearer.
>>
>> Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>

>> @@ -1845,11 +1835,12 @@ unsigned long init_guest_space(unsigned long host_start,
>>      /* If just a starting address is given, then just verify that
>>       * address.  */
>>      if (host_start && !host_size) {
>> +#if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
>
> I would strongly prefer us not to add new "these targets do
> this" ifdefs, please. The current approach means that any
> target can say it needs an implementation of this hook by
> providing one and defining the TARGET_HAS_VALIDATE_GUEST_SPACE
> macro to say so. I think that's a better approach.

Looking through some of the rest of this patchset I might change
my mind on that (the code in master is very confusing). I won't
have time to get to this til Tuesday now, though.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 01/10] linux-user: Use #if to only call validate_guest_space for 32-bit ARM target

[Peter Maydell]

On 28 December 2017 at 18:08, Luke Shumaker <lukeshu@lukeshu.com> wrote:
> From: Luke Shumaker <lukeshu@parabola.nu>
>
> Instead of defining a bogus validate_guest_space that always returns 1 on
> targets other than 32-bit ARM, use #if blocks to only call it on 32-bit ARM
> targets.  This makes the "normal" flow control clearer.
>
> Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>

Yes, I think on reflection that you're right and we're better off
leaving this arm-specific. If/when we find another target that needs
special handling, we can look at a more generic API then.

> @@ -1845,11 +1835,12 @@ unsigned long init_guest_space(unsigned long host_start,
>      /* 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 (validate_guest_space(host_start, host_size) == 1) {
> -            return host_start;
> -        } else {
>              return (unsigned long)-1;
>          }
> +#endif
> +        return host_start;

This is accidentally changing behaviour -- if validate_guest_space()
returned 1 we used to return host_start, and now we return -1.
The condition should be "!= 1". This change is made in patch 2,
but it should be done here, to avoid breaking bisection.

Otherwise

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 02/10] linux-user: Rename validate_guest_space => init_guest_commpage

[Peter Maydell]

On 28 December 2017 at 18:08, Luke Shumaker <lukeshu@lukeshu.com> wrote:
> From: Luke Shumaker <lukeshu@parabola.nu>
>
> init_guest_commpage is a much more honest description of what the function
> does.  validate_guest_space not only suggests that the function has no
> side-effects, but also introduces confusion as to why it is only needed on
> 32-bit ARM targets.
>
> Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
> ---
>  linux-user/elfload.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index cac991159c..453394239c 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -362,8 +362,8 @@ enum {
>   * The guest code may leave a page mapped and populate it if the
>   * address is suitable.
>   */
> -static int validate_guest_space(unsigned long guest_base,
> -                                unsigned long guest_size)
> +static int init_guest_commpage(unsigned long guest_base,
> +                               unsigned long guest_size)
>  {
>      unsigned long real_start, test_page_addr;
>
> @@ -1836,7 +1836,7 @@ unsigned long init_guest_space(unsigned long host_start,
>       * address.  */
>      if (host_start && !host_size) {
>  #if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
> -        if (validate_guest_space(host_start, host_size) == 1) {
> +        if (init_guest_commpage(host_start, host_size) != 1) {

this is the bit that includes the '==' to '!=' change that should
be in patch 1.

Otherwise

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 04/10] linux-user: init_guest_space: Correctly handle guest_start in commpage initialization

[Peter Maydell]

On 28 December 2017 at 18:08, Luke Shumaker <lukeshu@lukeshu.com> wrote:
> From: Luke Shumaker <lukeshu@parabola.nu>
>
> init_guest_commpage  needs to check if the mapped space, which ends at
> real_start+real_size overlaps with where it needs to put the commpage,
> which is (assuming sane qemu_host_page_size) guest_base + 0xffff000, where
> guest_base is real_start - guest_start.
>
>     [guest_base][       0xffff0000      ][commpage]
>     [guest_base][guest_start][real_size] [commpage]
>     [       real_start      ][real_size] [commpage]
>                                         ^
>                                  fail if this gap < 0
>
> Since init_guest_commpage wants to do everything relative to guest_base
> (rather than real_start), it obviously needs to be comparing 0xffff0000
> against guest_start+real_size, not just real_size.
>
> This bug has been present since 806d102141b99d4f1e55a97d68b7ea8c8ba3129f in
> 2012, but guest_start is usually 0, and prior to v2.11 real_size was
> usually much smaller than 0xffff0000, so it was uncommon for it to have
> made a difference.
>
> Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
> ---
>  linux-user/elfload.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 1a6b660b25..f41cecc3cb 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -1882,7 +1882,7 @@ unsigned long init_guest_space(unsigned long host_start,
>  #if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
>              /* On 32-bit ARM, we need to also be able to map the commpage.  */
>              int valid = init_guest_commpage(real_start - guest_start,
> -                                            real_size);
> +                                            real_size + guest_start);
>              if (valid == 1) {
>                  break;
>              } else if (valid == -1) {

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 05/10] linux-user: init_guest_space: Clarify page alignment logic

[Peter Maydell]

On 28 December 2017 at 18:08, Luke Shumaker <lukeshu@lukeshu.com> wrote:
> From: Luke Shumaker <lukeshu@parabola.nu>
>
> There are 3 parts to this change:
>  - Add a comment showing the relative sizes and positions of the blocks of
>    memory
>  - introduce and use new aligned_{start,size} instead of adjusting
>    real_{start_size}
>  - When we clean up (on failure), munmap(real_start, real_size) instead of
>    munmap(aligned_start, aligned_size).  It *shouldn't* make any
>    difference, but I will admit that this does mean we are making the
>    syscall with different values, so this isn't quite a no-op patch.
>
> Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
> ---
>  linux-user/elfload.c | 43 +++++++++++++++++++++++++++++++++----------
>  1 file changed, 33 insertions(+), 10 deletions(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 06/10] linux-user: init_guest_commpage: Add a comment about size check

[Peter Maydell]

On 28 December 2017 at 18:08, Luke Shumaker <lukeshu@lukeshu.com> wrote:
> From: Luke Shumaker <lukeshu@parabola.nu>
>
> Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
> ---
>  linux-user/elfload.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 22f2632dfa..b560f5d6fe 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -374,6 +374,11 @@ static int init_guest_commpage(unsigned long guest_base,
>
>      /* 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)) {
> --

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 07/10] linux-user: init_guest_space: Clean up control flow a bit

[Peter Maydell]

On 28 December 2017 at 18:08, Luke Shumaker <lukeshu@lukeshu.com> wrote:
> From: Luke Shumaker <lukeshu@parabola.nu>
>
> Instead of doing
>
>         if (check1) {
>             if (check2) {
>                success;
>             }
>         }
>
>         retry;
>
> Do a clearer
>
>         if (!check1) {
>            goto try_again;
>         }
>
>         if (!check2) {
>            goto try_again;
>         }
>
>         success;
>
>     try_again:
>         retry;
>
> Besides being clearer, this makes it easier to insert more checks that
> need to trigger a retry on check failure, or rearrange them, or anything
> like that.
>
> Because some indentation is changing, "ignore space change" may be useful
> for viewing this patch.
>
> Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
> ---
>  linux-user/elfload.c | 34 +++++++++++++++++++---------------
>  1 file changed, 19 insertions(+), 15 deletions(-)
>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 08/10] linux-user: init_guest_space: Don't try to align if we'll reject it

[Peter Maydell]

On 28 December 2017 at 18:08, Luke Shumaker <lukeshu@lukeshu.com> wrote:
> From: Luke Shumaker <lukeshu@parabola.nu>
>
> If the ensure-alignment code gets triggered, then the
> "if (host_start && real_start != current_start)" check will always trigger,
> so save 2 syscalls and put that check first.
>
> Note that we can't just switch to using MAP_FIXED for that check, because
> then we couldn't differentiate between a failure because "there isn't
> enough space" and "there isn't enough space *here*".
>
> Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 09/10] linux-user: init_guest_space: Add a comment about search strategy

[Peter Maydell]

On 28 December 2017 at 18:08, Luke Shumaker <lukeshu@lukeshu.com> wrote:
> From: Luke Shumaker <lukeshu@parabola.nu>
>
> Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
> ---
>  linux-user/elfload.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 1b7583d659..7736ea2c3a 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -1936,6 +1936,10 @@ unsigned long init_guest_space(unsigned long host_start,
>           * happen often.  Probably means we got unlucky and host
>           * address space randomization put a shared library somewhere
>           * inconvenient.
> +         *
> +         * This is probably a good strategy if host_start, but is
> +         * probably a bad strategy if not, which means we got here
> +         * because of trouble with ARM commpage setup.
>           */
>          munmap((void *)real_start, real_size);
>          current_start += qemu_host_page_size;
> --
> 2.15.1

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 10/10] linux-user: init_guest_space: Try to make ARM space+commpage continuous

[Peter Maydell]

On 28 December 2017 at 18:08, Luke Shumaker <lukeshu@lukeshu.com> wrote:
> From: Luke Shumaker <lukeshu@parabola.nu>
>
> At a fixed distance after the usable memory that init_guest_space maps, for
> 32-bit ARM targets we also need to map a commpage.  The normal
> init_guest_space logic doesn't keep this in mind when searching for an
> address range.
>
> If !host_start, then try to find a big continuous segment where we can put
> both the usable memory and the commpage; we then munmap that segment and
> set current_start to that address; and let the normal code mmap the usable
> memory and the commpage separately.  That is: if we don't have hint of
> where to start looking for memory, come up with one that is better than
> NULL.  Depending on host_size and guest_start, there may or may not be a
> gap between the usable memory and the commpage, so this is slightly more
> restrictive than it needs to be; but it's only a hint, so that's OK.
>
> We only do that for !host start, because if host_start, then either:
>  - we got an address passed in with -B, in which case we don't want to
>    interfere with what the user said;
>  - or host_start is based off of the ELF image's loaddr.  The check "if
>    (host_start && real_start != current_start)" suggests that we really
>    want lowest available address that is >= loaddr.  I don't know why that
>    is, but I'm trusting that Paul Brook knew what he was doing when he
>    wrote the original version of that check in
>    c581deda322080e8beb88b2e468d4af54454e4b3 way back in 2010.
>
> Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
> ---
>  linux-user/elfload.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
>
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 7736ea2c3a..cd3a7d877d 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -1857,6 +1857,55 @@ unsigned long init_guest_space(unsigned long host_start,
>
>      /* Otherwise, a non-zero size region of memory needs to be mapped
>       * and validated.  */
> +
> +#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;

I think this is probably more clearly written as 0x100000000ULL,
since rounding down to the host-page-size then adding the host-page-size
gets us the full 32-bit size of the guest address space.

That shows up that there's a potential problem here if the host
is 32-bit, because in that case guest_full_size (being only unsigned
long) will be 0, and we'll end up trying an mmap with an incorrect size.

> +        host_full_size = guest_full_size - guest_start;
> +        real_start = (unsigned long)
> +            mmap(NULL, host_full_size, PROT_NONE, flags, -1, 0);

I think the general approach is right, though. Sorry it took so long
for us to get to reviewing this patchset.

Incidentally, this code would be rather less complicated if it didn't
have to account for qemu_host_page_size not actually being the host
page size (since then you couldn't get a return from mmap() that wasn't
aligned properly). Does anybody know why we allow the user to specify
it on the command line? (git revision history doesn't help, it just says
there's been a -pagesize argument since commit 54936004fddc5 in 2003,
right back when mmap emulation was first added...)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 10/10] linux-user: init_guest_space: Try to make ARM space+commpage continuous

[Richard Henderson]

On 03/02/2018 06:13 AM, Peter Maydell wrote:
> Does anybody know why we allow the user to specify
> it on the command line? (git revision history doesn't help, it just says
> there's been a -pagesize argument since commit 54936004fddc5 in 2003,
> right back when mmap emulation was first added...)

I *think* it is to allow two different hosts with different page sizes to
achieve the same memory layout, so that tcg traces are more easily compared, to
root out bugs in the second host's tcg backend.

That's the only thing that I've used it for, anyway.


r~

Re: [Qemu-devel] [PATCH 01/10] linux-user: Use #if to only call validate_guest_space for 32-bit ARM target

[Laurent Vivier]

Le 28/12/2017 à 19:08, Luke Shumaker a écrit :
> From: Luke Shumaker <lukeshu@parabola.nu>
> 
> Instead of defining a bogus validate_guest_space that always returns 1 on
> targets other than 32-bit ARM, use #if blocks to only call it on 32-bit ARM
> targets.  This makes the "normal" flow control clearer.
> 
> Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
> ---
>  linux-user/elfload.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)

With the change request by Peter (condition "!= 1"),
applied to my 'linux-user-for-2.12' branch.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH 02/10] linux-user: Rename validate_guest_space => init_guest_commpage

[Laurent Vivier]

Le 28/12/2017 à 19:08, Luke Shumaker a écrit :
> From: Luke Shumaker <lukeshu@parabola.nu>
> 
> init_guest_commpage is a much more honest description of what the function
> does.  validate_guest_space not only suggests that the function has no
> side-effects, but also introduces confusion as to why it is only needed on
> 32-bit ARM targets.
> 
> Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
> ---
>  linux-user/elfload.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 

Applied to my 'linux-user-for-2.12' branch.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH 03/10] linux-user: init_guest_space: Clean up if we can't initialize the commpage

[Laurent Vivier]

Le 28/12/2017 à 19:08, Luke Shumaker a écrit :
> From: Luke Shumaker <lukeshu@parabola.nu>
> 
> We'll just exit with an error anyway, so it doesn't really matter, but it
> is cleaned up in all of the other places were we error out.
> 
> Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
> ---
>  linux-user/elfload.c | 1 +
>  1 file changed, 1 insertion(+)

Applied to my 'linux-user-for-2.12' branch.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH 04/10] linux-user: init_guest_space: Correctly handle guest_start in commpage initialization

[Laurent Vivier]

Le 28/12/2017 à 19:08, Luke Shumaker a écrit :
> From: Luke Shumaker <lukeshu@parabola.nu>
> 
> init_guest_commpage  needs to check if the mapped space, which ends at
> real_start+real_size overlaps with where it needs to put the commpage,
> which is (assuming sane qemu_host_page_size) guest_base + 0xffff000, where
> guest_base is real_start - guest_start.
> 
>     [guest_base][       0xffff0000      ][commpage]
>     [guest_base][guest_start][real_size] [commpage]
>     [       real_start      ][real_size] [commpage]
>                                         ^
>                                  fail if this gap < 0
> 
> Since init_guest_commpage wants to do everything relative to guest_base
> (rather than real_start), it obviously needs to be comparing 0xffff0000
> against guest_start+real_size, not just real_size.
> 
> This bug has been present since 806d102141b99d4f1e55a97d68b7ea8c8ba3129f in
> 2012, but guest_start is usually 0, and prior to v2.11 real_size was
> usually much smaller than 0xffff0000, so it was uncommon for it to have
> made a difference.
> 
> Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
> ---
>  linux-user/elfload.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied to my 'linux-user-for-2.12' branch.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH 05/10] linux-user: init_guest_space: Clarify page alignment logic

[Laurent Vivier]

Le 28/12/2017 à 19:08, Luke Shumaker a écrit :
> From: Luke Shumaker <lukeshu@parabola.nu>
> 
> There are 3 parts to this change:
>  - Add a comment showing the relative sizes and positions of the blocks of
>    memory
>  - introduce and use new aligned_{start,size} instead of adjusting
>    real_{start_size}
>  - When we clean up (on failure), munmap(real_start, real_size) instead of
>    munmap(aligned_start, aligned_size).  It *shouldn't* make any
>    difference, but I will admit that this does mean we are making the
>    syscall with different values, so this isn't quite a no-op patch.
> 
> Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
> ---
>  linux-user/elfload.c | 43 +++++++++++++++++++++++++++++++++----------
>  1 file changed, 33 insertions(+), 10 deletions(-)
> 

Applied to my 'linux-user-for-2.12' branch.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH 06/10] linux-user: init_guest_commpage: Add a comment about size check

[Laurent Vivier]

Le 28/12/2017 à 19:08, Luke Shumaker a écrit :
> From: Luke Shumaker <lukeshu@parabola.nu>
> 
> Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
> ---
>  linux-user/elfload.c | 5 +++++
>  1 file changed, 5 insertions(+)

Applied to my 'linux-user-for-2.12' branch.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH 07/10] linux-user: init_guest_space: Clean up control flow a bit

[Laurent Vivier]

Le 28/12/2017 à 19:08, Luke Shumaker a écrit :
> From: Luke Shumaker <lukeshu@parabola.nu>
> 
> Instead of doing
> 
>         if (check1) {
>             if (check2) {
>                success;
>             }
>         }
> 
>         retry;
> 
> Do a clearer
> 
>         if (!check1) {
>            goto try_again;
>         }
> 
>         if (!check2) {
>            goto try_again;
>         }
> 
>         success;
> 
>     try_again:
>         retry;
> 
> Besides being clearer, this makes it easier to insert more checks that
> need to trigger a retry on check failure, or rearrange them, or anything
> like that.
> 
> Because some indentation is changing, "ignore space change" may be useful
> for viewing this patch.
> 
> Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
> ---
>  linux-user/elfload.c | 34 +++++++++++++++++++---------------
>  1 file changed, 19 insertions(+), 15 deletions(-)
> 
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index b560f5d6fe..5c0ad65611 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -1906,24 +1906,28 @@ unsigned long init_guest_space(unsigned long host_start,
>          }
>  
>          /* Check to see if the address is valid.  */
> -        if (!host_start || aligned_start == current_start) {
> +        if (host_start && aligned_start != current_start) {
> +            goto try_again;
> +        }
> +
>  #if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
> -            /* On 32-bit ARM, we need to also be able to map the commpage.  */
> -            int valid = init_guest_commpage(aligned_start - guest_start,
> -                                            aligned_size + guest_start);
> -            if (valid == 1) {
> -                break;
> -            } else if (valid == -1) {
> -                munmap((void *)real_start, real_size);
> -                return (unsigned long)-1;
> -            }
> -            /* valid == 0, so try again. */
> -#else
> -            /* On other architectures, whatever we have here is fine.  */
> -            break;
> -#endif
> +        /* On 32-bit ARM, we need to also be able to map the commpage.  */
> +        int valid = init_guest_commpage(aligned_start - guest_start,
> +                                        aligned_size + guest_start);
> +        if (valid == -1) {
> +            munmap((void *)real_start, real_size);
> +            return (unsigned long)-1;
> +        } else if (valid == -1) {

I think it should be "if (valid == 0)" here.

> +            goto try_again;
>          }
> +#endif
> +
> +        /* If nothing has said `return -1` or `goto try_again` yet,
> +         * then the address we have is good.
> +         */
> +        break;
>  
> +    try_again:
>          /* 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
> 

Thanks,
Laurent

Re: [Qemu-devel] [PATCH 07/10] linux-user: init_guest_space: Clean up control flow a bit

[Laurent Vivier]

Le 09/03/2018 à 21:37, Laurent Vivier a écrit :
> Le 28/12/2017 à 19:08, Luke Shumaker a écrit :
>> From: Luke Shumaker <lukeshu@parabola.nu>
>>
>> Instead of doing
>>
>>         if (check1) {
>>             if (check2) {
>>                success;
>>             }
>>         }
>>
>>         retry;
>>
>> Do a clearer
>>
>>         if (!check1) {
>>            goto try_again;
>>         }
>>
>>         if (!check2) {
>>            goto try_again;
>>         }
>>
>>         success;
>>
>>     try_again:
>>         retry;
>>
>> Besides being clearer, this makes it easier to insert more checks that
>> need to trigger a retry on check failure, or rearrange them, or anything
>> like that.
>>
>> Because some indentation is changing, "ignore space change" may be useful
>> for viewing this patch.
>>
>> Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
>> ---
>>  linux-user/elfload.c | 34 +++++++++++++++++++---------------
>>  1 file changed, 19 insertions(+), 15 deletions(-)
>>
>> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
>> index b560f5d6fe..5c0ad65611 100644
>> --- a/linux-user/elfload.c
>> +++ b/linux-user/elfload.c
>> @@ -1906,24 +1906,28 @@ unsigned long init_guest_space(unsigned long host_start,
>>          }
>>  
>>          /* Check to see if the address is valid.  */
>> -        if (!host_start || aligned_start == current_start) {
>> +        if (host_start && aligned_start != current_start) {
>> +            goto try_again;
>> +        }
>> +
>>  #if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
>> -            /* On 32-bit ARM, we need to also be able to map the commpage.  */
>> -            int valid = init_guest_commpage(aligned_start - guest_start,
>> -                                            aligned_size + guest_start);
>> -            if (valid == 1) {
>> -                break;
>> -            } else if (valid == -1) {
>> -                munmap((void *)real_start, real_size);
>> -                return (unsigned long)-1;
>> -            }
>> -            /* valid == 0, so try again. */
>> -#else
>> -            /* On other architectures, whatever we have here is fine.  */
>> -            break;
>> -#endif
>> +        /* On 32-bit ARM, we need to also be able to map the commpage.  */
>> +        int valid = init_guest_commpage(aligned_start - guest_start,
>> +                                        aligned_size + guest_start);
>> +        if (valid == -1) {
>> +            munmap((void *)real_start, real_size);
>> +            return (unsigned long)-1;
>> +        } else if (valid == -1) {
> 
> I think it should be "if (valid == 0)" here.

Any comment?

Thanks,
Laurent

Re: [Qemu-devel] [PATCH 07/10] linux-user: init_guest_space: Clean up control flow a bit

[Peter Maydell]

On 13 March 2018 at 13:30, Laurent Vivier <laurent@vivier.eu> wrote:
> Le 09/03/2018 à 21:37, Laurent Vivier a écrit :
>> Le 28/12/2017 à 19:08, Luke Shumaker a écrit :
>>> --- a/linux-user/elfload.c
>>> +++ b/linux-user/elfload.c
>>> @@ -1906,24 +1906,28 @@ unsigned long init_guest_space(unsigned long host_start,
>>>          }
>>>
>>>          /* Check to see if the address is valid.  */
>>> -        if (!host_start || aligned_start == current_start) {
>>> +        if (host_start && aligned_start != current_start) {
>>> +            goto try_again;
>>> +        }
>>> +
>>>  #if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
>>> -            /* On 32-bit ARM, we need to also be able to map the commpage.  */
>>> -            int valid = init_guest_commpage(aligned_start - guest_start,
>>> -                                            aligned_size + guest_start);
>>> -            if (valid == 1) {
>>> -                break;
>>> -            } else if (valid == -1) {
>>> -                munmap((void *)real_start, real_size);
>>> -                return (unsigned long)-1;
>>> -            }
>>> -            /* valid == 0, so try again. */
>>> -#else
>>> -            /* On other architectures, whatever we have here is fine.  */
>>> -            break;
>>> -#endif
>>> +        /* On 32-bit ARM, we need to also be able to map the commpage.  */
>>> +        int valid = init_guest_commpage(aligned_start - guest_start,
>>> +                                        aligned_size + guest_start);
>>> +        if (valid == -1) {
>>> +            munmap((void *)real_start, real_size);
>>> +            return (unsigned long)-1;
>>> +        } else if (valid == -1) {
>>
>> I think it should be "if (valid == 0)" here.
>
> Any comment?

Yes, I think I agree.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 07/10] linux-user: init_guest_space: Clean up control flow a bit

[Laurent Vivier]

Le 13/03/2018 à 14:54, Peter Maydell a écrit :
> On 13 March 2018 at 13:30, Laurent Vivier <laurent@vivier.eu> wrote:
>> Le 09/03/2018 à 21:37, Laurent Vivier a écrit :
>>> Le 28/12/2017 à 19:08, Luke Shumaker a écrit :
>>>> --- a/linux-user/elfload.c
>>>> +++ b/linux-user/elfload.c
>>>> @@ -1906,24 +1906,28 @@ unsigned long init_guest_space(unsigned long host_start,
>>>>          }
>>>>
>>>>          /* Check to see if the address is valid.  */
>>>> -        if (!host_start || aligned_start == current_start) {
>>>> +        if (host_start && aligned_start != current_start) {
>>>> +            goto try_again;
>>>> +        }
>>>> +
>>>>  #if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
>>>> -            /* On 32-bit ARM, we need to also be able to map the commpage.  */
>>>> -            int valid = init_guest_commpage(aligned_start - guest_start,
>>>> -                                            aligned_size + guest_start);
>>>> -            if (valid == 1) {
>>>> -                break;
>>>> -            } else if (valid == -1) {
>>>> -                munmap((void *)real_start, real_size);
>>>> -                return (unsigned long)-1;
>>>> -            }
>>>> -            /* valid == 0, so try again. */
>>>> -#else
>>>> -            /* On other architectures, whatever we have here is fine.  */
>>>> -            break;
>>>> -#endif
>>>> +        /* On 32-bit ARM, we need to also be able to map the commpage.  */
>>>> +        int valid = init_guest_commpage(aligned_start - guest_start,
>>>> +                                        aligned_size + guest_start);
>>>> +        if (valid == -1) {
>>>> +            munmap((void *)real_start, real_size);
>>>> +            return (unsigned long)-1;
>>>> +        } else if (valid == -1) {
>>>
>>> I think it should be "if (valid == 0)" here.
>>
>> Any comment?
> 
> Yes, I think I agree.
> 

Applied to my 'linux-user-for-2.12' branch with this change.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH 08/10] linux-user: init_guest_space: Don't try to align if we'll reject it

[Laurent Vivier]

Le 28/12/2017 à 19:08, Luke Shumaker a écrit :
> From: Luke Shumaker <lukeshu@parabola.nu>
> 
> If the ensure-alignment code gets triggered, then the
> "if (host_start && real_start != current_start)" check will always trigger,
> so save 2 syscalls and put that check first.
> 
> Note that we can't just switch to using MAP_FIXED for that check, because
> then we couldn't differentiate between a failure because "there isn't
> enough space" and "there isn't enough space *here*".
> 
> Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
> ---
>  linux-user/elfload.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 5c0ad65611..1b7583d659 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -1871,6 +1871,11 @@ unsigned long init_guest_space(unsigned long host_start,
>              return (unsigned long)-1;
>          }
>  
> +        /* Check to see if the address is valid.  */
> +        if (host_start && real_start != current_start) {
> +            goto try_again;
> +        }
> +
>          /* Ensure the address is properly aligned.  */
>          if (real_start & ~qemu_host_page_mask) {
>              /* Ideally, we adjust like
> @@ -1905,11 +1910,6 @@ unsigned long init_guest_space(unsigned long host_start,
>              aligned_start = real_start;
>          }
>  
> -        /* Check to see if the address is valid.  */
> -        if (host_start && aligned_start != current_start) {
> -            goto try_again;
> -        }
> -
>  #if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
>          /* On 32-bit ARM, we need to also be able to map the commpage.  */
>          int valid = init_guest_commpage(aligned_start - guest_start,
> 

Applied to my 'linux-user-for-2.12' branch.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH 09/10] linux-user: init_guest_space: Add a comment about search strategy

[Laurent Vivier]

Le 28/12/2017 à 19:08, Luke Shumaker a écrit :
> From: Luke Shumaker <lukeshu@parabola.nu>
> 
> Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
> ---
>  linux-user/elfload.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 1b7583d659..7736ea2c3a 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -1936,6 +1936,10 @@ unsigned long init_guest_space(unsigned long host_start,
>           * happen often.  Probably means we got unlucky and host
>           * address space randomization put a shared library somewhere
>           * inconvenient.
> +         *
> +         * This is probably a good strategy if host_start, but is
> +         * probably a bad strategy if not, which means we got here
> +         * because of trouble with ARM commpage setup.
>           */
>          munmap((void *)real_start, real_size);
>          current_start += qemu_host_page_size;
> 

Applied to my 'linux-user-for-2.12' branch.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH 10/10] linux-user: init_guest_space: Try to make ARM space+commpage continuous

[Laurent Vivier]

Le 02/03/2018 à 15:13, Peter Maydell a écrit :
> On 28 December 2017 at 18:08, Luke Shumaker <lukeshu@lukeshu.com> wrote:
>> From: Luke Shumaker <lukeshu@parabola.nu>
>>
>> At a fixed distance after the usable memory that init_guest_space maps, for
>> 32-bit ARM targets we also need to map a commpage.  The normal
>> init_guest_space logic doesn't keep this in mind when searching for an
>> address range.
>>
>> If !host_start, then try to find a big continuous segment where we can put
>> both the usable memory and the commpage; we then munmap that segment and
>> set current_start to that address; and let the normal code mmap the usable
>> memory and the commpage separately.  That is: if we don't have hint of
>> where to start looking for memory, come up with one that is better than
>> NULL.  Depending on host_size and guest_start, there may or may not be a
>> gap between the usable memory and the commpage, so this is slightly more
>> restrictive than it needs to be; but it's only a hint, so that's OK.
>>
>> We only do that for !host start, because if host_start, then either:
>>  - we got an address passed in with -B, in which case we don't want to
>>    interfere with what the user said;
>>  - or host_start is based off of the ELF image's loaddr.  The check "if
>>    (host_start && real_start != current_start)" suggests that we really
>>    want lowest available address that is >= loaddr.  I don't know why that
>>    is, but I'm trusting that Paul Brook knew what he was doing when he
>>    wrote the original version of that check in
>>    c581deda322080e8beb88b2e468d4af54454e4b3 way back in 2010.
>>
>> Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
>> ---
>>  linux-user/elfload.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 49 insertions(+)
>>
>> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
>> index 7736ea2c3a..cd3a7d877d 100644
>> --- a/linux-user/elfload.c
>> +++ b/linux-user/elfload.c
>> @@ -1857,6 +1857,55 @@ unsigned long init_guest_space(unsigned long host_start,
>>
>>      /* Otherwise, a non-zero size region of memory needs to be mapped
>>       * and validated.  */
>> +
>> +#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;
> 
> I think this is probably more clearly written as 0x100000000ULL,
> since rounding down to the host-page-size then adding the host-page-size
> gets us the full 32-bit size of the guest address space.

Perhaps, I've missed something, but it seems not true.

On x86_64, we have:

qemu_host_page_mask = 0xfffffffffffff000
qemu_host_page_size = 0x0000000000001000

but

0xffff0f00 & 0xfffffffffffff000 = 0xffff0000
then
0xffff0000 + 0x0000000000001000 = 0xffff1000

Thanks,
Laurent

Re: [Qemu-devel] [PATCH 10/10] linux-user: init_guest_space: Try to make ARM space+commpage continuous

[Peter Maydell]

On 20 March 2018 at 15:23, Laurent Vivier <laurent@vivier.eu> wrote:
> Le 02/03/2018 à 15:13, Peter Maydell a écrit :
>> On 28 December 2017 at 18:08, Luke Shumaker <lukeshu@lukeshu.com> wrote:
>>> +#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;
>>
>> I think this is probably more clearly written as 0x100000000ULL,
>> since rounding down to the host-page-size then adding the host-page-size
>> gets us the full 32-bit size of the guest address space.
>
> Perhaps, I've missed something, but it seems not true.
>
> On x86_64, we have:
>
> qemu_host_page_mask = 0xfffffffffffff000
> qemu_host_page_size = 0x0000000000001000
>
> but
>
> 0xffff0f00 & 0xfffffffffffff000 = 0xffff0000
> then
> 0xffff0000 + 0x0000000000001000 = 0xffff1000

Yes, you're right -- I'd thought that the kernel commpage was
right at the top of memory, but it isn't.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 10/10] linux-user: init_guest_space: Try to make ARM space+commpage continuous

[Luke Shumaker]

On Fri, 02 Mar 2018 09:13:12 -0500,
Peter Maydell wrote:
> On 28 December 2017 at 18:08, Luke Shumaker <lukeshu@lukeshu.com> wrote:
> > +        guest_full_size =
> > +            (0xffff0f00 & qemu_host_page_mask) + qemu_host_page_size;
                        ^
> I think this is probably more clearly written as 0x100000000ULL,
> since rounding down to the host-page-size then adding the host-page-size
> gets us the full 32-bit size of the guest address space.

Wait, is that right?  Isn't that only true if qemu_host_page_size is
at least 8KiB (16 bits), enough to fill the zero in the middle?  Won't
a typical qemu_host_page_size be only 4KiB?

> That shows up that there's a potential problem here if the host
> is 32-bit, because in that case guest_full_size (being only unsigned
> long) will be 0, and we'll end up trying an mmap with an incorrect size.
> 
> > +        host_full_size = guest_full_size - guest_start;
> > +        real_start = (unsigned long)
> > +            mmap(NULL, host_full_size, PROT_NONE, flags, -1, 0);
> 
> I think the general approach is right, though. Sorry it took so long
> for us to get to reviewing this patchset.

It's all good.  I'm amazed at the amount of traffic qemu-devel gets!

> Incidentally, this code would be rather less complicated if it didn't
> have to account for qemu_host_page_size not actually being the host
> page size (since then you couldn't get a return from mmap() that wasn't
> aligned properly). Does anybody know why we allow the user to specify
> it on the command line? (git revision history doesn't help, it just says
> there's been a -pagesize argument since commit 54936004fddc5 in 2003,
> right back when mmap emulation was first added...)

I have no idea, I just assumed that it was a feature useful to people
far smarter than me.

-- 
Happy hacking,
~ Luke Shumaker

Re: [Qemu-devel] [PATCH 10/10] linux-user: init_guest_space: Try to make ARM space+commpage continuous

[Laurent Vivier]

Le 20/03/2018 à 19:49, Luke Shumaker a écrit :
> On Fri, 02 Mar 2018 09:13:12 -0500,
> Peter Maydell wrote:
>> On 28 December 2017 at 18:08, Luke Shumaker <lukeshu@lukeshu.com> wrote:
>>> +        guest_full_size =
>>> +            (0xffff0f00 & qemu_host_page_mask) + qemu_host_page_size;
>                         ^
>> I think this is probably more clearly written as 0x100000000ULL,
>> since rounding down to the host-page-size then adding the host-page-size
>> gets us the full 32-bit size of the guest address space.
> 
> Wait, is that right?  Isn't that only true if qemu_host_page_size is
> at least 8KiB (16 bits), enough to fill the zero in the middle?  Won't
> a typical qemu_host_page_size be only 4KiB?
> 
>> That shows up that there's a potential problem here if the host
>> is 32-bit, because in that case guest_full_size (being only unsigned
>> long) will be 0, and we'll end up trying an mmap with an incorrect size.
>>
>>> +        host_full_size = guest_full_size - guest_start;
>>> +        real_start = (unsigned long)
>>> +            mmap(NULL, host_full_size, PROT_NONE, flags, -1, 0);
>>
>> I think the general approach is right, though. Sorry it took so long
>> for us to get to reviewing this patchset.
> 
> It's all good.  I'm amazed at the amount of traffic qemu-devel gets!
> 
>> Incidentally, this code would be rather less complicated if it didn't
>> have to account for qemu_host_page_size not actually being the host
>> page size (since then you couldn't get a return from mmap() that wasn't
>> aligned properly). Does anybody know why we allow the user to specify
>> it on the command line? (git revision history doesn't help, it just says
>> there's been a -pagesize argument since commit 54936004fddc5 in 2003,
>> right back when mmap emulation was first added...)
> 
> I have no idea, I just assumed that it was a feature useful to people
> far smarter than me.
> 

I'm going to add this patch in my upcoming linux-user pull-request
(currently running regression tests).

Thanks,
Laurent

Re: [Qemu-devel] [PATCH 10/10] linux-user: init_guest_space: Try to make ARM space+commpage continuous

[Peter Maydell]

On 20 March 2018 at 18:49, Luke Shumaker <lukeshu@lukeshu.com> wrote:
> On Fri, 02 Mar 2018 09:13:12 -0500,
> Peter Maydell wrote:
>> On 28 December 2017 at 18:08, Luke Shumaker <lukeshu@lukeshu.com> wrote:
>> > +        guest_full_size =
>> > +            (0xffff0f00 & qemu_host_page_mask) + qemu_host_page_size;
>                         ^
>> I think this is probably more clearly written as 0x100000000ULL,
>> since rounding down to the host-page-size then adding the host-page-size
>> gets us the full 32-bit size of the guest address space.
>
> Wait, is that right?  Isn't that only true if qemu_host_page_size is
> at least 8KiB (16 bits), enough to fill the zero in the middle?  Won't
> a typical qemu_host_page_size be only 4KiB?

Yeah, I just got the arithmetic wrong here because I had it
stuck in my head that the commpage was the top page of the
guest address space.

-- PMM