Re: [PATCH v5 3/4] target/riscv: implement Zicbom extension

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

On 2/15/23 10:59, Daniel Henrique Barboza wrote:
> From: Christoph Muellner <cmuellner@linux.com>
> 
> Zicbom is the Cache-Block Management extension defined in the already
> ratified RISC-V Base Cache Management Operation (CBO) ISA extension [1].
> 
> The extension contains three instructions: cbo.clean, cbo.flush and
> cbo.inval. All of them must be implemented in the same group as LQ and
> cbo.zero due to overlapping patterns.
> 
> All these instructions can throw a Illegal Instruction/Virtual
> Instruction exception, similar to the existing cbo.zero. The same
> check_zicbo_envcfg() is used to handle these exceptions.
> 
> Aside from that, these instructions also need to handle page faults and
> guest page faults. This is done in a new check_zicbom_access() helper.
> 
> As with Zicboz, the cache block size for Zicbom is also configurable.
> Note that the spec determines that Zicbo[mp] and Zicboz can have
> different cache sizes (Section 2.7 of [1]), so we also include a
> 'cbom_blocksize' to go along with the existing 'cboz_blocksize'. They
> are set to the same size, so unless users want to play around with the
> settings both sizes will be the same.
> 
> [1] https://github.com/riscv/riscv-CMOs/blob/master/specifications/cmobase-v1.0.1.pdf
> 
> Co-developed-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> Signed-off-by: Christoph Muellner <cmuellner@linux.com>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   target/riscv/cpu.c                          |   3 +
>   target/riscv/cpu.h                          |   2 +
>   target/riscv/helper.h                       |   2 +
>   target/riscv/insn32.decode                  |   5 +
>   target/riscv/insn_trans/trans_rvzicbo.c.inc |  27 +++++
>   target/riscv/op_helper.c                    | 107 ++++++++++++++++++++
>   6 files changed, 146 insertions(+)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 7dd37de7f9..4b779b1775 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -74,6 +74,7 @@ struct isa_ext_data {
>   static const struct isa_ext_data isa_edata_arr[] = {
>       ISA_EXT_DATA_ENTRY(h, false, PRIV_VERSION_1_12_0, ext_h),
>       ISA_EXT_DATA_ENTRY(v, false, PRIV_VERSION_1_10_0, ext_v),
> +    ISA_EXT_DATA_ENTRY(zicbom, true, PRIV_VERSION_1_12_0, ext_icbom),
>       ISA_EXT_DATA_ENTRY(zicboz, true, PRIV_VERSION_1_12_0, ext_icboz),
>       ISA_EXT_DATA_ENTRY(zicsr, true, PRIV_VERSION_1_10_0, ext_icsr),
>       ISA_EXT_DATA_ENTRY(zifencei, true, PRIV_VERSION_1_10_0, ext_ifencei),
> @@ -1127,6 +1128,8 @@ static Property riscv_cpu_extensions[] = {
>       DEFINE_PROP_BOOL("zhinx", RISCVCPU, cfg.ext_zhinx, false),
>       DEFINE_PROP_BOOL("zhinxmin", RISCVCPU, cfg.ext_zhinxmin, false),
>   
> +    DEFINE_PROP_BOOL("zicbom", RISCVCPU, cfg.ext_icbom, true),
> +    DEFINE_PROP_UINT16("cbom_blocksize", RISCVCPU, cfg.cbom_blocksize, 64),
>       DEFINE_PROP_BOOL("zicboz", RISCVCPU, cfg.ext_icboz, true),
>       DEFINE_PROP_UINT16("cboz_blocksize", RISCVCPU, cfg.cboz_blocksize, 64),
>   
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 6b4c714d3a..a0673b4604 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -447,6 +447,7 @@ struct RISCVCPUConfig {
>       bool ext_zkt;
>       bool ext_ifencei;
>       bool ext_icsr;
> +    bool ext_icbom;
>       bool ext_icboz;
>       bool ext_zihintpause;
>       bool ext_smstateen;
> @@ -495,6 +496,7 @@ struct RISCVCPUConfig {
>       char *vext_spec;
>       uint16_t vlen;
>       uint16_t elen;
> +    uint16_t cbom_blocksize;
>       uint16_t cboz_blocksize;
>       bool mmu;
>       bool pmp;
> diff --git a/target/riscv/helper.h b/target/riscv/helper.h
> index ce165821b8..37b54e0991 100644
> --- a/target/riscv/helper.h
> +++ b/target/riscv/helper.h
> @@ -98,6 +98,8 @@ DEF_HELPER_FLAGS_2(fcvt_h_lu, TCG_CALL_NO_RWG, i64, env, tl)
>   DEF_HELPER_FLAGS_2(fclass_h, TCG_CALL_NO_RWG_SE, tl, env, i64)
>   
>   /* Cache-block operations */
> +DEF_HELPER_2(cbo_clean_flush, void, env, tl)
> +DEF_HELPER_2(cbo_inval, void, env, tl)
>   DEF_HELPER_2(cbo_zero, void, env, tl)
>   
>   /* Special functions */
> diff --git a/target/riscv/insn32.decode b/target/riscv/insn32.decode
> index 3985bc703f..3788f86528 100644
> --- a/target/riscv/insn32.decode
> +++ b/target/riscv/insn32.decode
> @@ -181,6 +181,11 @@ sraw     0100000 .....  ..... 101 ..... 0111011 @r
>   ldu      ............   ..... 111 ..... 0000011 @i
>   {
>     [
> +    # *** RV32 Zicbom Standard Extension ***
> +    cbo_clean  0000000 00001 ..... 010 00000 0001111 @sfence_vm
> +    cbo_flush  0000000 00010 ..... 010 00000 0001111 @sfence_vm
> +    cbo_inval  0000000 00000 ..... 010 00000 0001111 @sfence_vm
> +
>       # *** RV32 Zicboz Standard Extension ***
>       cbo_zero   0000000 00100 ..... 010 00000 0001111 @sfence_vm
>     ]
> diff --git a/target/riscv/insn_trans/trans_rvzicbo.c.inc b/target/riscv/insn_trans/trans_rvzicbo.c.inc
> index feabc28342..7df9c30b58 100644
> --- a/target/riscv/insn_trans/trans_rvzicbo.c.inc
> +++ b/target/riscv/insn_trans/trans_rvzicbo.c.inc
> @@ -16,12 +16,39 @@
>    * this program.  If not, see <http://www.gnu.org/licenses/>.
>    */
>   
> +#define REQUIRE_ZICBOM(ctx) do {    \
> +    if (!ctx->cfg_ptr->ext_icbom) { \
> +        return false;               \
> +    }                               \
> +} while (0)
> +
>   #define REQUIRE_ZICBOZ(ctx) do {    \
>       if (!ctx->cfg_ptr->ext_icboz) { \
>           return false;               \
>       }                               \
>   } while (0)
>   
> +static bool trans_cbo_clean(DisasContext *ctx, arg_cbo_clean *a)
> +{
> +    REQUIRE_ZICBOM(ctx);
> +    gen_helper_cbo_clean_flush(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_flush(cpu_env, cpu_gpr[a->rs1]);
> +    return true;
> +}
> +
> +static bool trans_cbo_inval(DisasContext *ctx, arg_cbo_inval *a)
> +{
> +    REQUIRE_ZICBOM(ctx);
> +    gen_helper_cbo_inval(cpu_env, cpu_gpr[a->rs1]);
> +    return true;
> +}
> +
>   static bool trans_cbo_zero(DisasContext *ctx, arg_cbo_zero *a)
>   {
>       REQUIRE_ZICBOZ(ctx);
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index 154007af80..573cca4cd3 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -176,6 +176,113 @@ void helper_cbo_zero(CPURISCVState *env, target_ulong address)
>       memset(mem, 0, cbozlen);
>   }
>   
> +/*
> + * check_zicbom_access
> + *
> + * Check access permissions (LOAD, STORE or FETCH as specified in
> + * section 2.5.2 of the CMO specification) for Zicbom, raising
> + * either store page-fault (non-virtualized) or store guest-page
> + * fault (virtualized).
> + */
> +static void check_zicbom_access(CPURISCVState *env,
> +                                target_ulong address,
> +                                uintptr_t ra)
> +{
> +    RISCVCPU *cpu = env_archcpu(env);
> +    int mmu_idx = cpu_mmu_index(env, false);
> +    uint16_t cbomlen = cpu->cfg.cbom_blocksize;
> +    void *phost;
> +    int ret;
> +
> +    /* Mask off low-bits to align-down to the cache-block. */
> +    address &= ~(cbomlen - 1);
> +
> +    /*
> +     * Section 2.5.2 of cmobase v1.0.1:
> +     *
> +     * "A cache-block management instruction is permitted to
> +     * access the specified cache block whenever a load instruction
> +     * or store instruction is permitted to access the corresponding
> +     * physical addresses. If neither a load instruction nor store
> +     * instruction is permitted to access the physical addresses,
> +     * but an instruction fetch is permitted to access the physical
> +     * addresses, whether a cache-block management instruction is
> +     * permitted to access the cache block is UNSPECIFIED.
> +     *
> +     * This means we have to make a choice of whether checking
> +     * MMU_INST_FETCH is worth it or not. We'll go the easier
> +     * route and check MMU_DATA_LOAD and MMU_DATA_STORE only.
> +     */
> +    ret = probe_access_range_flags(env, address, cbomlen,
> +                                   MMU_DATA_LOAD,
> +                                   mmu_idx, true, &phost, ra);
> +
> +    if (ret == TLB_INVALID_MASK) {
> +        probe_access_range_flags(env, address, cbomlen,
> +                                 MMU_DATA_STORE,
> +                                 mmu_idx, true, &phost, ra);

Not assigning to ret.

That said, it seems like all this is too complicated.

The paragraph you quote above says that either LOAD or STORE are required (not both), but 
UNSPECIFIED if only execute.

Thus

     ret = probe_access_flags(env, address, MMU_DATA_LOAD, mmu_idx, true, &phost, ra);
     if (ret != TLB_INVALID_MASK) {
         /* Success: readable */
         return;
     }

     /*
      * Since not readable, must be writable.
      * On failure, store-amo fault will be raised by riscv_cpu_tlb_fill.
      */
     probe_write(env, address, cbomlen, mmu_idx, ra);


seems like it would be sufficient.
At which point the new probe_acccess_range_flags is not needed.


r~