Re: [PATCH 2/5] accel/tcg: move jit thread manipulation into do_tb_phys_invalidate

["Alex Bennée" <alex.bennee@linaro.org>]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 5/5/26 05:36, Alex Bennée wrote:
>> To invalidate a TB on MacOS we need to enable write access to the JIT
>> buffer. We were doing this for tb_phys_invalidate__locked but that is
>> not the only path into do_tb_phys_invalidate. Move the manipulation
>> into the shared function that does the work.
>> This enables watchpoints to work in MacOS TCG guests.
>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3444
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>   accel/tcg/tb-maint.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>> diff --git a/accel/tcg/tb-maint.c b/accel/tcg/tb-maint.c
>> index cd7c32361bb..9a648f97865 100644
>> --- a/accel/tcg/tb-maint.c
>> +++ b/accel/tcg/tb-maint.c
>> @@ -925,6 +925,7 @@ static void do_tb_phys_invalidate(TranslationBlock *tb, bool rm_from_page_list)
>>       uint32_t orig_cflags = tb_cflags(tb);
>>         assert_memory_lock();
>> +    qemu_thread_jit_write();
>>         /* make sure no further incoming jumps will be chained to
>> this TB */
>>       qemu_spin_lock(&tb->jmp_lock);
>> @@ -954,15 +955,15 @@ static void do_tb_phys_invalidate(TranslationBlock *tb, bool rm_from_page_list)
>>       /* suppress any remaining jumps to this TB */
>>       tb_jmp_unlink(tb);
>>   +    qemu_thread_jit_execute();
>
> You've missed the early return path from the middle of the function, which should be fatal.
>
> But the place that needs this is tb_reset_jump, which is called from
> tb_remove_from_jmp_list and tb_jmp_unlink.  Which is entirely covered
> by moving this down toward the end of the function like so:
>
> ---
> +   qemu_thread_jit_write();

I think we also need to cover the spinlock:

    /* make sure no further incoming jumps will be chained to this TB */
    qemu_spin_lock(&tb->jmp_lock);
    qatomic_set(&tb->cflags, tb->cflags | CF_INVALID);
    qemu_spin_unlock(&tb->jmp_lock);

as that is what the original bug reported it was stuck spinning on.

>
>     /* suppress this TB from the two jump lists */
>     tb_remove_from_jmp_list(tb, 0);
>     tb_remove_from_jmp_list(tb, 1);
>
>     /* suppress any remaining jumps to this TB */
>     tb_jmp_unlink(tb);
>
> +   qemu_thread_jit_execute();
> ---
>
>
>> +
>>       qatomic_set(&tb_ctx.tb_phys_invalidate_count,
>>                   tb_ctx.tb_phys_invalidate_count + 1);
>>   }
>>     static void tb_phys_invalidate__locked(TranslationBlock *tb)
>>   {
>> -    qemu_thread_jit_write();
>>       do_tb_phys_invalidate(tb, true);
>> -    qemu_thread_jit_execute();
>>   }
>
> Might as well remove tb_phys_invalidate__locked entirely, and
> propagate the direct call to do_tb_phys_invalidate.
>
> The __locked suffix does appear to be for the user
> assert_memory_locked().  As evidenced by tb_phys_invalidate, for
> system mode, we only sometimes take the page lock.
>
> Given that this jit protection is via pthread_jit_write_protect_np, I
> assume the W^X protection is a magic Apple per-thread bit.  If so, we
> don't actually require cross-thread locking at all, and all is well.
>
>
> r~

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro