qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] linux-user: brk/mmap fixes
@ 2023-07-31  8:03 Akihiko Odaki
  2023-07-31  8:03 ` [PATCH 1/5] linux-user: Unset MAP_FIXED_NOREPLACE for host Akihiko Odaki
                   ` (5 more replies)
  0 siblings, 6 replies; 25+ messages in thread
From: Akihiko Odaki @ 2023-07-31  8:03 UTC (permalink / raw)
  Cc: qemu-devel, Laurent Vivier, Akihiko Odaki

linux-user was failing on M2 MacBook Air. Digging into the details, I found
several bugs in brk and mmap so here are fixes.

Akihiko Odaki (5):
  linux-user: Unset MAP_FIXED_NOREPLACE for host
  linux-user: Do not call get_errno() in do_brk()
  linux-user: Use MAP_FIXED_NOREPLACE for do_brk()
  linux-user: Do nothing if too small brk is specified
  linux-user: Do not align brk with host page size

 linux-user/elfload.c |  4 +--
 linux-user/mmap.c    |  2 ++
 linux-user/syscall.c | 67 +++++++++-----------------------------------
 3 files changed, 17 insertions(+), 56 deletions(-)

-- 
2.41.0



^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH 1/5] linux-user: Unset MAP_FIXED_NOREPLACE for host
  2023-07-31  8:03 [PATCH 0/5] linux-user: brk/mmap fixes Akihiko Odaki
@ 2023-07-31  8:03 ` Akihiko Odaki
  2023-07-31 15:43   ` Richard Henderson
  2023-07-31  8:03 ` [PATCH 2/5] linux-user: Do not call get_errno() in do_brk() Akihiko Odaki
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 25+ messages in thread
From: Akihiko Odaki @ 2023-07-31  8:03 UTC (permalink / raw)
  Cc: qemu-devel, Laurent Vivier, Akihiko Odaki

Passing MAP_FIXED_NOREPLACE to host will fail if the virtual
address space is reserved with mmap. Replace it with MAP_FIXED.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 linux-user/mmap.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index a5dfb56545..2f26cbaf5d 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -610,6 +610,8 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot,
             goto fail;
         }
 
+        flags = (flags & ~MAP_FIXED_NOREPLACE) | MAP_FIXED;
+
         /*
          * worst case: we cannot map the file because the offset is not
          * aligned, so we read it
-- 
2.41.0



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 2/5] linux-user: Do not call get_errno() in do_brk()
  2023-07-31  8:03 [PATCH 0/5] linux-user: brk/mmap fixes Akihiko Odaki
  2023-07-31  8:03 ` [PATCH 1/5] linux-user: Unset MAP_FIXED_NOREPLACE for host Akihiko Odaki
@ 2023-07-31  8:03 ` Akihiko Odaki
  2023-07-31 15:13   ` Helge Deller
  2023-07-31  8:03 ` [PATCH 3/5] linux-user: Use MAP_FIXED_NOREPLACE for do_brk() Akihiko Odaki
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 25+ messages in thread
From: Akihiko Odaki @ 2023-07-31  8:03 UTC (permalink / raw)
  Cc: qemu-devel, Laurent Vivier, Akihiko Odaki

Later the returned value is compared with -1, and negated errno is not
expected.

Fixes: 00faf08c95 ("linux-user: Don't use MAP_FIXED in do_brk()")
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 linux-user/syscall.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 95727a816a..b9d2ec02f9 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -862,9 +862,9 @@ abi_long do_brk(abi_ulong brk_val)
      */
     if (new_host_brk_page > brk_page) {
         new_alloc_size = new_host_brk_page - brk_page;
-        mapped_addr = get_errno(target_mmap(brk_page, new_alloc_size,
-                                        PROT_READ|PROT_WRITE,
-                                        MAP_ANON|MAP_PRIVATE, 0, 0));
+        mapped_addr = target_mmap(brk_page, new_alloc_size,
+                                  PROT_READ|PROT_WRITE,
+                                  MAP_ANON|MAP_PRIVATE, 0, 0);
     } else {
         new_alloc_size = 0;
         mapped_addr = brk_page;
-- 
2.41.0



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 3/5] linux-user: Use MAP_FIXED_NOREPLACE for do_brk()
  2023-07-31  8:03 [PATCH 0/5] linux-user: brk/mmap fixes Akihiko Odaki
  2023-07-31  8:03 ` [PATCH 1/5] linux-user: Unset MAP_FIXED_NOREPLACE for host Akihiko Odaki
  2023-07-31  8:03 ` [PATCH 2/5] linux-user: Do not call get_errno() in do_brk() Akihiko Odaki
@ 2023-07-31  8:03 ` Akihiko Odaki
  2023-07-31 11:44   ` Peter Maydell
  2023-07-31  8:03 ` [PATCH 4/5] linux-user: Do nothing if too small brk is specified Akihiko Odaki
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 25+ messages in thread
From: Akihiko Odaki @ 2023-07-31  8:03 UTC (permalink / raw)
  Cc: qemu-devel, Laurent Vivier, Akihiko Odaki

MAP_FIXED_NOREPLACE can ensure the mapped address is fixed without
concerning that the new mapping overwrites something else.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 linux-user/syscall.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index b9d2ec02f9..ac429a185a 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -854,17 +854,12 @@ abi_long do_brk(abi_ulong brk_val)
         return target_brk;
     }
 
-    /* We need to allocate more memory after the brk... Note that
-     * we don't use MAP_FIXED because that will map over the top of
-     * any existing mapping (like the one with the host libc or qemu
-     * itself); instead we treat "mapped but at wrong address" as
-     * a failure and unmap again.
-     */
     if (new_host_brk_page > brk_page) {
         new_alloc_size = new_host_brk_page - brk_page;
         mapped_addr = target_mmap(brk_page, new_alloc_size,
-                                  PROT_READ|PROT_WRITE,
-                                  MAP_ANON|MAP_PRIVATE, 0, 0);
+                                  PROT_READ | PROT_WRITE,
+                                  MAP_FIXED_NOREPLACE | MAP_ANON | MAP_PRIVATE,
+                                  0, 0);
     } else {
         new_alloc_size = 0;
         mapped_addr = brk_page;
@@ -883,12 +878,6 @@ abi_long do_brk(abi_ulong brk_val)
         target_brk = brk_val;
         brk_page = new_host_brk_page;
         return target_brk;
-    } else if (mapped_addr != -1) {
-        /* Mapped but at wrong address, meaning there wasn't actually
-         * enough space for this brk.
-         */
-        target_munmap(mapped_addr, new_alloc_size);
-        mapped_addr = -1;
     }
 
 #if defined(TARGET_ALPHA)
-- 
2.41.0



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 4/5] linux-user: Do nothing if too small brk is specified
  2023-07-31  8:03 [PATCH 0/5] linux-user: brk/mmap fixes Akihiko Odaki
                   ` (2 preceding siblings ...)
  2023-07-31  8:03 ` [PATCH 3/5] linux-user: Use MAP_FIXED_NOREPLACE for do_brk() Akihiko Odaki
@ 2023-07-31  8:03 ` Akihiko Odaki
  2023-07-31 15:16   ` Helge Deller
  2023-07-31  8:03 ` [PATCH 5/5] linux-user: Do not align brk with host page size Akihiko Odaki
  2023-07-31  9:31 ` [PATCH 0/5] linux-user: brk/mmap fixes Michael Tokarev
  5 siblings, 1 reply; 25+ messages in thread
From: Akihiko Odaki @ 2023-07-31  8:03 UTC (permalink / raw)
  Cc: qemu-devel, Laurent Vivier, Akihiko Odaki

Linux 6.4.7 does nothing when a value smaller than the initial brk is
specified.

Fixes: 86f04735ac ("linux-user: Fix brk() to release pages")
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 linux-user/syscall.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index ac429a185a..ebdc8c144c 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -820,14 +820,14 @@ abi_long do_brk(abi_ulong brk_val)
 
     /* brk pointers are always untagged */
 
-    /* return old brk value if brk_val unchanged or zero */
-    if (!brk_val || brk_val == target_brk) {
+    /* return old brk value if brk_val unchanged */
+    if (brk_val == target_brk) {
         return target_brk;
     }
 
     /* do not allow to shrink below initial brk value */
     if (brk_val < initial_target_brk) {
-        brk_val = initial_target_brk;
+        return target_brk;
     }
 
     new_brk = TARGET_PAGE_ALIGN(brk_val);
-- 
2.41.0



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 5/5] linux-user: Do not align brk with host page size
  2023-07-31  8:03 [PATCH 0/5] linux-user: brk/mmap fixes Akihiko Odaki
                   ` (3 preceding siblings ...)
  2023-07-31  8:03 ` [PATCH 4/5] linux-user: Do nothing if too small brk is specified Akihiko Odaki
