|
| From: | Lirong Yuan |
| Subject: | Re: [PATCH] linux-user: Add an argument QEMU_MMAP_BASE to set custom mmap base address in qemu user mode |
| Date: | Thu, 12 Mar 2020 15:35:46 -0700 |
Le 09/03/2020 à 19:07, Lirong Yuan a écrit :
>
> On Mon, Mar 2, 2020 at 11:51 AM Lirong Yuan <address@hidden
> <mailto:address@hidden>> wrote:
>
> On Mon, Mar 2, 2020 at 10:39 AM Laurent Vivier <address@hidden
> <mailto: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
> <mailto: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
> <mailto: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
> <mailto: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|
> <https://github.com/qemu/qemu/c%7Copen_self_maps%7C> 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.
>
>
> Friendly ping~
>
> Link to the page for the patch on patchwork:
> http://patchwork.ozlabs.org/patch/1242370/
I would prefer if you hardcode the value for aarch64 rather than adding
a new parameter.
Thanks,
Laurent
| [Prev in Thread] | Current Thread | [Next in Thread] |