diff mbox series

bsd-user: Properly allocate guest virtual address space

Message ID 20230727161148.444988-1-richard.henderson@linaro.org
State New
Headers show
Series bsd-user: Properly allocate guest virtual address space | expand

Commit Message

Richard Henderson July 27, 2023, 4:11 p.m. UTC
Do not hard-code guest_base at 32GB.
Do not override mmap_next_start for reserved_va.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---

Hi Warner,

With the blitz-trial branch you provided, the host libc allocates
thread-local storage within the [32GB, 36GB) region that you currently
assume is free.

The armv7-hello program happens to map on top of this thread-local
storage, and then we crash later accessing some host TLS variable.

While the linux-user probe_guest_base is significantly more complex,
we are also trying to handle 32-bit hosts.  I think freebsd is always
assuming 64-bit hosts, which makes this simpler.


r~
---
 bsd-user/main.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

Comments

Warner Losh July 28, 2023, 12:09 a.m. UTC | #1
On Thu, Jul 27, 2023 at 10:11 AM Richard Henderson <
richard.henderson@linaro.org> wrote:

> Do not hard-code guest_base at 32GB.
> Do not override mmap_next_start for reserved_va.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>
> Hi Warner,
>
> With the blitz-trial branch you provided, the host libc allocates
> thread-local storage within the [32GB, 36GB) region that you currently
> assume is free.
>
> The armv7-hello program happens to map on top of this thread-local
> storage, and then we crash later accessing some host TLS variable.
>
> While the linux-user probe_guest_base is significantly more complex,
> we are also trying to handle 32-bit hosts.  I think freebsd is always
> assuming 64-bit hosts, which makes this simpler.
>

Double mapping makes sense for problems to arise. I'd not have thought
this would be the problem, but it does eliminate a bunch of code that I'd
thought suspect (made worse by my trying to 'fix' old kludges with new
kludges of my own).

Yes. FreeBSD's bsd-user binary will only run on 64-bit hosts. The project
has started phasing out support for 32-bit hosts, and the role of bsd-user
(package builder tool) is such that 32-bit hosts don't make sense.

I've tested this out, and it works for me. Any chance we can get this into
8.1 as a bug fix for the last minute breakage of bsd-user (without this and
another patch, the static hello world that used to work broke). I can send
that second patch out for review. I can queue this fix in the mean time for
whenever the tree opens up.

Reviewed by: Warner Losh <imp@bsdimp.com>


