* [PATCH] tcg: Fix execution on Apple Silicon @ 2021-01-02 12:21 Roman Bolshakov 2021-01-02 12:39 ` no-reply 2021-01-02 16:13 ` Roman Bolshakov 0 siblings, 2 replies; 8+ messages in thread From: Roman Bolshakov @ 2021-01-02 12:21 UTC (permalink / raw) To: qemu-devel Cc: Richard Henderson, Roman Bolshakov, Alexander Graf, Joelle van Dyne, Stefan Hajnoczi, Paolo Bonzini 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] tcg: Fix execution on Apple Silicon 2021-01-02 12:21 [PATCH] tcg: Fix execution on Apple Silicon Roman Bolshakov @ 2021-01-02 12:39 ` no-reply 2021-01-02 16:13 ` Roman Bolshakov 1 sibling, 0 replies; 8+ messages in thread From: no-reply @ 2021-01-02 12:39 UTC (permalink / raw) To: r.bolshakov Cc: richard.henderson, qemu-devel, r.bolshakov, agraf, j, stefanha, pbonzini Patchew URL: https://patchew.org/QEMU/20210102122101.39617-1-r.bolshakov@yadro.com/ Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 20210102122101.39617-1-r.bolshakov@yadro.com Subject: [PATCH] tcg: Fix execution on Apple Silicon === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu - [tag update] patchew/20201215064200.28751-1-jiaxun.yang@flygoat.com -> patchew/20201215064200.28751-1-jiaxun.yang@flygoat.com - [tag update] patchew/20201225201956.692861-1-richard.henderson@linaro.org -> patchew/20201225201956.692861-1-richard.henderson@linaro.org * [new tag] patchew/20210102122101.39617-1-r.bolshakov@yadro.com -> patchew/20210102122101.39617-1-r.bolshakov@yadro.com Switched to a new branch 'test' 3286c7e tcg: Fix execution on Apple Silicon === OUTPUT BEGIN === WARNING: architecture specific defines should be avoided #80: FILE: accel/tcg/translate-all.c:1075: +#if defined(__APPLE__) && defined(__aarch64__) WARNING: architecture specific defines should be avoided #101: FILE: accel/tcg/translate-all.c:2731: +#if defined(__APPLE__) && defined(__aarch64__) ERROR: space required before the open brace '{' #104: FILE: accel/tcg/translate-all.c:2734: + if (pthread_jit_write_protect_supported_np()){ total: 1 errors, 2 warnings, 104 lines checked Commit 3286c7e4aef9 (tcg: Fix execution on Apple Silicon) has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/20210102122101.39617-1-r.bolshakov@yadro.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tcg: Fix execution on Apple Silicon 2021-01-02 12:21 [PATCH] tcg: Fix execution on Apple Silicon Roman Bolshakov 2021-01-02 12:39 ` no-reply @ 2021-01-02 16:13 ` Roman Bolshakov 2021-01-02 19:55 ` Joelle van Dyne 1 sibling, 1 reply; 8+ messages in thread From: Roman Bolshakov @ 2021-01-02 16:13 UTC (permalink / raw) To: qemu-devel Cc: Paolo Bonzini, Alexander Graf, Richard Henderson, Joelle van Dyne, Stefan Hajnoczi 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tcg: Fix execution on Apple Silicon 2021-01-02 16:13 ` Roman Bolshakov @ 2021-01-02 19:55 ` Joelle van Dyne 2021-01-03 14:20 ` Roman Bolshakov 0 siblings, 1 reply; 8+ messages in thread From: Joelle van Dyne @ 2021-01-02 19:55 UTC (permalink / raw) To: Roman Bolshakov; +Cc: QEMU Developers 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? -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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tcg: Fix execution on Apple Silicon 2021-01-02 19:55 ` Joelle van Dyne @ 2021-01-03 14:20 ` Roman Bolshakov 2021-01-03 16:46 ` Joelle van Dyne 0 siblings, 1 reply; 8+ messages in thread From: Roman Bolshakov @ 2021-01-03 14:20 UTC (permalink / raw) To: Joelle van Dyne; +Cc: QEMU Developers 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tcg: Fix execution on Apple Silicon 2021-01-03 14:20 ` Roman Bolshakov @ 2021-01-03 16:46 ` Joelle van Dyne 2021-01-03 22:54 ` Roman Bolshakov 0 siblings, 1 reply; 8+ messages in thread From: Joelle van Dyne @ 2021-01-03 16:46 UTC (permalink / raw) To: Roman Bolshakov; +Cc: QEMU Developers 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. -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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tcg: Fix execution on Apple Silicon 2021-01-03 16:46 ` Joelle van Dyne @ 2021-01-03 22:54 ` Roman Bolshakov 2021-01-04 1:47 ` Joelle van Dyne 0 siblings, 1 reply; 8+ messages in thread From: Roman Bolshakov @ 2021-01-03 22:54 UTC (permalink / raw) To: Joelle van Dyne; +Cc: QEMU Developers 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tcg: Fix execution on Apple Silicon 2021-01-03 22:54 ` Roman Bolshakov @ 2021-01-04 1:47 ` Joelle van Dyne 0 siblings, 0 replies; 8+ messages in thread From: Joelle van Dyne @ 2021-01-04 1:47 UTC (permalink / raw) To: Roman Bolshakov; +Cc: QEMU Developers [-- 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 --] ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-01-04 1:48 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-02 12:21 [PATCH] tcg: Fix execution on Apple Silicon Roman Bolshakov 2021-01-02 12:39 ` no-reply 2021-01-02 16:13 ` Roman Bolshakov 2021-01-02 19:55 ` Joelle van Dyne 2021-01-03 14:20 ` Roman Bolshakov 2021-01-03 16:46 ` Joelle van Dyne 2021-01-03 22:54 ` Roman Bolshakov 2021-01-04 1:47 ` Joelle van Dyne
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.