Re: [PATCH] tcg: Fix execution on Apple Silicon

[Joelle van Dyne <j@getutm.app>]

[-- Attachment #1: Type: text/plain, Size: 12548 bytes --]

The MTTCG issue is known and I’ve only used multiple core emulation with
single core.

I haven’t tried your patch yet but I plan to when I get time next week. I’m
surprised because it’s (your v1 patchset) exactly what I had originally
before hitting crashes and having to add the calls to more places. But
perhaps something changed between QEMU 4.2.0 and 5.2.0 that made things
work more “as expected”. If so then I plan to undo the changes I’ve added.

-j

On Sunday, January 3, 2021, Roman Bolshakov <r.bolshakov@yadro.com> wrote:

> On Sun, Jan 03, 2021 at 08:46:27AM -0800, Joelle van Dyne wrote:
> > Can you test with a low memory (-m 512) and also with single threaded
> > SMP (-smp 4)? Wondering if you're hitting all the edge cases because
> > there's oddities with cache flushing (can be done both in code gen and
> > code exec) and interrupt handling that caused issues for me.
> >
>
> I tested XP with default memory (128m) and -m 512. I did run Ubuntu with
> with
> -smp 1/2/4 and multiple variants of memory (2048,4096). I've just
> installed Windows 10 and it somehow works noticably faster than Ubuntu
> 20.04 (what makes me wonder why Ubuntu 20.04 peforms worse).
>
> But you know, I've noticed that MTTCG is disabled by default on arm
> hosts, so -smp 4 has no effect (it should print a warning IMO that smp
> is noop, or even quit from qemu to disallow single-threaded TCG and -smp
> flag with a value beyond 1).
>
> If I try to enable MTTCG, I get a warning from QEMU and only one CPU
> inside VM (according to Windows Task Manager).
>
> $ build/qemu-system-x86_64 -cpu Nehalem -accel tcg,thread=multi -smp 4 -m
> 4096 -hda ~/vms/win10.qcow2
>
> qemu-system-x86_64: -accel tcg,thread=multi: warning: Guest expects a
> stronger memory ordering than the host provides
> This may cause strange/hard to debug errors
>
> As far as I understand from the ticket below this is intentional:
> https://bugs.launchpad.net/qemu/+bug/1824768
>
> > There aren't many people overall who want to try to run emulation on
> > anything other than x86 host.
>
> Perhaps we could enable MTTCG by enabling TSO in M1 like it's done in
> Rosetta to avoid performance overheads of implicit barriers?
>
> BTW, I wonder if you tried my patch? Do you hit the mentioned issues?
>
> With regards to do_tb_phys_invalidate(), the function doesn't care
> about whether it was write or exec locked. It needs write permissions
> at least for TB spin lock. And something after return from
> tb_phys_invalidate() needs exec permssions. I can try to find "that
> something" and move change of permissions to rx closer to the place that
> needs exec permissions. And then, move change of permissions to rw
> inside do_tb_phys_invalidate() just before TB spin lock is acquired.
>
> Regards,
> Roman
>
> > -j
> >
> > On Sun, Jan 3, 2021 at 6:20 AM Roman Bolshakov <r.bolshakov@yadro.com>
> wrote:
> > >
> > > On Sat, Jan 02, 2021 at 11:55:29AM -0800, Joelle van Dyne wrote:
> > > > I see you didn't touch cpu_loop_exit() and I'm curious how async
> > > > interrupts are handled. Have you tested this and it works i.e.
> booting
> > > > Windows 7 or Ubuntu 20.04? Also I've seen do_tb_phys_invalidate()
> > > > called both from code generation context (write unlocked) and
> > > > execution context (write locked), how does this patch differentiate
> > > > the two?
> > > >
> > >
> > > Hi Joelle,
> > >
> > > I used the following rule of thumb when finding places where exec/write
> > > protection has to be lifted. If it returns EXC_BAD_ACCESS under lldb
> and
> > > stack backtrace is meaningful, then a write-protected region is
> > > accessed. If the stack couldn't be unwinded and EXC_BAD_ACCESS is
> > > returned then the region has exec restrictions.
> > >
> > > With the patch I wasn't able to see any EXC_BAD_ACCESS.
> > >
> > > I've tested x86_64 Ubuntu 20.04 Desktop. It boots but it's very slow
> > > (but faster than TCG on x86). Windows XP is much faster and quite
> > > responsive. I also tested Windows 95. I'll test Win 7/Win 10 a bit
> > > later.
> > >
> > > I'm going to update v2 shortly and going to introduce assymetric
> changes of
> > > permissions akin to Apple's JavaScriptCore. In v2, I'm not changing
> > > permission back and force unless it's needed to avoid EXC_BAD_ACCESS.
> > >
> > > Regards,
> > > Roman
> > >
> > > > -j
> > > >
> > > > On Sat, Jan 2, 2021 at 8:13 AM Roman Bolshakov <
> r.bolshakov@yadro.com> wrote:
> > > > >
> > > > > On Sat, Jan 02, 2021 at 03:21:02PM +0300, Roman Bolshakov wrote:
> > > > > > Pages can't be both write and executable at the same time on
> Apple
> > > > > > Silicon. macOS provides public API to switch write protection
> [1] for
> > > > > > JIT applications, like TCG.
> > > > > >
> > > > > > 1. https://developer.apple.com/documentation/apple_silicon/
> porting_just-in-time_compilers_to_apple_silicon
> > > > > >
> > > > > > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> > > > > > ---
> > > > > >
> > > > > > Happy holidays, everyone.
> > > > > >
> > > > > > This is somewhat similar to https://patchwork.kernel.org/
> project/qemu-devel/patch/20201108232425.1705-7-j@getutm.app/
> > > > > > but I couldn't apply the series so I started from scratch.
> > > > > >
> > > > > > The primary difference from the patch above is that public API
> is used.
> > > > > > Other differences:
> > > > > >   * TB pages are mostly kept write-locked except around
> tcg_qemu_tb_exec()
> > > > > >   * x86_64 macOS doesn't use MAP_JIT and W^X switches
> > > > > >
> > > > > > Regards,
> > > > > > Roman
> > > > > >
> > > > > >  accel/tcg/cpu-exec.c      | 10 ++++++++++
> > > > > >  accel/tcg/translate-all.c | 26 ++++++++++++++++++++++++++
> > > > > >  include/exec/exec-all.h   |  2 ++
> > > > > >  tcg/tcg.c                 |  1 +
> > > > > >  4 files changed, 39 insertions(+)
> > > > > >
> > > > > > diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> > > > > > index 8689c54499..0042fc9f2b 100644
> > > > > > --- a/accel/tcg/cpu-exec.c
> > > > > > +++ b/accel/tcg/cpu-exec.c
> > > > > > @@ -175,7 +175,9 @@ static inline tcg_target_ulong
> cpu_tb_exec(CPUState *cpu, TranslationBlock *itb)
> > > > > >      }
> > > > > >  #endif /* DEBUG_DISAS */
> > > > > >
> > > > > > +    tb_write_lock();
> > > > > >      ret = tcg_qemu_tb_exec(env, tb_ptr);
> > > > > > +    tb_write_unlock();
> > > > > >      cpu->can_do_io = 1;
> > > > > >      last_tb = (TranslationBlock *)(ret & ~TB_EXIT_MASK);
> > > > > >      tb_exit = ret & TB_EXIT_MASK;
> > > > > > @@ -220,9 +222,11 @@ static void cpu_exec_nocache(CPUState *cpu,
> int max_cycles,
> > > > > >      cflags |= MIN(max_cycles, CF_COUNT_MASK);
> > > > > >
> > > > > >      mmap_lock();
> > > > > > +    tb_write_unlock();
> > > > > >      tb = tb_gen_code(cpu, orig_tb->pc, orig_tb->cs_base,
> > > > > >                       orig_tb->flags, cflags);
> > > > > >      tb->orig_tb = orig_tb;
> > > > > > +    tb_write_lock();
> > > > > >      mmap_unlock();
> > > > > >
> > > > > >      /* execute the generated code */
> > > > > > @@ -268,7 +272,9 @@ void cpu_exec_step_atomic(CPUState *cpu)
> > > > > >          tb = tb_lookup__cpu_state(cpu, &pc, &cs_base, &flags,
> cf_mask);
> > > > > >          if (tb == NULL) {
> > > > > >              mmap_lock();
> > > > > > +            tb_write_unlock();
> > > > > >              tb = tb_gen_code(cpu, pc, cs_base, flags, cflags);
> > > > > > +            tb_write_lock();
> > > > > >              mmap_unlock();
> > > > > >          }
> > > > > >
> > > > > > @@ -428,7 +434,9 @@ static inline TranslationBlock
> *tb_find(CPUState *cpu,
> > > > > >      tb = tb_lookup__cpu_state(cpu, &pc, &cs_base, &flags,
> cf_mask);
> > > > > >      if (tb == NULL) {
> > > > > >          mmap_lock();
> > > > > > +        tb_write_unlock();
> > > > > >          tb = tb_gen_code(cpu, pc, cs_base, flags, cf_mask);
> > > > > > +        tb_write_lock();
> > > > > >          mmap_unlock();
> > > > > >          /* We add the TB in the virtual pc hash table for the
> fast lookup */
> > > > > >          qatomic_set(&cpu->tb_jmp_cache[tb_jmp_cache_hash_func(pc)],
> tb);
> > > > > > @@ -444,7 +452,9 @@ static inline TranslationBlock
> *tb_find(CPUState *cpu,
> > > > > >  #endif
> > > > > >      /* See if we can patch the calling TB. */
> > > > > >      if (last_tb) {
> > > > > > +        tb_write_unlock();
> > > > > >          tb_add_jump(last_tb, tb_exit, tb);
> > > > > > +        tb_write_lock();
> > > > > >      }
> > > > > >      return tb;
> > > > > >  }
> > > > > > diff --git a/accel/tcg/translate-all.c
> b/accel/tcg/translate-all.c
> > > > > > index b7d50a73d4..1562076ffb 100644
> > > > > > --- a/accel/tcg/translate-all.c
> > > > > > +++ b/accel/tcg/translate-all.c
> > > > > > @@ -1072,6 +1072,9 @@ static inline void
> *alloc_code_gen_buffer(void)
> > > > > >      size_t size = tcg_ctx->code_gen_buffer_size;
> > > > > >      void *buf;
> > > > > >
> > > > > > +#if defined(__APPLE__) && defined(__aarch64__)
> > > > > > +    flags |= MAP_JIT;
> > > > > > +#endif
> > > > > >      buf = mmap(NULL, size, prot, flags, -1, 0);
> > > > > >      if (buf == MAP_FAILED) {
> > > > > >          return NULL;
> > > > > > @@ -1485,7 +1488,9 @@ static void do_tb_phys_invalidate(TranslationBlock
> *tb, bool rm_from_page_list)
> > > > > >
> > > > > >  static void tb_phys_invalidate__locked(TranslationBlock *tb)
> > > > > >  {
> > > > > > +    tb_write_unlock();
> > > > > >      do_tb_phys_invalidate(tb, true);
> > > > > > +    tb_write_lock();
> > > > > >  }
> > > > > >
> > > > > >  /* invalidate one TB
> > > > > > @@ -2722,3 +2727,24 @@ void tcg_flush_softmmu_tlb(CPUState *cs)
> > > > > >      tlb_flush(cs);
> > > > > >  #endif
> > > > > >  }
> > > > > > +
> > > > > > +#if defined(__APPLE__) && defined(__aarch64__)
> > > > > > +static void tb_write_protect(bool locked)
> > > > > > +{
> > > > > > +    if (pthread_jit_write_protect_supported_np()){
> > > > > > +        pthread_jit_write_protect_np(locked);
> > > > > > +    }
> > > > > > +}
> > > > > > +#else
> > > > > > +static void tb_write_protect(bool locked) {}
> > > > > > +#endif
> > > > > > +
> > > > > > +void tb_write_lock(void)
> > > > > > +{
> > > > > > +    tb_write_protect(true);
> > > > > > +}
> > > > > > +
> > > > > > +void tb_write_unlock(void)
> > > > > > +{
> > > > > > +    tb_write_protect(false);
> > > > > > +}
> > > > > > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> > > > > > index fab573da06..962dca0975 100644
> > > > > > --- a/include/exec/exec-all.h
> > > > > > +++ b/include/exec/exec-all.h
> > > > > > @@ -549,6 +549,8 @@ TranslationBlock *tb_htable_lookup(CPUState
> *cpu, target_ulong pc,
> > > > > >                                     target_ulong cs_base,
> uint32_t flags,
> > > > > >                                     uint32_t cf_mask);
> > > > > >  void tb_set_jmp_target(TranslationBlock *tb, int n, uintptr_t
> addr);
> > > > > > +void tb_write_lock(void);
> > > > > > +void tb_write_unlock(void);
> > > > > >
> > > > > >  /* GETPC is the true target of the return instruction that
> we'll execute.  */
> > > > > >  #if defined(CONFIG_TCG_INTERPRETER)
> > > > > > diff --git a/tcg/tcg.c b/tcg/tcg.c
> > > > > > index 43c6cf8f52..303bb436bd 100644
> > > > > > --- a/tcg/tcg.c
> > > > > > +++ b/tcg/tcg.c
> > > > > > @@ -1065,6 +1065,7 @@ void tcg_prologue_init(TCGContext *s)
> > > > > >      s->pool_labels = NULL;
> > > > > >  #endif
> > > > > >
> > > > > > +    tb_write_unlock();
> > > > > >      /* Generate the prologue.  */
> > > > > >      tcg_target_qemu_prologue(s);
> > > > > >
> > > > > > --
> > > > > > 2.29.2
> > > > > >
> > > > >
> > > > > I've also noticed that Apple doesn't worry about sticking to
> particular
> > > > > W^X mode:
> > > > >
> > > > > https://bugs.webkit.org/attachment.cgi?id=402515&
> action=prettypatch
> > > > >
> > > > > We might also drop lock/unlock symmetry from here. E.g. we can
> have two
> > > > > functions that switch the mode (they might be moved to
> util/osdep.c):
> > > > >
> > > > >   qemu_jit_write();
> > > > >   qemu_jit_execute();
> > > > >
> > > > > Then we use them just before writing or before executing like
> advised on
> > > > > their documentation page.
> > > > >
> > > > > -Roman
>

[-- Attachment #2: Type: text/html, Size: 17243 bytes --]