@ 2023-07-31  8:03 ` Akihiko Odaki
  2023-07-31 15:51   ` Helge Deller
  2023-07-31  9:31 ` [PATCH 0/5] linux-user: brk/mmap fixes Michael Tokarev
  5 siblings, 1 reply; 25+ messages in thread
From: Akihiko Odaki @ 2023-07-31  8:03 UTC (permalink / raw)
  Cc: qemu-devel, Laurent Vivier, Akihiko Odaki

do_brk() minimizes calls into target_mmap() by aligning the address
with host page size, which is potentially larger than the target page
size. However, the current implementation of this optimization has two
bugs:

- The start of brk is rounded up with the host page size while brk
  advertises an address aligned with the target page size as the
  beginning of brk. This makes the beginning of brk unmapped.
- Content clearing after mapping is flawed. The size to clear is
  specified as HOST_PAGE_ALIGN(brk_page) - brk_page, but brk_page is
  aligned with the host page size so it is always zero.

This optimization actually has no practical benefit. It makes difference
when brk() is called multiple times with values in a range of the host
page size. However, sophisticated memory allocators try to avoid to
make such frequent brk() calls. For example, glibc 2.37 calls brk() to
shrink the heap only when there is a room more than 128 KiB. It is
rare to have a page size larger than 128 KiB if it happens.

Let's remove the optimization to fix the bugs and make the code simpler.

Fixes: 86f04735ac ("linux-user: Fix brk() to release pages")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1616
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 linux-user/elfload.c |  4 ++--
 linux-user/syscall.c | 54 ++++++++++----------------------------------
 2 files changed, 14 insertions(+), 44 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 861ec07abc..2aee2298ec 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -3678,8 +3678,8 @@ int load_elf_binary(struct linux_binprm *bprm, struct image_info *info)
      * to mmap pages in this space.
      */
     if (info->reserve_brk) {
-        abi_ulong start_brk = HOST_PAGE_ALIGN(info->brk);
-        abi_ulong end_brk = HOST_PAGE_ALIGN(info->brk + info->reserve_brk);
+        abi_ulong start_brk = TARGET_PAGE_ALIGN(info->brk);
+        abi_ulong end_brk = TARGET_PAGE_ALIGN(info->brk + info->reserve_brk);
         target_munmap(start_brk, end_brk - start_brk);
     }
 
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index ebdc8c144c..475260b7ce 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -802,81 +802,51 @@ static inline int host_to_target_sock_type(int host_type)
 }
 
 static abi_ulong target_brk, initial_target_brk;
-static abi_ulong brk_page;
 
 void target_set_brk(abi_ulong new_brk)
 {
     target_brk = TARGET_PAGE_ALIGN(new_brk);
     initial_target_brk = target_brk;
-    brk_page = HOST_PAGE_ALIGN(target_brk);
 }
 
 /* do_brk() must return target values and target errnos. */
 abi_long do_brk(abi_ulong brk_val)
 {
     abi_long mapped_addr;
-    abi_ulong new_alloc_size;
-    abi_ulong new_brk, new_host_brk_page;
+    abi_ulong new_brk;
+    abi_ulong old_brk;
 
     /* brk pointers are always untagged */
 
-    /* return old brk value if brk_val unchanged */
-    if (brk_val == target_brk) {
-        return target_brk;
-    }
-
     /* do not allow to shrink below initial brk value */
     if (brk_val < initial_target_brk) {
         return target_brk;
     }
 
     new_brk = TARGET_PAGE_ALIGN(brk_val);
-    new_host_brk_page = HOST_PAGE_ALIGN(brk_val);
+    old_brk = TARGET_PAGE_ALIGN(target_brk);
 
-    /* brk_val and old target_brk might be on the same page */
-    if (new_brk == TARGET_PAGE_ALIGN(target_brk)) {
-        /* empty remaining bytes in (possibly larger) host page */
-        memset(g2h_untagged(new_brk), 0, new_host_brk_page - new_brk);
+    /* new and old target_brk might be on the same page */
+    if (new_brk == old_brk) {
         target_brk = brk_val;
         return target_brk;
     }
 
     /* Release heap if necesary */
-    if (new_brk < target_brk) {
-        /* empty remaining bytes in (possibly larger) host page */
-        memset(g2h_untagged(new_brk), 0, new_host_brk_page - new_brk);
-
-        /* free unused host pages and set new brk_page */
-        target_munmap(new_host_brk_page, brk_page - new_host_brk_page);
-        brk_page = new_host_brk_page;
+    if (new_brk < old_brk) {
+        target_munmap(new_brk, old_brk - new_brk);
 
         target_brk = brk_val;
         return target_brk;
     }
 
-    if (new_host_brk_page > brk_page) {
-        new_alloc_size = new_host_brk_page - brk_page;
-        mapped_addr = target_mmap(brk_page, new_alloc_size,
-                                  PROT_READ | PROT_WRITE,
-                                  MAP_FIXED_NOREPLACE | MAP_ANON | MAP_PRIVATE,
-                                  0, 0);
-    } else {
-        new_alloc_size = 0;
-        mapped_addr = brk_page;
-    }
-
-    if (mapped_addr == brk_page) {
-        /* Heap contents are initialized to zero, as for anonymous
-         * mapped pages.  Technically the new pages are already
-         * initialized to zero since they *are* anonymous mapped
-         * pages, however we have to take care with the contents that
-         * come from the remaining part of the previous page: it may
-         * contains garbage data due to a previous heap usage (grown
-         * then shrunken).  */
-        memset(g2h_untagged(brk_page), 0, HOST_PAGE_ALIGN(brk_page) - brk_page);
+    mapped_addr = target_mmap(old_brk, new_brk - old_brk,
+                              PROT_READ | PROT_WRITE,
+                              MAP_FIXED_NOREPLACE | MAP_ANON | MAP_PRIVATE,
+                              0, 0);
 
+    if (mapped_addr == old_brk) {
         target_brk = brk_val;
-        brk_page = new_host_brk_page;
         return target_brk;
     }
 
-- 
2.41.0



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [PATCH 0/5] linux-user: brk/mmap fixes
  2023-07-31  8:03 [PATCH 0/5] linux-user: brk/mmap fixes Akihiko Odaki
                   ` (4 preceding siblings ...)
  2023-07-31  8:03 ` [PATCH 5/5] linux-user: Do not align brk with host page size Akihiko Odaki
@ 2023-07-31  9:31 ` Michael Tokarev
  2023-07-31 10:17   ` Joel Stanley
  5 siblings, 1 reply; 25+ messages in thread
From: Michael Tokarev @ 2023-07-31  9:31 UTC (permalink / raw)
  To: Akihiko Odaki; +Cc: qemu-devel, Laurent Vivier

31.07.2023 11:03, Akihiko Odaki wrote:
> linux-user was failing on M2 MacBook Air. Digging into the details, I found
> several bugs in brk and mmap so here are fixes.

There's another work in this area by Helge Deller, have you seen it?
("linux-user: Fix and optimize target memory layout", a v5 already).

FWIW,

/mjt


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 0/5] linux-user: brk/mmap fixes
  2023-07-31  9:31 ` [PATCH 0/5] linux-user: brk/mmap fixes Michael Tokarev
@ 2023-07-31 10:17   ` Joel Stanley
  2023-07-31 10:23     ` Akihiko Odaki
  0 siblings, 1 reply; 25+ messages in thread
From: Joel Stanley @ 2023-07-31 10:17 UTC (permalink / raw)
  To: Michael Tokarev; +Cc: Akihiko Odaki, qemu-devel, Laurent Vivier

On Mon, 31 Jul 2023 at 09:31, Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> 31.07.2023 11:03, Akihiko Odaki wrote:
> > linux-user was failing on M2 MacBook Air. Digging into the details, I found
> > several bugs in brk and mmap so here are fixes.
>
> There's another work in this area by Helge Deller, have you seen it?
> ("linux-user: Fix and optimize target memory layout", a v5 already).

Applying this series fixes the qemu-arm running the static armhf
binary on my ppc64le host that I reported here[1].

Tested-by: Joel Stanley <joel@jms.id.au>

The changes conflict with Helge's patches, so it would be good to work
out which of your changes should be combined with his.

Cheers,

Joel

[1] https://lore.kernel.org/qemu-devel/CACPK8XeyqcEDyyL3Jw2WYWs_gGdtTCf2=Ly04CMgkshSMdj7RA@mail.gmail.com/


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 0/5] linux-user: brk/mmap fixes
  2023-07-31 10:17   ` Joel Stanley
@ 2023-07-31 10:23     ` Akihiko Odaki
  2023-07-31 16:47       ` Helge Deller
  0 siblings, 1 reply; 25+ messages in thread
From: Akihiko Odaki @ 2023-07-31 10:23 UTC (permalink / raw)
  To: Helge Deller, Joel Stanley, Michael Tokarev; +Cc: qemu-devel, Laurent Vivier

On 2023/07/31 19:17, Joel Stanley wrote:
> On Mon, 31 Jul 2023 at 09:31, Michael Tokarev <mjt@tls.msk.ru> wrote:
>>
>> 31.07.2023 11:03, Akihiko Odaki wrote:
>>> linux-user was failing on M2 MacBook Air. Digging into the details, I found
>>> several bugs in brk and mmap so here are fixes.
>>
>> There's another work in this area by Helge Deller, have you seen it?
>> ("linux-user: Fix and optimize target memory layout", a v5 already).
> 
> Applying this series fixes the qemu-arm running the static armhf
> binary on my ppc64le host that I reported here[1].
> 
> Tested-by: Joel Stanley <joel@jms.id.au>

Thanks for testing.

> 
> The changes conflict with Helge's patches, so it would be good to work
> out which of your changes should be combined with his.

I suggest Helge to rebase his change to my series. The below is some 
detailed explanation:

It is almost orthogonal to my series, but patch 2 will conflict with my 
series since it changes how the initial brk is calculated.

Unfortunately I think patch 2 has a bug. Without my patch, do_brk() 
assumes the heap is aligned with the host page size, but the patch does 
not consider the case that the host and target page sizes are different.

My patch makes the heap aligned with the target page size so it will fix 
the bug.

Regards,
Akihiko Odaki

> 
> Cheers,
> 
> Joel
> 
> [1] https://lore.kernel.org/qemu-devel/CACPK8XeyqcEDyyL3Jw2WYWs_gGdtTCf2=Ly04CMgkshSMdj7RA@mail.gmail.com/


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 3/5] linux-user: Use MAP_FIXED_NOREPLACE for do_brk()
  2023-07-31  8:03 ` [PATCH 3/5] linux-user: Use MAP_FIXED_NOREPLACE for do_brk() Akihiko Odaki
@ 2023-07-31 11:44   ` Peter Maydell
  2023-07-31 13:32     ` Akihiko Odaki
  0 siblings, 1 reply; 25+ messages in thread
From: Peter Maydell @ 2023-07-31 11:44 UTC (permalink / raw)
  To: Akihiko Odaki; +Cc: qemu-devel, Laurent Vivier

On Mon, 31 Jul 2023 at 09:04, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> MAP_FIXED_NOREPLACE can ensure the mapped address is fixed without
> concerning that the new mapping overwrites something else.

MAP_FIXED_NOREPLACE only came in with Linux 4.17. So
I think we still need to handle the "mapped address
is not the one we asked for" error condition, because
it can happen on older host kernels that ignore the
MAP_FIXED_NOREPLACE flag.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 3/5] linux-user: Use MAP_FIXED_NOREPLACE for do_brk()
  2023-07-31 11:44   ` Peter Maydell
@ 2023-07-31 13:32     ` Akihiko Odaki
  2023-07-31 13:45       ` Peter Maydell
  2023-07-31 15:44       ` Richard Henderson
  0 siblings, 2 replies; 25+ messages in thread
From: Akihiko Odaki @ 2023-07-31 13:32 UTC (permalink / raw)
  To: Peter Maydell; +Cc: qemu-devel, Laurent Vivier

On 2023/07/31 20:44, Peter Maydell wrote:
> On Mon, 31 Jul 2023 at 09:04, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> MAP_FIXED_NOREPLACE can ensure the mapped address is fixed without
>> concerning that the new mapping overwrites something else.
> 
> MAP_FIXED_NOREPLACE only came in with Linux 4.17. So
> I think we still need to handle the "mapped address
> is not the one we asked for" error condition, because
> it can happen on older host kernels that ignore the
> MAP_FIXED_NOREPLACE flag.
> 
> thanks
> -- PMM

