Re: [Qemu-devel] [PATCH v2 06/29] target/riscv: Convert RVXI fence insns to decodetree

[Richard Henderson]

On 10/20/18 8:14 AM, Bastian Koppelmann wrote:
> Signed-off-by: Bastian Koppelmann <address@hidden>
> Signed-off-by: Peer Adelt <address@hidden>
> ---
> v1 -> v2:
>     - simplified fence/fence_i as suggested by Richard
> 
>  target/riscv/insn32.decode              |  6 ++++++
>  target/riscv/insn_trans/trans_rvi.inc.c | 20 ++++++++++++++++++++
>  target/riscv/translate.c                | 14 --------------
>  3 files changed, 26 insertions(+), 14 deletions(-)
> 
> diff --git a/target/riscv/insn32.decode b/target/riscv/insn32.decode
> index cb7622e223..00e30dbc71 100644
> --- a/target/riscv/insn32.decode
> +++ b/target/riscv/insn32.decode
> @@ -24,6 +24,9 @@
>  %sh6    20:6
>  %sh5    20:5
>  
> +%pred   24:4
> +%succ   20:4

Unused.

> +static bool trans_fence(DisasContext *ctx, arg_fence *a, uint32_t insn)
> +{
> +#ifndef CONFIG_USER_ONLY
> +    /* FENCE is a full memory barrier. */
> +    tcg_gen_mb(TCG_MO_ALL | TCG_BAR_SC);
> +#endif
> +    return true;
> +}
> +static bool trans_fence_i(DisasContext *ctx, arg_fence_i *a, uint32_t insn)

Spacing.

> +{
> +#ifndef CONFIG_USER_ONLY
> +    /* FENCE_I is a no-op in QEMU,
> +     * however we need to end the translation block */
> +    tcg_gen_movi_tl(cpu_pc, ctx->pc_succ_insn);
> +    tcg_gen_exit_tb(NULL, 0);
> +    ctx->base.is_jmp = DISAS_NORETURN;
> +#endif
> +    return true;
> +}

I will note that both of these should apply to user-only as well...

> -    case OPC_RISC_FENCE:
> -#ifndef CONFIG_USER_ONLY
> -        if (ctx->opcode & 0x1000) {
> -            /* FENCE_I is a no-op in QEMU,
> -             * however we need to end the translation block */
> -            tcg_gen_movi_tl(cpu_pc, ctx->pc_succ_insn);
> -            tcg_gen_exit_tb(NULL, 0);
> -            ctx->base.is_jmp = DISAS_NORETURN;
> -        } else {
> -            /* FENCE is a full memory barrier. */
> -            tcg_gen_mb(TCG_MO_ALL | TCG_BAR_SC);
> -        }
> -#endif

... even though you're simply copying the bug.

I mentioned this twice during initial review of risc-v, but it seems to have
been missed during commit.

Also note that one can do better than TCG_MO_ALL by actually using the
pred/succ bits to select TCG_MO_{LD,ST}_{LD,ST}.

That said, no new bugs with this patch, so
Reviewed-by: Richard Henderson <address@hidden>


r~