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

["Alex Bennée" <alex.bennee@linaro.org>]

Alex Bennée <alex.bennee@linaro.org> writes:

> This tries to simplify the init_guest_space code to be a little less
> convoluted and remove the brute force mapping algorithm that gets
> tripped up so badly by the sanitizers.
>
> 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.
>
> Less mind-binding gotos and hopefully clearer logic although perhaps
> more sloppy casting than I'm totally happy with.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  linux-user/elfload.c | 279 +++++++++++++++++++------------------------
>  1 file changed, 125 insertions(+), 154 deletions(-)

Richard,

I think I overthought the magic alignment code as we can deal with it
all in advance and not have to add extra pages which might get in the
way:

1 file changed, 5 insertions(+), 7 deletions(-)
linux-user/elfload.c | 12 +++++-------

modified   linux-user/elfload.c
@@ -2169,17 +2169,15 @@ unsigned long init_guest_space(unsigned long host_start,
      * that will accept the initial mapping as well as being able to
      * map the (ARM32 specific) commpage later.
      *
-     * We need to ensure the address is properly aligned. 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 aforementioned ARM32 emulation.
+     * We need to ensure the address is properly aligned but we can
+     * take that into account when looking for the gap.
      */
     {
 #if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
         uint64_t required_size =
             (0xffff0f00 & qemu_host_page_mask) + qemu_host_page_size;
 #else
-        uint64_t required_size = host_size + align;
+        uint64_t required_size = host_size;
 #endif
         GSList *map_info = read_self_maps();
         GSList *last, *next;
@@ -2211,10 +2209,10 @@ unsigned long init_guest_space(unsigned long host_start,
      * From this point on it should be a formality but lets go through
      * the steps anyway.
      */
-    if (mmap(map_addr, host_size + align , PROT_NONE,
+    if (mmap(map_addr, host_size, PROT_NONE,
              flags | MAP_FIXED, -1, 0) == map_addr) {
         unsigned long addr = (unsigned long) map_addr;
-        if (!check_commpage(addr, host_size + align)) {
+        if (!check_commpage(addr, host_size)) {
             error_report("%s: commpage won't fit in guest_memory @ %p",
                          __func__, map_addr);
             abort();


-- 
Alex Bennée