MAP_FIXED_NOREPLACE is substituted with MAP_FIXED before passing to the 
host with patch 1. The NOREPLACE constraint is still ensured by 
inspecting the guest page table.

Regards,
Akihiko Odaki


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 3/5] linux-user: Use MAP_FIXED_NOREPLACE for do_brk()
  2023-07-31 13:32     ` Akihiko Odaki
@ 2023-07-31 13:45       ` Peter Maydell
  2023-07-31 15:44       ` Richard Henderson
  1 sibling, 0 replies; 25+ messages in thread
From: Peter Maydell @ 2023-07-31 13:45 UTC (permalink / raw)
  To: Akihiko Odaki; +Cc: qemu-devel, Laurent Vivier

On Mon, 31 Jul 2023 at 14:32, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2023/07/31 20:44, Peter Maydell wrote:
> > On Mon, 31 Jul 2023 at 09:04, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>
> >> MAP_FIXED_NOREPLACE can ensure the mapped address is fixed without
> >> concerning that the new mapping overwrites something else.
> >
> > MAP_FIXED_NOREPLACE only came in with Linux 4.17. So
> > I think we still need to handle the "mapped address
> > is not the one we asked for" error condition, because
> > it can happen on older host kernels that ignore the
> > MAP_FIXED_NOREPLACE flag.

> MAP_FIXED_NOREPLACE is substituted with MAP_FIXED before passing to the
> host with patch 1. The NOREPLACE constraint is still ensured by
> inspecting the guest page table.

Oh, I see, this patch is a call to target_mmap(), not host
mmap(). Sorry, I misread that.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 2/5] linux-user: Do not call get_errno() in do_brk()
  2023-07-31  8:03 ` [PATCH 2/5] linux-user: Do not call get_errno() in do_brk() Akihiko Odaki
@ 2023-07-31 15:13   ` Helge Deller
  0 siblings, 0 replies; 25+ messages in thread
From: Helge Deller @ 2023-07-31 15:13 UTC (permalink / raw)
  To: Akihiko Odaki; +Cc: qemu-devel, Laurent Vivier

On 7/31/23 10:03, Akihiko Odaki wrote:
> Later the returned value is compared with -1, and negated errno is not
> expected.
>
> Fixes: 00faf08c95 ("linux-user: Don't use MAP_FIXED in do_brk()")
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>

Reviewed-by: Helge Deller <deller@gmx.de>

Helge

> ---
>   linux-user/syscall.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 95727a816a..b9d2ec02f9 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -862,9 +862,9 @@ abi_long do_brk(abi_ulong brk_val)
>        */
>       if (new_host_brk_page > brk_page) {
>           new_alloc_size = new_host_brk_page - brk_page;
> -        mapped_addr = get_errno(target_mmap(brk_page, new_alloc_size,
> -                                        PROT_READ|PROT_WRITE,
> -                                        MAP_ANON|MAP_PRIVATE, 0, 0));
> +        mapped_addr = target_mmap(brk_page, new_alloc_size,
> +                                  PROT_READ|PROT_WRITE,
> +                                  MAP_ANON|MAP_PRIVATE, 0, 0);
>       } else {
>           new_alloc_size = 0;
>           mapped_addr = brk_page;



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 4/5] linux-user: Do nothing if too small brk is specified
  2023-07-31  8:03 ` [PATCH 4/5] linux-user: Do nothing if too small brk is specified Akihiko Odaki
@ 2023-07-31 15:16   ` Helge Deller
  0 siblings, 0 replies; 25+ messages in thread
From: Helge Deller @ 2023-07-31 15:16 UTC (permalink / raw)
  To: Akihiko Odaki; +Cc: qemu-devel, Laurent Vivier

On 7/31/23 10:03, Akihiko Odaki wrote:
> Linux 6.4.7 does nothing when a value smaller than the initial brk is
> specified.
>
> Fixes: 86f04735ac ("linux-user: Fix brk() to release pages")
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>

Reviewed-by: Helge Deller <deller@gmx.de>

Helge

> ---
>   linux-user/syscall.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index ac429a185a..ebdc8c144c 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -820,14 +820,14 @@ abi_long do_brk(abi_ulong brk_val)
>
>       /* brk pointers are always untagged */
>
> -    /* return old brk value if brk_val unchanged or zero */
> -    if (!brk_val || brk_val == target_brk) {
> +    /* return old brk value if brk_val unchanged */
> +    if (brk_val == target_brk) {
>           return target_brk;
>       }
>
>       /* do not allow to shrink below initial brk value */
>       if (brk_val < initial_target_brk) {
> -        brk_val = initial_target_brk;
> +        return target_brk;
>       }
>
>       new_brk = TARGET_PAGE_ALIGN(brk_val);



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 1/5] linux-user: Unset MAP_FIXED_NOREPLACE for host
  2023-07-31  8:03 ` [PATCH 1/5] linux-user: Unset MAP_FIXED_NOREPLACE for host Akihiko Odaki
@ 2023-07-31 15:43   ` Richard Henderson
  0 siblings, 0 replies; 25+ messages in thread
From: Richard Henderson @ 2023-07-31 15:43 UTC (permalink / raw)
  To: Akihiko Odaki; +Cc: qemu-devel, Laurent Vivier

On 7/31/23 01:03, Akihiko Odaki wrote:
> Passing MAP_FIXED_NOREPLACE to host will fail if the virtual
> address space is reserved with mmap. Replace it with MAP_FIXED.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>   linux-user/mmap.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
> index a5dfb56545..2f26cbaf5d 100644
> --- a/linux-user/mmap.c
> +++ b/linux-user/mmap.c
> @@ -610,6 +610,8 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot,
>               goto fail;
>           }
>   
> +        flags = (flags & ~MAP_FIXED_NOREPLACE) | MAP_FIXED;

This is wrong for 64-bit guests, when reserved_va is not in effect.


r~



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 3/5] linux-user: Use MAP_FIXED_NOREPLACE for do_brk()
  2023-07-31 13:32     ` Akihiko Odaki
  2023-07-31 13:45       ` Peter Maydell
@ 2023-07-31 15:44       ` Richard Henderson
  1 sibling, 0 replies; 25+ messages in thread
From: Richard Henderson @ 2023-07-31 15:44 UTC (permalink / raw)
  To: Akihiko Odaki, Peter Maydell; +Cc: qemu-devel, Laurent Vivier

On 7/31/23 06:32, Akihiko Odaki wrote:
> On 2023/07/31 20:44, Peter Maydell wrote:
>> On Mon, 31 Jul 2023 at 09:04, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>
>>> MAP_FIXED_NOREPLACE can ensure the mapped address is fixed without
>>> concerning that the new mapping overwrites something else.
>>
>> MAP_FIXED_NOREPLACE only came in with Linux 4.17. So
>> I think we still need to handle the "mapped address
>> is not the one we asked for" error condition, because
>> it can happen on older host kernels that ignore the
>> MAP_FIXED_NOREPLACE flag.
>>
>> thanks
>> -- PMM
> 
> MAP_FIXED_NOREPLACE is substituted with MAP_FIXED before passing to the host with patch 1. 
> The NOREPLACE constraint is still ensured by inspecting the guest page table.

Won't work for 64-bit guests.


r~



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 5/5] linux-user: Do not align brk with host page size
  2023-07-31  8:03 ` [PATCH 5/5] linux-user: Do not align brk with host page size Akihiko Odaki
@ 2023-07-31 15:51   ` Helge Deller
  2023-07-31 22:43     ` Akihiko Odaki
  0 siblings, 1 reply; 25+ messages in thread
From: Helge Deller @ 2023-07-31 15:51 UTC (permalink / raw)
  To: Akihiko Odaki; +Cc: qemu-devel, Laurent Vivier

On 7/31/23 10:03, Akihiko Odaki wrote:
> do_brk() minimizes calls into target_mmap() by aligning the address
> with host page size, which is potentially larger than the target page
> size.

Keep in mind, that host page size can be smaller than target page size too.
That's the reason why host brk (brk_page) and target brk (target_brk)
needs to be tracked individually.
So, it's not an optimization, but required.
Btw, I think we have been there before with the idea of
just keeping track of host pages...

> However, the current implementation of this optimization has two
> bugs:
>
> - The start of brk is rounded up with the host page size while brk
>    advertises an address aligned with the target page size as the
>    beginning of brk. This makes the beginning of brk unmapped.
> - Content clearing after mapping is flawed. The size to clear is
>    specified as HOST_PAGE_ALIGN(brk_page) - brk_page, but brk_page is
>    aligned with the host page size so it is always zero.
>
> This optimization actually has no practical benefit. It makes difference
> when brk() is called multiple times with values in a range of the host
> page size. However, sophisticated memory allocators try to avoid to
> make such frequent brk() calls. For example, glibc 2.37 calls brk() to
> shrink the heap only when there is a room more than 128 KiB. It is
> rare to have a page size larger than 128 KiB if it happens.
>
> Let's remove the optimization to fix the bugs and make the code simpler.
>
> Fixes: 86f04735ac ("linux-user: Fix brk() to release pages")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1616
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>   linux-user/elfload.c |  4 ++--
>   linux-user/syscall.c | 54 ++++++++++----------------------------------
>   2 files changed, 14 insertions(+), 44 deletions(-)
>
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 861ec07abc..2aee2298ec 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -3678,8 +3678,8 @@ int load_elf_binary(struct linux_binprm *bprm, struct image_info *info)
>        * to mmap pages in this space.
>        */
>       if (info->reserve_brk) {
> -        abi_ulong start_brk = HOST_PAGE_ALIGN(info->brk);
> -        abi_ulong end_brk = HOST_PAGE_ALIGN(info->brk + info->reserve_brk);
> +        abi_ulong start_brk = TARGET_PAGE_ALIGN(info->brk);
> +        abi_ulong end_brk = TARGET_PAGE_ALIGN(info->brk + info->reserve_brk);

In my patchset I removed the reserve_brk stuff...

>           target_munmap(start_brk, end_brk - start_brk);
>       }
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index ebdc8c144c..475260b7ce 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -802,81 +802,51 @@ static inline int host_to_target_sock_type(int host_type)
>   }
>
>   static abi_ulong target_brk, initial_target_brk;
> -static abi_ulong brk_page;

with that you loose the ability to track the brk address on host.


>   void target_set_brk(abi_ulong new_brk)
>   {
>       target_brk = TARGET_PAGE_ALIGN(new_brk);
>       initial_target_brk = target_brk;
> -    brk_page = HOST_PAGE_ALIGN(target_brk);
>   }
>
>   /* do_brk() must return target values and target errnos. */
>   abi_long do_brk(abi_ulong brk_val)
>   {
>       abi_long mapped_addr;
> -    abi_ulong new_alloc_size;
> -    abi_ulong new_brk, new_host_brk_page;
> +    abi_ulong new_brk;
> +    abi_ulong old_brk;
>
>       /* brk pointers are always untagged */
>
> -    /* return old brk value if brk_val unchanged */
> -    if (brk_val == target_brk) {
> -        return target_brk;
> -    }
> -
>       /* do not allow to shrink below initial brk value */
>       if (brk_val < initial_target_brk) {
>           return target_brk;
>       }
>
>       new_brk = TARGET_PAGE_ALIGN(brk_val);
> -    new_host_brk_page = HOST_PAGE_ALIGN(brk_val);
> +    old_brk = TARGET_PAGE_ALIGN(target_brk);
>
> -    /* brk_val and old target_brk might be on the same page */
> -    if (new_brk == TARGET_PAGE_ALIGN(target_brk)) {
> -        /* empty remaining bytes in (possibly larger) host page */
> -        memset(g2h_untagged(new_brk), 0, new_host_brk_page - new_brk);

you can't drop that.
guest could have clobbered the upper parts, e.g. when it first mapped
all (e.g. 8k), wrote to it, then released upper half of the host page (4k) and
then remaps the upper 4k again.
In that case we need to ensure that the upper 4k gets cleaned. On a physical
machine the kernel would have requested a new page, but here we are stuck
with the bigger host page which we can't simply release.

Helge

> +    /* new and old target_brk might be on the same page */
> +    if (new_brk == old_brk) {
>           target_brk = brk_val;
>           return target_brk;
>       }
>
>       /* Release heap if necesary */
> -    if (new_brk < target_brk) {
> -        /* empty remaining bytes in (possibly larger) host page */
> -        memset(g2h_untagged(new_brk), 0, new_host_brk_page - new_brk);
> -
> -        /* free unused host pages and set new brk_page */
> -        target_munmap(new_host_brk_page, brk_page - new_host_brk_page);
> -        brk_page = new_host_brk_page;
> +    if (new_brk < old_brk) {
> +        target_munmap(new_brk, old_brk - new_brk);
>
>           target_brk = brk_val;
>           return target_brk;
>       }
>
> -    if (new_host_brk_page > brk_page) {
> -        new_alloc_size = new_host_brk_page - brk_page;
> -        mapped_addr = target_mmap(brk_page, new_alloc_size,
> -                                  PROT_READ | PROT_WRITE,
> -                                  MAP_FIXED_NOREPLACE | MAP_ANON | MAP_PRIVATE,
> -                                  0, 0);
> -    } else {
> -        new_alloc_size = 0;
> -        mapped_addr = brk_page;
> -    }
> -
> -    if (mapped_addr == brk_page) {
> -        /* Heap contents are initialized to zero, as for anonymous
> -         * mapped pages.  Technically the new pages are already
> -         * initialized to zero since they *are* anonymous mapped
> -         * pages, however we have to take care with the contents that
> -         * come from the remaining part of the previous page: it may
> -         * contains garbage data due to a previous heap usage (grown
> -         * then shrunken).  */
> -        memset(g2h_untagged(brk_page), 0, HOST_PAGE_ALIGN(brk_page) - brk_page);
> +    mapped_addr = target_mmap(old_brk, new_brk - old_brk,
> +                              PROT_READ | PROT_WRITE,
> +                              MAP_FIXED_NOREPLACE | MAP_ANON | MAP_PRIVATE,
> +                              0, 0);
>
> +    if (mapped_addr == old_brk) {
>           target_brk = brk_val;
> -        brk_page = new_host_brk_page;
>           return target_brk;
>       }
>



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 0/5] linux-user: brk/mmap fixes
  2023-07-31 10:23     ` Akihiko Odaki
@ 2023-07-31 16:47       ` Helge Deller
  2023-07-31 17:43         ` Helge Deller
  0 siblings, 1 reply; 25+ messages in thread
From: Helge Deller @ 2023-07-31 16:47 UTC (permalink / raw)
  To: Akihiko Odaki, Joel Stanley, Michael Tokarev; +Cc: qemu-devel, Laurent Vivier

On 7/31/23 12:23, Akihiko Odaki wrote:
> On 2023/07/31 19:17, Joel Stanley wrote:
>> On Mon, 31 Jul 2023 at 09:31, Michael Tokarev <mjt@tls.msk.ru> wrote:
>>>
>>> 31.07.2023 11:03, Akihiko Odaki wrote:
>>>> linux-user was failing on M2 MacBook Air. Digging into the details, I found
>>>> several bugs in brk and mmap so here are fixes.
>>>
>>> There's another work in this area by Helge Deller, have you seen it?
>>> ("linux-user: Fix and optimize target memory layout", a v5 already).
>>
>> Applying this series fixes the qemu-arm running the static armhf
>> binary on my ppc64le host that I reported here[1].
> >> [1] https://lore.kernel.org/qemu-devel/CACPK8XeyqcEDyyL3Jw2WYWs_gGdtTCf2=Ly04CMgkshSMdj7RA@mail.gmail.com/
>>
>> Tested-by: Joel Stanley <joel@jms.id.au>
>
> Thanks for testing.
>
>>
>> The changes conflict with Helge's patches, so it would be good to work
>> out which of your changes should be combined with his.
>
> I suggest Helge to rebase his change to my series. The below is some
> detailed explanation:

> It is almost orthogonal to my series, but patch 2 will conflict with
> my series since it changes how the initial brk is calculated.
>
> Unfortunately I think patch 2 has a bug. Without my patch, do_brk()
> assumes the heap is aligned with the host page size, but the patch
> does not consider the case that the host and target page sizes are
> different.
I've included your patches #2 (bugfix) and #3 (cleanup) to my latest
patch series and published it at
https://github.com/hdeller/qemu-hppa/tree/brk-fixes-akihiko

Maybe you and Joel could try it out?

Helge


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 0/5] linux-user: brk/mmap fixes
  2023-07-31 16:47       ` Helge Deller
@ 2023-07-31 17:43         ` Helge Deller
  2023-07-31 18:24           ` Helge Deller
  0 siblings, 1 reply; 25+ messages in thread
From: Helge Deller @ 2023-07-31 17:43 UTC (permalink / raw)
  To: Akihiko Odaki, Joel Stanley, Michael Tokarev; +Cc: qemu-devel, Laurent Vivier

On 7/31/23 18:47, Helge Deller wrote:
> On 7/31/23 12:23, Akihiko Odaki wrote:
>> On 2023/07/31 19:17, Joel Stanley wrote:
>>> On Mon, 31 Jul 2023 at 09:31, Michael Tokarev <mjt@tls.msk.ru> wrote:
>>>>
>>>> 31.07.2023 11:03, Akihiko Odaki wrote:
>>>>> linux-user was failing on M2 MacBook Air. Digging into the details, I found
>>>>> several bugs in brk and mmap so here are fixes.
>>>>
>>>> There's another work in this area by Helge Deller, have you seen it?
>>>> ("linux-user: Fix and optimize target memory layout", a v5 already).
>>>
>>> Applying this series fixes the qemu-arm running the static armhf
>>> binary on my ppc64le host that I reported here[1].
>> >> [1] https://lore.kernel.org/qemu-devel/CACPK8XeyqcEDyyL3Jw2WYWs_gGdtTCf2=Ly04CMgkshSMdj7RA@mail.gmail.com/
>>>
>>> Tested-by: Joel Stanley <joel@jms.id.au>
>>
>> Thanks for testing.
>>
>>>
>>> The changes conflict with Helge's patches, so it would be good to work
>>> out which of your changes should be combined with his.
>>
>> I suggest Helge to rebase his change to my series. The below is some
>> detailed explanation:
>
>> It is almost orthogonal to my series, but patch 2 will conflict with
>> my series since it changes how the initial brk is calculated.
>>
>> Unfortunately I think patch 2 has a bug. Without my patch, do_brk()
>> assumes the heap is aligned with the host page size, but the patch
>> does not consider the case that the host and target page sizes are
>> different.
> I've included your patches #2 (bugfix) and #3 (cleanup) to my latest
> patch series and published it at
> https://github.com/hdeller/qemu-hppa/tree/brk-fixes-akihiko
>
> Maybe you and Joel could try it out?

I re-read the thread again. As it seems Joel already tried the latest
version from me? Sadly I can't test myself on ppc64le (static binary
needs klibc-PupSAGgtpafMlSLXOLgje1kXFo8.so in /usr/lib which I can't
install on a debian porterbox).

I still believe we need to track host and target brk page, but I'll give
your patch a try.

Helge


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 0/5] linux-user: brk/mmap fixes
  2023-07-31 17:43         ` Helge Deller
@ 2023-07-31 18:24           ` Helge Deller
  2023-08-01  4:49             ` Joel Stanley
  0 siblings, 1 reply; 25+ messages in thread
From: Helge Deller @ 2023-07-31 18:24 UTC (permalink / raw)
  To: Akihiko Odaki, Joel Stanley, Michael Tokarev; +Cc: qemu-devel, Laurent Vivier

On 7/31/23 19:43, Helge Deller wrote:
> On 7/31/23 18:47, Helge Deller wrote:
>> On 7/31/23 12:23, Akihiko Odaki wrote:
>>> On 2023/07/31 19:17, Joel Stanley wrote:
>>>> On Mon, 31 Jul 2023 at 09:31, Michael Tokarev <mjt@tls.msk.ru> wrote:
>>>>>
>>>>> 31.07.2023 11:03, Akihiko Odaki wrote:
>>>>>> linux-user was failing on M2 MacBook Air. Digging into the details, I found
>>>>>> several bugs in brk and mmap so here are fixes.
>>>>>
>>>>> There's another work in this area by Helge Deller, have you seen it?
>>>>> ("linux-user: Fix and optimize target memory layout", a v5 already).
>>>>
>>>> Applying this series fixes the qemu-arm running the static armhf
>>>> binary on my ppc64le host that I reported here[1].
>>> >> [1] https://lore.kernel.org/qemu-devel/CACPK8XeyqcEDyyL3Jw2WYWs_gGdtTCf2=Ly04CMgkshSMdj7RA@mail.gmail.com/
>>>>
>>>> Tested-by: Joel Stanley <joel@jms.id.au>
>>>
>>> Thanks for testing.
>>>
>>>>
>>>> The changes conflict with Helge's patches, so it would be good to work
>>>> out which of your changes should be combined with his.
>>>
>>> I suggest Helge to rebase his change to my series. The below is some
>>> detailed explanation:
>>
>>> It is almost orthogonal to my series, but patch 2 will conflict with
>>> my series since it changes how the initial brk is calculated.
>>>
>>> Unfortunately I think patch 2 has a bug. Without my patch, do_brk()
>>> assumes the heap is aligned with the host page size, but the patch
>>> does not consider the case that the host and target page sizes are
>>> different.
>> I've included your patches #2 (bugfix) and #3 (cleanup) to my latest
>> patch series and published it at
>> https://github.com/hdeller/qemu-hppa/tree/brk-fixes-akihiko
>>
>> Maybe you and Joel could try it out?
>
> I re-read the thread again. As it seems Joel already tried the latest
> version from me? Sadly I can't test myself on ppc64le (static binary
> needs klibc-PupSAGgtpafMlSLXOLgje1kXFo8.so in /usr/lib which I can't
> install on a debian porterbox).
>
> I still believe we need to track host and target brk page, but I'll give
> your patch a try.

As suggested, I've based my patches on top of yours and the tree can be
pulled from:
git pull https://github.com/hdeller/qemu-hppa/   brk-fixes-akihiko-2

My patches are neccessary to fix an arm-static testcase:
	/usr/bin/qemu-arm-static ./fstype

Let's try this patch series...

Helge


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 5/5] linux-user: Do not align brk with host page size
  2023-07-31 15:51   ` Helge Deller
@ 2023-07-31 22:43     ` Akihiko Odaki
  0 siblings, 0 replies; 25+ messages in thread
From: Akihiko Odaki @ 2023-07-31 22:43 UTC (permalink / raw)
  To: Helge Deller; +Cc: qemu-devel, Laurent Vivier

On 2023/08/01 0:51, Helge Deller wrote:
> On 7/31/23 10:03, Akihiko Odaki wrote:
>> do_brk() minimizes calls into target_mmap() by aligning the address
>> with host page size, which is potentially larger than the target page
>> size.
> 
> Keep in mind, that host page size can be smaller than target page size too.
> That's the reason why host brk (brk_page) and target brk (target_brk)
> needs to be tracked individually.
> So, it's not an optimization, but required.
> Btw, I think we have been there before with the idea of
> just keeping track of host pages...

It does not matter even if the host page size is smaller or larger. 
do_brk() solely relies on target_mmap(), which works with target page size.

> 
>> However, the current implementation of this optimization has two
>> bugs:
>>
>> - The start of brk is rounded up with the host page size while brk
>>    advertises an address aligned with the target page size as the
>>    beginning of brk. This makes the beginning of brk unmapped.
>> - Content clearing after mapping is flawed. The size to clear is
>>    specified as HOST_PAGE_ALIGN(brk_page) - brk_page, but brk_page is
>>    aligned with the host page size so it is always zero.
>>
>> This optimization actually has no practical benefit. It makes difference
>> when brk() is called multiple times with values in a range of the host
>> page size. However, sophisticated memory allocators try to avoid to
>> make such frequent brk() calls. For example, glibc 2.37 calls brk() to
>> shrink the heap only when there is a room more than 128 KiB. It is
>> rare to have a page size larger than 128 KiB if it happens.
>>
>> Let's remove the optimization to fix the bugs and make the code simpler.
>>
>> Fixes: 86f04735ac ("linux-user: Fix brk() to release pages")
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1616
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>>   linux-user/elfload.c |  4 ++--
>>   linux-user/syscall.c | 54 ++++++++++----------------------------------
>>   2 files changed, 14 insertions(+), 44 deletions(-)
>>
>> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
>> index 861ec07abc..2aee2298ec 100644
>> --- a/linux-user/elfload.c
>> +++ b/linux-user/elfload.c
>> @@ -3678,8 +3678,8 @@ int load_elf_binary(struct linux_binprm *bprm, 
>> struct image_info *info)
>>        * to mmap pages in this space.
>>        */
>>       if (info->reserve_brk) {
>> -        abi_ulong start_brk = HOST_PAGE_ALIGN(info->brk);
>> -        abi_ulong end_brk = HOST_PAGE_ALIGN(info->brk + 
>> info->reserve_brk);
>> +        abi_ulong start_brk = TARGET_PAGE_ALIGN(info->brk);
>> +        abi_ulong end_brk = TARGET_PAGE_ALIGN(info->brk + 
>> info->reserve_brk);
> 
> In my patchset I removed the reserve_brk stuff...
> 
>>           target_munmap(start_brk, end_brk - start_brk);
>>       }
>>
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index ebdc8c144c..475260b7ce 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -802,81 +802,51 @@ static inline int host_to_target_sock_type(int 
>> host_type)
>>   }
>>
>>   static abi_ulong target_brk, initial_target_brk;
>> -static abi_ulong brk_page;
> 
> with that you loose the ability to track the brk address on host.

brk_page actually doesn't track the address on the host. It's just an 
address on the target but aligned with host page size. And the alignment 
is purely for optimization.

