Re: [Qemu-devel] [PATCH 06/11] tcg/s390: Make direct jump patching thread-safe

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

Sergey Fedorov <sergey.fedorov@linaro.org> writes:

> From: Sergey Fedorov <serge.fdrv@gmail.com>
>
> Ensure direct jump patching in s390 is atomic by:
>  * naturally aligning a location of direct jump address;
>  * using atomic_read()/atomic_set() for code patching.
>
> Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
> Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
> ---
>  include/exec/exec-all.h   | 2 +-
>  tcg/s390/tcg-target.inc.c | 6 ++++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 82399175fe80..e18cc24e50f0 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -320,7 +320,7 @@ static inline void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr)
>  {
>      /* patch the branch destination */
>      intptr_t disp = addr - (jmp_addr - 2);
> -    stl_be_p((void*)jmp_addr, disp / 2);
> +    atomic_set((int32_t *)jmp_addr, disp / 2);
>      /* no need to flush icache explicitly */
>  }
>  #elif defined(__aarch64__)
> diff --git a/tcg/s390/tcg-target.inc.c b/tcg/s390/tcg-target.inc.c
> index fbf97bb2e15d..84221dfb68c9 100644
> --- a/tcg/s390/tcg-target.inc.c
> +++ b/tcg/s390/tcg-target.inc.c
> @@ -219,6 +219,8 @@ typedef enum S390Opcode {
>      RX_ST       = 0x50,
>      RX_STC      = 0x42,
>      RX_STH      = 0x40,
> +
> +    NOP         = 0x0707,
>  } S390Opcode;
>
>  #ifndef NDEBUG
> @@ -1716,6 +1718,10 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode opc,
>
>      case INDEX_op_goto_tb:
>          if (s->tb_jmp_offset) {
> +            /* align branch displacement for atomic pathing */

s/pathing/patching/

> +            if (((uintptr_t)s->code_ptr & 3) == 0) {
> +                tcg_out16(s, NOP);
> +            }

Isn't this the wrong way around? Shouldn't we insert the NOP is code_ptr & 3
== 2 (I assume 1 & 3 are impossible). Or is it that we need to be
unaligned when we out the jmp so the offset itself is aligned.

>              tcg_out16(s, RIL_BRCL | (S390_CC_ALWAYS << 4));
>              s->tb_jmp_offset[args[0]] = tcg_current_code_size(s);
>              s->code_ptr += 2;


--
Alex Bennée