| Message ID | 20230727161148.444988-1-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | bsd-user: Properly allocate guest virtual address space | expand |
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 > >
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~
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 --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) {
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(-)