> 
> 
>>   void target_set_brk(abi_ulong new_brk)
>>   {
>>       target_brk = TARGET_PAGE_ALIGN(new_brk);
>>       initial_target_brk = target_brk;
>> -    brk_page = HOST_PAGE_ALIGN(target_brk);
>>   }
>>
>>   /* do_brk() must return target values and target errnos. */
>>   abi_long do_brk(abi_ulong brk_val)
>>   {
>>       abi_long mapped_addr;
>> -    abi_ulong new_alloc_size;
>> -    abi_ulong new_brk, new_host_brk_page;
>> +    abi_ulong new_brk;
>> +    abi_ulong old_brk;
>>
>>       /* brk pointers are always untagged */
>>
>> -    /* return old brk value if brk_val unchanged */
>> -    if (brk_val == target_brk) {
>> -        return target_brk;
>> -    }
>> -
>>       /* do not allow to shrink below initial brk value */
>>       if (brk_val < initial_target_brk) {
>>           return target_brk;
>>       }
>>
>>       new_brk = TARGET_PAGE_ALIGN(brk_val);
>> -    new_host_brk_page = HOST_PAGE_ALIGN(brk_val);
>> +    old_brk = TARGET_PAGE_ALIGN(target_brk);
>>
>> -    /* brk_val and old target_brk might be on the same page */
>> -    if (new_brk == TARGET_PAGE_ALIGN(target_brk)) {
>> -        /* empty remaining bytes in (possibly larger) host page */
>> -        memset(g2h_untagged(new_brk), 0, new_host_brk_page - new_brk);
> 
> you can't drop that.
> guest could have clobbered the upper parts, e.g. when it first mapped
> all (e.g. 8k), wrote to it, then released upper half of the host page 
> (4k) and
> then remaps the upper 4k again.
> In that case we need to ensure that the upper 4k gets cleaned. On a 
> physical
> machine the kernel would have requested a new page, but here we are stuck
> with the bigger host page which we can't simply release.

Such a case should also be handled with target_mmap().

Regards,
Akihiko Odaki


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 0/5] linux-user: brk/mmap fixes
  2023-07-31 18:24           ` Helge Deller
@ 2023-08-01  4:49             ` Joel Stanley
  2023-08-01 10:43               ` Helge Deller
  0 siblings, 1 reply; 25+ messages in thread
From: Joel Stanley @ 2023-08-01  4:49 UTC (permalink / raw)
  To: Helge Deller; +Cc: Akihiko Odaki, Michael Tokarev, qemu-devel, Laurent Vivier

On Mon, 31 Jul 2023 at 18:24, Helge Deller <deller@gmx.de> wrote:

> > I re-read the thread again. As it seems Joel already tried the latest
> > version from me? Sadly I can't test myself on ppc64le (static binary
> > needs klibc-PupSAGgtpafMlSLXOLgje1kXFo8.so in /usr/lib which I can't
> > install on a debian porterbox).
> >
> > I still believe we need to track host and target brk page, but I'll give
> > your patch a try.
>
> As suggested, I've based my patches on top of yours and the tree can be
> pulled from:
> git pull https://github.com/hdeller/qemu-hppa/   brk-fixes-akihiko-2
>
> My patches are neccessary to fix an arm-static testcase:
>         /usr/bin/qemu-arm-static ./fstype
>
> Let's try this patch series...

The armhf static binary works with expected output.

The arm static binary causes qemu to segfault:

$ gdb -quiet --args ./build/qemu-arm -d guest_errors,page,strace ~/hello
Reading symbols from ./build/qemu-arm...
(gdb) r
Starting program: build/qemu-arm -d guest_errors,page,strace
/home/joel/hello
Using host libthread_db library "/lib/powerpc64le-linux-gnu/libthread_db.so.1".
[New Thread 0x7ffff762ece0 (LWP 118359)]
host mmap_min_addr=0x10000
pgb_find_hole: base @ 140420000 for 4294967296 bytes
pgb_static: base @ 140420000 for 4294967295 bytes
pgb_reserved_va: base @ 0x140420000 for 4294967296 bytes
Locating guest address space @ 0x140420000
page layout changed following mmap
start    end      size     prot
00010000-00090000 00080000 ---
00090000-0009b000 0000b000 ---
ffff0000-00000000 00010000 r-x
page layout changed following mmap
start    end      size     prot
00010000-00090000 00080000 r-x
00090000-0009b000 0000b000 ---
ffff0000-00000000 00010000 r-x
page layout changed following mmap
start    end      size     prot
00010000-00090000 00080000 r-x
00090000-000a0000 00010000 rw-
ffff0000-00000000 00010000 r-x
page layout changed following mmap
start    end      size     prot
00010000-00090000 00080000 r-x
00090000-000a0000 00010000 rw-
e0000000-e0810000 00810000 rw-
ffff0000-00000000 00010000 r-x
page layout changed following mmap
start    end      size     prot
00010000-00090000 00080000 r-x
00090000-000a0000 00010000 rw-
e0000000-e0010000 00010000 ---
e0010000-e0811000 00801000 rw-
ffff0000-00000000 00010000 r-x
guest_base  0x140420000
page layout changed following binary load
start    end      size     prot
00010000-00090000 00080000 r-x
00090000-000a0000 00010000 rw-
e0000000-e0010000 00010000 ---
e0010000-e0810000 00800000 rw-
e0810000-e0811000 00001000 r-x
ffff0000-00000000 00010000 r-x
start_brk   0x00000000
end_code    0x00084f7c
start_code  0x00010000
start_data  0x00095098
end_data    0x00098394
start_stack 0xe080f410
brk         0x0009b000
entry       0x00010418
argv_start  0xe080f414
env_start   0xe080f41c
auxv_start  0xe080f4a0
118357 brk(NULL) = 0x0009b000
118357 brk(0x0009b8fc) = 0x0009b000

Thread 1 "qemu-arm" received signal SIGSEGV, Segmentation fault.
0x00007fffeed9bb74 in code_gen_buffer ()
(gdb)
(gdb) bt
#0  0x00007fffeed9bb74 in code_gen_buffer ()
#1  0x0000000100169e3c in cpu_tb_exec (cpu=cpu@entry=0x1003d4aa0,
    itb=itb@entry=0x7fffeed9ba60 <code_gen_buffer+47512>,
tb_exit=tb_exit@entry=0x7fffffffe50c)
    at ../accel/tcg/cpu-exec.c:457
#2  0x000000010016a564 in cpu_loop_exec_tb (tb_exit=0x7fffffffe50c,
last_tb=<synthetic pointer>,
    pc=<optimised out>, tb=0x7fffeed9ba60 <code_gen_buffer+47512>,
cpu=<optimised out>)
    at ../accel/tcg/cpu-exec.c:919
#3  cpu_exec_loop (cpu=cpu@entry=0x1003d4aa0, sc=<optimised out>) at
../accel/tcg/cpu-exec.c:1040
#4  0x000000010016aa0c in cpu_exec_setjmp (cpu=cpu@entry=0x1003d4aa0,
sc=<optimised out>)
    at ../accel/tcg/cpu-exec.c:1057
#5  0x000000010016b0d0 in cpu_exec (cpu=0x1003d4aa0) at
../accel/tcg/cpu-exec.c:1083
#6  0x000000010004d780 in cpu_loop (env=0x1003d4fb0) at
../linux-user/arm/cpu_loop.c:323
#7  0x0000000100047534 in main (argc=<optimised out>,
argv=0x7ffffffff178, envp=<optimised out>)
    at ../linux-user/main.c:975

I tested 74a22a175c4340a01f6f860f72307093e3307681.


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 0/5] linux-user: brk/mmap fixes
  2023-08-01  4:49             ` Joel Stanley
@ 2023-08-01 10:43               ` Helge Deller
  2023-08-02  7:26                 ` Akihiko Odaki
  0 siblings, 1 reply; 25+ messages in thread
From: Helge Deller @ 2023-08-01 10:43 UTC (permalink / raw)
  To: Joel Stanley; +Cc: Akihiko Odaki, Michael Tokarev, qemu-devel, Laurent Vivier

On 8/1/23 06:49, Joel Stanley wrote:
> On Mon, 31 Jul 2023 at 18:24, Helge Deller <deller@gmx.de> wrote:
>> As suggested, I've based my patches on top of yours and the tree can be
>> pulled from:
>> git pull https://github.com/hdeller/qemu-hppa/   brk-fixes-akihiko-2
>>
>> My patches are neccessary to fix an arm-static testcase:
>>          /usr/bin/qemu-arm-static ./fstype
>>
>> Let's try this patch series...
>
> The armhf static binary works with expected output.

Good!

> The arm static binary causes qemu to segfault:

I can't reproduce here.
I tried it in an arm64 chroot which provided the cross-compiler and worked for me:

(arm64-chroot)root@p100:/# uname -a
Linux p100 6.4.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Jul 24 20:51:12 UTC 2023 aarch64 GNU/Linux
(arm64-chroot)root@p100:/# arm-linux-gnueabi-gcc-13 -o hello hello.c -static
(arm64-chroot)root@p100:/# file hello
hello: ELF 32-bit LSB executable, ARM, EABI5 version 1 (SYSV), statically linked, BuildID[sha1]=fa0f7cd6e1779fa8cd76c6e5d3123900ceefa952, for GNU/Linux 3.2.0, not stripped
(arm64-chroot)root@p100:/# ./hello
Hello, World!

Maybe you can send me your binary (and the needed klibc*so)?
Btw, I tested a whole bunch of platforms too, see below...

Helge

> $ gdb -quiet --args ./build/qemu-arm -d guest_errors,page,strace ~/hello
> Reading symbols from ./build/qemu-arm...
> (gdb) r
> Starting program: build/qemu-arm -d guest_errors,page,strace
> /home/joel/hello
> Using host libthread_db library "/lib/powerpc64le-linux-gnu/libthread_db.so.1".
> [New Thread 0x7ffff762ece0 (LWP 118359)]
> host mmap_min_addr=0x10000
> pgb_find_hole: base @ 140420000 for 4294967296 bytes
> pgb_static: base @ 140420000 for 4294967295 bytes
> pgb_reserved_va: base @ 0x140420000 for 4294967296 bytes
> Locating guest address space @ 0x140420000
> page layout changed following mmap
> start    end      size     prot
> 00010000-00090000 00080000 ---
> 00090000-0009b000 0000b000 ---
> ffff0000-00000000 00010000 r-x
> page layout changed following mmap
> start    end      size     prot
> 00010000-00090000 00080000 r-x
> 00090000-0009b000 0000b000 ---
> ffff0000-00000000 00010000 r-x
> page layout changed following mmap
> start    end      size     prot
> 00010000-00090000 00080000 r-x
> 00090000-000a0000 00010000 rw-
> ffff0000-00000000 00010000 r-x
> page layout changed following mmap
> start    end      size     prot
> 00010000-00090000 00080000 r-x
> 00090000-000a0000 00010000 rw-
> e0000000-e0810000 00810000 rw-
> ffff0000-00000000 00010000 r-x
> page layout changed following mmap
> start    end      size     prot
> 00010000-00090000 00080000 r-x
> 00090000-000a0000 00010000 rw-
> e0000000-e0010000 00010000 ---
> e0010000-e0811000 00801000 rw-
> ffff0000-00000000 00010000 r-x
> guest_base  0x140420000
> page layout changed following binary load
> start    end      size     prot
> 00010000-00090000 00080000 r-x
> 00090000-000a0000 00010000 rw-
> e0000000-e0010000 00010000 ---
> e0010000-e0810000 00800000 rw-
> e0810000-e0811000 00001000 r-x
> ffff0000-00000000 00010000 r-x
> start_brk   0x00000000
> end_code    0x00084f7c
> start_code  0x00010000
> start_data  0x00095098
> end_data    0x00098394
> start_stack 0xe080f410
> brk         0x0009b000
> entry       0x00010418
> argv_start  0xe080f414
> env_start   0xe080f41c
> auxv_start  0xe080f4a0
> 118357 brk(NULL) = 0x0009b000
> 118357 brk(0x0009b8fc) = 0x0009b000
>
> Thread 1 "qemu-arm" received signal SIGSEGV, Segmentation fault.
> 0x00007fffeed9bb74 in code_gen_buffer ()
> (gdb)
> (gdb) bt
> #0  0x00007fffeed9bb74 in code_gen_buffer ()
> #1  0x0000000100169e3c in cpu_tb_exec (cpu=cpu@entry=0x1003d4aa0,
>      itb=itb@entry=0x7fffeed9ba60 <code_gen_buffer+47512>,
> tb_exit=tb_exit@entry=0x7fffffffe50c)
>      at ../accel/tcg/cpu-exec.c:457
> #2  0x000000010016a564 in cpu_loop_exec_tb (tb_exit=0x7fffffffe50c,
> last_tb=<synthetic pointer>,
>      pc=<optimised out>, tb=0x7fffeed9ba60 <code_gen_buffer+47512>,
> cpu=<optimised out>)
>      at ../accel/tcg/cpu-exec.c:919
> #3  cpu_exec_loop (cpu=cpu@entry=0x1003d4aa0, sc=<optimised out>) at
> ../accel/tcg/cpu-exec.c:1040
> #4  0x000000010016aa0c in cpu_exec_setjmp (cpu=cpu@entry=0x1003d4aa0,
> sc=<optimised out>)
>      at ../accel/tcg/cpu-exec.c:1057
> #5  0x000000010016b0d0 in cpu_exec (cpu=0x1003d4aa0) at
> ../accel/tcg/cpu-exec.c:1083
> #6  0x000000010004d780 in cpu_loop (env=0x1003d4fb0) at
> ../linux-user/arm/cpu_loop.c:323
> #7  0x0000000100047534 in main (argc=<optimised out>,
> argv=0x7ffffffff178, envp=<optimised out>)
>      at ../linux-user/main.c:975
>
> I tested 74a22a175c4340a01f6f860f72307093e3307681.

Those I did tested sucessfully (static binary):

alpha-chroot:
Linux p100 6.4.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Jul 24 20:51:12 UTC 2023 alpha GNU/Linux
/hello: ELF 64-bit LSB executable, Alpha (unofficial), version 1 (SYSV), statically linked, BuildID[sha1]=5bf21139aa3937121e8843b062619de8e53d035a, for GNU/Linux 3.2.0, not stripped
Hello, World!

arm64-chroot:
Linux p100 6.4.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Jul 24 20:51:12 UTC 2023 aarch64 GNU/Linux
/hello: ELF 64-bit LSB executable, ARM aarch64, version 1 (GNU/Linux), statically linked, BuildID[sha1]=201827af1ffdef4fc2afa404047c6d1a41e4825e, for GNU/Linux 3.7.0, not stripped
Hello, World!

armel-chroot:
Linux p100 6.4.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Jul 24 20:51:12 UTC 2023 armv7l GNU/Linux
/hello: ELF 32-bit LSB executable, ARM, EABI5 version 1 (SYSV), statically linked, BuildID[sha1]=6e6a52f60037690052b2e54e750a56543ed9d7a0, for GNU/Linux 3.2.0, not stripped
Hello, World!

armhf-chroot:
Linux p100 6.4.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Jul 24 20:51:12 UTC 2023 armv7l GNU/Linux
/hello: ELF 32-bit LSB executable, ARM, EABI5 version 1 (GNU/Linux), statically linked, BuildID[sha1]=842df9fd0bf910f6a00c19d61435387efa591390, for GNU/Linux 3.2.0, not stripped
Hello, World!

hppa-chroot:
Linux p100 6.4.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Jul 24 20:51:12 UTC 2023 parisc GNU/Linux
/hello: ELF 32-bit MSB executable, PA-RISC, 1.1 version 1 (GNU/Linux), statically linked, BuildID[sha1]=03d4b299b31d30b5920e9fdcfccce071b77e4447, for GNU/Linux 3.2.0, not stripped
Hello, World!

m68k-chroot:
Linux p100 6.4.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Jul 24 20:51:12 UTC 2023 m68k GNU/Linux
/hello: ELF 32-bit MSB executable, Motorola m68k, 68020, version 1 (SYSV), statically linked, BuildID[sha1]=c01101b8ae6a6a0161a08b6ac24821b28daa5b73, for GNU/Linux 3.2.0, not stripped
Hello, World!

mips64el-chroot:
Linux p100 6.4.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Jul 24 20:51:12 UTC 2023 mips64 GNU/Linux
/hello: ELF 64-bit LSB executable, MIPS, MIPS64 rel2 version 1 (SYSV), statically linked, BuildID[sha1]=0c50fc29be7ef781cdfb4ec4c47b4e350cab218b, for GNU/Linux 3.2.0, not stripped
Hello, World!

mipsel-chroot:
Linux p100 6.4.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Jul 24 20:51:12 UTC 2023 mips GNU/Linux
/hello: ELF 32-bit LSB executable, MIPS, MIPS32 rel2 version 1 (SYSV), statically linked, BuildID[sha1]=e0db11bbc59070f5fefb4355d73df76791e96c29, for GNU/Linux 3.2.0, not stripped
Hello, World!

powerpc-chroot:
Linux p100 6.4.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Jul 24 20:51:12 UTC 2023 ppc GNU/Linux
/hello: ELF 32-bit MSB executable, PowerPC or cisco 4500, version 1 (SYSV), statically linked, BuildID[sha1]=4fe85ef8ebd86eb383ccf4fd741ce224143da2b2, for GNU/Linux 3.2.0, not stripped
Hello, World!

ppc64-chroot:
Linux p100 6.4.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Jul 24 20:51:12 UTC 2023 ppc64 GNU/Linux
/hello: ELF 64-bit MSB executable, 64-bit PowerPC or cisco 7500, Power ELF V1 ABI, version 1 (GNU/Linux), statically linked, BuildID[sha1]=c3bb5c4d94b2096f70261bf0ab1f3fc93813df8f, for GNU/Linux 3.2.0, not stripped
Hello, World!

ppc64el-chroot:
Linux p100 6.4.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Jul 24 20:51:12 UTC 2023 ppc64le GNU/Linux
/hello: ELF 64-bit LSB executable, 64-bit PowerPC or cisco 7500, OpenPOWER ELF V2 ABI, version 1 (GNU/Linux), statically linked, BuildID[sha1]=645abb5dcd9075d826d539675258fa5f9c7bc777, for GNU/Linux 3.10.0, not stripped
Hello, World!

s390x-chroot:
Linux p100 6.4.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Jul 24 20:51:12 UTC 2023 s390x GNU/Linux
/hello: ELF 64-bit MSB executable, IBM S/390, version 1 (GNU/Linux), statically linked, BuildID[sha1]=f512d5ac759962ab66ae947d1308c8ceedef8fd3, for GNU/Linux 3.2.0, not stripped
Hello, World!

sh4-chroot:
Linux p100 6.4.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Jul 24 20:51:12 UTC 2023 sh4 GNU/Linux
/hello: ELF 32-bit LSB executable, Renesas SH, version 1 (SYSV), statically linked, BuildID[sha1]=4cf38c7f67b5d7dc7a93c6ab513aaf0d2d21c4fc, for GNU/Linux 3.2.0, not stripped
Hello, World!

sparc64-chroot:
Linux p100 6.4.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Jul 24 20:51:12 UTC 2023 sparc64 GNU/Linux
/hello: ELF 64-bit MSB executable, SPARC V9, Sun UltraSPARC1 Extensions Required, relaxed memory ordering, version 1 (GNU/Linux), statically linked, BuildID[sha1]=36f02b1b3acc94f61dff6dc26205f82314c899e0, for GNU/Linux 3.2.0, not stripped
Hello, World!


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 0/5] linux-user: brk/mmap fixes
  2023-08-01 10:43               ` Helge Deller
@ 2023-08-02  7:26                 ` Akihiko Odaki
  2023-08-02 10:37                   ` Helge Deller
  0 siblings, 1 reply; 25+ messages in thread
From: Akihiko Odaki @ 2023-08-02  7:26 UTC (permalink / raw)
  To: Helge Deller, Joel Stanley; +Cc: Michael Tokarev, qemu-devel, Laurent Vivier

On 2023/08/01 19:43, Helge Deller wrote:
> On 8/1/23 06:49, Joel Stanley wrote:
>> On Mon, 31 Jul 2023 at 18:24, Helge Deller <deller@gmx.de> wrote:
>>> As suggested, I've based my patches on top of yours and the tree can be
>>> pulled from:
>>> git pull https://github.com/hdeller/qemu-hppa/   brk-fixes-akihiko-2
>>>
>>> My patches are neccessary to fix an arm-static testcase:
>>>          /usr/bin/qemu-arm-static ./fstype
>>>
>>> Let's try this patch series...
>>
>> The armhf static binary works with expected output.
> 
> Good!
> 
>> The arm static binary causes qemu to segfault:
> 
> I can't reproduce here.
> I tried it in an arm64 chroot which provided the cross-compiler and 
> worked for me:
> 
> (arm64-chroot)root@p100:/# uname -a
> Linux p100 6.4.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Jul 24 
> 20:51:12 UTC 2023 aarch64 GNU/Linux
> (arm64-chroot)root@p100:/# arm-linux-gnueabi-gcc-13 -o hello hello.c 
> -static
> (arm64-chroot)root@p100:/# file hello
> hello: ELF 32-bit LSB executable, ARM, EABI5 version 1 (SYSV), 
> statically linked, 
> BuildID[sha1]=fa0f7cd6e1779fa8cd76c6e5d3123900ceefa952, for GNU/Linux 
> 3.2.0, not stripped
> (arm64-chroot)root@p100:/# ./hello
> Hello, World!
> 
> Maybe you can send me your binary (and the needed klibc*so)?

Binaries will certainly help. I also suggest adding -trace target_mmap 
so that we can see what's passed to target_mmap().

I also sent a new version so please rebase to it and try again.
https://patchew.org/QEMU/20230802071754.14876-1-akihiko.odaki@daynix.com/

Helge, please rebase your series to the series, include only your 
patches in your series, and add the following to the cover letter:

Based-on: <20230802071754.14876-1-akihiko.odaki@daynix.com>
("[PATCH v2 0/6] linux-user: brk/mmap fixes")

> Btw, I tested a whole bunch of platforms too, see below...
> 
> Helge
> 
>> $ gdb -quiet --args ./build/qemu-arm -d guest_errors,page,strace ~/hello
>> Reading symbols from ./build/qemu-arm...
>> (gdb) r
>> Starting program: build/qemu-arm -d guest_errors,page,strace
>> /home/joel/hello
>> Using host libthread_db library 
>> "/lib/powerpc64le-linux-gnu/libthread_db.so.1".
>> [New Thread 0x7ffff762ece0 (LWP 118359)]
>> host mmap_min_addr=0x10000
>> pgb_find_hole: base @ 140420000 for 4294967296 bytes
>> pgb_static: base @ 140420000 for 4294967295 bytes
>> pgb_reserved_va: base @ 0x140420000 for 4294967296 bytes
>> Locating guest address space @ 0x140420000
>> page layout changed following mmap
>> start    end      size     prot
>> 00010000-00090000 00080000 ---
>> 00090000-0009b000 0000b000 ---
>> ffff0000-00000000 00010000 r-x
>> page layout changed following mmap
>> start    end      size     prot
>> 00010000-00090000 00080000 r-x
>> 00090000-0009b000 0000b000 ---
>> ffff0000-00000000 00010000 r-x
>> page layout changed following mmap
>> start    end      size     prot
>> 00010000-00090000 00080000 r-x
>> 00090000-000a0000 00010000 rw-
>> ffff0000-00000000 00010000 r-x
>> page layout changed following mmap
>> start    end      size     prot
>> 00010000-00090000 00080000 r-x
>> 00090000-000a0000 00010000 rw-
>> e0000000-e0810000 00810000 rw-
>> ffff0000-00000000 00010000 r-x
>> page layout changed following mmap
>> start    end      size     prot
>> 00010000-00090000 00080000 r-x
>> 00090000-000a0000 00010000 rw-
>> e0000000-e0010000 00010000 ---
>> e0010000-e0811000 00801000 rw-
>> ffff0000-00000000 00010000 r-x
>> guest_base  0x140420000
>> page layout changed following binary load
>> start    end      size     prot
>> 00010000-00090000 00080000 r-x
>> 00090000-000a0000 00010000 rw-
>> e0000000-e0010000 00010000 ---
>> e0010000-e0810000 00800000 rw-
>> e0810000-e0811000 00001000 r-x
>> ffff0000-00000000 00010000 r-x
>> start_brk   0x00000000
>> end_code    0x00084f7c
>> start_code  0x00010000
>> start_data  0x00095098
>> end_data    0x00098394
>> start_stack 0xe080f410
>> brk         0x0009b000
>> entry       0x00010418
>> argv_start  0xe080f414
>> env_start   0xe080f41c
>> auxv_start  0xe080f4a0
>> 118357 brk(NULL) = 0x0009b000
>> 118357 brk(0x0009b8fc) = 0x0009b000
>>
>> Thread 1 "qemu-arm" received signal SIGSEGV, Segmentation fault.
>> 0x00007fffeed9bb74 in code_gen_buffer ()
>> (gdb)
>> (gdb) bt
>> #0  0x00007fffeed9bb74 in code_gen_buffer ()
>> #1  0x0000000100169e3c in cpu_tb_exec (cpu=cpu@entry=0x1003d4aa0,
>>      itb=itb@entry=0x7fffeed9ba60 <code_gen_buffer+47512>,
>> tb_exit=tb_exit@entry=0x7fffffffe50c)
>>      at ../accel/tcg/cpu-exec.c:457
>> #2  0x000000010016a564 in cpu_loop_exec_tb (tb_exit=0x7fffffffe50c,
>> last_tb=<synthetic pointer>,
>>      pc=<optimised out>, tb=0x7fffeed9ba60 <code_gen_buffer+47512>,
>> cpu=<optimised out>)
>>      at ../accel/tcg/cpu-exec.c:919
>> #3  cpu_exec_loop (cpu=cpu@entry=0x1003d4aa0, sc=<optimised out>) at
>> ../accel/tcg/cpu-exec.c:1040
>> #4  0x000000010016aa0c in cpu_exec_setjmp (cpu=cpu@entry=0x1003d4aa0,
>> sc=<optimised out>)
>>      at ../accel/tcg/cpu-exec.c:1057
>> #5  0x000000010016b0d0 in cpu_exec (cpu=0x1003d4aa0) at
>> ../accel/tcg/cpu-exec.c:1083
>> #6  0x000000010004d780 in cpu_loop (env=0x1003d4fb0) at
>> ../linux-user/arm/cpu_loop.c:323
>> #7  0x0000000100047534 in main (argc=<optimised out>,
>> argv=0x7ffffffff178, envp=<optimised out>)
>>      at ../linux-user/main.c:975
>>
>> I tested 74a22a175c4340a01f6f860f72307093e3307681.
> 
> Those I did tested sucessfully (static binary):
> 
> alpha-chroot:
> Linux p100 6.4.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Jul 24 
> 20:51:12 UTC 2023 alpha GNU/Linux
> /hello: ELF 64-bit LSB executable, Alpha (unofficial), version 1 (SYSV), 
> statically linked, 
> BuildID[sha1]=5bf21139aa3937121e8843b062619de8e53d035a, for GNU/Linux 
> 3.2.0, not stripped
> Hello, World!
> 
> arm64-chroot:
> Linux p100 6.4.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Jul 24 
> 20:51:12 UTC 2023 aarch64 GNU/Linux
> /hello: ELF 64-bit LSB executable, ARM aarch64, version 1 (GNU/Linux), 
> statically linked, 
> BuildID[sha1]=201827af1ffdef4fc2afa404047c6d1a41e4825e, for GNU/Linux 
> 3.7.0, not stripped
> Hello, World!
> 
> armel-chroot:
> Linux p100 6.4.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Jul 24 
> 20:51:12 UTC 2023 armv7l GNU/Linux
> /hello: ELF 32-bit LSB executable, ARM, EABI5 version 1 (SYSV), 
> statically linked, 
> BuildID[sha1]=6e6a52f60037690052b2e54e750a56543ed9d7a0, for GNU/Linux 
> 3.2.0, not stripped
> Hello, World!
> 
> armhf-chroot:
> Linux p100 6.4.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Jul 24 
> 20:51:12 UTC 2023 armv7l GNU/Linux
> /hello: ELF 32-bit LSB executable, ARM, EABI5 version 1 (GNU/Linux), 
> statically linked, 
> BuildID[sha1]=842df9fd0bf910f6a00c19d61435387efa591390, for GNU/Linux 
> 3.2.0, not stripped
> Hello, World!
> 
> hppa-chroot:
> Linux p100 6.4.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Jul 24 
> 20:51:12 UTC 2023 parisc GNU/Linux
> /hello: ELF 32-bit MSB executable, PA-RISC, 1.1 version 1 (GNU/Linux), 
> statically linked, 
> BuildID[sha1]=03d4b299b31d30b5920e9fdcfccce071b77e4447, for GNU/Linux 
> 3.2.0, not stripped
> Hello, World!
> 
> m68k-chroot:
> Linux p100 6.4.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Jul 24 
> 20:51:12 UTC 2023 m68k GNU/Linux
> /hello: ELF 32-bit MSB executable, Motorola m68k, 68020, version 1 
> (SYSV), statically linked, 
> BuildID[sha1]=c01101b8ae6a6a0161a08b6ac24821b28daa5b73, for GNU/Linux 
> 3.2.0, not stripped
> Hello, World!
> 
> mips64el-chroot:
> Linux p100 6.4.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Jul 24 
> 20:51:12 UTC 2023 mips64 GNU/Linux
> /hello: ELF 64-bit LSB executable, MIPS, MIPS64 rel2 version 1 (SYSV), 
> statically linked, 
> BuildID[sha1]=0c50fc29be7ef781cdfb4ec4c47b4e350cab218b, for GNU/Linux 
> 3.2.0, not stripped
> Hello, World!
> 
> mipsel-chroot:
> Linux p100 6.4.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Jul 24 
> 20:51:12 UTC 2023 mips GNU/Linux
> /hello: ELF 32-bit LSB executable, MIPS, MIPS32 rel2 version 1 (SYSV), 
> statically linked, 
> BuildID[sha1]=e0db11bbc59070f5fefb4355d73df76791e96c29, for GNU/Linux 
> 3.2.0, not stripped
> Hello, World!
> 
> powerpc-chroot:
> Linux p100 6.4.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Jul 24 
> 20:51:12 UTC 2023 ppc GNU/Linux
> /hello: ELF 32-bit MSB executable, PowerPC or cisco 4500, version 1 
> (SYSV), statically linked, 
> BuildID[sha1]=4fe85ef8ebd86eb383ccf4fd741ce224143da2b2, for GNU/Linux 
> 3.2.0, not stripped
> Hello, World!
> 
> ppc64-chroot:
> Linux p100 6.4.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Jul 24 
> 20:51:12 UTC 2023 ppc64 GNU/Linux
> /hello: ELF 64-bit MSB executable, 64-bit PowerPC or cisco 7500, Power 
> ELF V1 ABI, version 1 (GNU/Linux), statically linked, 
> BuildID[sha1]=c3bb5c4d94b2096f70261bf0ab1f3fc93813df8f, for GNU/Linux 
> 3.2.0, not stripped
> Hello, World!
> 
> ppc64el-chroot:
> Linux p100 6.4.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Jul 24 
> 20:51:12 UTC 2023 ppc64le GNU/Linux
> /hello: ELF 64-bit LSB executable, 64-bit PowerPC or cisco 7500, 
> OpenPOWER ELF V2 ABI, version 1 (GNU/Linux), statically linked, 
> BuildID[sha1]=645abb5dcd9075d826d539675258fa5f9c7bc777, for GNU/Linux 
> 3.10.0, not stripped
> Hello, World!
> 
> s390x-chroot:
> Linux p100 6.4.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Jul 24 
> 20:51:12 UTC 2023 s390x GNU/Linux
> /hello: ELF 64-bit MSB executable, IBM S/390, version 1 (GNU/Linux), 
> statically linked, 
> BuildID[sha1]=f512d5ac759962ab66ae947d1308c8ceedef8fd3, for GNU/Linux 
> 3.2.0, not stripped
> Hello, World!
> 
> sh4-chroot:
> Linux p100 6.4.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Jul 24 
> 20:51:12 UTC 2023 sh4 GNU/Linux
> /hello: ELF 32-bit LSB executable, Renesas SH, version 1 (SYSV), 
> statically linked, 
> BuildID[sha1]=4cf38c7f67b5d7dc7a93c6ab513aaf0d2d21c4fc, for GNU/Linux 
> 3.2.0, not stripped
> Hello, World!
> 
> sparc64-chroot:
> Linux p100 6.4.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Jul 24 
> 20:51:12 UTC 2023 sparc64 GNU/Linux
> /hello: ELF 64-bit MSB executable, SPARC V9, Sun UltraSPARC1 Extensions 
> Required, relaxed memory ordering, version 1 (GNU/Linux), statically 
> linked, BuildID[sha1]=36f02b1b3acc94f61dff6dc26205f82314c899e0, for 
> GNU/Linux 3.2.0, not stripped
> Hello, World!


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 0/5] linux-user: brk/mmap fixes
  2023-08-02  7:26                 ` Akihiko Odaki
