Re: [PATCH] linux-user: Add an argument QEMU_MMAP_BASE to set custom mmap base address in qemu user mode

[Lirong Yuan]

On Mon, Mar 2, 2020 at 10:39 AM Laurent Vivier <address@hidden> wrote:
>
> Le 02/03/2020 à 18:53, Lirong Yuan a écrit :
> > On Mon, Mar 2, 2020 at 6:56 AM Laurent Vivier <address@hidden> wrote:
> >>
> >> Le 29/02/2020 à 01:43, Lirong Yuan a écrit :
> >>> On Fri, Feb 21, 2020 at 5:09 PM Lirong Yuan <address@hidden> wrote:
> >>>>
> >>>> This change allows us to set custom base address for guest programs. It 
> >>>> is needed to allow qemu to work with Thread Sanitizer (TSan), which has 
> >>>> specific boundary definitions for memory mappings on different platforms:
> >>>> https://github.com/llvm/llvm-project/blob/master/compiler-rt/lib/tsan/rtl/tsan_platform.h
> >>
> >> Could you give more details and some examples?
> >>
> >> Thanks,
> >> Laurent
> >>
> >>>> Signed-off-by: Lirong Yuan <address@hidden>
> >>>> ---
> >>>>  linux-user/main.c | 12 ++++++++++++
> >>>>  linux-user/mmap.c |  3 ++-
> >>>>  linux-user/qemu.h |  5 +++++
> >>>>  3 files changed, 19 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/linux-user/main.c b/linux-user/main.c
> >>>> index fba833aac9..c01af6bfee 100644
> >>>> --- a/linux-user/main.c
> >>>> +++ b/linux-user/main.c
> >>>> @@ -336,6 +336,16 @@ static void handle_arg_guest_base(const char *arg)
> >>>>      have_guest_base = 1;
> >>>>  }
> >>>>
> >>>> +static void handle_arg_mmap_base(const char *arg)
> >>>> +{
> >>>> +    int err = qemu_strtoul(arg, NULL, 0, &mmap_base);
> >>>> +    if (err) {
> >>>> +        fprintf(stderr, "Invalid mmap_base: %s, err: %d\n", arg, err);
> >>>> +        exit(EXIT_FAILURE);
> >>>> +    }
> >>>> +    mmap_next_start = mmap_base;
> >>>> +}
> >>>> +
> >>>>  static void handle_arg_reserved_va(const char *arg)
> >>>>  {
> >>>>      char *p;
> >>>> @@ -440,6 +450,8 @@ static const struct qemu_argument arg_table[] = {
> >>>>       "uname",      "set qemu uname release string to 'uname'"},
> >>>>      {"B",          "QEMU_GUEST_BASE",  true,  handle_arg_guest_base,
> >>>>       "address",    "set guest_base address to 'address'"},
> >>>> +    {"mmap_base",  "QEMU_MMAP_BASE",   true,  handle_arg_mmap_base,
> >>>> +     "",           "begin allocating guest pages at this host address"},
> >>>>      {"R",          "QEMU_RESERVED_VA", true,  handle_arg_reserved_va,
> >>>>       "size",       "reserve 'size' bytes for guest virtual address 
> >>>> space"},
> >>>>      {"d",          "QEMU_LOG",         true,  handle_arg_log,
> >>>> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
> >>>> index 8685f02e7e..3f35543acf 100644
> >>>> --- a/linux-user/mmap.c
> >>>> +++ b/linux-user/mmap.c
> >>>> @@ -189,6 +189,7 @@ static int mmap_frag(abi_ulong real_start,
> >>>>  # define TASK_UNMAPPED_BASE  0x40000000
> >>>>  #endif
> >>>>  abi_ulong mmap_next_start = TASK_UNMAPPED_BASE;
> >>>> +abi_ulong mmap_base = TASK_UNMAPPED_BASE;
> >>>>
> >>>>  unsigned long last_brk;
> >>>>
> >>>> @@ -299,7 +300,7 @@ abi_ulong mmap_find_vma(abi_ulong start, abi_ulong 
> >>>> size, abi_ulong align)
> >>>>
> >>>>              if ((addr & (align - 1)) == 0) {
> >>>>                  /* Success.  */
> >>>> -                if (start == mmap_next_start && addr >= 
> >>>> TASK_UNMAPPED_BASE) {
> >>>> +                if (start == mmap_next_start && addr >= mmap_base) {
> >>>>                      mmap_next_start = addr + size;
> >>>>                  }
> >>>>                  return addr;
> >>>> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> >>>> index 560a68090e..83c00cfea2 100644
> >>>> --- a/linux-user/qemu.h
> >>>> +++ b/linux-user/qemu.h
> >>>> @@ -161,6 +161,11 @@ void task_settid(TaskState *);
> >>>>  void stop_all_tasks(void);
> >>>>  extern const char *qemu_uname_release;
> >>>>  extern unsigned long mmap_min_addr;
> >>>> +/*
> >>>> + * mmap_base is minimum address to use when allocating guest pages. All 
> >>>> guest
> >>>> + * pages will be allocated at this (guest) address or higher addresses.
> >>>> + */
> >>>> +extern abi_ulong mmap_base;
> >>>>
> >>>>  /* ??? See if we can avoid exposing so much of the loader internals.  */
> >>>>
> >>>> --
> >>>> 2.25.0.265.gbab2e86ba0-goog
> >>>>
> >>>
> >>> Friendly ping~
> >>>
> >>> Link to the page for the patch on patchwork:
> >>> http://patchwork.ozlabs.org/patch/1242370/
> >>>
> >>
> >
> > Hi Laurent,
> >
> > Sure! We tried to run a program with TSAN enabled
> > (https://github.com/google/sanitizers/wiki/ThreadSanitizerCppManual)
> > in qemu, and got this error message:
> > "FATAL: ThreadSanitizer: unexpected memory mapping
> > 0x004000000000-0x004000253000"
> >
> > The root cause is that the default guest base address that qemu uses
> > is 0x4000000000 (1ul<<38), and does not align with TSAN's expectation:
> > https://github.com/qemu/qemu/blob/c81acb643a61db199b9198add7972d8a8496b27c/linux-user/mmap.c#L187
> > https://github.com/llvm/llvm-project/blob/e7de00cf974a4e30d4900518ae8473a117efbd6c/compiler-rt/lib/tsan/rtl/tsan_platform.h#L150
> >
> > By setting QEMU_GUEST_BASE, we can place the guest program at a
> > different base address in the host program. However, the h2g function
> > (in |open_self_maps| in syscall.c) translates the address back to be
> > based at 0x4000000000. E.g. the base address
> > 0x4000000000+QEMU_GUEST_BASE will be converted to 0x4000000000 with
> > function h2g:
> > https://github.com/qemu/qemu/blob/c81acb643a61db199b9198add7972d8a8496b27c/linux-user/syscall.c#L7076
> >
> > One solution then, is to update |open_self_maps| in syscall.c to not
> > use h2g. However this changes the meaning of QEMU_GUEST_BASE and could
> > break existing programs that set non-zero QEMU_GUEST_BASE.
> >
> > So, how did qemu pick the base address 0x4000000000 then? Looking at
> > the blame output in github, one recent change for the base address was
> > committed 10 years ago:
> > https://github.com/qemu/qemu/c|open_self_maps| in
> > syscall.commit/14f24e1465edc44b9b4d89fbbea66e06088154e1
> >
> > Another one was committed 12 years ago:
> > https://github.com/qemu/qemu/commit/a03e2d421e7f33316750d6b7396d1a7e14b18d53
> >
> > The description of the first change is "place the default mapping base
> > for 64-bit guests (on 64-bit hosts) outside the low 4G". It would seem
> > that minimum requirements for the base address are:
> > 1) addr >= 4G (for 64-bit)
> > 2) addr < lowest address used by the host qemu program by some margin
> >
> > Given that
> > 1) only TSAN explicitly check for the validity of addresses
> > 2) 0x4000000000 is not a valid address for programs on aarch64
> > (according to TSAN)
> > 3) different architectures have different valid addresses,
> > it would seem that adding an argument for changing the hard-coded base
> > address is a viable solution.
>
> Thank you for the detailed explanation.
>
> Could you show me an example of the QEMU command line you use?
>
> I'm wondering if hardcoding directly the good value would be a better
> solution?
>
> Richard, do you have some thoughts on this?
>
> Thanks,
> Laurent

Sure! First we compile a simple race program with TSAN enabled:
( Simple race program is here:
https://github.com/google/sanitizers/wiki/ThreadSanitizerCppManual#usage
)
$ clang++ simple_race.cc -fsanitize=thread -fPIE -pie -g -o simple_race

Next we run a script for executing the program, and it exports
environment variables:
QEMU_CPU=max
QEMU_MMAP_BASE=0x0000005500000000

And runs the QEMU program:
$ qemu-aarch64 simple_race

I changed the default value for all other programs that I am working
with, and so far we haven't seen any problems.
For the patch, it might be better to err on the safe side and not
change the hard-coded value, as it might cause potential breakages for
other users.
Though I don't know much about how the default value might be used or
depended on by other programs, so if you see no concerns for updating
the value, I'd be happy to change it too.