>
> r~
> ---
>  bsd-user/main.c | 37 +++++++++++++++++++------------------
>  1 file changed, 19 insertions(+), 18 deletions(-)
>
> diff --git a/bsd-user/main.c b/bsd-user/main.c
> index f500ec292b..9760aad9f6 100644
> --- a/bsd-user/main.c
> +++ b/bsd-user/main.c
> @@ -50,20 +50,8 @@
>
>  int do_strace;
>
> -/*
> - * Going hand in hand with the va space needed (see below), we need
> - * to find a host address to map the guest to. Assume that qemu
> - * itself doesn't need memory above 32GB (or that we don't collide
> - * with anything interesting). This is selected rather arbitrarily,
> - * but seems to produce good results in tests to date.
> - */
> -# if HOST_LONG_BITS >= 64
> -uintptr_t guest_base = 0x800000000ul;    /* at 32GB */
> -bool have_guest_base = true;
> -#else
> -uintptr_t guest_base;    /* TODO: use sysctl to find big enough hole */
> +uintptr_t guest_base;
>  bool have_guest_base;
> -#endif
>  static bool opt_one_insn_per_tb;
>  static const char *cpu_model;
>  static const char *cpu_type;
> @@ -522,10 +510,6 @@ int main(int argc, char **argv)
>      target_environ = envlist_to_environ(envlist, NULL);
>      envlist_free(envlist);
>
> -    if (reserved_va) {
> -        mmap_next_start = reserved_va + 1;
> -    }
> -
>      {
>          Error *err = NULL;
>          if (seed_optarg != NULL) {
> @@ -543,7 +527,24 @@ int main(int argc, char **argv)
>       * Now that page sizes are configured we can do
>       * proper page alignment for guest_base.
>       */
> -    guest_base = HOST_PAGE_ALIGN(guest_base);
> +    if (have_guest_base) {
> +       if (guest_base & ~qemu_host_page_mask) {
> +            error_report("Selected guest base not host page aligned");
> +            exit(1);
> +        }
> +    } else if (reserved_va) {
> +        void *p = mmap(NULL, reserved_va + 1, PROT_NONE, MAP_GUARD, -1,
> 0);
> +        if (p == MAP_FAILED) {
> +            const char *err = strerror(errno);
> +            char *sz = size_to_str(reserved_va + 1);
> +
> +            error_report("Cannot allocate %s bytes for guest address
> space: %s",
> +                         sz, err);
> +            exit(1);
> +        }
> +        guest_base = (uintptr_t)p;
> +        have_guest_base = true;
> +    }
>
>      if (loader_exec(filename, argv + optind, target_environ, regs, info,
>                      &bprm) != 0) {
> --
> 2.41.0
>
>
Richard Henderson July 28, 2023, 12:31 a.m. UTC | #2
On 7/27/23 17:09, Warner Losh wrote:
> Yes. FreeBSD's bsd-user binary will only run on 64-bit hosts. The project
> has started phasing out support for 32-bit hosts, and the role of bsd-user
> (package builder tool) is such that 32-bit hosts don't make sense.

Ok, fine.

> I've tested this out, and it works for me. Any chance we can get this into
> 8.1 as a bug fix for the last minute breakage of bsd-user (without this and
> another patch, the static hello world that used to work broke). I can send
> that second patch out for review. I can queue this fix in the mean time for
> whenever the tree opens up.

Yes. I can queue this to tcg-next, which will have another round before rc2.
Send the alignment patch tomorrow and I can include that too.


r~
Warner Losh July 28, 2023, 4:06 a.m. UTC | #3
On Thu, Jul 27, 2023 at 6:31 PM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 7/27/23 17:09, Warner Losh wrote:
> > Yes. FreeBSD's bsd-user binary will only run on 64-bit hosts. The project
> > has started phasing out support for 32-bit hosts, and the role of
> bsd-user
> > (package builder tool) is such that 32-bit hosts don't make sense.
>
> Ok, fine.
>
> > I've tested this out, and it works for me. Any chance we can get this
> into
> > 8.1 as a bug fix for the last minute breakage of bsd-user (without this
> and
> > another patch, the static hello world that used to work broke). I can
> send
> > that second patch out for review. I can queue this fix in the mean time
> for
> > whenever the tree opens up.
>
> Yes. I can queue this to tcg-next, which will have another round before
> rc2.
> Send the alignment patch tomorrow and I can include that too.
>

Great!  I've just sent the patch. Thanks! With it, I can now get 'hello
world'
working again...  This should also give my GSoC student a good place to
merge
against to finish off his mmap and other patches.

Warner
diff mbox series

Patch

diff --git a/bsd-user/main.c b/bsd-user/main.c
index f500ec292b..9760aad9f6 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -50,20 +50,8 @@ 
 
 int do_strace;
 
-/*
- * Going hand in hand with the va space needed (see below), we need
- * to find a host address to map the guest to. Assume that qemu
- * itself doesn't need memory above 32GB (or that we don't collide
- * with anything interesting). This is selected rather arbitrarily,
- * but seems to produce good results in tests to date.
- */
-# if HOST_LONG_BITS >= 64
-uintptr_t guest_base = 0x800000000ul;    /* at 32GB */
-bool have_guest_base = true;
-#else
-uintptr_t guest_base;    /* TODO: use sysctl to find big enough hole */
+uintptr_t guest_base;
 bool have_guest_base;
-#endif
 static bool opt_one_insn_per_tb;
 static const char *cpu_model;
 static const char *cpu_type;
@@ -522,10 +510,6 @@  int main(int argc, char **argv)
     target_environ = envlist_to_environ(envlist, NULL);
     envlist_free(envlist);
 
-    if (reserved_va) {
-        mmap_next_start = reserved_va + 1;
-    }
-
     {
         Error *err = NULL;
         if (seed_optarg != NULL) {
@@ -543,7 +527,24 @@  int main(int argc, char **argv)
      * Now that page sizes are configured we can do
      * proper page alignment for guest_base.
      */
-    guest_base = HOST_PAGE_ALIGN(guest_base);
+    if (have_guest_base) {
+       if (guest_base & ~qemu_host_page_mask) {
+            error_report("Selected guest base not host page aligned");
+            exit(1);
+        }
+    } else if (reserved_va) {
+        void *p = mmap(NULL, reserved_va + 1, PROT_NONE, MAP_GUARD, -1, 0);
+        if (p == MAP_FAILED) {
+            const char *err = strerror(errno);
+            char *sz = size_to_str(reserved_va + 1);
+
+            error_report("Cannot allocate %s bytes for guest address space: %s",
+                         sz, err);
+            exit(1);
+        }
+        guest_base = (uintptr_t)p;
+        have_guest_base = true;
+    }
 
     if (loader_exec(filename, argv + optind, target_environ, regs, info,
                     &bprm) != 0) {