@ 2023-08-02 10:37                   ` Helge Deller
  0 siblings, 0 replies; 25+ messages in thread
From: Helge Deller @ 2023-08-02 10:37 UTC (permalink / raw)
  To: Akihiko Odaki, Joel Stanley; +Cc: Michael Tokarev, qemu-devel, Laurent Vivier

On 8/2/23 09:26, Akihiko Odaki wrote:
> On 2023/08/01 19:43, Helge Deller wrote:
>> On 8/1/23 06:49, Joel Stanley wrote:
>>> On Mon, 31 Jul 2023 at 18:24, Helge Deller <deller@gmx.de> wrote:
>>>> As suggested, I've based my patches on top of yours and the tree can be
>>>> pulled from:
>>>> git pull https://github.com/hdeller/qemu-hppa/   brk-fixes-akihiko-2
>>>>
>>>> My patches are neccessary to fix an arm-static testcase:
>>>>          /usr/bin/qemu-arm-static ./fstype
>>>>
>>>> Let's try this patch series...
>>>
>>> The armhf static binary works with expected output.
>>
>> Good!
>>
>>> The arm static binary causes qemu to segfault:
>>
>> I can't reproduce here.
>> I tried it in an arm64 chroot which provided the cross-compiler and worked for me:
>>
>> (arm64-chroot)root@p100:/# uname -a
>> Linux p100 6.4.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Jul 24 20:51:12 UTC 2023 aarch64 GNU/Linux
>> (arm64-chroot)root@p100:/# arm-linux-gnueabi-gcc-13 -o hello hello.c -static
>> (arm64-chroot)root@p100:/# file hello
>> hello: ELF 32-bit LSB executable, ARM, EABI5 version 1 (SYSV), statically linked, BuildID[sha1]=fa0f7cd6e1779fa8cd76c6e5d3123900ceefa952, for GNU/Linux 3.2.0, not stripped
>> (arm64-chroot)root@p100:/# ./hello
>> Hello, World!
>>
>> Maybe you can send me your binary (and the needed klibc*so)?
>
> Binaries will certainly help. I also suggest adding -trace target_mmap so that we can see what's passed to target_mmap().
>
> I also sent a new version so please rebase to it and try again.
> https://patchew.org/QEMU/20230802071754.14876-1-akihiko.odaki@daynix.com/
>
> Helge, please rebase your series to the series, include only your patches in your series, and add the following to the cover letter:

Will do. I sent them out bundled for convinience.

Helge

> Based-on: <20230802071754.14876-1-akihiko.odaki@daynix.com>
> ("[PATCH v2 0/6] linux-user: brk/mmap fixes")



^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2023-08-02 10:37 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-31  8:03 [PATCH 0/5] linux-user: brk/mmap fixes Akihiko Odaki
2023-07-31  8:03 ` [PATCH 1/5] linux-user: Unset MAP_FIXED_NOREPLACE for host Akihiko Odaki
2023-07-31 15:43   ` Richard Henderson
2023-07-31  8:03 ` [PATCH 2/5] linux-user: Do not call get_errno() in do_brk() Akihiko Odaki
2023-07-31 15:13   ` Helge Deller
2023-07-31  8:03 ` [PATCH 3/5] linux-user: Use MAP_FIXED_NOREPLACE for do_brk() Akihiko Odaki
2023-07-31 11:44   ` Peter Maydell
2023-07-31 13:32     ` Akihiko Odaki
2023-07-31 13:45       ` Peter Maydell
2023-07-31 15:44       ` Richard Henderson
2023-07-31  8:03 ` [PATCH 4/5] linux-user: Do nothing if too small brk is specified Akihiko Odaki
2023-07-31 15:16   ` Helge Deller
2023-07-31  8:03 ` [PATCH 5/5] linux-user: Do not align brk with host page size Akihiko Odaki
2023-07-31 15:51   ` Helge Deller
2023-07-31 22:43     ` Akihiko Odaki
2023-07-31  9:31 ` [PATCH 0/5] linux-user: brk/mmap fixes Michael Tokarev
2023-07-31 10:17   ` Joel Stanley
2023-07-31 10:23     ` Akihiko Odaki
2023-07-31 16:47       ` Helge Deller
2023-07-31 17:43         ` Helge Deller
2023-07-31 18:24           ` Helge Deller
2023-08-01  4:49             ` Joel Stanley
2023-08-01 10:43               ` Helge Deller
2023-08-02  7:26                 ` Akihiko Odaki
2023-08-02 10:37                   ` Helge Deller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).