Re: [PATCH v5 1/1] target/riscv: Add Zihintpause support

(now committed as 4696f0ab5c436ed53567ce6baec67c921d9b70ae)

On 2022-07-25 5:47, Dao Lu wrote:
> Added support for RISC-V PAUSE instruction from Zihintpause extension,
> enabled by default.
> 
> Tested-by: Heiko Stuebner <[email protected]>
> Reviewed-by: Alistair Francis <[email protected]>
> Signed-off-by: Dao Lu <[email protected]>
> ---
>   target/riscv/cpu.c                      |  2 ++
>   target/riscv/cpu.h                      |  1 +
>   target/riscv/insn32.decode              |  7 ++++++-
>   target/riscv/insn_trans/trans_rvi.c.inc | 16 ++++++++++++++++
>   4 files changed, 25 insertions(+), 1 deletion(-)


> diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc
> index ca8e3d1ea1..c49dbec0eb 100644
> --- a/target/riscv/insn_trans/trans_rvi.c.inc
> +++ b/target/riscv/insn_trans/trans_rvi.c.inc
> @@ -792,6 +792,22 @@ static bool trans_srad(DisasContext *ctx, arg_srad *a)
>       return gen_shift(ctx, a, EXT_SIGN, tcg_gen_sar_tl, NULL);
>   }
>   
> +static bool trans_pause(DisasContext *ctx, arg_pause *a)
> +{
> +    if (!ctx->cfg_ptr->ext_zihintpause) {
> +        return false;
> +    }
> +
> +    /*
> +     * PAUSE is a no-op in QEMU,
> +     * end the TB and return to main loop
> +     */
> +    gen_set_pc_imm(ctx, ctx->pc_succ_insn);
> +    tcg_gen_exit_tb(NULL, 0);

Looking at this because I'm auditing the overall exception scheduling.

This is a clever way to quickly return the inner vcpu dispatch loop,
but doing so we can not reach the outter loop to yield to another vcpu,
so I wonder if we shouldn't use EXCP_YIELD here. The problem has be
unnoticed because most of us use MTTCG (the default) where EXCP_YIELD
is irrelevant, but I expect this to be problematic on single threaded
round robin TCG (although not reproduced). I noticed that when working
on the hybrid accelerator project -- which is not merged yet -- where
we use MTTCG and EXCP_YIELD is returned by hw accelerators. I'm happy
using this simpler helper, but since this is a big logical change and
I'm not familiar with RISCV I'm am not certain:

   +void helper_pause(CPURISCVState *env)
   +{
   +    CPUState *cs = env_cpu(env);
   +
   +    cs->exception_index = EXCP_YIELD;
   +    cpu_loop_exit(cs);
   +}

> +    ctx->base.is_jmp = DISAS_NORETURN;
> +
> +    return true;
> +}
>   
>   static bool trans_fence(DisasContext *ctx, arg_fence *a)
>   {