* [RFC PATCH v1 1/2] riscv: Add preliminary infra for custom instrcution handling @ 2021-10-21 15:11 Ruinland Chuan-Tzu Tsai 2021-10-21 15:11 ` [RFC PATCH v1 2/2] Enable custom instruction suport for Andes A25 and AX25 CPU model Ruinland Chuan-Tzu Tsai 2021-10-21 16:11 ` [RFC PATCH v1 1/2] riscv: Add preliminary infra for custom instrcution handling Richard Henderson 0 siblings, 2 replies; 8+ messages in thread From: Ruinland Chuan-Tzu Tsai @ 2021-10-21 15:11 UTC (permalink / raw) To: alistair23, wangjunqiang, bmeng.cn Cc: ycliang, alankao, dylan, qemu-devel, Ruinland Chuan-Tzu Tsai, qemu-riscv This is inspired by Antmicro's work on their fork of TCG. We try to decode custom instrucions first - - if the decoders reject what they encounter, invoke the decoders for standard instructions. Caveats: the pc_succ_next shall be corrected if custom decoder rejcets the incoming opcode. Signed-off-by: Ruinland Chuan-Tzu Tsai <ruinland@andestech.com> --- target/riscv/cpu.c | 25 ++++++++++--- target/riscv/translate.c | 78 ++++++++++++++++++++++++++++++++-------- 2 files changed, 85 insertions(+), 18 deletions(-) diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index fe63e68b8e..e8ab5734c2 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -36,6 +36,14 @@ static const char riscv_exts[26] = "IEMAFDQCLBJTPVNSUHKORWXYZG"; GHashTable *custom_csr_map = NULL; #include "custom_csr_defs.h" +#include "custom_insn_defs.h" + +/* Custom instruction related prototypes/variables */ +bool (*custom_c_insn_handler) (DisasContext *, uint16_t); +bool (*custom_insn_handler) (DisasContext *, uint32_t); +__thread bool cpu_has_custom_insns = false; +/* Custom instruction handlers sometimes needs hart state */ +__thread CPURISCVState *HartState = NULL; const char * const riscv_int_regnames[] = { "x0/zero", "x1/ra", "x2/sp", "x3/gp", "x4/tp", "x5/t0", "x6/t1", @@ -151,12 +159,20 @@ static void set_resetvec(CPURISCVState *env, target_ulong resetvec) env->resetvec = resetvec; #endif } +void setup_custom_csr(CPURISCVState *env, + riscv_custom_csr_operations csr_map_struct[] + ); + +void setup_custom_csr(CPURISCVState *env, + riscv_custom_csr_operations csr_map_struct[] + ) { + + env->custom_csr_map = g_hash_table_new_full(g_direct_hash, \ + g_direct_equal, \ + NULL, NULL); + -static void setup_custom_csr(CPURISCVState *env, - riscv_custom_csr_operations csr_map_struct[]) -{ int i; - env->custom_csr_map = g_hash_table_new(g_direct_hash, g_direct_equal); for (i = 0; i < MAX_CUSTOM_CSR_NUM; i++) { if (csr_map_struct[i].csrno != 0) { g_hash_table_insert(env->custom_csr_map, @@ -187,6 +203,7 @@ static void rv64_base_cpu_init(Object *obj) set_misa(env, RV64); } + static void ax25_cpu_init(Object *obj) { CPURISCVState *env = &RISCV_CPU(obj)->env; diff --git a/target/riscv/translate.c b/target/riscv/translate.c index 74b33fa3c9..f755749380 100644 --- a/target/riscv/translate.c +++ b/target/riscv/translate.c @@ -485,27 +485,77 @@ static uint32_t opcode_at(DisasContextBase *dcbase, target_ulong pc) /* Include the auto-generated decoder for 16 bit insn */ #include "decode-insn16.c.inc" -static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode) +/* Custom insn related definitions/prototypes */ +extern __thread bool cpu_has_custom_insns; +/* + * These 2 are for indication if decode fails. + * We don't want to interfere the following regular extension decoding + * We assume MTTCG is 1-1 mapping for now. + */ +__thread bool custom_c_insn_decoded; +__thread bool custom_insn_decoded; + +extern bool (*custom_c_insn_handler)(DisasContext *, uint16_t); +extern bool (*custom_insn_handler)(DisasContext *, uint32_t); + +static void try_decode_custom_insn(CPURISCVState *env, DisasContext *ctx, + uint16_t opcode) { - /* check for compressed insn */ - if (extract16(opcode, 0, 2) != 3) { + bool is_compressed_custom = false; + is_compressed_custom = extract16(opcode, 0, 2) != 3 ? true : false; + if (is_compressed_custom) { if (!has_ext(ctx, RVC)) { gen_exception_illegal(ctx); + } + ctx->pc_succ_insn = ctx->base.pc_next + 2; + custom_c_insn_decoded = custom_c_insn_handler(ctx, opcode); + if(custom_c_insn_decoded) {custom_insn_decoded = true;} + } else { + uint32_t opcode32 = opcode; + opcode32 = deposit32(opcode32, 16, 16, translator_lduw(env, &ctx->base, + ctx->base.pc_next + 2)); + ctx->opcode = opcode32; + ctx->pc_succ_insn = ctx->base.pc_next + 4; + custom_insn_decoded = custom_insn_handler(ctx, opcode32); + } +} + +static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode) +{ + /* + * Try to deocde and trans the custom instruction before any others + * This is a bad idea which introduces great performance penalties, + * but we don't have other choices here ... + */ + custom_insn_decoded = false; + custom_c_insn_decoded = false; + if (unlikely(cpu_has_custom_insns)) { + try_decode_custom_insn(env, ctx, opcode); + } + + if (likely(!custom_insn_decoded)) { + if(unlikely(cpu_has_custom_insns)) + ctx->pc_succ_insn = ctx->base.pc_next - !custom_c_insn_decoded ? 4 : 2; + /* check for compressed insn */ + if (extract16(opcode, 0, 2) != 3) { + if (!has_ext(ctx, RVC)) { + gen_exception_illegal(ctx); + } else { + ctx->pc_succ_insn = ctx->base.pc_next + 2; + if (!decode_insn16(ctx, opcode)) { + gen_exception_illegal(ctx); + } + } } else { - ctx->pc_succ_insn = ctx->base.pc_next + 2; - if (!decode_insn16(ctx, opcode)) { + uint32_t opcode32 = opcode; + opcode32 = deposit32(opcode32, 16, 16, + translator_lduw(env, &ctx->base, + ctx->base.pc_next + 2)); + ctx->pc_succ_insn = ctx->base.pc_next + 4; + if (!decode_insn32(ctx, opcode32)) { gen_exception_illegal(ctx); } } - } else { - uint32_t opcode32 = opcode; - opcode32 = deposit32(opcode32, 16, 16, - translator_lduw(env, &ctx->base, - ctx->base.pc_next + 2)); - ctx->pc_succ_insn = ctx->base.pc_next + 4; - if (!decode_insn32(ctx, opcode32)) { - gen_exception_illegal(ctx); - } } } -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [RFC PATCH v1 2/2] Enable custom instruction suport for Andes A25 and AX25 CPU model 2021-10-21 15:11 [RFC PATCH v1 1/2] riscv: Add preliminary infra for custom instrcution handling Ruinland Chuan-Tzu Tsai @ 2021-10-21 15:11 ` Ruinland Chuan-Tzu Tsai 2021-10-21 19:17 ` Richard Henderson 2021-10-21 16:11 ` [RFC PATCH v1 1/2] riscv: Add preliminary infra for custom instrcution handling Richard Henderson 1 sibling, 1 reply; 8+ messages in thread From: Ruinland Chuan-Tzu Tsai @ 2021-10-21 15:11 UTC (permalink / raw) To: alistair23, wangjunqiang, bmeng.cn Cc: ycliang, alankao, dylan, qemu-devel, Ruinland Chuan-Tzu Tsai, qemu-riscv In this patch, we demonstrate how Andes Performance Extension(c) insn : bfos and bfoz could be used with Andes CoDense : exec.it. By doing so, an Andes vendor designed CSR : uitb must be used. Signed-off-by: Ruinland Chuan-Tzu Tsai <ruinland@andestech.com> --- target/riscv/andes_codense.decode | 23 +++++ target/riscv/andes_custom_rv32.decode | 27 +++++ target/riscv/andes_custom_rv64.decode | 27 +++++ target/riscv/andes_helper.c | 49 +++++++++ target/riscv/andes_helper.h | 1 + target/riscv/cpu.c | 8 ++ target/riscv/helper.h | 2 + target/riscv/insn_trans/trans_andes.c.inc | 115 ++++++++++++++++++++++ target/riscv/meson.build | 13 +++ target/riscv/translate.c | 12 +++ 10 files changed, 277 insertions(+) create mode 100644 target/riscv/andes_codense.decode create mode 100644 target/riscv/andes_custom_rv32.decode create mode 100644 target/riscv/andes_custom_rv64.decode create mode 100644 target/riscv/andes_helper.c create mode 100644 target/riscv/andes_helper.h create mode 100644 target/riscv/insn_trans/trans_andes.c.inc diff --git a/target/riscv/andes_codense.decode b/target/riscv/andes_codense.decode new file mode 100644 index 0000000000..639d22ac67 --- /dev/null +++ b/target/riscv/andes_codense.decode @@ -0,0 +1,23 @@ +# +# RISC-V translation routines for the RVXI Base Integer Instruction Set. +# +# Copyright (c) 2018 Peer Adelt, peer.adelt@hni.uni-paderborn.de +# Bastian Koppelmann, kbastian@mail.uni-paderborn.de +# +# This program is free software; you can redistribute it and/or modify it +# under the terms and conditions of the GNU General Public License, +# version 2 or later, as published by the Free Software Foundation. +# +# This program is distributed in the hope it will be useful, but WITHOUT +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +# more details. +# +# You should have received a copy of the GNU General Public License along with +# this program. If not, see <http://www.gnu.org/licenses/>. + + +%imm_ex10 8:s1 12:1 3:1 9:1 5:2 2:1 10:2 4:1 +&codense imm_codense +@ex10 ... .... . . ..... .. &codense imm_codense=%imm_ex10 +execit 100 .... . 0 ..... 00 @ex10 diff --git a/target/riscv/andes_custom_rv32.decode b/target/riscv/andes_custom_rv32.decode new file mode 100644 index 0000000000..f69d75018d --- /dev/null +++ b/target/riscv/andes_custom_rv32.decode @@ -0,0 +1,27 @@ +# +# Andes Technology V5 Performance Extension +# +# Copyright (c) 2021 Andes Technology, ruinland@andestech.com +# +# This program is free software; you can redistribute it and/or modify it +# under the terms and conditions of the GNU General Public License, +# version 2 or later, as published by the Free Software Foundation. +# +# This program is distributed in the hope it will be useful, but WITHOUT +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +# more details. +# +# You should have received a copy of the GNU General Public License along with +# this program. If not, see <http://www.gnu.org/licenses/>. + +# Fields: +%rs1 15:5 +%rd 7:5 +## Similar to I-Type +%mbfob 26:5 +%lbfob 20:5 +&i_b mbfob lbfob rs1 rd +@i_bfo . ..... . ..... ..... ... ..... ....... &i_b %mbfob %lbfob %rs1 %rd +bfos 0 ..... 0 ..... ..... 011 ..... 1011011 @i_bfo #0x03 +bfoz 0 ..... 0 ..... ..... 010 ..... 1011011 @i_bfo #0x02 diff --git a/target/riscv/andes_custom_rv64.decode b/target/riscv/andes_custom_rv64.decode new file mode 100644 index 0000000000..61f1fb208b --- /dev/null +++ b/target/riscv/andes_custom_rv64.decode @@ -0,0 +1,27 @@ +# +# Andes Technology V5 Performance Extension +# +# Copyright (c) 2021 Andes Technology, ruinland@andestech.com +# +# This program is free software; you can redistribute it and/or modify it +# under the terms and conditions of the GNU General Public License, +# version 2 or later, as published by the Free Software Foundation. +# +# This program is distributed in the hope it will be useful, but WITHOUT +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +# more details. +# +# You should have received a copy of the GNU General Public License along with +# this program. If not, see <http://www.gnu.org/licenses/>. + +# Fields: +%rs1 15:5 +%rd 7:5 +## Similar to I-Type +%mbfob 26:6 +%lbfob 20:6 +&i_b mbfob lbfob rs1 rd +@i_bfo . ..... . ..... ..... ... ..... ....... &i_b %mbfob %lbfob %rs1 %rd +bfos . ..... . ..... ..... 011 ..... 1011011 @i_bfo #0x03 +bfoz . ..... . ..... ..... 010 ..... 1011011 @i_bfo #0x02 diff --git a/target/riscv/andes_helper.c b/target/riscv/andes_helper.c new file mode 100644 index 0000000000..5e1e52fccb --- /dev/null +++ b/target/riscv/andes_helper.c @@ -0,0 +1,49 @@ +#include "qemu/osdep.h" +#include "cpu.h" +#include "qemu/host-utils.h" +#include "exec/exec-all.h" +#include "exec/helper-proto.h" +#include "fpu/softfloat.h" +#include "internals.h" +typedef int (*test_function)(uint8_t a, uint8_t b); +target_ulong helper_andes_v5_bfo_x(target_ulong rd, target_ulong rs1, + target_ulong insn) +{ + int msb, lsb, is_se; + int lsbp1, msbm1, lsbm1, lenm1; + uint64_t se; + uint64_t nxrd = rd; /* for safety sake */ + + msb = extract64(insn, 26, 6); + lsb = extract64(insn, 20, 6); + is_se = 0x3 == (0x7 & (insn >> 12)); /* BFOS */ + lsbp1 = lsb + 1; + msbm1 = msb - 1; + lsbm1 = lsb - 1; + + if (msb == 0) { + nxrd = deposit64(nxrd, lsb, 1, 1 & rs1); + if (lsb > 0) { + nxrd = deposit64(nxrd, 0, lsbm1 + 1, 0); + } + if (lsb < 63) { + se = (is_se && (1 & rs1)) ? -1LL : 0; + nxrd = deposit64(nxrd, lsbp1, 64 - lsbp1, se); + } + } else if (msb < lsb) { + lenm1 = lsb - msb; + nxrd = deposit64(nxrd, msb, lenm1 + 1, rs1 >> 0); + if (lsb < 63) { + se = (is_se && (1 & (rs1 >> lenm1))) ? -1LL : 0; + nxrd = deposit64(nxrd, lsbp1, 64 - lsbp1, se); + } + nxrd = deposit64(nxrd, 0, msbm1 + 1, 0); + } else { /* msb >= lsb */ + lenm1 = msb - lsb; + nxrd = deposit64(nxrd, 0, lenm1 + 1, rs1 >> lsb); + se = (is_se && (1 & (rs1 >> msb))) ? -1LL : 0; + nxrd = deposit64(nxrd, lenm1 + 1, 63 - lenm1, se); + } + + return (target_long)nxrd; +} diff --git a/target/riscv/andes_helper.h b/target/riscv/andes_helper.h new file mode 100644 index 0000000000..dfe15d8841 --- /dev/null +++ b/target/riscv/andes_helper.h @@ -0,0 +1 @@ +DEF_HELPER_FLAGS_3(andes_v5_bfo_x, TCG_CALL_NO_RWG, tl, tl, tl, tl) diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index e8ab5734c2..fc20673c1a 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -207,8 +207,12 @@ static void rv64_base_cpu_init(Object *obj) static void ax25_cpu_init(Object *obj) { CPURISCVState *env = &RISCV_CPU(obj)->env; + HartState = env; set_misa(env, RV64 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU); set_priv_version(env, PRIV_VERSION_1_10_0); + cpu_has_custom_insns = true; + custom_insn_handler = decode_andes; + custom_c_insn_handler = decode_codense; setup_custom_csr(env, andes_custom_csr_table); env->custom_csr_val = g_malloc(andes_custom_csr_size); } @@ -271,8 +275,12 @@ static void rv32_imafcu_nommu_cpu_init(Object *obj) static void a25_cpu_init(Object *obj) { CPURISCVState *env = &RISCV_CPU(obj)->env; + HartState = env; set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU); set_priv_version(env, PRIV_VERSION_1_10_0); + cpu_has_custom_insns = true; + custom_insn_handler = decode_andes; + custom_c_insn_handler = decode_codense; setup_custom_csr(env, andes_custom_csr_table); env->custom_csr_val = g_malloc(andes_custom_csr_size); } diff --git a/target/riscv/helper.h b/target/riscv/helper.h index 460eee9988..ca60825e80 100644 --- a/target/riscv/helper.h +++ b/target/riscv/helper.h @@ -1149,3 +1149,5 @@ DEF_HELPER_6(vcompress_vm_b, void, ptr, ptr, ptr, ptr, env, i32) DEF_HELPER_6(vcompress_vm_h, void, ptr, ptr, ptr, ptr, env, i32) DEF_HELPER_6(vcompress_vm_w, void, ptr, ptr, ptr, ptr, env, i32) DEF_HELPER_6(vcompress_vm_d, void, ptr, ptr, ptr, ptr, env, i32) + +#include "andes_helper.h" diff --git a/target/riscv/insn_trans/trans_andes.c.inc b/target/riscv/insn_trans/trans_andes.c.inc new file mode 100644 index 0000000000..384745fceb --- /dev/null +++ b/target/riscv/insn_trans/trans_andes.c.inc @@ -0,0 +1,115 @@ +#define GP 3 +#define ANDES_V5_CODE_DENSE_MASK (0xe083) +#define ANDES_V5_CODE_DENSE_ID(x) ((x)&ANDES_V5_CODE_DENSE_MASK) +#define ANDES_V5_CODE_DENSE_IMM11(x) ( \ + (extract32(x, 4, 1) << 2) | \ + (extract32(x, 10, 2) << 3) | \ + (extract32(x, 2, 1) << 5) | \ + (extract32(x, 5, 2) << 6) | \ + (extract32(x, 9, 1) << 8) | \ + (extract32(x, 3, 1) << 9) | \ + (extract32(x, 12, 1) << 10) | \ + (extract64(x, 8, 1) << 11) | \ + 0) + +#define ANDES_V5_GET_JAL_UIMM(inst) ((extract32(inst, 21, 10) << 1) \ + | (extract32(inst, 20, 1) << 11) \ + | (extract32(inst, 12, 8) << 12) \ + | (extract32(inst, 31, 1) << 20)) + + +enum andes_v5_inst_id +{ + /* Code Dense Extension */ + EXEC_IT = (0x8000), + /* custom 2 */ + BFOS = 0x03, + BFOZ = 0x02, +}; + +static bool trans_bfos(DisasContext *ctx, arg_bfos *a) +{ + TCGv v0, v1, v2; + v2 = tcg_const_tl(ctx->opcode); + v0 = get_gpr(ctx, a->rs1, EXT_NONE); + v1 = get_gpr(ctx, a->rd, EXT_NONE); + gen_helper_andes_v5_bfo_x(v1, v1, v0, v2); + gen_set_gpr(ctx, a->rd, v1); + tcg_temp_free(v2); + return true; +} + +static bool trans_bfoz(DisasContext *ctx, arg_bfoz *a) +{ + TCGv v0, v1, v2; + v2 = tcg_const_tl(ctx->opcode); + v0 = get_gpr(ctx, a->rs1, EXT_NONE); + v1 = get_gpr(ctx, a->rd, EXT_NONE); + gen_helper_andes_v5_bfo_x(v1, v1, v0, v2); + gen_set_gpr(ctx, a->rd, v1); + tcg_temp_free(v2); + return true; +} + +static void decode_opc_andes(CPURISCVState *env, DisasContext *ctx, uint32_t opcode32) +{ + uint32_t old_opcode = ctx->opcode; + ctx->opcode = opcode32; + custom_insn_decoded = decode_andes(ctx, opcode32); + if(!custom_insn_decoded) { + if (!decode_insn32(ctx, opcode32)) { + gen_exception_illegal(ctx); + } + } + ctx->opcode = old_opcode; +} + +static int andes_v5_gen_codense_exec_it(CPURISCVState *env, DisasContext *ctx, arg_execit *a) +{ + uint32_t insn; + uint32_t imm_ex10 = a->imm_codense; + target_ulong uitb_val = 0; + riscv_csrrw(env, 0x800, &uitb_val, 0, 0); + + if (extract32(uitb_val, 0, 1)) { /* UTIB.HW == 1 */ + qemu_log_mask(LOG_GUEST_ERROR, "exec.it: UITB.HW == 1 is not supported by now!\n"); + gen_exception_illegal(ctx); + + uint32_t instruction_table[0]; + insn = instruction_table[imm_ex10 >> 2]; + + return false; + } else { /* UTIB.HW == 0 */ + target_ulong vaddr = (uitb_val & ~0x3) + (imm_ex10<<2); + insn = cpu_ldl_code(env, vaddr); + } + +/* Execute(insn) + * do as the replaced instruction, even exceptions, + * except ctx->pc_succ_insn value (2). + */ + uint32_t op = MASK_OP_MAJOR(insn); + if (op == OPC_RISC_JAL) { + /* implement this by hack imm */ + int rd = GET_RD(ctx->opcode); + target_long imm = ANDES_V5_GET_JAL_UIMM(ctx->opcode); + target_ulong next_pc = (ctx->base.pc_next >> 21 << 21) | imm; + imm = next_pc - ctx->base.pc_next; + gen_jal(ctx, rd, imm); + } else { + /* JARL done as SPEC already */ + /* presume ctx->pc_succ_insn not changed in any ISA extension */ + decode_opc_andes(env, ctx, insn); + } + + return true; +} +extern __thread CPURISCVState *HartState; + +static bool trans_execit(DisasContext *ctx, arg_execit *a) +{ + /* This is merely something to make things happen */ + CPURISCVState *env = HartState; + andes_v5_gen_codense_exec_it(env ,ctx, a); + return true; +} diff --git a/target/riscv/meson.build b/target/riscv/meson.build index 5c87672ff9..002aec2746 100644 --- a/target/riscv/meson.build +++ b/target/riscv/meson.build @@ -6,10 +6,23 @@ gen = [ decodetree.process('insn32.decode', extra_args: '--static-decode=decode_insn32'), ] +gen_custom_RV32 = [ + decodetree.process('andes_codense.decode', extra_args: ['--decode=decode_codense', '--insnwidth=16']), + decodetree.process('andes_custom_rv32.decode', extra_args: '--decode=decode_andes'), +] + +gen_custom_RV64 = [ + decodetree.process('andes_codense.decode', extra_args: ['--decode=decode_codense', '--insnwidth=16']), + decodetree.process('andes_custom_rv64.decode', extra_args: '--decode=decode_andes'), +] + riscv_ss = ss.source_set() riscv_ss.add(gen) +riscv_ss.add(when: 'TARGET_RISCV32', if_true: gen_custom_RV32) +riscv_ss.add(when: 'TARGET_RISCV64', if_true: gen_custom_RV64) riscv_ss.add(files( 'csr_andes.c', + 'andes_helper.c', 'cpu.c', 'cpu_helper.c', 'csr.c', diff --git a/target/riscv/translate.c b/target/riscv/translate.c index f755749380..4c0085dde5 100644 --- a/target/riscv/translate.c +++ b/target/riscv/translate.c @@ -498,6 +498,18 @@ __thread bool custom_insn_decoded; extern bool (*custom_c_insn_handler)(DisasContext *, uint16_t); extern bool (*custom_insn_handler)(DisasContext *, uint32_t); +/* Include the auto-generated decoder for andes custom insn */ +bool decode_andes(DisasContext *, uint32_t); +bool decode_codense(DisasContext *, uint16_t); +#include "decode-andes_codense.c.inc" +#ifdef TARGET_RISCV32 +#include "decode-andes_custom_rv32.c.inc" +#endif +#ifdef TARGET_RISCV64 +#include "decode-andes_custom_rv64.c.inc" +#endif +#include "insn_trans/trans_andes.c.inc" + static void try_decode_custom_insn(CPURISCVState *env, DisasContext *ctx, uint16_t opcode) { -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC PATCH v1 2/2] Enable custom instruction suport for Andes A25 and AX25 CPU model 2021-10-21 15:11 ` [RFC PATCH v1 2/2] Enable custom instruction suport for Andes A25 and AX25 CPU model Ruinland Chuan-Tzu Tsai @ 2021-10-21 19:17 ` Richard Henderson 2021-10-22 8:48 ` Ruinland ChuanTzu Tsai 0 siblings, 1 reply; 8+ messages in thread From: Richard Henderson @ 2021-10-21 19:17 UTC (permalink / raw) To: Ruinland Chuan-Tzu Tsai, alistair23, wangjunqiang, bmeng.cn Cc: qemu-riscv, dylan, ycliang, qemu-devel, alankao On 10/21/21 8:11 AM, Ruinland Chuan-Tzu Tsai wrote: > In this patch, we demonstrate how Andes Performance Extension(c) insn : > bfos and bfoz could be used with Andes CoDense : exec.it. > > By doing so, an Andes vendor designed CSR : uitb must be used. > > Signed-off-by: Ruinland Chuan-Tzu Tsai <ruinland@andestech.com> > --- > target/riscv/andes_codense.decode | 23 +++++ > target/riscv/andes_custom_rv32.decode | 27 +++++ > target/riscv/andes_custom_rv64.decode | 27 +++++ > target/riscv/andes_helper.c | 49 +++++++++ > target/riscv/andes_helper.h | 1 + > target/riscv/cpu.c | 8 ++ > target/riscv/helper.h | 2 + > target/riscv/insn_trans/trans_andes.c.inc | 115 ++++++++++++++++++++++ > target/riscv/meson.build | 13 +++ > target/riscv/translate.c | 12 +++ > 10 files changed, 277 insertions(+) > create mode 100644 target/riscv/andes_codense.decode > create mode 100644 target/riscv/andes_custom_rv32.decode > create mode 100644 target/riscv/andes_custom_rv64.decode > create mode 100644 target/riscv/andes_helper.c > create mode 100644 target/riscv/andes_helper.h > create mode 100644 target/riscv/insn_trans/trans_andes.c.inc > > diff --git a/target/riscv/andes_codense.decode b/target/riscv/andes_codense.decode > new file mode 100644 > index 0000000000..639d22ac67 > --- /dev/null > +++ b/target/riscv/andes_codense.decode > @@ -0,0 +1,23 @@ > +# > +# RISC-V translation routines for the RVXI Base Integer Instruction Set. > +# > +# Copyright (c) 2018 Peer Adelt, peer.adelt@hni.uni-paderborn.de > +# Bastian Koppelmann, kbastian@mail.uni-paderborn.de > +# > +# This program is free software; you can redistribute it and/or modify it > +# under the terms and conditions of the GNU General Public License, > +# version 2 or later, as published by the Free Software Foundation. > +# > +# This program is distributed in the hope it will be useful, but WITHOUT > +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > +# more details. > +# > +# You should have received a copy of the GNU General Public License along with > +# this program. If not, see <http://www.gnu.org/licenses/>. > + > + > +%imm_ex10 8:s1 12:1 3:1 9:1 5:2 2:1 10:2 4:1 > +&codense imm_codense > +@ex10 ... .... . . ..... .. &codense imm_codense=%imm_ex10 > +execit 100 .... . 0 ..... 00 @ex10 It would probably be a bit clearer without the argument set and format, which in this case are used exactly once, so there's no actual savings. execit 100 ..... 0 ..... 00 imm=%imm_ex10 > +++ b/target/riscv/andes_custom_rv32.decode ... > +++ b/target/riscv/andes_custom_rv64.decode Note that we're moving toward a single riscv64 executable, and so these two files should be combined. In this case, just taking the rv64 file will work for RV32, with an extra check during translate. > +# Fields: > +%rs1 15:5 > +%rd 7:5 > +## Similar to I-Type > +%mbfob 26:6 > +%lbfob 20:6 > +&i_b mbfob lbfob rs1 rd > +@i_bfo . ..... . ..... ..... ... ..... ....... &i_b %mbfob %lbfob %rs1 %rd Better as @i_bfo msb:6 lsb:6 rs1:5 ... rd:5 ....... &i_bfo There's quite a lot of riscv code that could be cleaned up like this. > +++ b/target/riscv/andes_helper.c > @@ -0,0 +1,49 @@ > +#include "qemu/osdep.h" All new files must have copyright boilerplate. That said... > +#include "cpu.h" > +#include "qemu/host-utils.h" > +#include "exec/exec-all.h" > +#include "exec/helper-proto.h" > +#include "fpu/softfloat.h" > +#include "internals.h" > +typedef int (*test_function)(uint8_t a, uint8_t b); > +target_ulong helper_andes_v5_bfo_x(target_ulong rd, target_ulong rs1, > + target_ulong insn) Never pass the instruction to a helper. This is an indication that you should have done more work during translate. > --- /dev/null > +++ b/target/riscv/insn_trans/trans_andes.c.inc > @@ -0,0 +1,115 @@ > +#define GP 3 Again, boilerplate. > +#define ANDES_V5_CODE_DENSE_MASK (0xe083) > +#define ANDES_V5_CODE_DENSE_ID(x) ((x)&ANDES_V5_CODE_DENSE_MASK) > +#define ANDES_V5_CODE_DENSE_IMM11(x) ( \ > + (extract32(x, 4, 1) << 2) | \ > + (extract32(x, 10, 2) << 3) | \ > + (extract32(x, 2, 1) << 5) | \ > + (extract32(x, 5, 2) << 6) | \ > + (extract32(x, 9, 1) << 8) | \ > + (extract32(x, 3, 1) << 9) | \ > + (extract32(x, 12, 1) << 10) | \ > + (extract64(x, 8, 1) << 11) | \ > + 0) > + > +#define ANDES_V5_GET_JAL_UIMM(inst) ((extract32(inst, 21, 10) << 1) \ > + | (extract32(inst, 20, 1) << 11) \ > + | (extract32(inst, 12, 8) << 12) \ > + | (extract32(inst, 31, 1) << 20)) > + > + > +enum andes_v5_inst_id > +{ > + /* Code Dense Extension */ > + EXEC_IT = (0x8000), > + /* custom 2 */ > + BFOS = 0x03, > + BFOZ = 0x02, > +}; Left over from something else? This all looks like something that should be handled by decodetree. > +static bool trans_bfos(DisasContext *ctx, arg_bfos *a) > +{ > + TCGv v0, v1, v2; > + v2 = tcg_const_tl(ctx->opcode); > + v0 = get_gpr(ctx, a->rs1, EXT_NONE); > + v1 = get_gpr(ctx, a->rd, EXT_NONE); > + gen_helper_andes_v5_bfo_x(v1, v1, v0, v2); > + gen_set_gpr(ctx, a->rd, v1); > + tcg_temp_free(v2); > + return true; > +} > + > +static bool trans_bfoz(DisasContext *ctx, arg_bfoz *a) > +{ > + TCGv v0, v1, v2; > + v2 = tcg_const_tl(ctx->opcode); > + v0 = get_gpr(ctx, a->rs1, EXT_NONE); > + v1 = get_gpr(ctx, a->rd, EXT_NONE); > + gen_helper_andes_v5_bfo_x(v1, v1, v0, v2); > + gen_set_gpr(ctx, a->rd, v1); > + tcg_temp_free(v2); > + return true; > +} So, you've just passed off everything to the helper. Not good. First, these two instructions are so close we add a common helper. static bool do_bfox(DisasContext *ctx, arg_i_bfo *a, bool is_signed) { ... } static bool trans_bfos(DisasContext *ctx, arg_i_bfo *a) { return do_bfox(ctx, a, true); } static bool trans_bfoz(DisasContext *ctx, arg_i_bfo *a) { return do_bfox(ctx, a, false); } Second, re the RV32/RV64 merge, range check the bits: if (a->msb >= get_xlen(ctx) || a->lsb >= get_xlen(ctx)) { return false; } Third, TCG can easily handle extract/deposit inline: dest = dest_gpr(ctx, a->rd); src1 = get_gpr(ctx, a->rs1, EXT_NONE); if (a->msb >= a->lsb) { len = a->msb - a->lsb + 1; if (is_signed) { tcg_gen_sextract_tl(dest, src1, a->lsb, len); } else { tcg_gen_extract_tl(dest, src1, a->lsb, len); } } else { len = a->lsb - a->msb + 1; if (is_signed) { tcg_gen_sextract_tl(dest, src1, 0, len); } else { tcg_gen_extract_tl(dest, src1, 0, len); } tcg_gen_shli_tl(dest, dest, a->msb); } gen_set_gpr(ctx, a->rd, dest); > +static int andes_v5_gen_codense_exec_it(CPURISCVState *env, DisasContext *ctx, arg_execit *a) > +{ > + uint32_t insn; > + uint32_t imm_ex10 = a->imm_codense; > + target_ulong uitb_val = 0; > + riscv_csrrw(env, 0x800, &uitb_val, 0, 0); This won't work -- you can't access env during translation. So all this faff around passing env through HartState is for naught. Having a look through the rest of this: > + > + if (extract32(uitb_val, 0, 1)) { /* UTIB.HW == 1 */ > + qemu_log_mask(LOG_GUEST_ERROR, "exec.it: UITB.HW == 1 is not supported by now!\n"); > + gen_exception_illegal(ctx); > + > + uint32_t instruction_table[0]; > + insn = instruction_table[imm_ex10 >> 2]; > + > + return false; > + } else { /* UTIB.HW == 0 */ > + target_ulong vaddr = (uitb_val & ~0x3) + (imm_ex10<<2); > + insn = cpu_ldl_code(env, vaddr); > + } > + > +/* Execute(insn) > + * do as the replaced instruction, even exceptions, > + * except ctx->pc_succ_insn value (2). > + */ > + uint32_t op = MASK_OP_MAJOR(insn); > + if (op == OPC_RISC_JAL) { > + /* implement this by hack imm */ > + int rd = GET_RD(ctx->opcode); > + target_long imm = ANDES_V5_GET_JAL_UIMM(ctx->opcode); > + target_ulong next_pc = (ctx->base.pc_next >> 21 << 21) | imm; > + imm = next_pc - ctx->base.pc_next; > + gen_jal(ctx, rd, imm); > + } else { > + /* JARL done as SPEC already */ > + /* presume ctx->pc_succ_insn not changed in any ISA extension */ > + decode_opc_andes(env, ctx, insn); > + } > + > + return true; > +} This looks quite a lot like the S390X EXECUTE instruction. It's hard to suggest exactly how to structure this, because I can't find documentation, and thus the edge cases are unknown. Because of the .HW check, I would expect all of this do be in a helper function. The qemu_log_mask would be followed by riscv_raise_exception. As for the actual execute, I'd suggest following the lead of s390x and utilize the cs_base field of cpu_get_tb_cpu_state to hold the opcode to be executed. You'd load the opcode in the helper, install the proper state into env, and end the current TranslationBlock. The next TranslationBlock will find the opcode and decode it in the normal way. Have a look at target/s390x/tcg/mem_helper.c, HELPER(ex), where we load the opcode and store state. Then translate.c, extract_insn, where we consume the state. r~ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH v1 2/2] Enable custom instruction suport for Andes A25 and AX25 CPU model 2021-10-21 19:17 ` Richard Henderson @ 2021-10-22 8:48 ` Ruinland ChuanTzu Tsai 2021-10-22 11:52 ` Alex Bennée 0 siblings, 1 reply; 8+ messages in thread From: Ruinland ChuanTzu Tsai @ 2021-10-22 8:48 UTC (permalink / raw) To: Richard Henderson Cc: ycliang, qemu-riscv, alankao, wangjunqiang, dylan, qemu-devel, alistair23, bmeng.cn On Thu, Oct 21, 2021 at 12:17:47PM -0700, Richard Henderson wrote: > On 10/21/21 8:11 AM, Ruinland Chuan-Tzu Tsai wrote: > > In this patch, we demonstrate how Andes Performance Extension(c) insn : > > bfos and bfoz could be used with Andes CoDense : exec.it. > > > > By doing so, an Andes vendor designed CSR : uitb must be used. > > > > Signed-off-by: Ruinland Chuan-Tzu Tsai <ruinland@andestech.com> > > --- > > target/riscv/andes_codense.decode | 23 +++++ > > target/riscv/andes_custom_rv32.decode | 27 +++++ > > target/riscv/andes_custom_rv64.decode | 27 +++++ > > target/riscv/andes_helper.c | 49 +++++++++ > > target/riscv/andes_helper.h | 1 + > > target/riscv/cpu.c | 8 ++ > > target/riscv/helper.h | 2 + > > target/riscv/insn_trans/trans_andes.c.inc | 115 ++++++++++++++++++++++ > > target/riscv/meson.build | 13 +++ > > target/riscv/translate.c | 12 +++ > > 10 files changed, 277 insertions(+) > > create mode 100644 target/riscv/andes_codense.decode > > create mode 100644 target/riscv/andes_custom_rv32.decode > > create mode 100644 target/riscv/andes_custom_rv64.decode > > create mode 100644 target/riscv/andes_helper.c > > create mode 100644 target/riscv/andes_helper.h > > create mode 100644 target/riscv/insn_trans/trans_andes.c.inc > > > > diff --git a/target/riscv/andes_codense.decode b/target/riscv/andes_codense.decode > > new file mode 100644 > > index 0000000000..639d22ac67 > > --- /dev/null > > +++ b/target/riscv/andes_codense.decode > > @@ -0,0 +1,23 @@ > > +# > > +# RISC-V translation routines for the RVXI Base Integer Instruction Set. > > +# > > +# Copyright (c) 2018 Peer Adelt, peer.adelt@hni.uni-paderborn.de > > +# Bastian Koppelmann, kbastian@mail.uni-paderborn.de > > +# > > +# This program is free software; you can redistribute it and/or modify it > > +# under the terms and conditions of the GNU General Public License, > > +# version 2 or later, as published by the Free Software Foundation. > > +# > > +# This program is distributed in the hope it will be useful, but WITHOUT > > +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > > +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > > +# more details. > > +# > > +# You should have received a copy of the GNU General Public License along with > > +# this program. If not, see <http://www.gnu.org/licenses/>. > > + > > + > > +%imm_ex10 8:s1 12:1 3:1 9:1 5:2 2:1 10:2 4:1 > > +&codense imm_codense > > +@ex10 ... .... . . ..... .. &codense imm_codense=%imm_ex10 > > +execit 100 .... . 0 ..... 00 @ex10 > > It would probably be a bit clearer without the argument set and format, > which in this case are used exactly once, so there's no actual savings. > > execit 100 ..... 0 ..... 00 imm=%imm_ex10 > > > > +++ b/target/riscv/andes_custom_rv32.decode > ... > > +++ b/target/riscv/andes_custom_rv64.decode > > Note that we're moving toward a single riscv64 executable, and so these two > files should be combined. In this case, just taking the rv64 file will work > for RV32, with an extra check during translate. > > > +# Fields: > > +%rs1 15:5 > > +%rd 7:5 > > +## Similar to I-Type > > +%mbfob 26:6 > > +%lbfob 20:6 > > +&i_b mbfob lbfob rs1 rd > > +@i_bfo . ..... . ..... ..... ... ..... ....... &i_b %mbfob %lbfob %rs1 %rd > > Better as > > @i_bfo msb:6 lsb:6 rs1:5 ... rd:5 ....... &i_bfo > > There's quite a lot of riscv code that could be cleaned up like this. > > > +++ b/target/riscv/andes_helper.c > > @@ -0,0 +1,49 @@ > > +#include "qemu/osdep.h" > > All new files must have copyright boilerplate. > That said... I deeply apologize for that. Was way too fatigued last night. > > > +#include "cpu.h" > > +#include "qemu/host-utils.h" > > +#include "exec/exec-all.h" > > +#include "exec/helper-proto.h" > > +#include "fpu/softfloat.h" > > +#include "internals.h" > > +typedef int (*test_function)(uint8_t a, uint8_t b); > > +target_ulong helper_andes_v5_bfo_x(target_ulong rd, target_ulong rs1, > > + target_ulong insn) > > Never pass the instruction to a helper. This is an indication that you > should have done more work during translate. > > > --- /dev/null > > +++ b/target/riscv/insn_trans/trans_andes.c.inc > > @@ -0,0 +1,115 @@ > > +#define GP 3 > > Again, boilerplate. Wilco. > > > +#define ANDES_V5_CODE_DENSE_MASK (0xe083) > > +#define ANDES_V5_CODE_DENSE_ID(x) ((x)&ANDES_V5_CODE_DENSE_MASK) > > +#define ANDES_V5_CODE_DENSE_IMM11(x) ( \ > > + (extract32(x, 4, 1) << 2) | \ > > + (extract32(x, 10, 2) << 3) | \ > > + (extract32(x, 2, 1) << 5) | \ > > + (extract32(x, 5, 2) << 6) | \ > > + (extract32(x, 9, 1) << 8) | \ > > + (extract32(x, 3, 1) << 9) | \ > > + (extract32(x, 12, 1) << 10) | \ > > + (extract64(x, 8, 1) << 11) | \ > > + 0) > > + > > +#define ANDES_V5_GET_JAL_UIMM(inst) ((extract32(inst, 21, 10) << 1) \ > > + | (extract32(inst, 20, 1) << 11) \ > > + | (extract32(inst, 12, 8) << 12) \ > > + | (extract32(inst, 31, 1) << 20)) > > + > > + > > +enum andes_v5_inst_id > > +{ > > + /* Code Dense Extension */ > > + EXEC_IT = (0x8000), > > + /* custom 2 */ > > + BFOS = 0x03, > > + BFOZ = 0x02, > > +}; > > Left over from something else? > This all looks like something that should be handled by decodetree. Oops, sorry for that. This part was cutting out from our inhouse code. > > > +static bool trans_bfos(DisasContext *ctx, arg_bfos *a) > > +{ > > + TCGv v0, v1, v2; > > + v2 = tcg_const_tl(ctx->opcode); > > + v0 = get_gpr(ctx, a->rs1, EXT_NONE); > > + v1 = get_gpr(ctx, a->rd, EXT_NONE); > > + gen_helper_andes_v5_bfo_x(v1, v1, v0, v2); > > + gen_set_gpr(ctx, a->rd, v1); > > + tcg_temp_free(v2); > > + return true; > > +} > > + > > +static bool trans_bfoz(DisasContext *ctx, arg_bfoz *a) > > +{ > > + TCGv v0, v1, v2; > > + v2 = tcg_const_tl(ctx->opcode); > > + v0 = get_gpr(ctx, a->rs1, EXT_NONE); > > + v1 = get_gpr(ctx, a->rd, EXT_NONE); > > + gen_helper_andes_v5_bfo_x(v1, v1, v0, v2); > > + gen_set_gpr(ctx, a->rd, v1); > > + tcg_temp_free(v2); > > + return true; > > +} > > So, you've just passed off everything to the helper. Not good. > > First, these two instructions are so close we add a common helper. > > static bool do_bfox(DisasContext *ctx, arg_i_bfo *a, bool is_signed) > { > ... > } > > static bool trans_bfos(DisasContext *ctx, arg_i_bfo *a) > { > return do_bfox(ctx, a, true); > } > > static bool trans_bfoz(DisasContext *ctx, arg_i_bfo *a) > { > return do_bfox(ctx, a, false); > } > > Second, re the RV32/RV64 merge, range check the bits: > > if (a->msb >= get_xlen(ctx) || a->lsb >= get_xlen(ctx)) { > return false; > } Okay, that's new to me. > > Third, TCG can easily handle extract/deposit inline: > > dest = dest_gpr(ctx, a->rd); > src1 = get_gpr(ctx, a->rs1, EXT_NONE); > if (a->msb >= a->lsb) { > len = a->msb - a->lsb + 1; > if (is_signed) { > tcg_gen_sextract_tl(dest, src1, a->lsb, len); > } else { > tcg_gen_extract_tl(dest, src1, a->lsb, len); > } > } else { > len = a->lsb - a->msb + 1; > if (is_signed) { > tcg_gen_sextract_tl(dest, src1, 0, len); > } else { > tcg_gen_extract_tl(dest, src1, 0, len); > } > tcg_gen_shli_tl(dest, dest, a->msb); > } > gen_set_gpr(ctx, a->rd, dest); > > > > +static int andes_v5_gen_codense_exec_it(CPURISCVState *env, DisasContext *ctx, arg_execit *a) > > +{ > > + uint32_t insn; > > + uint32_t imm_ex10 = a->imm_codense; > > + target_ulong uitb_val = 0; > > + riscv_csrrw(env, 0x800, &uitb_val, 0, 0); > > This won't work -- you can't access env during translation. So all this > faff around passing env through HartState is for naught. Sorry, please elaborate me a little bit more. > > Having a look through the rest of this: > > > + > > + if (extract32(uitb_val, 0, 1)) { /* UTIB.HW == 1 */ > > + qemu_log_mask(LOG_GUEST_ERROR, "exec.it: UITB.HW == 1 is not supported by now!\n"); > > + gen_exception_illegal(ctx); > > + > > + uint32_t instruction_table[0]; > > + insn = instruction_table[imm_ex10 >> 2]; > > + > > + return false; > > + } else { /* UTIB.HW == 0 */ > > + target_ulong vaddr = (uitb_val & ~0x3) + (imm_ex10<<2); > > + insn = cpu_ldl_code(env, vaddr); > > + } > > + > > +/* Execute(insn) > > + * do as the replaced instruction, even exceptions, > > + * except ctx->pc_succ_insn value (2). > > + */ > > + uint32_t op = MASK_OP_MAJOR(insn); > > + if (op == OPC_RISC_JAL) { > > + /* implement this by hack imm */ > > + int rd = GET_RD(ctx->opcode); > > + target_long imm = ANDES_V5_GET_JAL_UIMM(ctx->opcode); > > + target_ulong next_pc = (ctx->base.pc_next >> 21 << 21) | imm; > > + imm = next_pc - ctx->base.pc_next; > > + gen_jal(ctx, rd, imm); > > + } else { > > + /* JARL done as SPEC already */ > > + /* presume ctx->pc_succ_insn not changed in any ISA extension */ > > + decode_opc_andes(env, ctx, insn); > > + } > > + > > + return true; > > +} > > This looks quite a lot like the S390X EXECUTE instruction. It's hard to > suggest exactly how to structure this, because I can't find documentation, > and thus the edge cases are unknown. Sorry for the inconvience, I'll try harder to make the spec to be publicily accessible. > > Because of the .HW check, I would expect all of this do be in a helper > function. The qemu_log_mask would be followed by riscv_raise_exception. The HW instruction table is a feature we still have no idea how to implement in QEMU, it was meant to be a security extension that rogue users couldn't copy out vendor kept code - - the only permission is execution, no read access on that region. > > As for the actual execute, I'd suggest following the lead of s390x and > utilize the cs_base field of cpu_get_tb_cpu_state to hold the opcode to be > executed. You'd load the opcode in the helper, install the proper state > into env, and end the current TranslationBlock. The next TranslationBlock > will find the opcode and decode it in the normal way. > > Have a look at target/s390x/tcg/mem_helper.c, HELPER(ex), where we load the > opcode and store state. Then translate.c, extract_insn, where we consume > the state. > > > r~ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH v1 2/2] Enable custom instruction suport for Andes A25 and AX25 CPU model 2021-10-22 8:48 ` Ruinland ChuanTzu Tsai @ 2021-10-22 11:52 ` Alex Bennée 2021-10-22 17:24 ` Richard Henderson 0 siblings, 1 reply; 8+ messages in thread From: Alex Bennée @ 2021-10-22 11:52 UTC (permalink / raw) To: Ruinland ChuanTzu Tsai Cc: ycliang, alankao, wangjunqiang, dylan, Richard Henderson, qemu-devel, alistair23, qemu-riscv, bmeng.cn Ruinland ChuanTzu Tsai <ruinland@andestech.com> writes: > On Thu, Oct 21, 2021 at 12:17:47PM -0700, Richard Henderson wrote: >> On 10/21/21 8:11 AM, Ruinland Chuan-Tzu Tsai wrote: >> > In this patch, we demonstrate how Andes Performance Extension(c) insn : >> > bfos and bfoz could be used with Andes CoDense : exec.it. >> > <snip> >> > +static int andes_v5_gen_codense_exec_it(CPURISCVState *env, DisasContext *ctx, arg_execit *a) >> > +{ >> > + uint32_t insn; >> > + uint32_t imm_ex10 = a->imm_codense; >> > + target_ulong uitb_val = 0; >> > + riscv_csrrw(env, 0x800, &uitb_val, 0, 0); >> >> This won't work -- you can't access env during translation. So all this >> faff around passing env through HartState is for naught. > > Sorry, please elaborate me a little bit more. When dealing with translation you need to separate the translation phase from the execution phase. CPUEnv is the current run time state of the vCPU so the value that might be stored in it when you do translation could very well be different when the translation runs (or runs again). The correct way to deal with this is by the use of translation flags. If for example the translation is only valid for a particular execution state you represent that in a translation flag which you compute in cpu_get_tb_cpu_state. This ensures that the translated block will only get looked up when you are in that translation state - if it's not you will generate a new block for the current state. See the section: https://qemu.readthedocs.io/en/latest/devel/tcg.html#cpu-state-optimisations of the developer guide. -- Alex Bennée ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH v1 2/2] Enable custom instruction suport for Andes A25 and AX25 CPU model 2021-10-22 11:52 ` Alex Bennée @ 2021-10-22 17:24 ` Richard Henderson 0 siblings, 0 replies; 8+ messages in thread From: Richard Henderson @ 2021-10-22 17:24 UTC (permalink / raw) To: Alex Bennée, Ruinland ChuanTzu Tsai Cc: qemu-riscv, alankao, wangjunqiang, dylan, qemu-devel, alistair23, ycliang, bmeng.cn On 10/22/21 4:52 AM, Alex Bennée wrote: > > Ruinland ChuanTzu Tsai <ruinland@andestech.com> writes: > >> On Thu, Oct 21, 2021 at 12:17:47PM -0700, Richard Henderson wrote: >>> On 10/21/21 8:11 AM, Ruinland Chuan-Tzu Tsai wrote: >>>> In this patch, we demonstrate how Andes Performance Extension(c) insn : >>>> bfos and bfoz could be used with Andes CoDense : exec.it. >>>> > <snip> >>>> +static int andes_v5_gen_codense_exec_it(CPURISCVState *env, DisasContext *ctx, arg_execit *a) >>>> +{ >>>> + uint32_t insn; >>>> + uint32_t imm_ex10 = a->imm_codense; >>>> + target_ulong uitb_val = 0; >>>> + riscv_csrrw(env, 0x800, &uitb_val, 0, 0); >>> >>> This won't work -- you can't access env during translation. So all this >>> faff around passing env through HartState is for naught. >> >> Sorry, please elaborate me a little bit more. > > When dealing with translation you need to separate the translation phase > from the execution phase. CPUEnv is the current run time state of the > vCPU so the value that might be stored in it when you do translation > could very well be different when the translation runs (or runs again). > > The correct way to deal with this is by the use of translation flags... Yes translation flags will be important for this, but first, the cpu state which you are reading here at translation time, which you are baking into this TranslationBlock forevermore, is (1) the uitb csr value and (2) the contents of the memory at the final address. Both of these things might vary in between translation of the exec_it instruction and execution of the TranslationBlock. At which point the translation you did earlier is wrong. Avoiding these kinds of mistakes is exactly why we have *not* provided env to you at this point in translation. Which is why all of the changes you did to recover env are also wrong. r~ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH v1 1/2] riscv: Add preliminary infra for custom instrcution handling 2021-10-21 15:11 [RFC PATCH v1 1/2] riscv: Add preliminary infra for custom instrcution handling Ruinland Chuan-Tzu Tsai 2021-10-21 15:11 ` [RFC PATCH v1 2/2] Enable custom instruction suport for Andes A25 and AX25 CPU model Ruinland Chuan-Tzu Tsai @ 2021-10-21 16:11 ` Richard Henderson 2021-10-22 8:41 ` Ruinland ChuanTzu Tsai 1 sibling, 1 reply; 8+ messages in thread From: Richard Henderson @ 2021-10-21 16:11 UTC (permalink / raw) To: Ruinland Chuan-Tzu Tsai, alistair23, wangjunqiang, bmeng.cn Cc: qemu-riscv, dylan, ycliang, qemu-devel, alankao On 10/21/21 8:11 AM, Ruinland Chuan-Tzu Tsai wrote: > -static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode) > +/* Custom insn related definitions/prototypes */ > +extern __thread bool cpu_has_custom_insns; > +/* > + * These 2 are for indication if decode fails. > + * We don't want to interfere the following regular extension decoding > + * We assume MTTCG is 1-1 mapping for now. > + */ > +__thread bool custom_c_insn_decoded; > +__thread bool custom_insn_decoded; > + > +extern bool (*custom_c_insn_handler)(DisasContext *, uint16_t); > +extern bool (*custom_insn_handler)(DisasContext *, uint32_t); > + > +static void try_decode_custom_insn(CPURISCVState *env, DisasContext *ctx, > + uint16_t opcode) This is way, way over-engineered. You seem to be trying to design something that can be plugged in, which the rest of QEMU knows nothing of. I think that's a mistake. Your custom cpu extensions should be treated as any other RISC-V extension, since it is in fact built in to QEMU. Begin with adding the "bool ext_andes" field in RISCVCPU. Propagate that into the DisasContext just like the other extensions. Changes to decode_opc should be quite simple: diff --git a/target/riscv/translate.c b/target/riscv/translate.c index 6d7fbca1fa..ea1e159259 100644 --- a/target/riscv/translate.c +++ b/target/riscv/translate.c @@ -473,13 +473,16 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode) { /* check for compressed insn */ if (extract16(opcode, 0, 2) != 3) { + ctx->pc_succ_insn = ctx->base.pc_next + 2; if (!has_ext(ctx, RVC)) { gen_exception_illegal(ctx); - } else { - ctx->pc_succ_insn = ctx->base.pc_next + 2; - if (!decode_insn16(ctx, opcode)) { - gen_exception_illegal(ctx); - } + return; + } + if (ctx->ext_andes && decode_andes16(ctx, opcode)) { + return; + } + if (!decode_insn16(ctx, opcode)) { + gen_exception_illegal(ctx); } } else { uint32_t opcode32 = opcode; @@ -487,6 +490,9 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode) translator_lduw(env, &ctx->base, ctx->base.pc_next + 2)); ctx->pc_succ_insn = ctx->base.pc_next + 4; + if (ctx->ext_andes && decode_andes32(ctx, opcode)) { + return; + } if (!decode_insn32(ctx, opcode32)) { gen_exception_illegal(ctx); } r~ ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC PATCH v1 1/2] riscv: Add preliminary infra for custom instrcution handling 2021-10-21 16:11 ` [RFC PATCH v1 1/2] riscv: Add preliminary infra for custom instrcution handling Richard Henderson @ 2021-10-22 8:41 ` Ruinland ChuanTzu Tsai 0 siblings, 0 replies; 8+ messages in thread From: Ruinland ChuanTzu Tsai @ 2021-10-22 8:41 UTC (permalink / raw) To: Richard Henderson Cc: ycliang, qemu-riscv, alankao, wangjunqiang, dylan, qemu-devel, alistair23, bmeng.cn On Thu, Oct 21, 2021 at 09:11:44AM -0700, Richard Henderson wrote: > On 10/21/21 8:11 AM, Ruinland Chuan-Tzu Tsai wrote: > > -static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode) > > +/* Custom insn related definitions/prototypes */ > > +extern __thread bool cpu_has_custom_insns; > > +/* > > + * These 2 are for indication if decode fails. > > + * We don't want to interfere the following regular extension decoding > > + * We assume MTTCG is 1-1 mapping for now. > > + */ > > +__thread bool custom_c_insn_decoded; > > +__thread bool custom_insn_decoded; > > + > > +extern bool (*custom_c_insn_handler)(DisasContext *, uint16_t); > > +extern bool (*custom_insn_handler)(DisasContext *, uint32_t); > > + > > +static void try_decode_custom_insn(CPURISCVState *env, DisasContext *ctx, > > + uint16_t opcode) > > > This is way, way over-engineered. > > You seem to be trying to design something that can be plugged in, which the > rest of QEMU knows nothing of. I think that's a mistake. Your custom cpu > extensions should be treated as any other RISC-V extension, since it is in > fact built in to QEMU. > > Begin with adding the "bool ext_andes" field in RISCVCPU. Propagate that > into the DisasContext just like the other extensions. Wilco, thanks for the tips. Cordially yours, Ruinland > > Changes to decode_opc should be quite simple: > > diff --git a/target/riscv/translate.c b/target/riscv/translate.c > index 6d7fbca1fa..ea1e159259 100644 > --- a/target/riscv/translate.c > +++ b/target/riscv/translate.c > @@ -473,13 +473,16 @@ static void decode_opc(CPURISCVState *env, > DisasContext *ctx, uint16_t opcode) > { > /* check for compressed insn */ > if (extract16(opcode, 0, 2) != 3) { > + ctx->pc_succ_insn = ctx->base.pc_next + 2; > if (!has_ext(ctx, RVC)) { > gen_exception_illegal(ctx); > - } else { > - ctx->pc_succ_insn = ctx->base.pc_next + 2; > - if (!decode_insn16(ctx, opcode)) { > - gen_exception_illegal(ctx); > - } > + return; > + } > + if (ctx->ext_andes && decode_andes16(ctx, opcode)) { > + return; > + } > + if (!decode_insn16(ctx, opcode)) { > + gen_exception_illegal(ctx); > } > } else { > uint32_t opcode32 = opcode; > @@ -487,6 +490,9 @@ static void decode_opc(CPURISCVState *env, DisasContext > *ctx, uint16_t opcode) > translator_lduw(env, &ctx->base, > ctx->base.pc_next + 2)); > ctx->pc_succ_insn = ctx->base.pc_next + 4; > + if (ctx->ext_andes && decode_andes32(ctx, opcode)) { > + return; > + } > if (!decode_insn32(ctx, opcode32)) { > gen_exception_illegal(ctx); > } > > > r~ ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-10-22 17:25 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-10-21 15:11 [RFC PATCH v1 1/2] riscv: Add preliminary infra for custom instrcution handling Ruinland Chuan-Tzu Tsai 2021-10-21 15:11 ` [RFC PATCH v1 2/2] Enable custom instruction suport for Andes A25 and AX25 CPU model Ruinland Chuan-Tzu Tsai 2021-10-21 19:17 ` Richard Henderson 2021-10-22 8:48 ` Ruinland ChuanTzu Tsai 2021-10-22 11:52 ` Alex Bennée 2021-10-22 17:24 ` Richard Henderson 2021-10-21 16:11 ` [RFC PATCH v1 1/2] riscv: Add preliminary infra for custom instrcution handling Richard Henderson 2021-10-22 8:41 ` Ruinland ChuanTzu Tsai
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).