Re: [PATCH v2] target/riscv: Enable bitmanip Zicbo[m,z,p] instructions

[Richard Henderson <richard.henderson@linaro.org>]

On 1/25/22 1:00 AM, Christoph Muellner wrote:
> -ori      ............     ..... 110 ..... 0010011 @i
> +{
> +  [
> +    # *** RV32 Zicbop Sandard Extension (hints in the ori-space) ***
> +    prefetch_i  ....... 00000 ..... 110 00000 0010011 @cbo_pref
> +    prefetch_r  ....... 00001 ..... 110 00000 0010011 @cbo_pref
> +    prefetch_w  ....... 00011 ..... 110 00000 0010011 @cbo_pref
> +  ]
> +
> +  # *** RV32I ori ***
> +  ori      ............     ..... 110 ..... 0010011 @i
> +}

Hmm.  I would simply add a comment about these, without changing any code.  They are 
implemented as nops, so there's no point in the decode distinguishing these from the 
"normal" nop that ori r0, rx, y will (not) generate.

> +static bool trans_cbo_clean(DisasContext *ctx, arg_cbo_clean *a)
> +{
> +    REQUIRE_ZICBOM(ctx);
> +    gen_helper_cbo_clean(cpu_env, cpu_gpr[a->rs1]);
> +    return true;
> +}
> +
> +static bool trans_cbo_flush(DisasContext *ctx, arg_cbo_flush *a)
> +{
> +    REQUIRE_ZICBOM(ctx);
> +    gen_helper_cbo_clean(cpu_env, cpu_gpr[a->rs1]);
> +    return true;
> +}

Clean and flush are the same thing?

> +/* helper_zicbo_envcfg
> + *
> + * Raise virtual exceptions and illegal instruction exceptions for
> + * Zicbo[mz] instructions based on the settings of [mhs]envcfg.
> + */
> +static void helper_zicbo_envcfg(CPURISCVState *env, target_ulong envbits)
> +{
> +#ifndef CONFIG_USER_ONLY
> +    target_ulong ra = GETPC();

GETPC may only be called from the outermost helper function (the one directly invoked from 
tcg generated code).  This will not unwind the cpu state correctly.

> +static void helper_zicbom_access(CPURISCVState *env, target_ulong address)
> +{
> +    void* phost;
> +    int ret = TLB_INVALID_MASK;
> +    MMUAccessType access_type = MMU_DATA_LOAD;
> +    target_ulong ra = GETPC();

Likewise.

> +    address &= ~(RISCV_CPU(env)->cfg.cbolen - 1);

RISCV_CPU is to be applied to CPUState, not CPUArchState.  You've dereferenced the wrong 
pointer.  You want env_archcpu() instead.  Pull that out to a local variable for clarity 
and do not...

> +    /* Zeroize the block */
> +    memset(mem, 0, RISCV_CPU(env)->cfg.cbolen);

... call it twice.  Also, s/zeroize/zero/.


r~