* [PATCH v2 1/2] target/riscv: iterate over a table of decoders
@ 2022-01-13 20:20 ` Philipp Tomsich
0 siblings, 0 replies; 26+ messages in thread
From: Philipp Tomsich @ 2022-01-13 20:20 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-riscv, Bin Meng, Philipp Tomsich, Greg Favor,
Palmer Dabbelt, Alistair Francis, Kito Cheng
To split up the decoder into multiple functions (both to support
vendor-specific opcodes in separate files and to simplify maintenance
of orthogonal extensions), this changes decode_op to iterate over a
table of decoders predicated on guard functions.
This commit only adds the new structure and the table, allowing for
the easy addition of additional decoders in the future.
Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
---
Changes in v2:
- (new patch) iterate over a table of guarded decoder functions
target/riscv/translate.c | 38 ++++++++++++++++++++++++++++++++------
1 file changed, 32 insertions(+), 6 deletions(-)
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 615048ec87..2cbf9cbb6f 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -116,6 +116,12 @@ static inline bool has_ext(DisasContext *ctx, uint32_t ext)
return ctx->misa_ext & ext;
}
+static inline bool always_true_p(CPURISCVState *env __attribute__((__unused__)),
+ DisasContext *ctx __attribute__((__unused__)))
+{
+ return true;
+}
+
#ifdef TARGET_RISCV32
#define get_xl(ctx) MXL_RV32
#elif defined(CONFIG_USER_ONLY)
@@ -844,16 +850,28 @@ static uint32_t opcode_at(DisasContextBase *dcbase, target_ulong pc)
static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
{
- /* check for compressed insn */
+ /* If not handled, we'll raise an illegal instruction exception */
+ bool handled = false;
+
+ /*
+ * A table with predicate (i.e., guard) functions and decoder functions
+ * that are tested in-order until a decoder matches onto the opcode.
+ */
+ const struct {
+ bool (*guard_func)(CPURISCVState *, DisasContext *);
+ bool (*decode_func)(DisasContext *, uint32_t);
+ } decoders[] = {
+ { always_true_p, decode_insn32 },
+ };
+
+ /* Check for compressed insn */
if (extract16(opcode, 0, 2) != 3) {
if (!has_ext(ctx, RVC)) {
gen_exception_illegal(ctx);
} else {
ctx->opcode = opcode;
ctx->pc_succ_insn = ctx->base.pc_next + 2;
- if (!decode_insn16(ctx, opcode)) {
- gen_exception_illegal(ctx);
- }
+ handled = decode_insn16(ctx, opcode);
}
} else {
uint32_t opcode32 = opcode;
@@ -862,10 +880,18 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
ctx->base.pc_next + 2));
ctx->opcode = opcode32;
ctx->pc_succ_insn = ctx->base.pc_next + 4;
- if (!decode_insn32(ctx, opcode32)) {
- gen_exception_illegal(ctx);
+
+ for (size_t i = 0; i < ARRAY_SIZE(decoders); ++i) {
+ if (!decoders[i].guard_func(env, ctx))
+ continue;
+
+ if ((handled = decoders[i].decode_func(ctx, opcode32)))
+ break;
}
}
+
+ if (!handled)
+ gen_exception_illegal(ctx);
}
static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
--
2.33.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 1/2] target/riscv: iterate over a table of decoders
@ 2022-01-13 20:20 ` Philipp Tomsich
0 siblings, 0 replies; 26+ messages in thread
From: Philipp Tomsich @ 2022-01-13 20:20 UTC (permalink / raw)
To: qemu-devel
Cc: Greg Favor, Alistair Francis, Kito Cheng, Philipp Tomsich,
Bin Meng, Palmer Dabbelt, qemu-riscv
To split up the decoder into multiple functions (both to support
vendor-specific opcodes in separate files and to simplify maintenance
of orthogonal extensions), this changes decode_op to iterate over a
table of decoders predicated on guard functions.
This commit only adds the new structure and the table, allowing for
the easy addition of additional decoders in the future.
Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
---
Changes in v2:
- (new patch) iterate over a table of guarded decoder functions
target/riscv/translate.c | 38 ++++++++++++++++++++++++++++++++------
1 file changed, 32 insertions(+), 6 deletions(-)
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 615048ec87..2cbf9cbb6f 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -116,6 +116,12 @@ static inline bool has_ext(DisasContext *ctx, uint32_t ext)
return ctx->misa_ext & ext;
}
+static inline bool always_true_p(CPURISCVState *env __attribute__((__unused__)),
+ DisasContext *ctx __attribute__((__unused__)))
+{
+ return true;
+}
+
#ifdef TARGET_RISCV32
#define get_xl(ctx) MXL_RV32
#elif defined(CONFIG_USER_ONLY)
@@ -844,16 +850,28 @@ static uint32_t opcode_at(DisasContextBase *dcbase, target_ulong pc)
static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
{
- /* check for compressed insn */
+ /* If not handled, we'll raise an illegal instruction exception */
+ bool handled = false;
+
+ /*
+ * A table with predicate (i.e., guard) functions and decoder functions
+ * that are tested in-order until a decoder matches onto the opcode.
+ */
+ const struct {
+ bool (*guard_func)(CPURISCVState *, DisasContext *);
+ bool (*decode_func)(DisasContext *, uint32_t);
+ } decoders[] = {
+ { always_true_p, decode_insn32 },
+ };
+
+ /* Check for compressed insn */
if (extract16(opcode, 0, 2) != 3) {
if (!has_ext(ctx, RVC)) {
gen_exception_illegal(ctx);
} else {
ctx->opcode = opcode;
ctx->pc_succ_insn = ctx->base.pc_next + 2;
- if (!decode_insn16(ctx, opcode)) {
- gen_exception_illegal(ctx);
- }
+ handled = decode_insn16(ctx, opcode);
}
} else {
uint32_t opcode32 = opcode;
@@ -862,10 +880,18 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
ctx->base.pc_next + 2));
ctx->opcode = opcode32;
ctx->pc_succ_insn = ctx->base.pc_next + 4;
- if (!decode_insn32(ctx, opcode32)) {
- gen_exception_illegal(ctx);
+
+ for (size_t i = 0; i < ARRAY_SIZE(decoders); ++i) {
+ if (!decoders[i].guard_func(env, ctx))
+ continue;
+
+ if ((handled = decoders[i].decode_func(ctx, opcode32)))
+ break;
}
}
+
+ if (!handled)
+ gen_exception_illegal(ctx);
}
static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
--
2.33.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension
2022-01-13 20:20 ` Philipp Tomsich
@ 2022-01-13 20:20 ` Philipp Tomsich
-1 siblings, 0 replies; 26+ messages in thread
From: Philipp Tomsich @ 2022-01-13 20:20 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-riscv, Bin Meng, Philipp Tomsich, Greg Favor,
Palmer Dabbelt, Alistair Francis, Kito Cheng
This adds the decoder and translation for the XVentanaCondOps custom
extension (vendor-defined by Ventana Micro Systems), which is
documented at https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.0/ventana-custom-extensions-v1.0.0.pdf
This commit then also adds a guard-function (has_XVentanaCondOps_p)
and the decoder function to the table of decoders, enabling the
support for the XVentanaCondOps extension.
Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
---
Changes in v2:
- Split off decode table into XVentanaCondOps.decode
- Wire up XVentanaCondOps in the decoder-table
target/riscv/XVentanaCondOps.decode | 25 ++++++++++++
target/riscv/cpu.c | 3 ++
target/riscv/cpu.h | 3 ++
.../insn_trans/trans_xventanacondops.inc | 39 +++++++++++++++++++
target/riscv/meson.build | 1 +
target/riscv/translate.c | 13 +++++++
6 files changed, 84 insertions(+)
create mode 100644 target/riscv/XVentanaCondOps.decode
create mode 100644 target/riscv/insn_trans/trans_xventanacondops.inc
diff --git a/target/riscv/XVentanaCondOps.decode b/target/riscv/XVentanaCondOps.decode
new file mode 100644
index 0000000000..5aef7c3d72
--- /dev/null
+++ b/target/riscv/XVentanaCondOps.decode
@@ -0,0 +1,25 @@
+#
+# RISC-V translation routines for the XVentanaCondOps extension
+#
+# Copyright (c) 2022 Dr. Philipp Tomsich, philipp.tomsich@vrull.eu
+#
+# SPDX-License-Identifier: LGPL-2.1-or-later
+#
+# Reference: VTx-family custom instructions
+# Custom ISA extensions for Ventana Micro Systems RISC-V cores
+# (https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.0/ventana-custom-extensions-v1.0.0.pdf)
+
+# Fields
+%rs2 20:5
+%rs1 15:5
+%rd 7:5
+
+# Argument sets
+&r rd rs1 rs2 !extern
+
+# Formats
+@r ....... ..... ..... ... ..... ....... &r %rs2 %rs1 %rd
+
+# *** RV64 Custom-3 Extension ***
+vt_maskc 0000000 ..... ..... 110 ..... 1111011 @r
+vt_maskcn 0000000 ..... ..... 111 ..... 1111011 @r
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 9bc25d3055..fc8ab1dc2b 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -673,6 +673,9 @@ static Property riscv_cpu_properties[] = {
DEFINE_PROP_BOOL("zbc", RISCVCPU, cfg.ext_zbc, true),
DEFINE_PROP_BOOL("zbs", RISCVCPU, cfg.ext_zbs, true),
+ /* Vendor-specific custom extensions */
+ DEFINE_PROP_BOOL("xventanacondops", RISCVCPU, cfg.ext_XVentanaCondOps, false),
+
/* These are experimental so mark with 'x-' */
DEFINE_PROP_BOOL("x-j", RISCVCPU, cfg.ext_j, false),
/* ePMP 0.9.3 */
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 4d63086765..ffde94fd1a 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -330,6 +330,9 @@ struct RISCVCPU {
bool ext_zfh;
bool ext_zfhmin;
+ /* Vendor-specific custom extensions */
+ bool ext_XVentanaCondOps;
+
char *priv_spec;
char *user_spec;
char *bext_spec;
diff --git a/target/riscv/insn_trans/trans_xventanacondops.inc b/target/riscv/insn_trans/trans_xventanacondops.inc
new file mode 100644
index 0000000000..b8a5d031b5
--- /dev/null
+++ b/target/riscv/insn_trans/trans_xventanacondops.inc
@@ -0,0 +1,39 @@
+/*
+ * RISC-V translation routines for the XVentanaCondOps extension.
+ *
+ * Copyright (c) 2021-2022 VRULL GmbH.
+ *
+ * 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/>.
+ */
+
+static bool gen_condmask(DisasContext *ctx, arg_r *a, TCGCond cond)
+{
+ TCGv dest = dest_gpr(ctx, a->rd);
+ TCGv src1 = get_gpr(ctx, a->rs1, EXT_NONE);
+ TCGv src2 = get_gpr(ctx, a->rs2, EXT_NONE);
+
+ tcg_gen_movcond_tl(cond, dest, src2, ctx->zero, src1, ctx->zero);
+
+ gen_set_gpr(ctx, a->rd, dest);
+ return true;
+}
+
+static bool trans_vt_maskc(DisasContext *ctx, arg_r *a)
+{
+ return gen_condmask(ctx, a, TCG_COND_NE);
+}
+
+static bool trans_vt_maskcn(DisasContext *ctx, arg_r *a)
+{
+ return gen_condmask(ctx, a, TCG_COND_EQ);
+}
diff --git a/target/riscv/meson.build b/target/riscv/meson.build
index a32158da93..1f3a15398b 100644
--- a/target/riscv/meson.build
+++ b/target/riscv/meson.build
@@ -4,6 +4,7 @@ dir = meson.current_source_dir()
gen = [
decodetree.process('insn16.decode', extra_args: ['--static-decode=decode_insn16', '--insnwidth=16']),
decodetree.process('insn32.decode', extra_args: '--static-decode=decode_insn32'),
+ decodetree.process('XVentanaCondOps.decode', extra_args: '--static-decode=decode_XVentanaCodeOps'),
]
riscv_ss = ss.source_set()
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 2cbf9cbb6f..efdf8a7bdb 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -122,6 +122,15 @@ static inline bool always_true_p(CPURISCVState *env __attribute__((__unused__))
return true;
}
+#define MATERIALISE_EXT_PREDICATE(ext) \
+ static inline bool has_ ## ext ## _p(CPURISCVState *env, \
+ DisasContext *ctx __attribute__((__unused__))) \
+ { \
+ return RISCV_CPU(ctx->cs)->cfg.ext_ ## ext ; \
+ }
+
+MATERIALISE_EXT_PREDICATE(XVentanaCondOps);
+
#ifdef TARGET_RISCV32
#define get_xl(ctx) MXL_RV32
#elif defined(CONFIG_USER_ONLY)
@@ -844,9 +853,12 @@ static uint32_t opcode_at(DisasContextBase *dcbase, target_ulong pc)
#include "insn_trans/trans_rvb.c.inc"
#include "insn_trans/trans_rvzfh.c.inc"
#include "insn_trans/trans_privileged.c.inc"
+#include "insn_trans/trans_xventanacondops.inc"
/* Include the auto-generated decoder for 16 bit insn */
#include "decode-insn16.c.inc"
+/* Include decoders for factored-out extensions */
+#include "decode-XVentanaCondOps.c.inc"
static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
{
@@ -862,6 +874,7 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
bool (*decode_func)(DisasContext *, uint32_t);
} decoders[] = {
{ always_true_p, decode_insn32 },
+ { has_XVentanaCondOps_p, decode_XVentanaCodeOps },
};
/* Check for compressed insn */
--
2.33.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension
@ 2022-01-13 20:20 ` Philipp Tomsich
0 siblings, 0 replies; 26+ messages in thread
From: Philipp Tomsich @ 2022-01-13 20:20 UTC (permalink / raw)
To: qemu-devel
Cc: Greg Favor, Alistair Francis, Kito Cheng, Philipp Tomsich,
Bin Meng, Palmer Dabbelt, qemu-riscv
This adds the decoder and translation for the XVentanaCondOps custom
extension (vendor-defined by Ventana Micro Systems), which is
documented at https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.0/ventana-custom-extensions-v1.0.0.pdf
This commit then also adds a guard-function (has_XVentanaCondOps_p)
and the decoder function to the table of decoders, enabling the
support for the XVentanaCondOps extension.
Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
---
Changes in v2:
- Split off decode table into XVentanaCondOps.decode
- Wire up XVentanaCondOps in the decoder-table
target/riscv/XVentanaCondOps.decode | 25 ++++++++++++
target/riscv/cpu.c | 3 ++
target/riscv/cpu.h | 3 ++
.../insn_trans/trans_xventanacondops.inc | 39 +++++++++++++++++++
target/riscv/meson.build | 1 +
target/riscv/translate.c | 13 +++++++
6 files changed, 84 insertions(+)
create mode 100644 target/riscv/XVentanaCondOps.decode
create mode 100644 target/riscv/insn_trans/trans_xventanacondops.inc
diff --git a/target/riscv/XVentanaCondOps.decode b/target/riscv/XVentanaCondOps.decode
new file mode 100644
index 0000000000..5aef7c3d72
--- /dev/null
+++ b/target/riscv/XVentanaCondOps.decode
@@ -0,0 +1,25 @@
+#
+# RISC-V translation routines for the XVentanaCondOps extension
+#
+# Copyright (c) 2022 Dr. Philipp Tomsich, philipp.tomsich@vrull.eu
+#
+# SPDX-License-Identifier: LGPL-2.1-or-later
+#
+# Reference: VTx-family custom instructions
+# Custom ISA extensions for Ventana Micro Systems RISC-V cores
+# (https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.0/ventana-custom-extensions-v1.0.0.pdf)
+
+# Fields
+%rs2 20:5
+%rs1 15:5
+%rd 7:5
+
+# Argument sets
+&r rd rs1 rs2 !extern
+
+# Formats
+@r ....... ..... ..... ... ..... ....... &r %rs2 %rs1 %rd
+
+# *** RV64 Custom-3 Extension ***
+vt_maskc 0000000 ..... ..... 110 ..... 1111011 @r
+vt_maskcn 0000000 ..... ..... 111 ..... 1111011 @r
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 9bc25d3055..fc8ab1dc2b 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -673,6 +673,9 @@ static Property riscv_cpu_properties[] = {
DEFINE_PROP_BOOL("zbc", RISCVCPU, cfg.ext_zbc, true),
DEFINE_PROP_BOOL("zbs", RISCVCPU, cfg.ext_zbs, true),
+ /* Vendor-specific custom extensions */
+ DEFINE_PROP_BOOL("xventanacondops", RISCVCPU, cfg.ext_XVentanaCondOps, false),
+
/* These are experimental so mark with 'x-' */
DEFINE_PROP_BOOL("x-j", RISCVCPU, cfg.ext_j, false),
/* ePMP 0.9.3 */
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 4d63086765..ffde94fd1a 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -330,6 +330,9 @@ struct RISCVCPU {
bool ext_zfh;
bool ext_zfhmin;
+ /* Vendor-specific custom extensions */
+ bool ext_XVentanaCondOps;
+
char *priv_spec;
char *user_spec;
char *bext_spec;
diff --git a/target/riscv/insn_trans/trans_xventanacondops.inc b/target/riscv/insn_trans/trans_xventanacondops.inc
new file mode 100644
index 0000000000..b8a5d031b5
--- /dev/null
+++ b/target/riscv/insn_trans/trans_xventanacondops.inc
@@ -0,0 +1,39 @@
+/*
+ * RISC-V translation routines for the XVentanaCondOps extension.
+ *
+ * Copyright (c) 2021-2022 VRULL GmbH.
+ *
+ * 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/>.
+ */
+
+static bool gen_condmask(DisasContext *ctx, arg_r *a, TCGCond cond)
+{
+ TCGv dest = dest_gpr(ctx, a->rd);
+ TCGv src1 = get_gpr(ctx, a->rs1, EXT_NONE);
+ TCGv src2 = get_gpr(ctx, a->rs2, EXT_NONE);
+
+ tcg_gen_movcond_tl(cond, dest, src2, ctx->zero, src1, ctx->zero);
+
+ gen_set_gpr(ctx, a->rd, dest);
+ return true;
+}
+
+static bool trans_vt_maskc(DisasContext *ctx, arg_r *a)
+{
+ return gen_condmask(ctx, a, TCG_COND_NE);
+}
+
+static bool trans_vt_maskcn(DisasContext *ctx, arg_r *a)
+{
+ return gen_condmask(ctx, a, TCG_COND_EQ);
+}
diff --git a/target/riscv/meson.build b/target/riscv/meson.build
index a32158da93..1f3a15398b 100644
--- a/target/riscv/meson.build
+++ b/target/riscv/meson.build
@@ -4,6 +4,7 @@ dir = meson.current_source_dir()
gen = [
decodetree.process('insn16.decode', extra_args: ['--static-decode=decode_insn16', '--insnwidth=16']),
decodetree.process('insn32.decode', extra_args: '--static-decode=decode_insn32'),
+ decodetree.process('XVentanaCondOps.decode', extra_args: '--static-decode=decode_XVentanaCodeOps'),
]
riscv_ss = ss.source_set()
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 2cbf9cbb6f..efdf8a7bdb 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -122,6 +122,15 @@ static inline bool always_true_p(CPURISCVState *env __attribute__((__unused__))
return true;
}
+#define MATERIALISE_EXT_PREDICATE(ext) \
+ static inline bool has_ ## ext ## _p(CPURISCVState *env, \
+ DisasContext *ctx __attribute__((__unused__))) \
+ { \
+ return RISCV_CPU(ctx->cs)->cfg.ext_ ## ext ; \
+ }
+
+MATERIALISE_EXT_PREDICATE(XVentanaCondOps);
+
#ifdef TARGET_RISCV32
#define get_xl(ctx) MXL_RV32
#elif defined(CONFIG_USER_ONLY)
@@ -844,9 +853,12 @@ static uint32_t opcode_at(DisasContextBase *dcbase, target_ulong pc)
#include "insn_trans/trans_rvb.c.inc"
#include "insn_trans/trans_rvzfh.c.inc"
#include "insn_trans/trans_privileged.c.inc"
+#include "insn_trans/trans_xventanacondops.inc"
/* Include the auto-generated decoder for 16 bit insn */
#include "decode-insn16.c.inc"
+/* Include decoders for factored-out extensions */
+#include "decode-XVentanaCondOps.c.inc"
static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
{
@@ -862,6 +874,7 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
bool (*decode_func)(DisasContext *, uint32_t);
} decoders[] = {
{ always_true_p, decode_insn32 },
+ { has_XVentanaCondOps_p, decode_XVentanaCodeOps },
};
/* Check for compressed insn */
--
2.33.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension
2022-01-13 20:20 ` Philipp Tomsich
@ 2022-01-18 22:53 ` Alistair Francis
-1 siblings, 0 replies; 26+ messages in thread
From: Alistair Francis @ 2022-01-18 22:53 UTC (permalink / raw)
To: Philipp Tomsich
Cc: open list:RISC-V, Bin Meng, qemu-devel@nongnu.org Developers,
Greg Favor, Palmer Dabbelt, Alistair Francis, Kito Cheng
On Fri, Jan 14, 2022 at 6:22 AM Philipp Tomsich
<philipp.tomsich@vrull.eu> wrote:
>
> This adds the decoder and translation for the XVentanaCondOps custom
> extension (vendor-defined by Ventana Micro Systems), which is
> documented at https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.0/ventana-custom-extensions-v1.0.0.pdf
>
> This commit then also adds a guard-function (has_XVentanaCondOps_p)
> and the decoder function to the table of decoders, enabling the
> support for the XVentanaCondOps extension.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
This looks reasonable to me.
I'm going to leave this for a bit in case there are any more comments.
I was a little worried that taking vendor extensions isn't the right
move, as we might get stuck with a large number of them. But this is
pretty self contained and I think with the growing RISC-V interest
it's something we will eventually need to support.
I'm going to update the QEMU RISC-V wiki page with this to make the
position clear (comments very welcome)
=== RISC-V Extensions ===
As RISC-V has a range of possible extensions, QEMU has guidelines for
supporting them all.
If an extension is frozen or ratified by the RISC-V foundation, it can
be supported in QEMU.
If an official RISC-V foundation extension is in a reasonable draft
state, that is not too many changes are still expected, it can be
supported experimentally by QEMU. Experimental support means it must
be disabled by default and marked with a "x-" in the properties. QEMU
will only support the latest version of patches submitted for a draft
extension. A draft extension can also be removed at any time if it
conflicts with other extensions.
QEMU will also support vendor extensions. Vendor extensions must be
disabled by default, but can be enabled for specific vendor CPUs and
boards. Vendor extensions must be maintained and tested by the vendor.
Vendor extensions can not interfere with other extensions and can not
be obtrusive to the RISC-V target code.
Alistair
>
> ---
>
> Changes in v2:
> - Split off decode table into XVentanaCondOps.decode
> - Wire up XVentanaCondOps in the decoder-table
>
> target/riscv/XVentanaCondOps.decode | 25 ++++++++++++
> target/riscv/cpu.c | 3 ++
> target/riscv/cpu.h | 3 ++
> .../insn_trans/trans_xventanacondops.inc | 39 +++++++++++++++++++
> target/riscv/meson.build | 1 +
> target/riscv/translate.c | 13 +++++++
> 6 files changed, 84 insertions(+)
> create mode 100644 target/riscv/XVentanaCondOps.decode
> create mode 100644 target/riscv/insn_trans/trans_xventanacondops.inc
>
> diff --git a/target/riscv/XVentanaCondOps.decode b/target/riscv/XVentanaCondOps.decode
> new file mode 100644
> index 0000000000..5aef7c3d72
> --- /dev/null
> +++ b/target/riscv/XVentanaCondOps.decode
> @@ -0,0 +1,25 @@
> +#
> +# RISC-V translation routines for the XVentanaCondOps extension
> +#
> +# Copyright (c) 2022 Dr. Philipp Tomsich, philipp.tomsich@vrull.eu
> +#
> +# SPDX-License-Identifier: LGPL-2.1-or-later
> +#
> +# Reference: VTx-family custom instructions
> +# Custom ISA extensions for Ventana Micro Systems RISC-V cores
> +# (https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.0/ventana-custom-extensions-v1.0.0.pdf)
> +
> +# Fields
> +%rs2 20:5
> +%rs1 15:5
> +%rd 7:5
> +
> +# Argument sets
> +&r rd rs1 rs2 !extern
> +
> +# Formats
> +@r ....... ..... ..... ... ..... ....... &r %rs2 %rs1 %rd
> +
> +# *** RV64 Custom-3 Extension ***
> +vt_maskc 0000000 ..... ..... 110 ..... 1111011 @r
> +vt_maskcn 0000000 ..... ..... 111 ..... 1111011 @r
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 9bc25d3055..fc8ab1dc2b 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -673,6 +673,9 @@ static Property riscv_cpu_properties[] = {
> DEFINE_PROP_BOOL("zbc", RISCVCPU, cfg.ext_zbc, true),
> DEFINE_PROP_BOOL("zbs", RISCVCPU, cfg.ext_zbs, true),
>
> + /* Vendor-specific custom extensions */
> + DEFINE_PROP_BOOL("xventanacondops", RISCVCPU, cfg.ext_XVentanaCondOps, false),
> +
> /* These are experimental so mark with 'x-' */
> DEFINE_PROP_BOOL("x-j", RISCVCPU, cfg.ext_j, false),
> /* ePMP 0.9.3 */
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 4d63086765..ffde94fd1a 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -330,6 +330,9 @@ struct RISCVCPU {
> bool ext_zfh;
> bool ext_zfhmin;
>
> + /* Vendor-specific custom extensions */
> + bool ext_XVentanaCondOps;
> +
> char *priv_spec;
> char *user_spec;
> char *bext_spec;
> diff --git a/target/riscv/insn_trans/trans_xventanacondops.inc b/target/riscv/insn_trans/trans_xventanacondops.inc
> new file mode 100644
> index 0000000000..b8a5d031b5
> --- /dev/null
> +++ b/target/riscv/insn_trans/trans_xventanacondops.inc
> @@ -0,0 +1,39 @@
> +/*
> + * RISC-V translation routines for the XVentanaCondOps extension.
> + *
> + * Copyright (c) 2021-2022 VRULL GmbH.
> + *
> + * 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/>.
> + */
> +
> +static bool gen_condmask(DisasContext *ctx, arg_r *a, TCGCond cond)
> +{
> + TCGv dest = dest_gpr(ctx, a->rd);
> + TCGv src1 = get_gpr(ctx, a->rs1, EXT_NONE);
> + TCGv src2 = get_gpr(ctx, a->rs2, EXT_NONE);
> +
> + tcg_gen_movcond_tl(cond, dest, src2, ctx->zero, src1, ctx->zero);
> +
> + gen_set_gpr(ctx, a->rd, dest);
> + return true;
> +}
> +
> +static bool trans_vt_maskc(DisasContext *ctx, arg_r *a)
> +{
> + return gen_condmask(ctx, a, TCG_COND_NE);
> +}
> +
> +static bool trans_vt_maskcn(DisasContext *ctx, arg_r *a)
> +{
> + return gen_condmask(ctx, a, TCG_COND_EQ);
> +}
> diff --git a/target/riscv/meson.build b/target/riscv/meson.build
> index a32158da93..1f3a15398b 100644
> --- a/target/riscv/meson.build
> +++ b/target/riscv/meson.build
> @@ -4,6 +4,7 @@ dir = meson.current_source_dir()
> gen = [
> decodetree.process('insn16.decode', extra_args: ['--static-decode=decode_insn16', '--insnwidth=16']),
> decodetree.process('insn32.decode', extra_args: '--static-decode=decode_insn32'),
> + decodetree.process('XVentanaCondOps.decode', extra_args: '--static-decode=decode_XVentanaCodeOps'),
> ]
>
> riscv_ss = ss.source_set()
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index 2cbf9cbb6f..efdf8a7bdb 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -122,6 +122,15 @@ static inline bool always_true_p(CPURISCVState *env __attribute__((__unused__))
> return true;
> }
>
> +#define MATERIALISE_EXT_PREDICATE(ext) \
> + static inline bool has_ ## ext ## _p(CPURISCVState *env, \
> + DisasContext *ctx __attribute__((__unused__))) \
> + { \
> + return RISCV_CPU(ctx->cs)->cfg.ext_ ## ext ; \
> + }
> +
> +MATERIALISE_EXT_PREDICATE(XVentanaCondOps);
> +
> #ifdef TARGET_RISCV32
> #define get_xl(ctx) MXL_RV32
> #elif defined(CONFIG_USER_ONLY)
> @@ -844,9 +853,12 @@ static uint32_t opcode_at(DisasContextBase *dcbase, target_ulong pc)
> #include "insn_trans/trans_rvb.c.inc"
> #include "insn_trans/trans_rvzfh.c.inc"
> #include "insn_trans/trans_privileged.c.inc"
> +#include "insn_trans/trans_xventanacondops.inc"
>
> /* Include the auto-generated decoder for 16 bit insn */
> #include "decode-insn16.c.inc"
> +/* Include decoders for factored-out extensions */
> +#include "decode-XVentanaCondOps.c.inc"
>
> static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
> {
> @@ -862,6 +874,7 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
> bool (*decode_func)(DisasContext *, uint32_t);
> } decoders[] = {
> { always_true_p, decode_insn32 },
> + { has_XVentanaCondOps_p, decode_XVentanaCodeOps },
> };
>
> /* Check for compressed insn */
> --
> 2.33.1
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension
@ 2022-01-18 22:53 ` Alistair Francis
0 siblings, 0 replies; 26+ messages in thread
From: Alistair Francis @ 2022-01-18 22:53 UTC (permalink / raw)
To: Philipp Tomsich
Cc: qemu-devel@nongnu.org Developers, open list:RISC-V, Bin Meng,
Greg Favor, Palmer Dabbelt, Alistair Francis, Kito Cheng
On Fri, Jan 14, 2022 at 6:22 AM Philipp Tomsich
<philipp.tomsich@vrull.eu> wrote:
>
> This adds the decoder and translation for the XVentanaCondOps custom
> extension (vendor-defined by Ventana Micro Systems), which is
> documented at https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.0/ventana-custom-extensions-v1.0.0.pdf
>
> This commit then also adds a guard-function (has_XVentanaCondOps_p)
> and the decoder function to the table of decoders, enabling the
> support for the XVentanaCondOps extension.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
This looks reasonable to me.
I'm going to leave this for a bit in case there are any more comments.
I was a little worried that taking vendor extensions isn't the right
move, as we might get stuck with a large number of them. But this is
pretty self contained and I think with the growing RISC-V interest
it's something we will eventually need to support.
I'm going to update the QEMU RISC-V wiki page with this to make the
position clear (comments very welcome)
=== RISC-V Extensions ===
As RISC-V has a range of possible extensions, QEMU has guidelines for
supporting them all.
If an extension is frozen or ratified by the RISC-V foundation, it can
be supported in QEMU.
If an official RISC-V foundation extension is in a reasonable draft
state, that is not too many changes are still expected, it can be
supported experimentally by QEMU. Experimental support means it must
be disabled by default and marked with a "x-" in the properties. QEMU
will only support the latest version of patches submitted for a draft
extension. A draft extension can also be removed at any time if it
conflicts with other extensions.
QEMU will also support vendor extensions. Vendor extensions must be
disabled by default, but can be enabled for specific vendor CPUs and
boards. Vendor extensions must be maintained and tested by the vendor.
Vendor extensions can not interfere with other extensions and can not
be obtrusive to the RISC-V target code.
Alistair
>
> ---
>
> Changes in v2:
> - Split off decode table into XVentanaCondOps.decode
> - Wire up XVentanaCondOps in the decoder-table
>
> target/riscv/XVentanaCondOps.decode | 25 ++++++++++++
> target/riscv/cpu.c | 3 ++
> target/riscv/cpu.h | 3 ++
> .../insn_trans/trans_xventanacondops.inc | 39 +++++++++++++++++++
> target/riscv/meson.build | 1 +
> target/riscv/translate.c | 13 +++++++
> 6 files changed, 84 insertions(+)
> create mode 100644 target/riscv/XVentanaCondOps.decode
> create mode 100644 target/riscv/insn_trans/trans_xventanacondops.inc
>
> diff --git a/target/riscv/XVentanaCondOps.decode b/target/riscv/XVentanaCondOps.decode
> new file mode 100644
> index 0000000000..5aef7c3d72
> --- /dev/null
> +++ b/target/riscv/XVentanaCondOps.decode
> @@ -0,0 +1,25 @@
> +#
> +# RISC-V translation routines for the XVentanaCondOps extension
> +#
> +# Copyright (c) 2022 Dr. Philipp Tomsich, philipp.tomsich@vrull.eu
> +#
> +# SPDX-License-Identifier: LGPL-2.1-or-later
> +#
> +# Reference: VTx-family custom instructions
> +# Custom ISA extensions for Ventana Micro Systems RISC-V cores
> +# (https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.0/ventana-custom-extensions-v1.0.0.pdf)
> +
> +# Fields
> +%rs2 20:5
> +%rs1 15:5
> +%rd 7:5
> +
> +# Argument sets
> +&r rd rs1 rs2 !extern
> +
> +# Formats
> +@r ....... ..... ..... ... ..... ....... &r %rs2 %rs1 %rd
> +
> +# *** RV64 Custom-3 Extension ***
> +vt_maskc 0000000 ..... ..... 110 ..... 1111011 @r
> +vt_maskcn 0000000 ..... ..... 111 ..... 1111011 @r
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 9bc25d3055..fc8ab1dc2b 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -673,6 +673,9 @@ static Property riscv_cpu_properties[] = {
> DEFINE_PROP_BOOL("zbc", RISCVCPU, cfg.ext_zbc, true),
> DEFINE_PROP_BOOL("zbs", RISCVCPU, cfg.ext_zbs, true),
>
> + /* Vendor-specific custom extensions */
> + DEFINE_PROP_BOOL("xventanacondops", RISCVCPU, cfg.ext_XVentanaCondOps, false),
> +
> /* These are experimental so mark with 'x-' */
> DEFINE_PROP_BOOL("x-j", RISCVCPU, cfg.ext_j, false),
> /* ePMP 0.9.3 */
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 4d63086765..ffde94fd1a 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -330,6 +330,9 @@ struct RISCVCPU {
> bool ext_zfh;
> bool ext_zfhmin;
>
> + /* Vendor-specific custom extensions */
> + bool ext_XVentanaCondOps;
> +
> char *priv_spec;
> char *user_spec;
> char *bext_spec;
> diff --git a/target/riscv/insn_trans/trans_xventanacondops.inc b/target/riscv/insn_trans/trans_xventanacondops.inc
> new file mode 100644
> index 0000000000..b8a5d031b5
> --- /dev/null
> +++ b/target/riscv/insn_trans/trans_xventanacondops.inc
> @@ -0,0 +1,39 @@
> +/*
> + * RISC-V translation routines for the XVentanaCondOps extension.
> + *
> + * Copyright (c) 2021-2022 VRULL GmbH.
> + *
> + * 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/>.
> + */
> +
> +static bool gen_condmask(DisasContext *ctx, arg_r *a, TCGCond cond)
> +{
> + TCGv dest = dest_gpr(ctx, a->rd);
> + TCGv src1 = get_gpr(ctx, a->rs1, EXT_NONE);
> + TCGv src2 = get_gpr(ctx, a->rs2, EXT_NONE);
> +
> + tcg_gen_movcond_tl(cond, dest, src2, ctx->zero, src1, ctx->zero);
> +
> + gen_set_gpr(ctx, a->rd, dest);
> + return true;
> +}
> +
> +static bool trans_vt_maskc(DisasContext *ctx, arg_r *a)
> +{
> + return gen_condmask(ctx, a, TCG_COND_NE);
> +}
> +
> +static bool trans_vt_maskcn(DisasContext *ctx, arg_r *a)
> +{
> + return gen_condmask(ctx, a, TCG_COND_EQ);
> +}
> diff --git a/target/riscv/meson.build b/target/riscv/meson.build
> index a32158da93..1f3a15398b 100644
> --- a/target/riscv/meson.build
> +++ b/target/riscv/meson.build
> @@ -4,6 +4,7 @@ dir = meson.current_source_dir()
> gen = [
> decodetree.process('insn16.decode', extra_args: ['--static-decode=decode_insn16', '--insnwidth=16']),
> decodetree.process('insn32.decode', extra_args: '--static-decode=decode_insn32'),
> + decodetree.process('XVentanaCondOps.decode', extra_args: '--static-decode=decode_XVentanaCodeOps'),
> ]
>
> riscv_ss = ss.source_set()
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index 2cbf9cbb6f..efdf8a7bdb 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -122,6 +122,15 @@ static inline bool always_true_p(CPURISCVState *env __attribute__((__unused__))
> return true;
> }
>
> +#define MATERIALISE_EXT_PREDICATE(ext) \
> + static inline bool has_ ## ext ## _p(CPURISCVState *env, \
> + DisasContext *ctx __attribute__((__unused__))) \
> + { \
> + return RISCV_CPU(ctx->cs)->cfg.ext_ ## ext ; \
> + }
> +
> +MATERIALISE_EXT_PREDICATE(XVentanaCondOps);
> +
> #ifdef TARGET_RISCV32
> #define get_xl(ctx) MXL_RV32
> #elif defined(CONFIG_USER_ONLY)
> @@ -844,9 +853,12 @@ static uint32_t opcode_at(DisasContextBase *dcbase, target_ulong pc)
> #include "insn_trans/trans_rvb.c.inc"
> #include "insn_trans/trans_rvzfh.c.inc"
> #include "insn_trans/trans_privileged.c.inc"
> +#include "insn_trans/trans_xventanacondops.inc"
>
> /* Include the auto-generated decoder for 16 bit insn */
> #include "decode-insn16.c.inc"
> +/* Include decoders for factored-out extensions */
> +#include "decode-XVentanaCondOps.c.inc"
>
> static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
> {
> @@ -862,6 +874,7 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
> bool (*decode_func)(DisasContext *, uint32_t);
> } decoders[] = {
> { always_true_p, decode_insn32 },
> + { has_XVentanaCondOps_p, decode_XVentanaCodeOps },
> };
>
> /* Check for compressed insn */
> --
> 2.33.1
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension
2022-01-18 22:53 ` Alistair Francis
@ 2022-01-18 23:21 ` Philipp Tomsich
-1 siblings, 0 replies; 26+ messages in thread
From: Philipp Tomsich @ 2022-01-18 23:21 UTC (permalink / raw)
To: Alistair Francis
Cc: open list:RISC-V, Bin Meng, qemu-devel@nongnu.org Developers,
Greg Favor, Palmer Dabbelt, Alistair Francis, Kito Cheng
Alistair,
Some of us (the merit almost exclusively goes to Kito) have been
working towards a similar policy for GCC/binutils and LLVM.
This currently lives in:
https://github.com/riscv-non-isa/riscv-toolchain-conventions/pull/17
A few comments & a question below.
Thanks,
Philipp.
On Tue, 18 Jan 2022 at 23:53, Alistair Francis <alistair23@gmail.com> wrote:
>
> On Fri, Jan 14, 2022 at 6:22 AM Philipp Tomsich
> <philipp.tomsich@vrull.eu> wrote:
> >
> > This adds the decoder and translation for the XVentanaCondOps custom
> > extension (vendor-defined by Ventana Micro Systems), which is
> > documented at https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.0/ventana-custom-extensions-v1.0.0.pdf
> >
> > This commit then also adds a guard-function (has_XVentanaCondOps_p)
> > and the decoder function to the table of decoders, enabling the
> > support for the XVentanaCondOps extension.
> >
> > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
>
> This looks reasonable to me.
>
> I'm going to leave this for a bit in case there are any more comments.
>
> I was a little worried that taking vendor extensions isn't the right
> move, as we might get stuck with a large number of them. But this is
> pretty self contained and I think with the growing RISC-V interest
> it's something we will eventually need to support.
>
> I'm going to update the QEMU RISC-V wiki page with this to make the
> position clear (comments very welcome)
>
> === RISC-V Extensions ===
> As RISC-V has a range of possible extensions, QEMU has guidelines for
> supporting them all.
>
> If an extension is frozen or ratified by the RISC-V foundation, it can
> be supported in QEMU.
>
> If an official RISC-V foundation extension is in a reasonable draft
> state, that is not too many changes are still expected, it can be
> supported experimentally by QEMU. Experimental support means it must
> be disabled by default and marked with a "x-" in the properties. QEMU
> will only support the latest version of patches submitted for a draft
> extension. A draft extension can also be removed at any time if it
> conflicts with other extensions.
>
> QEMU will also support vendor extensions. Vendor extensions must be
> disabled by default, but can be enabled for specific vendor CPUs and
> boards. Vendor extensions must be maintained and tested by the vendor.
I guess I should create a v3 with appropriate paths in the MAINTAINERS file?
> Vendor extensions can not interfere with other extensions and can not
> be obtrusive to the RISC-V target code.
I know that there is some interest to have the XtheadV (the
instructions previously known as vectors 0.7.1-draft) supported and we
have the reality of a deployed base that implements it in hardware.
This would conflict with the opcode space used by the standard RISC-V
vectors, so it makes for an interesting test case (even if just to
clarify our intent)...
Personally, I would like to avoid precluding inclusion of something
useful (of course, "Vendor extensions must be maintained and tested by
the vendor." has to apply!), if a vendor was going to step up and also
offers to maintain it.
So let's assume such a (very significant) addition were factored out
similarly, interfacing just through a hook in decode_op and
argument-parsing logic that ensures that the conflicting
standard-extension is turned off: would this still be acceptable under
this policy — or would it trip the "obtrusive" condition?
>
> Alistair
>
> >
> > ---
> >
> > Changes in v2:
> > - Split off decode table into XVentanaCondOps.decode
> > - Wire up XVentanaCondOps in the decoder-table
> >
> > target/riscv/XVentanaCondOps.decode | 25 ++++++++++++
> > target/riscv/cpu.c | 3 ++
> > target/riscv/cpu.h | 3 ++
> > .../insn_trans/trans_xventanacondops.inc | 39 +++++++++++++++++++
> > target/riscv/meson.build | 1 +
> > target/riscv/translate.c | 13 +++++++
> > 6 files changed, 84 insertions(+)
> > create mode 100644 target/riscv/XVentanaCondOps.decode
> > create mode 100644 target/riscv/insn_trans/trans_xventanacondops.inc
> >
> > diff --git a/target/riscv/XVentanaCondOps.decode b/target/riscv/XVentanaCondOps.decode
> > new file mode 100644
> > index 0000000000..5aef7c3d72
> > --- /dev/null
> > +++ b/target/riscv/XVentanaCondOps.decode
> > @@ -0,0 +1,25 @@
> > +#
> > +# RISC-V translation routines for the XVentanaCondOps extension
> > +#
> > +# Copyright (c) 2022 Dr. Philipp Tomsich, philipp.tomsich@vrull.eu
> > +#
> > +# SPDX-License-Identifier: LGPL-2.1-or-later
> > +#
> > +# Reference: VTx-family custom instructions
> > +# Custom ISA extensions for Ventana Micro Systems RISC-V cores
> > +# (https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.0/ventana-custom-extensions-v1.0.0.pdf)
> > +
> > +# Fields
> > +%rs2 20:5
> > +%rs1 15:5
> > +%rd 7:5
> > +
> > +# Argument sets
> > +&r rd rs1 rs2 !extern
> > +
> > +# Formats
> > +@r ....... ..... ..... ... ..... ....... &r %rs2 %rs1 %rd
> > +
> > +# *** RV64 Custom-3 Extension ***
> > +vt_maskc 0000000 ..... ..... 110 ..... 1111011 @r
> > +vt_maskcn 0000000 ..... ..... 111 ..... 1111011 @r
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 9bc25d3055..fc8ab1dc2b 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -673,6 +673,9 @@ static Property riscv_cpu_properties[] = {
> > DEFINE_PROP_BOOL("zbc", RISCVCPU, cfg.ext_zbc, true),
> > DEFINE_PROP_BOOL("zbs", RISCVCPU, cfg.ext_zbs, true),
> >
> > + /* Vendor-specific custom extensions */
> > + DEFINE_PROP_BOOL("xventanacondops", RISCVCPU, cfg.ext_XVentanaCondOps, false),
> > +
> > /* These are experimental so mark with 'x-' */
> > DEFINE_PROP_BOOL("x-j", RISCVCPU, cfg.ext_j, false),
> > /* ePMP 0.9.3 */
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 4d63086765..ffde94fd1a 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -330,6 +330,9 @@ struct RISCVCPU {
> > bool ext_zfh;
> > bool ext_zfhmin;
> >
> > + /* Vendor-specific custom extensions */
> > + bool ext_XVentanaCondOps;
> > +
> > char *priv_spec;
> > char *user_spec;
> > char *bext_spec;
> > diff --git a/target/riscv/insn_trans/trans_xventanacondops.inc b/target/riscv/insn_trans/trans_xventanacondops.inc
> > new file mode 100644
> > index 0000000000..b8a5d031b5
> > --- /dev/null
> > +++ b/target/riscv/insn_trans/trans_xventanacondops.inc
> > @@ -0,0 +1,39 @@
> > +/*
> > + * RISC-V translation routines for the XVentanaCondOps extension.
> > + *
> > + * Copyright (c) 2021-2022 VRULL GmbH.
> > + *
> > + * 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/>.
> > + */
> > +
> > +static bool gen_condmask(DisasContext *ctx, arg_r *a, TCGCond cond)
> > +{
> > + TCGv dest = dest_gpr(ctx, a->rd);
> > + TCGv src1 = get_gpr(ctx, a->rs1, EXT_NONE);
> > + TCGv src2 = get_gpr(ctx, a->rs2, EXT_NONE);
> > +
> > + tcg_gen_movcond_tl(cond, dest, src2, ctx->zero, src1, ctx->zero);
> > +
> > + gen_set_gpr(ctx, a->rd, dest);
> > + return true;
> > +}
> > +
> > +static bool trans_vt_maskc(DisasContext *ctx, arg_r *a)
> > +{
> > + return gen_condmask(ctx, a, TCG_COND_NE);
> > +}
> > +
> > +static bool trans_vt_maskcn(DisasContext *ctx, arg_r *a)
> > +{
> > + return gen_condmask(ctx, a, TCG_COND_EQ);
> > +}
> > diff --git a/target/riscv/meson.build b/target/riscv/meson.build
> > index a32158da93..1f3a15398b 100644
> > --- a/target/riscv/meson.build
> > +++ b/target/riscv/meson.build
> > @@ -4,6 +4,7 @@ dir = meson.current_source_dir()
> > gen = [
> > decodetree.process('insn16.decode', extra_args: ['--static-decode=decode_insn16', '--insnwidth=16']),
> > decodetree.process('insn32.decode', extra_args: '--static-decode=decode_insn32'),
> > + decodetree.process('XVentanaCondOps.decode', extra_args: '--static-decode=decode_XVentanaCodeOps'),
> > ]
> >
> > riscv_ss = ss.source_set()
> > diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> > index 2cbf9cbb6f..efdf8a7bdb 100644
> > --- a/target/riscv/translate.c
> > +++ b/target/riscv/translate.c
> > @@ -122,6 +122,15 @@ static inline bool always_true_p(CPURISCVState *env __attribute__((__unused__))
> > return true;
> > }
> >
> > +#define MATERIALISE_EXT_PREDICATE(ext) \
> > + static inline bool has_ ## ext ## _p(CPURISCVState *env, \
> > + DisasContext *ctx __attribute__((__unused__))) \
> > + { \
> > + return RISCV_CPU(ctx->cs)->cfg.ext_ ## ext ; \
> > + }
> > +
> > +MATERIALISE_EXT_PREDICATE(XVentanaCondOps);
> > +
> > #ifdef TARGET_RISCV32
> > #define get_xl(ctx) MXL_RV32
> > #elif defined(CONFIG_USER_ONLY)
> > @@ -844,9 +853,12 @@ static uint32_t opcode_at(DisasContextBase *dcbase, target_ulong pc)
> > #include "insn_trans/trans_rvb.c.inc"
> > #include "insn_trans/trans_rvzfh.c.inc"
> > #include "insn_trans/trans_privileged.c.inc"
> > +#include "insn_trans/trans_xventanacondops.inc"
> >
> > /* Include the auto-generated decoder for 16 bit insn */
> > #include "decode-insn16.c.inc"
> > +/* Include decoders for factored-out extensions */
> > +#include "decode-XVentanaCondOps.c.inc"
> >
> > static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
> > {
> > @@ -862,6 +874,7 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
> > bool (*decode_func)(DisasContext *, uint32_t);
> > } decoders[] = {
> > { always_true_p, decode_insn32 },
> > + { has_XVentanaCondOps_p, decode_XVentanaCodeOps },
> > };
> >
> > /* Check for compressed insn */
> > --
> > 2.33.1
> >
> >
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension
@ 2022-01-18 23:21 ` Philipp Tomsich
0 siblings, 0 replies; 26+ messages in thread
From: Philipp Tomsich @ 2022-01-18 23:21 UTC (permalink / raw)
To: Alistair Francis
Cc: qemu-devel@nongnu.org Developers, open list:RISC-V, Bin Meng,
Greg Favor, Palmer Dabbelt, Alistair Francis, Kito Cheng
Alistair,
Some of us (the merit almost exclusively goes to Kito) have been
working towards a similar policy for GCC/binutils and LLVM.
This currently lives in:
https://github.com/riscv-non-isa/riscv-toolchain-conventions/pull/17
A few comments & a question below.
Thanks,
Philipp.
On Tue, 18 Jan 2022 at 23:53, Alistair Francis <alistair23@gmail.com> wrote:
>
> On Fri, Jan 14, 2022 at 6:22 AM Philipp Tomsich
> <philipp.tomsich@vrull.eu> wrote:
> >
> > This adds the decoder and translation for the XVentanaCondOps custom
> > extension (vendor-defined by Ventana Micro Systems), which is
> > documented at https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.0/ventana-custom-extensions-v1.0.0.pdf
> >
> > This commit then also adds a guard-function (has_XVentanaCondOps_p)
> > and the decoder function to the table of decoders, enabling the
> > support for the XVentanaCondOps extension.
> >
> > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
>
> This looks reasonable to me.
>
> I'm going to leave this for a bit in case there are any more comments.
>
> I was a little worried that taking vendor extensions isn't the right
> move, as we might get stuck with a large number of them. But this is
> pretty self contained and I think with the growing RISC-V interest
> it's something we will eventually need to support.
>
> I'm going to update the QEMU RISC-V wiki page with this to make the
> position clear (comments very welcome)
>
> === RISC-V Extensions ===
> As RISC-V has a range of possible extensions, QEMU has guidelines for
> supporting them all.
>
> If an extension is frozen or ratified by the RISC-V foundation, it can
> be supported in QEMU.
>
> If an official RISC-V foundation extension is in a reasonable draft
> state, that is not too many changes are still expected, it can be
> supported experimentally by QEMU. Experimental support means it must
> be disabled by default and marked with a "x-" in the properties. QEMU
> will only support the latest version of patches submitted for a draft
> extension. A draft extension can also be removed at any time if it
> conflicts with other extensions.
>
> QEMU will also support vendor extensions. Vendor extensions must be
> disabled by default, but can be enabled for specific vendor CPUs and
> boards. Vendor extensions must be maintained and tested by the vendor.
I guess I should create a v3 with appropriate paths in the MAINTAINERS file?
> Vendor extensions can not interfere with other extensions and can not
> be obtrusive to the RISC-V target code.
I know that there is some interest to have the XtheadV (the
instructions previously known as vectors 0.7.1-draft) supported and we
have the reality of a deployed base that implements it in hardware.
This would conflict with the opcode space used by the standard RISC-V
vectors, so it makes for an interesting test case (even if just to
clarify our intent)...
Personally, I would like to avoid precluding inclusion of something
useful (of course, "Vendor extensions must be maintained and tested by
the vendor." has to apply!), if a vendor was going to step up and also
offers to maintain it.
So let's assume such a (very significant) addition were factored out
similarly, interfacing just through a hook in decode_op and
argument-parsing logic that ensures that the conflicting
standard-extension is turned off: would this still be acceptable under
this policy — or would it trip the "obtrusive" condition?
>
> Alistair
>
> >
> > ---
> >
> > Changes in v2:
> > - Split off decode table into XVentanaCondOps.decode
> > - Wire up XVentanaCondOps in the decoder-table
> >
> > target/riscv/XVentanaCondOps.decode | 25 ++++++++++++
> > target/riscv/cpu.c | 3 ++
> > target/riscv/cpu.h | 3 ++
> > .../insn_trans/trans_xventanacondops.inc | 39 +++++++++++++++++++
> > target/riscv/meson.build | 1 +
> > target/riscv/translate.c | 13 +++++++
> > 6 files changed, 84 insertions(+)
> > create mode 100644 target/riscv/XVentanaCondOps.decode
> > create mode 100644 target/riscv/insn_trans/trans_xventanacondops.inc
> >
> > diff --git a/target/riscv/XVentanaCondOps.decode b/target/riscv/XVentanaCondOps.decode
> > new file mode 100644
> > index 0000000000..5aef7c3d72
> > --- /dev/null
> > +++ b/target/riscv/XVentanaCondOps.decode
> > @@ -0,0 +1,25 @@
> > +#
> > +# RISC-V translation routines for the XVentanaCondOps extension
> > +#
> > +# Copyright (c) 2022 Dr. Philipp Tomsich, philipp.tomsich@vrull.eu
> > +#
> > +# SPDX-License-Identifier: LGPL-2.1-or-later
> > +#
> > +# Reference: VTx-family custom instructions
> > +# Custom ISA extensions for Ventana Micro Systems RISC-V cores
> > +# (https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.0/ventana-custom-extensions-v1.0.0.pdf)
> > +
> > +# Fields
> > +%rs2 20:5
> > +%rs1 15:5
> > +%rd 7:5
> > +
> > +# Argument sets
> > +&r rd rs1 rs2 !extern
> > +
> > +# Formats
> > +@r ....... ..... ..... ... ..... ....... &r %rs2 %rs1 %rd
> > +
> > +# *** RV64 Custom-3 Extension ***
> > +vt_maskc 0000000 ..... ..... 110 ..... 1111011 @r
> > +vt_maskcn 0000000 ..... ..... 111 ..... 1111011 @r
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 9bc25d3055..fc8ab1dc2b 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -673,6 +673,9 @@ static Property riscv_cpu_properties[] = {
> > DEFINE_PROP_BOOL("zbc", RISCVCPU, cfg.ext_zbc, true),
> > DEFINE_PROP_BOOL("zbs", RISCVCPU, cfg.ext_zbs, true),
> >
> > + /* Vendor-specific custom extensions */
> > + DEFINE_PROP_BOOL("xventanacondops", RISCVCPU, cfg.ext_XVentanaCondOps, false),
> > +
> > /* These are experimental so mark with 'x-' */
> > DEFINE_PROP_BOOL("x-j", RISCVCPU, cfg.ext_j, false),
> > /* ePMP 0.9.3 */
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 4d63086765..ffde94fd1a 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -330,6 +330,9 @@ struct RISCVCPU {
> > bool ext_zfh;
> > bool ext_zfhmin;
> >
> > + /* Vendor-specific custom extensions */
> > + bool ext_XVentanaCondOps;
> > +
> > char *priv_spec;
> > char *user_spec;
> > char *bext_spec;
> > diff --git a/target/riscv/insn_trans/trans_xventanacondops.inc b/target/riscv/insn_trans/trans_xventanacondops.inc
> > new file mode 100644
> > index 0000000000..b8a5d031b5
> > --- /dev/null
> > +++ b/target/riscv/insn_trans/trans_xventanacondops.inc
> > @@ -0,0 +1,39 @@
> > +/*
> > + * RISC-V translation routines for the XVentanaCondOps extension.
> > + *
> > + * Copyright (c) 2021-2022 VRULL GmbH.
> > + *
> > + * 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/>.
> > + */
> > +
> > +static bool gen_condmask(DisasContext *ctx, arg_r *a, TCGCond cond)
> > +{
> > + TCGv dest = dest_gpr(ctx, a->rd);
> > + TCGv src1 = get_gpr(ctx, a->rs1, EXT_NONE);
> > + TCGv src2 = get_gpr(ctx, a->rs2, EXT_NONE);
> > +
> > + tcg_gen_movcond_tl(cond, dest, src2, ctx->zero, src1, ctx->zero);
> > +
> > + gen_set_gpr(ctx, a->rd, dest);
> > + return true;
> > +}
> > +
> > +static bool trans_vt_maskc(DisasContext *ctx, arg_r *a)
> > +{
> > + return gen_condmask(ctx, a, TCG_COND_NE);
> > +}
> > +
> > +static bool trans_vt_maskcn(DisasContext *ctx, arg_r *a)
> > +{
> > + return gen_condmask(ctx, a, TCG_COND_EQ);
> > +}
> > diff --git a/target/riscv/meson.build b/target/riscv/meson.build
> > index a32158da93..1f3a15398b 100644
> > --- a/target/riscv/meson.build
> > +++ b/target/riscv/meson.build
> > @@ -4,6 +4,7 @@ dir = meson.current_source_dir()
> > gen = [
> > decodetree.process('insn16.decode', extra_args: ['--static-decode=decode_insn16', '--insnwidth=16']),
> > decodetree.process('insn32.decode', extra_args: '--static-decode=decode_insn32'),
> > + decodetree.process('XVentanaCondOps.decode', extra_args: '--static-decode=decode_XVentanaCodeOps'),
> > ]
> >
> > riscv_ss = ss.source_set()
> > diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> > index 2cbf9cbb6f..efdf8a7bdb 100644
> > --- a/target/riscv/translate.c
> > +++ b/target/riscv/translate.c
> > @@ -122,6 +122,15 @@ static inline bool always_true_p(CPURISCVState *env __attribute__((__unused__))
> > return true;
> > }
> >
> > +#define MATERIALISE_EXT_PREDICATE(ext) \
> > + static inline bool has_ ## ext ## _p(CPURISCVState *env, \
> > + DisasContext *ctx __attribute__((__unused__))) \
> > + { \
> > + return RISCV_CPU(ctx->cs)->cfg.ext_ ## ext ; \
> > + }
> > +
> > +MATERIALISE_EXT_PREDICATE(XVentanaCondOps);
> > +
> > #ifdef TARGET_RISCV32
> > #define get_xl(ctx) MXL_RV32
> > #elif defined(CONFIG_USER_ONLY)
> > @@ -844,9 +853,12 @@ static uint32_t opcode_at(DisasContextBase *dcbase, target_ulong pc)
> > #include "insn_trans/trans_rvb.c.inc"
> > #include "insn_trans/trans_rvzfh.c.inc"
> > #include "insn_trans/trans_privileged.c.inc"
> > +#include "insn_trans/trans_xventanacondops.inc"
> >
> > /* Include the auto-generated decoder for 16 bit insn */
> > #include "decode-insn16.c.inc"
> > +/* Include decoders for factored-out extensions */
> > +#include "decode-XVentanaCondOps.c.inc"
> >
> > static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
> > {
> > @@ -862,6 +874,7 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
> > bool (*decode_func)(DisasContext *, uint32_t);
> > } decoders[] = {
> > { always_true_p, decode_insn32 },
> > + { has_XVentanaCondOps_p, decode_XVentanaCodeOps },
> > };
> >
> > /* Check for compressed insn */
> > --
> > 2.33.1
> >
> >
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension
2022-01-18 23:21 ` Philipp Tomsich
@ 2022-01-19 1:19 ` Alistair Francis
-1 siblings, 0 replies; 26+ messages in thread
From: Alistair Francis @ 2022-01-19 1:19 UTC (permalink / raw)
To: Philipp Tomsich
Cc: open list:RISC-V, Bin Meng, qemu-devel@nongnu.org Developers,
Greg Favor, Palmer Dabbelt, Alistair Francis, Kito Cheng
On Wed, Jan 19, 2022 at 9:22 AM Philipp Tomsich
<philipp.tomsich@vrull.eu> wrote:
>
> Alistair,
>
> Some of us (the merit almost exclusively goes to Kito) have been
> working towards a similar policy for GCC/binutils and LLVM.
> This currently lives in:
> https://github.com/riscv-non-isa/riscv-toolchain-conventions/pull/17
Ah cool! We can use that as a good starting point.
>
> A few comments & a question below.
>
> Thanks,
> Philipp.
>
> On Tue, 18 Jan 2022 at 23:53, Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Fri, Jan 14, 2022 at 6:22 AM Philipp Tomsich
> > <philipp.tomsich@vrull.eu> wrote:
> > >
> > > This adds the decoder and translation for the XVentanaCondOps custom
> > > extension (vendor-defined by Ventana Micro Systems), which is
> > > documented at https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.0/ventana-custom-extensions-v1.0.0.pdf
> > >
> > > This commit then also adds a guard-function (has_XVentanaCondOps_p)
> > > and the decoder function to the table of decoders, enabling the
> > > support for the XVentanaCondOps extension.
> > >
> > > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> >
> > This looks reasonable to me.
> >
> > I'm going to leave this for a bit in case there are any more comments.
> >
> > I was a little worried that taking vendor extensions isn't the right
> > move, as we might get stuck with a large number of them. But this is
> > pretty self contained and I think with the growing RISC-V interest
> > it's something we will eventually need to support.
> >
> > I'm going to update the QEMU RISC-V wiki page with this to make the
> > position clear (comments very welcome)
> >
> > === RISC-V Extensions ===
> > As RISC-V has a range of possible extensions, QEMU has guidelines for
> > supporting them all.
> >
> > If an extension is frozen or ratified by the RISC-V foundation, it can
> > be supported in QEMU.
> >
> > If an official RISC-V foundation extension is in a reasonable draft
> > state, that is not too many changes are still expected, it can be
> > supported experimentally by QEMU. Experimental support means it must
> > be disabled by default and marked with a "x-" in the properties. QEMU
> > will only support the latest version of patches submitted for a draft
> > extension. A draft extension can also be removed at any time if it
> > conflicts with other extensions.
> >
> > QEMU will also support vendor extensions. Vendor extensions must be
> > disabled by default, but can be enabled for specific vendor CPUs and
> > boards. Vendor extensions must be maintained and tested by the vendor.
>
> I guess I should create a v3 with appropriate paths in the MAINTAINERS file?
Hmm... Good point. I don't think you have to if you don't want to.
My point here was more to just make it clear that upstream QEMU is not
a dumping ground for vendor extensions to get them maintained by
someone else. Obviously we won't purposely break things just for fun.
There is an expectation that the vendor tests their extensions and
responds to bug reports and things like that.
>
> > Vendor extensions can not interfere with other extensions and can not
> > be obtrusive to the RISC-V target code.
>
> I know that there is some interest to have the XtheadV (the
> instructions previously known as vectors 0.7.1-draft) supported and we
> have the reality of a deployed base that implements it in hardware.
> This would conflict with the opcode space used by the standard RISC-V
> vectors, so it makes for an interesting test case (even if just to
> clarify our intent)...
> Personally, I would like to avoid precluding inclusion of something
> useful (of course, "Vendor extensions must be maintained and tested by
> the vendor." has to apply!), if a vendor was going to step up and also
> offers to maintain it.
Yeah... this is unfortunate. I agree that having the 0.7.1-draft
extensions supported would be great. There is hardware that supports
it.
I think this point still stands though. IF the XtheadV implementation
is self contained and doesn't interfere with the vector extensions,
then that's great and we can support it. If instead it adds a large
amount of conditionals to the released vector extension code then I
don't think we can take it.
There is some wiggle room, but the RISC-V tree already has enough
going on and very little reviewers. If in the future we get more
reviewers and testers we can re-evaulate what is acceptable, but for
now I think we need to be a little strict. (Hint to any companies to
give developers time to review)
>
> So let's assume such a (very significant) addition were factored out
> similarly, interfacing just through a hook in decode_op and
> argument-parsing logic that ensures that the conflicting
> standard-extension is turned off: would this still be acceptable under
> this policy — or would it trip the "obtrusive" condition?
I think that would be acceptable, I wouldn't say that is obtrusive as
it's self contained.
Alistair
>
> >
> > Alistair
> >
> > >
> > > ---
> > >
> > > Changes in v2:
> > > - Split off decode table into XVentanaCondOps.decode
> > > - Wire up XVentanaCondOps in the decoder-table
> > >
> > > target/riscv/XVentanaCondOps.decode | 25 ++++++++++++
> > > target/riscv/cpu.c | 3 ++
> > > target/riscv/cpu.h | 3 ++
> > > .../insn_trans/trans_xventanacondops.inc | 39 +++++++++++++++++++
> > > target/riscv/meson.build | 1 +
> > > target/riscv/translate.c | 13 +++++++
> > > 6 files changed, 84 insertions(+)
> > > create mode 100644 target/riscv/XVentanaCondOps.decode
> > > create mode 100644 target/riscv/insn_trans/trans_xventanacondops.inc
> > >
> > > diff --git a/target/riscv/XVentanaCondOps.decode b/target/riscv/XVentanaCondOps.decode
> > > new file mode 100644
> > > index 0000000000..5aef7c3d72
> > > --- /dev/null
> > > +++ b/target/riscv/XVentanaCondOps.decode
> > > @@ -0,0 +1,25 @@
> > > +#
> > > +# RISC-V translation routines for the XVentanaCondOps extension
> > > +#
> > > +# Copyright (c) 2022 Dr. Philipp Tomsich, philipp.tomsich@vrull.eu
> > > +#
> > > +# SPDX-License-Identifier: LGPL-2.1-or-later
> > > +#
> > > +# Reference: VTx-family custom instructions
> > > +# Custom ISA extensions for Ventana Micro Systems RISC-V cores
> > > +# (https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.0/ventana-custom-extensions-v1.0.0.pdf)
> > > +
> > > +# Fields
> > > +%rs2 20:5
> > > +%rs1 15:5
> > > +%rd 7:5
> > > +
> > > +# Argument sets
> > > +&r rd rs1 rs2 !extern
> > > +
> > > +# Formats
> > > +@r ....... ..... ..... ... ..... ....... &r %rs2 %rs1 %rd
> > > +
> > > +# *** RV64 Custom-3 Extension ***
> > > +vt_maskc 0000000 ..... ..... 110 ..... 1111011 @r
> > > +vt_maskcn 0000000 ..... ..... 111 ..... 1111011 @r
> > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > index 9bc25d3055..fc8ab1dc2b 100644
> > > --- a/target/riscv/cpu.c
> > > +++ b/target/riscv/cpu.c
> > > @@ -673,6 +673,9 @@ static Property riscv_cpu_properties[] = {
> > > DEFINE_PROP_BOOL("zbc", RISCVCPU, cfg.ext_zbc, true),
> > > DEFINE_PROP_BOOL("zbs", RISCVCPU, cfg.ext_zbs, true),
> > >
> > > + /* Vendor-specific custom extensions */
> > > + DEFINE_PROP_BOOL("xventanacondops", RISCVCPU, cfg.ext_XVentanaCondOps, false),
> > > +
> > > /* These are experimental so mark with 'x-' */
> > > DEFINE_PROP_BOOL("x-j", RISCVCPU, cfg.ext_j, false),
> > > /* ePMP 0.9.3 */
> > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > > index 4d63086765..ffde94fd1a 100644
> > > --- a/target/riscv/cpu.h
> > > +++ b/target/riscv/cpu.h
> > > @@ -330,6 +330,9 @@ struct RISCVCPU {
> > > bool ext_zfh;
> > > bool ext_zfhmin;
> > >
> > > + /* Vendor-specific custom extensions */
> > > + bool ext_XVentanaCondOps;
> > > +
> > > char *priv_spec;
> > > char *user_spec;
> > > char *bext_spec;
> > > diff --git a/target/riscv/insn_trans/trans_xventanacondops.inc b/target/riscv/insn_trans/trans_xventanacondops.inc
> > > new file mode 100644
> > > index 0000000000..b8a5d031b5
> > > --- /dev/null
> > > +++ b/target/riscv/insn_trans/trans_xventanacondops.inc
> > > @@ -0,0 +1,39 @@
> > > +/*
> > > + * RISC-V translation routines for the XVentanaCondOps extension.
> > > + *
> > > + * Copyright (c) 2021-2022 VRULL GmbH.
> > > + *
> > > + * 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/>.
> > > + */
> > > +
> > > +static bool gen_condmask(DisasContext *ctx, arg_r *a, TCGCond cond)
> > > +{
> > > + TCGv dest = dest_gpr(ctx, a->rd);
> > > + TCGv src1 = get_gpr(ctx, a->rs1, EXT_NONE);
> > > + TCGv src2 = get_gpr(ctx, a->rs2, EXT_NONE);
> > > +
> > > + tcg_gen_movcond_tl(cond, dest, src2, ctx->zero, src1, ctx->zero);
> > > +
> > > + gen_set_gpr(ctx, a->rd, dest);
> > > + return true;
> > > +}
> > > +
> > > +static bool trans_vt_maskc(DisasContext *ctx, arg_r *a)
> > > +{
> > > + return gen_condmask(ctx, a, TCG_COND_NE);
> > > +}
> > > +
> > > +static bool trans_vt_maskcn(DisasContext *ctx, arg_r *a)
> > > +{
> > > + return gen_condmask(ctx, a, TCG_COND_EQ);
> > > +}
> > > diff --git a/target/riscv/meson.build b/target/riscv/meson.build
> > > index a32158da93..1f3a15398b 100644
> > > --- a/target/riscv/meson.build
> > > +++ b/target/riscv/meson.build
> > > @@ -4,6 +4,7 @@ dir = meson.current_source_dir()
> > > gen = [
> > > decodetree.process('insn16.decode', extra_args: ['--static-decode=decode_insn16', '--insnwidth=16']),
> > > decodetree.process('insn32.decode', extra_args: '--static-decode=decode_insn32'),
> > > + decodetree.process('XVentanaCondOps.decode', extra_args: '--static-decode=decode_XVentanaCodeOps'),
> > > ]
> > >
> > > riscv_ss = ss.source_set()
> > > diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> > > index 2cbf9cbb6f..efdf8a7bdb 100644
> > > --- a/target/riscv/translate.c
> > > +++ b/target/riscv/translate.c
> > > @@ -122,6 +122,15 @@ static inline bool always_true_p(CPURISCVState *env __attribute__((__unused__))
> > > return true;
> > > }
> > >
> > > +#define MATERIALISE_EXT_PREDICATE(ext) \
> > > + static inline bool has_ ## ext ## _p(CPURISCVState *env, \
> > > + DisasContext *ctx __attribute__((__unused__))) \
> > > + { \
> > > + return RISCV_CPU(ctx->cs)->cfg.ext_ ## ext ; \
> > > + }
> > > +
> > > +MATERIALISE_EXT_PREDICATE(XVentanaCondOps);
> > > +
> > > #ifdef TARGET_RISCV32
> > > #define get_xl(ctx) MXL_RV32
> > > #elif defined(CONFIG_USER_ONLY)
> > > @@ -844,9 +853,12 @@ static uint32_t opcode_at(DisasContextBase *dcbase, target_ulong pc)
> > > #include "insn_trans/trans_rvb.c.inc"
> > > #include "insn_trans/trans_rvzfh.c.inc"
> > > #include "insn_trans/trans_privileged.c.inc"
> > > +#include "insn_trans/trans_xventanacondops.inc"
> > >
> > > /* Include the auto-generated decoder for 16 bit insn */
> > > #include "decode-insn16.c.inc"
> > > +/* Include decoders for factored-out extensions */
> > > +#include "decode-XVentanaCondOps.c.inc"
> > >
> > > static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
> > > {
> > > @@ -862,6 +874,7 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
> > > bool (*decode_func)(DisasContext *, uint32_t);
> > > } decoders[] = {
> > > { always_true_p, decode_insn32 },
> > > + { has_XVentanaCondOps_p, decode_XVentanaCodeOps },
> > > };
> > >
> > > /* Check for compressed insn */
> > > --
> > > 2.33.1
> > >
> > >
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension
@ 2022-01-19 1:19 ` Alistair Francis
0 siblings, 0 replies; 26+ messages in thread
From: Alistair Francis @ 2022-01-19 1:19 UTC (permalink / raw)
To: Philipp Tomsich
Cc: qemu-devel@nongnu.org Developers, open list:RISC-V, Bin Meng,
Greg Favor, Palmer Dabbelt, Alistair Francis, Kito Cheng
On Wed, Jan 19, 2022 at 9:22 AM Philipp Tomsich
<philipp.tomsich@vrull.eu> wrote:
>
> Alistair,
>
> Some of us (the merit almost exclusively goes to Kito) have been
> working towards a similar policy for GCC/binutils and LLVM.
> This currently lives in:
> https://github.com/riscv-non-isa/riscv-toolchain-conventions/pull/17
Ah cool! We can use that as a good starting point.
>
> A few comments & a question below.
>
> Thanks,
> Philipp.
>
> On Tue, 18 Jan 2022 at 23:53, Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Fri, Jan 14, 2022 at 6:22 AM Philipp Tomsich
> > <philipp.tomsich@vrull.eu> wrote:
> > >
> > > This adds the decoder and translation for the XVentanaCondOps custom
> > > extension (vendor-defined by Ventana Micro Systems), which is
> > > documented at https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.0/ventana-custom-extensions-v1.0.0.pdf
> > >
> > > This commit then also adds a guard-function (has_XVentanaCondOps_p)
> > > and the decoder function to the table of decoders, enabling the
> > > support for the XVentanaCondOps extension.
> > >
> > > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> >
> > This looks reasonable to me.
> >
> > I'm going to leave this for a bit in case there are any more comments.
> >
> > I was a little worried that taking vendor extensions isn't the right
> > move, as we might get stuck with a large number of them. But this is
> > pretty self contained and I think with the growing RISC-V interest
> > it's something we will eventually need to support.
> >
> > I'm going to update the QEMU RISC-V wiki page with this to make the
> > position clear (comments very welcome)
> >
> > === RISC-V Extensions ===
> > As RISC-V has a range of possible extensions, QEMU has guidelines for
> > supporting them all.
> >
> > If an extension is frozen or ratified by the RISC-V foundation, it can
> > be supported in QEMU.
> >
> > If an official RISC-V foundation extension is in a reasonable draft
> > state, that is not too many changes are still expected, it can be
> > supported experimentally by QEMU. Experimental support means it must
> > be disabled by default and marked with a "x-" in the properties. QEMU
> > will only support the latest version of patches submitted for a draft
> > extension. A draft extension can also be removed at any time if it
> > conflicts with other extensions.
> >
> > QEMU will also support vendor extensions. Vendor extensions must be
> > disabled by default, but can be enabled for specific vendor CPUs and
> > boards. Vendor extensions must be maintained and tested by the vendor.
>
> I guess I should create a v3 with appropriate paths in the MAINTAINERS file?
Hmm... Good point. I don't think you have to if you don't want to.
My point here was more to just make it clear that upstream QEMU is not
a dumping ground for vendor extensions to get them maintained by
someone else. Obviously we won't purposely break things just for fun.
There is an expectation that the vendor tests their extensions and
responds to bug reports and things like that.
>
> > Vendor extensions can not interfere with other extensions and can not
> > be obtrusive to the RISC-V target code.
>
> I know that there is some interest to have the XtheadV (the
> instructions previously known as vectors 0.7.1-draft) supported and we
> have the reality of a deployed base that implements it in hardware.
> This would conflict with the opcode space used by the standard RISC-V
> vectors, so it makes for an interesting test case (even if just to
> clarify our intent)...
> Personally, I would like to avoid precluding inclusion of something
> useful (of course, "Vendor extensions must be maintained and tested by
> the vendor." has to apply!), if a vendor was going to step up and also
> offers to maintain it.
Yeah... this is unfortunate. I agree that having the 0.7.1-draft
extensions supported would be great. There is hardware that supports
it.
I think this point still stands though. IF the XtheadV implementation
is self contained and doesn't interfere with the vector extensions,
then that's great and we can support it. If instead it adds a large
amount of conditionals to the released vector extension code then I
don't think we can take it.
There is some wiggle room, but the RISC-V tree already has enough
going on and very little reviewers. If in the future we get more
reviewers and testers we can re-evaulate what is acceptable, but for
now I think we need to be a little strict. (Hint to any companies to
give developers time to review)
>
> So let's assume such a (very significant) addition were factored out
> similarly, interfacing just through a hook in decode_op and
> argument-parsing logic that ensures that the conflicting
> standard-extension is turned off: would this still be acceptable under
> this policy — or would it trip the "obtrusive" condition?
I think that would be acceptable, I wouldn't say that is obtrusive as
it's self contained.
Alistair
>
> >
> > Alistair
> >
> > >
> > > ---
> > >
> > > Changes in v2:
> > > - Split off decode table into XVentanaCondOps.decode
> > > - Wire up XVentanaCondOps in the decoder-table
> > >
> > > target/riscv/XVentanaCondOps.decode | 25 ++++++++++++
> > > target/riscv/cpu.c | 3 ++
> > > target/riscv/cpu.h | 3 ++
> > > .../insn_trans/trans_xventanacondops.inc | 39 +++++++++++++++++++
> > > target/riscv/meson.build | 1 +
> > > target/riscv/translate.c | 13 +++++++
> > > 6 files changed, 84 insertions(+)
> > > create mode 100644 target/riscv/XVentanaCondOps.decode
> > > create mode 100644 target/riscv/insn_trans/trans_xventanacondops.inc
> > >
> > > diff --git a/target/riscv/XVentanaCondOps.decode b/target/riscv/XVentanaCondOps.decode
> > > new file mode 100644
> > > index 0000000000..5aef7c3d72
> > > --- /dev/null
> > > +++ b/target/riscv/XVentanaCondOps.decode
> > > @@ -0,0 +1,25 @@
> > > +#
> > > +# RISC-V translation routines for the XVentanaCondOps extension
> > > +#
> > > +# Copyright (c) 2022 Dr. Philipp Tomsich, philipp.tomsich@vrull.eu
> > > +#
> > > +# SPDX-License-Identifier: LGPL-2.1-or-later
> > > +#
> > > +# Reference: VTx-family custom instructions
> > > +# Custom ISA extensions for Ventana Micro Systems RISC-V cores
> > > +# (https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.0/ventana-custom-extensions-v1.0.0.pdf)
> > > +
> > > +# Fields
> > > +%rs2 20:5
> > > +%rs1 15:5
> > > +%rd 7:5
> > > +
> > > +# Argument sets
> > > +&r rd rs1 rs2 !extern
> > > +
> > > +# Formats
> > > +@r ....... ..... ..... ... ..... ....... &r %rs2 %rs1 %rd
> > > +
> > > +# *** RV64 Custom-3 Extension ***
> > > +vt_maskc 0000000 ..... ..... 110 ..... 1111011 @r
> > > +vt_maskcn 0000000 ..... ..... 111 ..... 1111011 @r
> > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > index 9bc25d3055..fc8ab1dc2b 100644
> > > --- a/target/riscv/cpu.c
> > > +++ b/target/riscv/cpu.c
> > > @@ -673,6 +673,9 @@ static Property riscv_cpu_properties[] = {
> > > DEFINE_PROP_BOOL("zbc", RISCVCPU, cfg.ext_zbc, true),
> > > DEFINE_PROP_BOOL("zbs", RISCVCPU, cfg.ext_zbs, true),
> > >
> > > + /* Vendor-specific custom extensions */
> > > + DEFINE_PROP_BOOL("xventanacondops", RISCVCPU, cfg.ext_XVentanaCondOps, false),
> > > +
> > > /* These are experimental so mark with 'x-' */
> > > DEFINE_PROP_BOOL("x-j", RISCVCPU, cfg.ext_j, false),
> > > /* ePMP 0.9.3 */
> > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > > index 4d63086765..ffde94fd1a 100644
> > > --- a/target/riscv/cpu.h
> > > +++ b/target/riscv/cpu.h
> > > @@ -330,6 +330,9 @@ struct RISCVCPU {
> > > bool ext_zfh;
> > > bool ext_zfhmin;
> > >
> > > + /* Vendor-specific custom extensions */
> > > + bool ext_XVentanaCondOps;
> > > +
> > > char *priv_spec;
> > > char *user_spec;
> > > char *bext_spec;
> > > diff --git a/target/riscv/insn_trans/trans_xventanacondops.inc b/target/riscv/insn_trans/trans_xventanacondops.inc
> > > new file mode 100644
> > > index 0000000000..b8a5d031b5
> > > --- /dev/null
> > > +++ b/target/riscv/insn_trans/trans_xventanacondops.inc
> > > @@ -0,0 +1,39 @@
> > > +/*
> > > + * RISC-V translation routines for the XVentanaCondOps extension.
> > > + *
> > > + * Copyright (c) 2021-2022 VRULL GmbH.
> > > + *
> > > + * 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/>.
> > > + */
> > > +
> > > +static bool gen_condmask(DisasContext *ctx, arg_r *a, TCGCond cond)
> > > +{
> > > + TCGv dest = dest_gpr(ctx, a->rd);
> > > + TCGv src1 = get_gpr(ctx, a->rs1, EXT_NONE);
> > > + TCGv src2 = get_gpr(ctx, a->rs2, EXT_NONE);
> > > +
> > > + tcg_gen_movcond_tl(cond, dest, src2, ctx->zero, src1, ctx->zero);
> > > +
> > > + gen_set_gpr(ctx, a->rd, dest);
> > > + return true;
> > > +}
> > > +
> > > +static bool trans_vt_maskc(DisasContext *ctx, arg_r *a)
> > > +{
> > > + return gen_condmask(ctx, a, TCG_COND_NE);
> > > +}
> > > +
> > > +static bool trans_vt_maskcn(DisasContext *ctx, arg_r *a)
> > > +{
> > > + return gen_condmask(ctx, a, TCG_COND_EQ);
> > > +}
> > > diff --git a/target/riscv/meson.build b/target/riscv/meson.build
> > > index a32158da93..1f3a15398b 100644
> > > --- a/target/riscv/meson.build
> > > +++ b/target/riscv/meson.build
> > > @@ -4,6 +4,7 @@ dir = meson.current_source_dir()
> > > gen = [
> > > decodetree.process('insn16.decode', extra_args: ['--static-decode=decode_insn16', '--insnwidth=16']),
> > > decodetree.process('insn32.decode', extra_args: '--static-decode=decode_insn32'),
> > > + decodetree.process('XVentanaCondOps.decode', extra_args: '--static-decode=decode_XVentanaCodeOps'),
> > > ]
> > >
> > > riscv_ss = ss.source_set()
> > > diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> > > index 2cbf9cbb6f..efdf8a7bdb 100644
> > > --- a/target/riscv/translate.c
> > > +++ b/target/riscv/translate.c
> > > @@ -122,6 +122,15 @@ static inline bool always_true_p(CPURISCVState *env __attribute__((__unused__))
> > > return true;
> > > }
> > >
> > > +#define MATERIALISE_EXT_PREDICATE(ext) \
> > > + static inline bool has_ ## ext ## _p(CPURISCVState *env, \
> > > + DisasContext *ctx __attribute__((__unused__))) \
> > > + { \
> > > + return RISCV_CPU(ctx->cs)->cfg.ext_ ## ext ; \
> > > + }
> > > +
> > > +MATERIALISE_EXT_PREDICATE(XVentanaCondOps);
> > > +
> > > #ifdef TARGET_RISCV32
> > > #define get_xl(ctx) MXL_RV32
> > > #elif defined(CONFIG_USER_ONLY)
> > > @@ -844,9 +853,12 @@ static uint32_t opcode_at(DisasContextBase *dcbase, target_ulong pc)
> > > #include "insn_trans/trans_rvb.c.inc"
> > > #include "insn_trans/trans_rvzfh.c.inc"
> > > #include "insn_trans/trans_privileged.c.inc"
> > > +#include "insn_trans/trans_xventanacondops.inc"
> > >
> > > /* Include the auto-generated decoder for 16 bit insn */
> > > #include "decode-insn16.c.inc"
> > > +/* Include decoders for factored-out extensions */
> > > +#include "decode-XVentanaCondOps.c.inc"
> > >
> > > static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
> > > {
> > > @@ -862,6 +874,7 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
> > > bool (*decode_func)(DisasContext *, uint32_t);
> > > } decoders[] = {
> > > { always_true_p, decode_insn32 },
> > > + { has_XVentanaCondOps_p, decode_XVentanaCodeOps },
> > > };
> > >
> > > /* Check for compressed insn */
> > > --
> > > 2.33.1
> > >
> > >
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension
2022-01-19 1:19 ` Alistair Francis
@ 2022-01-19 1:30 ` Alistair Francis
-1 siblings, 0 replies; 26+ messages in thread
From: Alistair Francis @ 2022-01-19 1:30 UTC (permalink / raw)
To: Philipp Tomsich
Cc: open list:RISC-V, Bin Meng, qemu-devel@nongnu.org Developers,
Greg Favor, Palmer Dabbelt, Alistair Francis, Kito Cheng
On Wed, Jan 19, 2022 at 11:19 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Wed, Jan 19, 2022 at 9:22 AM Philipp Tomsich
> <philipp.tomsich@vrull.eu> wrote:
> >
> > Alistair,
> >
> > Some of us (the merit almost exclusively goes to Kito) have been
> > working towards a similar policy for GCC/binutils and LLVM.
> > This currently lives in:
> > https://github.com/riscv-non-isa/riscv-toolchain-conventions/pull/17
>
> Ah cool! We can use that as a good starting point.
>
> >
> > A few comments & a question below.
> >
> > Thanks,
> > Philipp.
> >
> > On Tue, 18 Jan 2022 at 23:53, Alistair Francis <alistair23@gmail.com> wrote:
> > >
> > > On Fri, Jan 14, 2022 at 6:22 AM Philipp Tomsich
> > > <philipp.tomsich@vrull.eu> wrote:
> > > >
> > > > This adds the decoder and translation for the XVentanaCondOps custom
> > > > extension (vendor-defined by Ventana Micro Systems), which is
> > > > documented at https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.0/ventana-custom-extensions-v1.0.0.pdf
> > > >
> > > > This commit then also adds a guard-function (has_XVentanaCondOps_p)
> > > > and the decoder function to the table of decoders, enabling the
> > > > support for the XVentanaCondOps extension.
> > > >
> > > > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> > >
> > > This looks reasonable to me.
> > >
> > > I'm going to leave this for a bit in case there are any more comments.
> > >
> > > I was a little worried that taking vendor extensions isn't the right
> > > move, as we might get stuck with a large number of them. But this is
> > > pretty self contained and I think with the growing RISC-V interest
> > > it's something we will eventually need to support.
> > >
> > > I'm going to update the QEMU RISC-V wiki page with this to make the
> > > position clear (comments very welcome)
> > >
> > > === RISC-V Extensions ===
> > > As RISC-V has a range of possible extensions, QEMU has guidelines for
> > > supporting them all.
> > >
> > > If an extension is frozen or ratified by the RISC-V foundation, it can
> > > be supported in QEMU.
> > >
> > > If an official RISC-V foundation extension is in a reasonable draft
> > > state, that is not too many changes are still expected, it can be
> > > supported experimentally by QEMU. Experimental support means it must
> > > be disabled by default and marked with a "x-" in the properties. QEMU
> > > will only support the latest version of patches submitted for a draft
> > > extension. A draft extension can also be removed at any time if it
> > > conflicts with other extensions.
> > >
> > > QEMU will also support vendor extensions. Vendor extensions must be
> > > disabled by default, but can be enabled for specific vendor CPUs and
> > > boards. Vendor extensions must be maintained and tested by the vendor.
> >
> > I guess I should create a v3 with appropriate paths in the MAINTAINERS file?
>
> Hmm... Good point. I don't think you have to if you don't want to.
>
> My point here was more to just make it clear that upstream QEMU is not
> a dumping ground for vendor extensions to get them maintained by
> someone else. Obviously we won't purposely break things just for fun.
> There is an expectation that the vendor tests their extensions and
> responds to bug reports and things like that.
>
> >
> > > Vendor extensions can not interfere with other extensions and can not
> > > be obtrusive to the RISC-V target code.
> >
> > I know that there is some interest to have the XtheadV (the
> > instructions previously known as vectors 0.7.1-draft) supported and we
> > have the reality of a deployed base that implements it in hardware.
> > This would conflict with the opcode space used by the standard RISC-V
> > vectors, so it makes for an interesting test case (even if just to
> > clarify our intent)...
> > Personally, I would like to avoid precluding inclusion of something
> > useful (of course, "Vendor extensions must be maintained and tested by
> > the vendor." has to apply!), if a vendor was going to step up and also
> > offers to maintain it.
>
> Yeah... this is unfortunate. I agree that having the 0.7.1-draft
> extensions supported would be great. There is hardware that supports
> it.
>
> I think this point still stands though. IF the XtheadV implementation
> is self contained and doesn't interfere with the vector extensions,
> then that's great and we can support it. If instead it adds a large
> amount of conditionals to the released vector extension code then I
> don't think we can take it.
>
> There is some wiggle room, but the RISC-V tree already has enough
> going on and very little reviewers. If in the future we get more
> reviewers and testers we can re-evaulate what is acceptable, but for
> now I think we need to be a little strict. (Hint to any companies to
> give developers time to review)
>
> >
> > So let's assume such a (very significant) addition were factored out
> > similarly, interfacing just through a hook in decode_op and
> > argument-parsing logic that ensures that the conflicting
> > standard-extension is turned off: would this still be acceptable under
> > this policy — or would it trip the "obtrusive" condition?
>
> I think that would be acceptable, I wouldn't say that is obtrusive as
> it's self contained.
Ok, take two:
=== RISC-V Foundation Extensions ===
As RISC-V has a range of possible extensions, QEMU has guidelines for
supporting them all.
If an extension is frozen or ratified by the RISC-V foundation, it can
be supported in QEMU. Generally we will try to support as many versions
as feasible, following the usual QEMU deprecation policy to remove old
versions.
If an official RISC-V foundation extension is in a reasonable draft
state, that is not too many changes are still expected, it can be
supported experimentally by QEMU. Experimental support means it must
be disabled by default and marked with a "x-" in the CPU/board properties.
Draft extensions can be enabled by specific CPUs or boards if the hardware
supports that extension.
QEMU will only support the latest version of patches submitted for a draft
extension. A draft extension can also be removed at any time and does not
follow QEMU's deprecation policy.
=== RISC-V Custom Extensions/Instructions ===
Support for custom instruction set extensions are an important part of RISC-V,
with large encoding spaces reserved of vendor extensions.
QEMU follows similar rules to the RISC-V toolchain convention, as described
here: https://github.com/riscv-non-isa/riscv-toolchain-conventions/pull/17
QEMU will support vendor extensions. Vendor extensions must be
disabled by default, but can be enabled for specific vendor CPUs and
boards. The vendor extensions should use prefixes and names as described in
https://github.com/riscv-non-isa/riscv-toolchain-conventions
Vendor extensions must be maintained and tested by the vendor. Upstream will
take efforts to not break extensions, but testing and bug fixes should be
done by the vendor. Patches to add support for open source toolchains are
unlikely to be accepted without specification documents being made available
publicly.
Vendor extensions can not interfere with other extensions and can not
be obtrusive to the core RISC-V target code.
If you are looking to add support for vendor extensions, it is recommended
that you get involved in the QEMU review process. It is also recommended that
you send your patches as early as possible to get community feedback before
they are fully implemented. This is especially important if you are modifying
core code.
Alistair
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension
@ 2022-01-19 1:30 ` Alistair Francis
0 siblings, 0 replies; 26+ messages in thread
From: Alistair Francis @ 2022-01-19 1:30 UTC (permalink / raw)
To: Philipp Tomsich
Cc: qemu-devel@nongnu.org Developers, open list:RISC-V, Bin Meng,
Greg Favor, Palmer Dabbelt, Alistair Francis, Kito Cheng
On Wed, Jan 19, 2022 at 11:19 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Wed, Jan 19, 2022 at 9:22 AM Philipp Tomsich
> <philipp.tomsich@vrull.eu> wrote:
> >
> > Alistair,
> >
> > Some of us (the merit almost exclusively goes to Kito) have been
> > working towards a similar policy for GCC/binutils and LLVM.
> > This currently lives in:
> > https://github.com/riscv-non-isa/riscv-toolchain-conventions/pull/17
>
> Ah cool! We can use that as a good starting point.
>
> >
> > A few comments & a question below.
> >
> > Thanks,
> > Philipp.
> >
> > On Tue, 18 Jan 2022 at 23:53, Alistair Francis <alistair23@gmail.com> wrote:
> > >
> > > On Fri, Jan 14, 2022 at 6:22 AM Philipp Tomsich
> > > <philipp.tomsich@vrull.eu> wrote:
> > > >
> > > > This adds the decoder and translation for the XVentanaCondOps custom
> > > > extension (vendor-defined by Ventana Micro Systems), which is
> > > > documented at https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.0/ventana-custom-extensions-v1.0.0.pdf
> > > >
> > > > This commit then also adds a guard-function (has_XVentanaCondOps_p)
> > > > and the decoder function to the table of decoders, enabling the
> > > > support for the XVentanaCondOps extension.
> > > >
> > > > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> > >
> > > This looks reasonable to me.
> > >
> > > I'm going to leave this for a bit in case there are any more comments.
> > >
> > > I was a little worried that taking vendor extensions isn't the right
> > > move, as we might get stuck with a large number of them. But this is
> > > pretty self contained and I think with the growing RISC-V interest
> > > it's something we will eventually need to support.
> > >
> > > I'm going to update the QEMU RISC-V wiki page with this to make the
> > > position clear (comments very welcome)
> > >
> > > === RISC-V Extensions ===
> > > As RISC-V has a range of possible extensions, QEMU has guidelines for
> > > supporting them all.
> > >
> > > If an extension is frozen or ratified by the RISC-V foundation, it can
> > > be supported in QEMU.
> > >
> > > If an official RISC-V foundation extension is in a reasonable draft
> > > state, that is not too many changes are still expected, it can be
> > > supported experimentally by QEMU. Experimental support means it must
> > > be disabled by default and marked with a "x-" in the properties. QEMU
> > > will only support the latest version of patches submitted for a draft
> > > extension. A draft extension can also be removed at any time if it
> > > conflicts with other extensions.
> > >
> > > QEMU will also support vendor extensions. Vendor extensions must be
> > > disabled by default, but can be enabled for specific vendor CPUs and
> > > boards. Vendor extensions must be maintained and tested by the vendor.
> >
> > I guess I should create a v3 with appropriate paths in the MAINTAINERS file?
>
> Hmm... Good point. I don't think you have to if you don't want to.
>
> My point here was more to just make it clear that upstream QEMU is not
> a dumping ground for vendor extensions to get them maintained by
> someone else. Obviously we won't purposely break things just for fun.
> There is an expectation that the vendor tests their extensions and
> responds to bug reports and things like that.
>
> >
> > > Vendor extensions can not interfere with other extensions and can not
> > > be obtrusive to the RISC-V target code.
> >
> > I know that there is some interest to have the XtheadV (the
> > instructions previously known as vectors 0.7.1-draft) supported and we
> > have the reality of a deployed base that implements it in hardware.
> > This would conflict with the opcode space used by the standard RISC-V
> > vectors, so it makes for an interesting test case (even if just to
> > clarify our intent)...
> > Personally, I would like to avoid precluding inclusion of something
> > useful (of course, "Vendor extensions must be maintained and tested by
> > the vendor." has to apply!), if a vendor was going to step up and also
> > offers to maintain it.
>
> Yeah... this is unfortunate. I agree that having the 0.7.1-draft
> extensions supported would be great. There is hardware that supports
> it.
>
> I think this point still stands though. IF the XtheadV implementation
> is self contained and doesn't interfere with the vector extensions,
> then that's great and we can support it. If instead it adds a large
> amount of conditionals to the released vector extension code then I
> don't think we can take it.
>
> There is some wiggle room, but the RISC-V tree already has enough
> going on and very little reviewers. If in the future we get more
> reviewers and testers we can re-evaulate what is acceptable, but for
> now I think we need to be a little strict. (Hint to any companies to
> give developers time to review)
>
> >
> > So let's assume such a (very significant) addition were factored out
> > similarly, interfacing just through a hook in decode_op and
> > argument-parsing logic that ensures that the conflicting
> > standard-extension is turned off: would this still be acceptable under
> > this policy — or would it trip the "obtrusive" condition?
>
> I think that would be acceptable, I wouldn't say that is obtrusive as
> it's self contained.
Ok, take two:
=== RISC-V Foundation Extensions ===
As RISC-V has a range of possible extensions, QEMU has guidelines for
supporting them all.
If an extension is frozen or ratified by the RISC-V foundation, it can
be supported in QEMU. Generally we will try to support as many versions
as feasible, following the usual QEMU deprecation policy to remove old
versions.
If an official RISC-V foundation extension is in a reasonable draft
state, that is not too many changes are still expected, it can be
supported experimentally by QEMU. Experimental support means it must
be disabled by default and marked with a "x-" in the CPU/board properties.
Draft extensions can be enabled by specific CPUs or boards if the hardware
supports that extension.
QEMU will only support the latest version of patches submitted for a draft
extension. A draft extension can also be removed at any time and does not
follow QEMU's deprecation policy.
=== RISC-V Custom Extensions/Instructions ===
Support for custom instruction set extensions are an important part of RISC-V,
with large encoding spaces reserved of vendor extensions.
QEMU follows similar rules to the RISC-V toolchain convention, as described
here: https://github.com/riscv-non-isa/riscv-toolchain-conventions/pull/17
QEMU will support vendor extensions. Vendor extensions must be
disabled by default, but can be enabled for specific vendor CPUs and
boards. The vendor extensions should use prefixes and names as described in
https://github.com/riscv-non-isa/riscv-toolchain-conventions
Vendor extensions must be maintained and tested by the vendor. Upstream will
take efforts to not break extensions, but testing and bug fixes should be
done by the vendor. Patches to add support for open source toolchains are
unlikely to be accepted without specification documents being made available
publicly.
Vendor extensions can not interfere with other extensions and can not
be obtrusive to the core RISC-V target code.
If you are looking to add support for vendor extensions, it is recommended
that you get involved in the QEMU review process. It is also recommended that
you send your patches as early as possible to get community feedback before
they are fully implemented. This is especially important if you are modifying
core code.
Alistair
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension
2022-01-13 20:20 ` Philipp Tomsich
@ 2022-01-19 11:17 ` Philippe Mathieu-Daudé
-1 siblings, 0 replies; 26+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-01-19 11:17 UTC (permalink / raw)
To: Philipp Tomsich, qemu-devel
Cc: qemu-riscv, Bin Meng, Greg Favor, Palmer Dabbelt,
Alistair Francis, Kito Cheng
On 13/1/22 21:20, Philipp Tomsich wrote:
> This adds the decoder and translation for the XVentanaCondOps custom
> extension (vendor-defined by Ventana Micro Systems), which is
> documented at https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.0/ventana-custom-extensions-v1.0.0.pdf
>
> This commit then also adds a guard-function (has_XVentanaCondOps_p)
> and the decoder function to the table of decoders, enabling the
> support for the XVentanaCondOps extension.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
>
> ---
>
> Changes in v2:
> - Split off decode table into XVentanaCondOps.decode
> - Wire up XVentanaCondOps in the decoder-table
> static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
> {
> @@ -862,6 +874,7 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
> bool (*decode_func)(DisasContext *, uint32_t);
> } decoders[] = {
> { always_true_p, decode_insn32 },
"always_true" is the first entry,
> + { has_XVentanaCondOps_p, decode_XVentanaCodeOps },
so is that ever called?
> };
>
> /* Check for compressed insn */
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension
@ 2022-01-19 11:17 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 26+ messages in thread
From: Philippe Mathieu-Daudé @ 2022-01-19 11:17 UTC (permalink / raw)
To: Philipp Tomsich, qemu-devel
Cc: qemu-riscv, Bin Meng, Greg Favor, Palmer Dabbelt,
Alistair Francis, Kito Cheng
On 13/1/22 21:20, Philipp Tomsich wrote:
> This adds the decoder and translation for the XVentanaCondOps custom
> extension (vendor-defined by Ventana Micro Systems), which is
> documented at https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.0/ventana-custom-extensions-v1.0.0.pdf
>
> This commit then also adds a guard-function (has_XVentanaCondOps_p)
> and the decoder function to the table of decoders, enabling the
> support for the XVentanaCondOps extension.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
>
> ---
>
> Changes in v2:
> - Split off decode table into XVentanaCondOps.decode
> - Wire up XVentanaCondOps in the decoder-table
> static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
> {
> @@ -862,6 +874,7 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
> bool (*decode_func)(DisasContext *, uint32_t);
> } decoders[] = {
> { always_true_p, decode_insn32 },
"always_true" is the first entry,
> + { has_XVentanaCondOps_p, decode_XVentanaCodeOps },
so is that ever called?
> };
>
> /* Check for compressed insn */
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/2] target/riscv: iterate over a table of decoders
2022-01-13 20:20 ` Philipp Tomsich
@ 2022-01-19 11:30 ` Philippe Mathieu-Daudé
-1 siblings, 0 replies; 26+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-01-19 11:30 UTC (permalink / raw)
To: Philipp Tomsich, qemu-devel
Cc: qemu-riscv, Bin Meng, Greg Favor, Palmer Dabbelt,
Alistair Francis, Kito Cheng
On 13/1/22 21:20, Philipp Tomsich wrote:
> To split up the decoder into multiple functions (both to support
> vendor-specific opcodes in separate files and to simplify maintenance
> of orthogonal extensions), this changes decode_op to iterate over a
> table of decoders predicated on guard functions.
>
> This commit only adds the new structure and the table, allowing for
> the easy addition of additional decoders in the future.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> ---
>
> Changes in v2:
> - (new patch) iterate over a table of guarded decoder functions
>
> target/riscv/translate.c | 38 ++++++++++++++++++++++++++++++++------
> 1 file changed, 32 insertions(+), 6 deletions(-)
>
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index 615048ec87..2cbf9cbb6f 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -116,6 +116,12 @@ static inline bool has_ext(DisasContext *ctx, uint32_t ext)
> return ctx->misa_ext & ext;
> }
>
> +static inline bool always_true_p(CPURISCVState *env __attribute__((__unused__)),
> + DisasContext *ctx __attribute__((__unused__)))
> +{
> + return true;
> +}
> +
> #ifdef TARGET_RISCV32
> #define get_xl(ctx) MXL_RV32
> #elif defined(CONFIG_USER_ONLY)
> @@ -844,16 +850,28 @@ static uint32_t opcode_at(DisasContextBase *dcbase, target_ulong pc)
>
> static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
> {
> - /* check for compressed insn */
> + /* If not handled, we'll raise an illegal instruction exception */
> + bool handled = false;
> +
> + /*
> + * A table with predicate (i.e., guard) functions and decoder functions
> + * that are tested in-order until a decoder matches onto the opcode.
> + */
> + const struct {
> + bool (*guard_func)(CPURISCVState *, DisasContext *);
> + bool (*decode_func)(DisasContext *, uint32_t);
> + } decoders[] = {
> + { always_true_p, decode_insn32 },
> + };
> +
> + /* Check for compressed insn */
> if (extract16(opcode, 0, 2) != 3) {
> if (!has_ext(ctx, RVC)) {
> gen_exception_illegal(ctx);
> } else {
> ctx->opcode = opcode;
> ctx->pc_succ_insn = ctx->base.pc_next + 2;
> - if (!decode_insn16(ctx, opcode)) {
> - gen_exception_illegal(ctx);
> - }
> + handled = decode_insn16(ctx, opcode);
> }
> } else {
> uint32_t opcode32 = opcode;
> @@ -862,10 +880,18 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
> ctx->base.pc_next + 2));
> ctx->opcode = opcode32;
> ctx->pc_succ_insn = ctx->base.pc_next + 4;
> - if (!decode_insn32(ctx, opcode32)) {
> - gen_exception_illegal(ctx);
> +
> + for (size_t i = 0; i < ARRAY_SIZE(decoders); ++i) {
> + if (!decoders[i].guard_func(env, ctx))
> + continue;
> +
> + if ((handled = decoders[i].decode_func(ctx, opcode32)))
> + break;
Again, while we might check whether "Vendor Extensions" are enabled or
not at runtime, they are specific to a (vendor) core model, so we know
their availability at instantiation time.
I don't understand the need to iterate. You can check for vendor
extensions in riscv_tr_init_disas_context() and set a vendor_decoder()
handler in DisasContext, which ends calling the generic decode_opc()
one.
> }
> }
> +
> + if (!handled)
> + gen_exception_illegal(ctx);
> }
>
> static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/2] target/riscv: iterate over a table of decoders
@ 2022-01-19 11:30 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 26+ messages in thread
From: Philippe Mathieu-Daudé @ 2022-01-19 11:30 UTC (permalink / raw)
To: Philipp Tomsich, qemu-devel
Cc: qemu-riscv, Bin Meng, Greg Favor, Palmer Dabbelt,
Alistair Francis, Kito Cheng
On 13/1/22 21:20, Philipp Tomsich wrote:
> To split up the decoder into multiple functions (both to support
> vendor-specific opcodes in separate files and to simplify maintenance
> of orthogonal extensions), this changes decode_op to iterate over a
> table of decoders predicated on guard functions.
>
> This commit only adds the new structure and the table, allowing for
> the easy addition of additional decoders in the future.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> ---
>
> Changes in v2:
> - (new patch) iterate over a table of guarded decoder functions
>
> target/riscv/translate.c | 38 ++++++++++++++++++++++++++++++++------
> 1 file changed, 32 insertions(+), 6 deletions(-)
>
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index 615048ec87..2cbf9cbb6f 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -116,6 +116,12 @@ static inline bool has_ext(DisasContext *ctx, uint32_t ext)
> return ctx->misa_ext & ext;
> }
>
> +static inline bool always_true_p(CPURISCVState *env __attribute__((__unused__)),
> + DisasContext *ctx __attribute__((__unused__)))
> +{
> + return true;
> +}
> +
> #ifdef TARGET_RISCV32
> #define get_xl(ctx) MXL_RV32
> #elif defined(CONFIG_USER_ONLY)
> @@ -844,16 +850,28 @@ static uint32_t opcode_at(DisasContextBase *dcbase, target_ulong pc)
>
> static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
> {
> - /* check for compressed insn */
> + /* If not handled, we'll raise an illegal instruction exception */
> + bool handled = false;
> +
> + /*
> + * A table with predicate (i.e., guard) functions and decoder functions
> + * that are tested in-order until a decoder matches onto the opcode.
> + */
> + const struct {
> + bool (*guard_func)(CPURISCVState *, DisasContext *);
> + bool (*decode_func)(DisasContext *, uint32_t);
> + } decoders[] = {
> + { always_true_p, decode_insn32 },
> + };
> +
> + /* Check for compressed insn */
> if (extract16(opcode, 0, 2) != 3) {
> if (!has_ext(ctx, RVC)) {
> gen_exception_illegal(ctx);
> } else {
> ctx->opcode = opcode;
> ctx->pc_succ_insn = ctx->base.pc_next + 2;
> - if (!decode_insn16(ctx, opcode)) {
> - gen_exception_illegal(ctx);
> - }
> + handled = decode_insn16(ctx, opcode);
> }
> } else {
> uint32_t opcode32 = opcode;
> @@ -862,10 +880,18 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
> ctx->base.pc_next + 2));
> ctx->opcode = opcode32;
> ctx->pc_succ_insn = ctx->base.pc_next + 4;
> - if (!decode_insn32(ctx, opcode32)) {
> - gen_exception_illegal(ctx);
> +
> + for (size_t i = 0; i < ARRAY_SIZE(decoders); ++i) {
> + if (!decoders[i].guard_func(env, ctx))
> + continue;
> +
> + if ((handled = decoders[i].decode_func(ctx, opcode32)))
> + break;
Again, while we might check whether "Vendor Extensions" are enabled or
not at runtime, they are specific to a (vendor) core model, so we know
their availability at instantiation time.
I don't understand the need to iterate. You can check for vendor
extensions in riscv_tr_init_disas_context() and set a vendor_decoder()
handler in DisasContext, which ends calling the generic decode_opc()
one.
> }
> }
> +
> + if (!handled)
> + gen_exception_illegal(ctx);
> }
>
> static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension
2022-01-19 11:17 ` Philippe Mathieu-Daudé
@ 2022-01-20 15:24 ` Philipp Tomsich
-1 siblings, 0 replies; 26+ messages in thread
From: Philipp Tomsich @ 2022-01-20 15:24 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-riscv, Bin Meng, qemu-devel, Greg Favor, Palmer Dabbelt,
Alistair Francis, Kito Cheng
On Wed, 19 Jan 2022 at 12:17, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 13/1/22 21:20, Philipp Tomsich wrote:
> > This adds the decoder and translation for the XVentanaCondOps custom
> > extension (vendor-defined by Ventana Micro Systems), which is
> > documented at https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.0/ventana-custom-extensions-v1.0.0.pdf
> >
> > This commit then also adds a guard-function (has_XVentanaCondOps_p)
> > and the decoder function to the table of decoders, enabling the
> > support for the XVentanaCondOps extension.
> >
> > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> >
> > ---
> >
> > Changes in v2:
> > - Split off decode table into XVentanaCondOps.decode
> > - Wire up XVentanaCondOps in the decoder-table
>
> > static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
> > {
> > @@ -862,6 +874,7 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
> > bool (*decode_func)(DisasContext *, uint32_t);
> > } decoders[] = {
> > { always_true_p, decode_insn32 },
>
> "always_true" is the first entry,
>
> > + { has_XVentanaCondOps_p, decode_XVentanaCodeOps },
>
> so is that ever called?
Please refer to patch 1/2:
1. The guard-function only gates whether a decoder function is enabled/called.
2. Enabled decoders are iterated over until a decoder handles the
instruction-word—or we run out of decoders.
3. If no enabled decoder handled an instruction word, we raise an
illegal instruction.
This really is just a table-based form of the what would be equivalent
to the following pseudocode:
if (guard_func_1() && decoder1(…))
/* pass */ ;
else if (guard_func_2() && decoder2(...))
/* pass */ ;
[...]
else
raise_illegal();
And just as an aside (before we start discussing performance), let's
make sure we all agree that this is perfectly optimizable (I may be
missing a 'const') by a compiler:
1. The number of entries in the array are known at compile-time and
small integer — a compiler can thus peel the loop.
2. The function pointers are in the same compilation unit, so this can
be converted from indirect to direct calls (a special case of
constant-propagation).
3. Predicate functions (given that they will be very small) can be inlined.
Best,
Philipp.
>
>
> > };
> >
> > /* Check for compressed insn */
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension
@ 2022-01-20 15:24 ` Philipp Tomsich
0 siblings, 0 replies; 26+ messages in thread
From: Philipp Tomsich @ 2022-01-20 15:24 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-devel, qemu-riscv, Bin Meng, Greg Favor, Palmer Dabbelt,
Alistair Francis, Kito Cheng
On Wed, 19 Jan 2022 at 12:17, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 13/1/22 21:20, Philipp Tomsich wrote:
> > This adds the decoder and translation for the XVentanaCondOps custom
> > extension (vendor-defined by Ventana Micro Systems), which is
> > documented at https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.0/ventana-custom-extensions-v1.0.0.pdf
> >
> > This commit then also adds a guard-function (has_XVentanaCondOps_p)
> > and the decoder function to the table of decoders, enabling the
> > support for the XVentanaCondOps extension.
> >
> > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> >
> > ---
> >
> > Changes in v2:
> > - Split off decode table into XVentanaCondOps.decode
> > - Wire up XVentanaCondOps in the decoder-table
>
> > static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
> > {
> > @@ -862,6 +874,7 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
> > bool (*decode_func)(DisasContext *, uint32_t);
> > } decoders[] = {
> > { always_true_p, decode_insn32 },
>
> "always_true" is the first entry,
>
> > + { has_XVentanaCondOps_p, decode_XVentanaCodeOps },
>
> so is that ever called?
Please refer to patch 1/2:
1. The guard-function only gates whether a decoder function is enabled/called.
2. Enabled decoders are iterated over until a decoder handles the
instruction-word—or we run out of decoders.
3. If no enabled decoder handled an instruction word, we raise an
illegal instruction.
This really is just a table-based form of the what would be equivalent
to the following pseudocode:
if (guard_func_1() && decoder1(…))
/* pass */ ;
else if (guard_func_2() && decoder2(...))
/* pass */ ;
[...]
else
raise_illegal();
And just as an aside (before we start discussing performance), let's
make sure we all agree that this is perfectly optimizable (I may be
missing a 'const') by a compiler:
1. The number of entries in the array are known at compile-time and
small integer — a compiler can thus peel the loop.
2. The function pointers are in the same compilation unit, so this can
be converted from indirect to direct calls (a special case of
constant-propagation).
3. Predicate functions (given that they will be very small) can be inlined.
Best,
Philipp.
>
>
> > };
> >
> > /* Check for compressed insn */
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension
2022-01-19 1:30 ` Alistair Francis
@ 2022-01-20 15:37 ` Philipp Tomsich
-1 siblings, 0 replies; 26+ messages in thread
From: Philipp Tomsich @ 2022-01-20 15:37 UTC (permalink / raw)
To: Alistair Francis
Cc: open list:RISC-V, Bin Meng, qemu-devel@nongnu.org Developers,
Greg Favor, Palmer Dabbelt, Alistair Francis, Kito Cheng
Thanks for taking the time to write this up!
On Wed, 19 Jan 2022 at 02:30, Alistair Francis <alistair23@gmail.com> wrote:
>
> On Wed, Jan 19, 2022 at 11:19 AM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Wed, Jan 19, 2022 at 9:22 AM Philipp Tomsich
> > <philipp.tomsich@vrull.eu> wrote:
> > >
> > > Alistair,
> > >
> > > Some of us (the merit almost exclusively goes to Kito) have been
> > > working towards a similar policy for GCC/binutils and LLVM.
> > > This currently lives in:
> > > https://github.com/riscv-non-isa/riscv-toolchain-conventions/pull/17
> >
> > Ah cool! We can use that as a good starting point.
> >
> > >
> > > A few comments & a question below.
> > >
> > > Thanks,
> > > Philipp.
> > >
> > > On Tue, 18 Jan 2022 at 23:53, Alistair Francis <alistair23@gmail.com> wrote:
> > > >
> > > > On Fri, Jan 14, 2022 at 6:22 AM Philipp Tomsich
> > > > <philipp.tomsich@vrull.eu> wrote:
> > > > >
> > > > > This adds the decoder and translation for the XVentanaCondOps custom
> > > > > extension (vendor-defined by Ventana Micro Systems), which is
> > > > > documented at https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.0/ventana-custom-extensions-v1.0.0.pdf
> > > > >
> > > > > This commit then also adds a guard-function (has_XVentanaCondOps_p)
> > > > > and the decoder function to the table of decoders, enabling the
> > > > > support for the XVentanaCondOps extension.
> > > > >
> > > > > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> > > >
> > > > This looks reasonable to me.
> > > >
> > > > I'm going to leave this for a bit in case there are any more comments.
> > > >
> > > > I was a little worried that taking vendor extensions isn't the right
> > > > move, as we might get stuck with a large number of them. But this is
> > > > pretty self contained and I think with the growing RISC-V interest
> > > > it's something we will eventually need to support.
> > > >
> > > > I'm going to update the QEMU RISC-V wiki page with this to make the
> > > > position clear (comments very welcome)
> > > >
> > > > === RISC-V Extensions ===
> > > > As RISC-V has a range of possible extensions, QEMU has guidelines for
> > > > supporting them all.
> > > >
> > > > If an extension is frozen or ratified by the RISC-V foundation, it can
> > > > be supported in QEMU.
> > > >
> > > > If an official RISC-V foundation extension is in a reasonable draft
> > > > state, that is not too many changes are still expected, it can be
> > > > supported experimentally by QEMU. Experimental support means it must
> > > > be disabled by default and marked with a "x-" in the properties. QEMU
> > > > will only support the latest version of patches submitted for a draft
> > > > extension. A draft extension can also be removed at any time if it
> > > > conflicts with other extensions.
> > > >
> > > > QEMU will also support vendor extensions. Vendor extensions must be
> > > > disabled by default, but can be enabled for specific vendor CPUs and
> > > > boards. Vendor extensions must be maintained and tested by the vendor.
> > >
> > > I guess I should create a v3 with appropriate paths in the MAINTAINERS file?
> >
> > Hmm... Good point. I don't think you have to if you don't want to.
> >
> > My point here was more to just make it clear that upstream QEMU is not
> > a dumping ground for vendor extensions to get them maintained by
> > someone else. Obviously we won't purposely break things just for fun.
> > There is an expectation that the vendor tests their extensions and
> > responds to bug reports and things like that.
> >
> > >
> > > > Vendor extensions can not interfere with other extensions and can not
> > > > be obtrusive to the RISC-V target code.
> > >
> > > I know that there is some interest to have the XtheadV (the
> > > instructions previously known as vectors 0.7.1-draft) supported and we
> > > have the reality of a deployed base that implements it in hardware.
> > > This would conflict with the opcode space used by the standard RISC-V
> > > vectors, so it makes for an interesting test case (even if just to
> > > clarify our intent)...
> > > Personally, I would like to avoid precluding inclusion of something
> > > useful (of course, "Vendor extensions must be maintained and tested by
> > > the vendor." has to apply!), if a vendor was going to step up and also
> > > offers to maintain it.
> >
> > Yeah... this is unfortunate. I agree that having the 0.7.1-draft
> > extensions supported would be great. There is hardware that supports
> > it.
> >
> > I think this point still stands though. IF the XtheadV implementation
> > is self contained and doesn't interfere with the vector extensions,
> > then that's great and we can support it. If instead it adds a large
> > amount of conditionals to the released vector extension code then I
> > don't think we can take it.
> >
> > There is some wiggle room, but the RISC-V tree already has enough
> > going on and very little reviewers. If in the future we get more
> > reviewers and testers we can re-evaulate what is acceptable, but for
> > now I think we need to be a little strict. (Hint to any companies to
> > give developers time to review)
> >
> > >
> > > So let's assume such a (very significant) addition were factored out
> > > similarly, interfacing just through a hook in decode_op and
> > > argument-parsing logic that ensures that the conflicting
> > > standard-extension is turned off: would this still be acceptable under
> > > this policy — or would it trip the "obtrusive" condition?
> >
> > I think that would be acceptable, I wouldn't say that is obtrusive as
> > it's self contained.
>
> Ok, take two:
>
> === RISC-V Foundation Extensions ===
> As RISC-V has a range of possible extensions, QEMU has guidelines for
> supporting them all.
>
> If an extension is frozen or ratified by the RISC-V foundation, it can
> be supported in QEMU. Generally we will try to support as many versions
> as feasible, following the usual QEMU deprecation policy to remove old
> versions.
>
> If an official RISC-V foundation extension is in a reasonable draft
> state, that is not too many changes are still expected, it can be
> supported experimentally by QEMU. Experimental support means it must
> be disabled by default and marked with a "x-" in the CPU/board properties.
> Draft extensions can be enabled by specific CPUs or boards if the hardware
> supports that extension.
Should we include a version number on experimental versions?
LLVM requires users to fully specify the version, when using
experimental versions.
This may be a useful stereotype also for QEmu, as it ensures that
users are aware that the underlying specification version has changed
(e.g., when someone requests x-zbb-0p92 and our implementation moves
to x-zbb-0p93 (there was a difference in encoding of the
minimum/maximum operations in-between)), an error will be raised early
instead of having a computation go wrong later.
> QEMU will only support the latest version of patches submitted for a draft
> extension. A draft extension can also be removed at any time and does not
> follow QEMU's deprecation policy.
>
> === RISC-V Custom Extensions/Instructions ===
> Support for custom instruction set extensions are an important part of RISC-V,
> with large encoding spaces reserved of vendor extensions.
>
> QEMU follows similar rules to the RISC-V toolchain convention, as described
> here: https://github.com/riscv-non-isa/riscv-toolchain-conventions/pull/17
>
> QEMU will support vendor extensions. Vendor extensions must be
> disabled by default, but can be enabled for specific vendor CPUs and
> boards. The vendor extensions should use prefixes and names as described in
> https://github.com/riscv-non-isa/riscv-toolchain-conventions
>
> Vendor extensions must be maintained and tested by the vendor. Upstream will
> take efforts to not break extensions, but testing and bug fixes should be
> done by the vendor. Patches to add support for open source toolchains are
> unlikely to be accepted without specification documents being made available
> publicly.
>
> Vendor extensions can not interfere with other extensions and can not
> be obtrusive to the core RISC-V target code.
>
> If you are looking to add support for vendor extensions, it is recommended
> that you get involved in the QEMU review process. It is also recommended that
> you send your patches as early as possible to get community feedback before
> they are fully implemented. This is especially important if you are modifying
> core code.
>
> Alistair
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension
@ 2022-01-20 15:37 ` Philipp Tomsich
0 siblings, 0 replies; 26+ messages in thread
From: Philipp Tomsich @ 2022-01-20 15:37 UTC (permalink / raw)
To: Alistair Francis
Cc: qemu-devel@nongnu.org Developers, open list:RISC-V, Bin Meng,
Greg Favor, Palmer Dabbelt, Alistair Francis, Kito Cheng
Thanks for taking the time to write this up!
On Wed, 19 Jan 2022 at 02:30, Alistair Francis <alistair23@gmail.com> wrote:
>
> On Wed, Jan 19, 2022 at 11:19 AM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Wed, Jan 19, 2022 at 9:22 AM Philipp Tomsich
> > <philipp.tomsich@vrull.eu> wrote:
> > >
> > > Alistair,
> > >
> > > Some of us (the merit almost exclusively goes to Kito) have been
> > > working towards a similar policy for GCC/binutils and LLVM.
> > > This currently lives in:
> > > https://github.com/riscv-non-isa/riscv-toolchain-conventions/pull/17
> >
> > Ah cool! We can use that as a good starting point.
> >
> > >
> > > A few comments & a question below.
> > >
> > > Thanks,
> > > Philipp.
> > >
> > > On Tue, 18 Jan 2022 at 23:53, Alistair Francis <alistair23@gmail.com> wrote:
> > > >
> > > > On Fri, Jan 14, 2022 at 6:22 AM Philipp Tomsich
> > > > <philipp.tomsich@vrull.eu> wrote:
> > > > >
> > > > > This adds the decoder and translation for the XVentanaCondOps custom
> > > > > extension (vendor-defined by Ventana Micro Systems), which is
> > > > > documented at https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.0/ventana-custom-extensions-v1.0.0.pdf
> > > > >
> > > > > This commit then also adds a guard-function (has_XVentanaCondOps_p)
> > > > > and the decoder function to the table of decoders, enabling the
> > > > > support for the XVentanaCondOps extension.
> > > > >
> > > > > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> > > >
> > > > This looks reasonable to me.
> > > >
> > > > I'm going to leave this for a bit in case there are any more comments.
> > > >
> > > > I was a little worried that taking vendor extensions isn't the right
> > > > move, as we might get stuck with a large number of them. But this is
> > > > pretty self contained and I think with the growing RISC-V interest
> > > > it's something we will eventually need to support.
> > > >
> > > > I'm going to update the QEMU RISC-V wiki page with this to make the
> > > > position clear (comments very welcome)
> > > >
> > > > === RISC-V Extensions ===
> > > > As RISC-V has a range of possible extensions, QEMU has guidelines for
> > > > supporting them all.
> > > >
> > > > If an extension is frozen or ratified by the RISC-V foundation, it can
> > > > be supported in QEMU.
> > > >
> > > > If an official RISC-V foundation extension is in a reasonable draft
> > > > state, that is not too many changes are still expected, it can be
> > > > supported experimentally by QEMU. Experimental support means it must
> > > > be disabled by default and marked with a "x-" in the properties. QEMU
> > > > will only support the latest version of patches submitted for a draft
> > > > extension. A draft extension can also be removed at any time if it
> > > > conflicts with other extensions.
> > > >
> > > > QEMU will also support vendor extensions. Vendor extensions must be
> > > > disabled by default, but can be enabled for specific vendor CPUs and
> > > > boards. Vendor extensions must be maintained and tested by the vendor.
> > >
> > > I guess I should create a v3 with appropriate paths in the MAINTAINERS file?
> >
> > Hmm... Good point. I don't think you have to if you don't want to.
> >
> > My point here was more to just make it clear that upstream QEMU is not
> > a dumping ground for vendor extensions to get them maintained by
> > someone else. Obviously we won't purposely break things just for fun.
> > There is an expectation that the vendor tests their extensions and
> > responds to bug reports and things like that.
> >
> > >
> > > > Vendor extensions can not interfere with other extensions and can not
> > > > be obtrusive to the RISC-V target code.
> > >
> > > I know that there is some interest to have the XtheadV (the
> > > instructions previously known as vectors 0.7.1-draft) supported and we
> > > have the reality of a deployed base that implements it in hardware.
> > > This would conflict with the opcode space used by the standard RISC-V
> > > vectors, so it makes for an interesting test case (even if just to
> > > clarify our intent)...
> > > Personally, I would like to avoid precluding inclusion of something
> > > useful (of course, "Vendor extensions must be maintained and tested by
> > > the vendor." has to apply!), if a vendor was going to step up and also
> > > offers to maintain it.
> >
> > Yeah... this is unfortunate. I agree that having the 0.7.1-draft
> > extensions supported would be great. There is hardware that supports
> > it.
> >
> > I think this point still stands though. IF the XtheadV implementation
> > is self contained and doesn't interfere with the vector extensions,
> > then that's great and we can support it. If instead it adds a large
> > amount of conditionals to the released vector extension code then I
> > don't think we can take it.
> >
> > There is some wiggle room, but the RISC-V tree already has enough
> > going on and very little reviewers. If in the future we get more
> > reviewers and testers we can re-evaulate what is acceptable, but for
> > now I think we need to be a little strict. (Hint to any companies to
> > give developers time to review)
> >
> > >
> > > So let's assume such a (very significant) addition were factored out
> > > similarly, interfacing just through a hook in decode_op and
> > > argument-parsing logic that ensures that the conflicting
> > > standard-extension is turned off: would this still be acceptable under
> > > this policy — or would it trip the "obtrusive" condition?
> >
> > I think that would be acceptable, I wouldn't say that is obtrusive as
> > it's self contained.
>
> Ok, take two:
>
> === RISC-V Foundation Extensions ===
> As RISC-V has a range of possible extensions, QEMU has guidelines for
> supporting them all.
>
> If an extension is frozen or ratified by the RISC-V foundation, it can
> be supported in QEMU. Generally we will try to support as many versions
> as feasible, following the usual QEMU deprecation policy to remove old
> versions.
>
> If an official RISC-V foundation extension is in a reasonable draft
> state, that is not too many changes are still expected, it can be
> supported experimentally by QEMU. Experimental support means it must
> be disabled by default and marked with a "x-" in the CPU/board properties.
> Draft extensions can be enabled by specific CPUs or boards if the hardware
> supports that extension.
Should we include a version number on experimental versions?
LLVM requires users to fully specify the version, when using
experimental versions.
This may be a useful stereotype also for QEmu, as it ensures that
users are aware that the underlying specification version has changed
(e.g., when someone requests x-zbb-0p92 and our implementation moves
to x-zbb-0p93 (there was a difference in encoding of the
minimum/maximum operations in-between)), an error will be raised early
instead of having a computation go wrong later.
> QEMU will only support the latest version of patches submitted for a draft
> extension. A draft extension can also be removed at any time and does not
> follow QEMU's deprecation policy.
>
> === RISC-V Custom Extensions/Instructions ===
> Support for custom instruction set extensions are an important part of RISC-V,
> with large encoding spaces reserved of vendor extensions.
>
> QEMU follows similar rules to the RISC-V toolchain convention, as described
> here: https://github.com/riscv-non-isa/riscv-toolchain-conventions/pull/17
>
> QEMU will support vendor extensions. Vendor extensions must be
> disabled by default, but can be enabled for specific vendor CPUs and
> boards. The vendor extensions should use prefixes and names as described in
> https://github.com/riscv-non-isa/riscv-toolchain-conventions
>
> Vendor extensions must be maintained and tested by the vendor. Upstream will
> take efforts to not break extensions, but testing and bug fixes should be
> done by the vendor. Patches to add support for open source toolchains are
> unlikely to be accepted without specification documents being made available
> publicly.
>
> Vendor extensions can not interfere with other extensions and can not
> be obtrusive to the core RISC-V target code.
>
> If you are looking to add support for vendor extensions, it is recommended
> that you get involved in the QEMU review process. It is also recommended that
> you send your patches as early as possible to get community feedback before
> they are fully implemented. This is especially important if you are modifying
> core code.
>
> Alistair
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/2] target/riscv: iterate over a table of decoders
2022-01-19 11:30 ` Philippe Mathieu-Daudé
@ 2022-01-20 20:05 ` Philipp Tomsich
-1 siblings, 0 replies; 26+ messages in thread
From: Philipp Tomsich @ 2022-01-20 20:05 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-riscv, Bin Meng, qemu-devel, Greg Favor, Palmer Dabbelt,
Alistair Francis, Kito Cheng
On Wed, 19 Jan 2022 at 12:30, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 13/1/22 21:20, Philipp Tomsich wrote:
> > To split up the decoder into multiple functions (both to support
> > vendor-specific opcodes in separate files and to simplify maintenance
> > of orthogonal extensions), this changes decode_op to iterate over a
> > table of decoders predicated on guard functions.
> >
> > This commit only adds the new structure and the table, allowing for
> > the easy addition of additional decoders in the future.
> >
> > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> > ---
> >
> > Changes in v2:
> > - (new patch) iterate over a table of guarded decoder functions
> >
> > target/riscv/translate.c | 38 ++++++++++++++++++++++++++++++++------
> > 1 file changed, 32 insertions(+), 6 deletions(-)
> >
> > diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> > index 615048ec87..2cbf9cbb6f 100644
> > --- a/target/riscv/translate.c
> > +++ b/target/riscv/translate.c
> > @@ -116,6 +116,12 @@ static inline bool has_ext(DisasContext *ctx, uint32_t ext)
> > return ctx->misa_ext & ext;
> > }
> >
> > +static inline bool always_true_p(CPURISCVState *env __attribute__((__unused__)),
> > + DisasContext *ctx __attribute__((__unused__)))
> > +{
> > + return true;
> > +}
> > +
> > #ifdef TARGET_RISCV32
> > #define get_xl(ctx) MXL_RV32
> > #elif defined(CONFIG_USER_ONLY)
> > @@ -844,16 +850,28 @@ static uint32_t opcode_at(DisasContextBase *dcbase, target_ulong pc)
> >
> > static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
> > {
> > - /* check for compressed insn */
> > + /* If not handled, we'll raise an illegal instruction exception */
> > + bool handled = false;
> > +
> > + /*
> > + * A table with predicate (i.e., guard) functions and decoder functions
> > + * that are tested in-order until a decoder matches onto the opcode.
> > + */
> > + const struct {
> > + bool (*guard_func)(CPURISCVState *, DisasContext *);
> > + bool (*decode_func)(DisasContext *, uint32_t);
> > + } decoders[] = {
> > + { always_true_p, decode_insn32 },
> > + };
> > +
> > + /* Check for compressed insn */
> > if (extract16(opcode, 0, 2) != 3) {
> > if (!has_ext(ctx, RVC)) {
> > gen_exception_illegal(ctx);
> > } else {
> > ctx->opcode = opcode;
> > ctx->pc_succ_insn = ctx->base.pc_next + 2;
> > - if (!decode_insn16(ctx, opcode)) {
> > - gen_exception_illegal(ctx);
> > - }
> > + handled = decode_insn16(ctx, opcode);
> > }
> > } else {
> > uint32_t opcode32 = opcode;
> > @@ -862,10 +880,18 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
> > ctx->base.pc_next + 2));
> > ctx->opcode = opcode32;
> > ctx->pc_succ_insn = ctx->base.pc_next + 4;
> > - if (!decode_insn32(ctx, opcode32)) {
> > - gen_exception_illegal(ctx);
> > +
> > + for (size_t i = 0; i < ARRAY_SIZE(decoders); ++i) {
> > + if (!decoders[i].guard_func(env, ctx))
> > + continue;
> > +
> > + if ((handled = decoders[i].decode_func(ctx, opcode32)))
> > + break;
>
> Again, while we might check whether "Vendor Extensions" are enabled or
> not at runtime, they are specific to a (vendor) core model, so we know
> their availability at instantiation time.
>
> I don't understand the need to iterate. You can check for vendor
> extensions in riscv_tr_init_disas_context() and set a vendor_decoder()
> handler in DisasContext, which ends calling the generic decode_opc()
> one.
While the design you propose is a valid variation that will achieve
most of the functionality, I don't believe that this is the best way
forward.
A key issue is that it will interfere with using the command-line to
enable/disable such vendor-defined extensions easily (i.e., "-cpu
any,XVentanaCondOps=true" will not work).
It also looks like there is a misunderstanding of how vendor-defined
extensions work: these will not be the same for every vendor core and
may be implemented by multiple vendors (after all: these are
vendor-defined, not vendor-specific). Trying to force the RISC-V
vendors down the route of handling this via a specialized decoder
function set up in riscv_tr_init_disas_context(), will eventually
force them to have multiple decode functions for
chip-families/generations — this is not conducive to easy
maintainability of the codebase.
Regards,
Philipp.
>
> > }
> > }
> > +
> > + if (!handled)
> > + gen_exception_illegal(ctx);
> > }
> >
> > static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/2] target/riscv: iterate over a table of decoders
@ 2022-01-20 20:05 ` Philipp Tomsich
0 siblings, 0 replies; 26+ messages in thread
From: Philipp Tomsich @ 2022-01-20 20:05 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-devel, qemu-riscv, Bin Meng, Greg Favor, Palmer Dabbelt,
Alistair Francis, Kito Cheng
On Wed, 19 Jan 2022 at 12:30, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 13/1/22 21:20, Philipp Tomsich wrote:
> > To split up the decoder into multiple functions (both to support
> > vendor-specific opcodes in separate files and to simplify maintenance
> > of orthogonal extensions), this changes decode_op to iterate over a
> > table of decoders predicated on guard functions.
> >
> > This commit only adds the new structure and the table, allowing for
> > the easy addition of additional decoders in the future.
> >
> > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> > ---
> >
> > Changes in v2:
> > - (new patch) iterate over a table of guarded decoder functions
> >
> > target/riscv/translate.c | 38 ++++++++++++++++++++++++++++++++------
> > 1 file changed, 32 insertions(+), 6 deletions(-)
> >
> > diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> > index 615048ec87..2cbf9cbb6f 100644
> > --- a/target/riscv/translate.c
> > +++ b/target/riscv/translate.c
> > @@ -116,6 +116,12 @@ static inline bool has_ext(DisasContext *ctx, uint32_t ext)
> > return ctx->misa_ext & ext;
> > }
> >
> > +static inline bool always_true_p(CPURISCVState *env __attribute__((__unused__)),
> > + DisasContext *ctx __attribute__((__unused__)))
> > +{
> > + return true;
> > +}
> > +
> > #ifdef TARGET_RISCV32
> > #define get_xl(ctx) MXL_RV32
> > #elif defined(CONFIG_USER_ONLY)
> > @@ -844,16 +850,28 @@ static uint32_t opcode_at(DisasContextBase *dcbase, target_ulong pc)
> >
> > static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
> > {
> > - /* check for compressed insn */
> > + /* If not handled, we'll raise an illegal instruction exception */
> > + bool handled = false;
> > +
> > + /*
> > + * A table with predicate (i.e., guard) functions and decoder functions
> > + * that are tested in-order until a decoder matches onto the opcode.
> > + */
> > + const struct {
> > + bool (*guard_func)(CPURISCVState *, DisasContext *);
> > + bool (*decode_func)(DisasContext *, uint32_t);
> > + } decoders[] = {
> > + { always_true_p, decode_insn32 },
> > + };
> > +
> > + /* Check for compressed insn */
> > if (extract16(opcode, 0, 2) != 3) {
> > if (!has_ext(ctx, RVC)) {
> > gen_exception_illegal(ctx);
> > } else {
> > ctx->opcode = opcode;
> > ctx->pc_succ_insn = ctx->base.pc_next + 2;
> > - if (!decode_insn16(ctx, opcode)) {
> > - gen_exception_illegal(ctx);
> > - }
> > + handled = decode_insn16(ctx, opcode);
> > }
> > } else {
> > uint32_t opcode32 = opcode;
> > @@ -862,10 +880,18 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
> > ctx->base.pc_next + 2));
> > ctx->opcode = opcode32;
> > ctx->pc_succ_insn = ctx->base.pc_next + 4;
> > - if (!decode_insn32(ctx, opcode32)) {
> > - gen_exception_illegal(ctx);
> > +
> > + for (size_t i = 0; i < ARRAY_SIZE(decoders); ++i) {
> > + if (!decoders[i].guard_func(env, ctx))
> > + continue;
> > +
> > + if ((handled = decoders[i].decode_func(ctx, opcode32)))
> > + break;
>
> Again, while we might check whether "Vendor Extensions" are enabled or
> not at runtime, they are specific to a (vendor) core model, so we know
> their availability at instantiation time.
>
> I don't understand the need to iterate. You can check for vendor
> extensions in riscv_tr_init_disas_context() and set a vendor_decoder()
> handler in DisasContext, which ends calling the generic decode_opc()
> one.
While the design you propose is a valid variation that will achieve
most of the functionality, I don't believe that this is the best way
forward.
A key issue is that it will interfere with using the command-line to
enable/disable such vendor-defined extensions easily (i.e., "-cpu
any,XVentanaCondOps=true" will not work).
It also looks like there is a misunderstanding of how vendor-defined
extensions work: these will not be the same for every vendor core and
may be implemented by multiple vendors (after all: these are
vendor-defined, not vendor-specific). Trying to force the RISC-V
vendors down the route of handling this via a specialized decoder
function set up in riscv_tr_init_disas_context(), will eventually
force them to have multiple decode functions for
chip-families/generations — this is not conducive to easy
maintainability of the codebase.
Regards,
Philipp.
>
> > }
> > }
> > +
> > + if (!handled)
> > + gen_exception_illegal(ctx);
> > }
> >
> > static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension
2022-01-20 15:37 ` Philipp Tomsich
@ 2022-01-21 3:02 ` Alistair Francis
-1 siblings, 0 replies; 26+ messages in thread
From: Alistair Francis @ 2022-01-21 3:02 UTC (permalink / raw)
To: Philipp Tomsich
Cc: open list:RISC-V, Bin Meng, qemu-devel@nongnu.org Developers,
Greg Favor, Palmer Dabbelt, Alistair Francis, Kito Cheng
On Fri, Jan 21, 2022 at 1:38 AM Philipp Tomsich
<philipp.tomsich@vrull.eu> wrote:
>
> Thanks for taking the time to write this up!
>
> On Wed, 19 Jan 2022 at 02:30, Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Wed, Jan 19, 2022 at 11:19 AM Alistair Francis <alistair23@gmail.com> wrote:
> > >
> > > On Wed, Jan 19, 2022 at 9:22 AM Philipp Tomsich
> > > <philipp.tomsich@vrull.eu> wrote:
> > > >
> > > > Alistair,
> > > >
> > > > Some of us (the merit almost exclusively goes to Kito) have been
> > > > working towards a similar policy for GCC/binutils and LLVM.
> > > > This currently lives in:
> > > > https://github.com/riscv-non-isa/riscv-toolchain-conventions/pull/17
> > >
> > > Ah cool! We can use that as a good starting point.
> > >
> > > >
> > > > A few comments & a question below.
> > > >
> > > > Thanks,
> > > > Philipp.
> > > >
> > > > On Tue, 18 Jan 2022 at 23:53, Alistair Francis <alistair23@gmail.com> wrote:
> > > > >
> > > > > On Fri, Jan 14, 2022 at 6:22 AM Philipp Tomsich
> > > > > <philipp.tomsich@vrull.eu> wrote:
> > > > > >
> > > > > > This adds the decoder and translation for the XVentanaCondOps custom
> > > > > > extension (vendor-defined by Ventana Micro Systems), which is
> > > > > > documented at https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.0/ventana-custom-extensions-v1.0.0.pdf
> > > > > >
> > > > > > This commit then also adds a guard-function (has_XVentanaCondOps_p)
> > > > > > and the decoder function to the table of decoders, enabling the
> > > > > > support for the XVentanaCondOps extension.
> > > > > >
> > > > > > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> > > > >
> > > > > This looks reasonable to me.
> > > > >
> > > > > I'm going to leave this for a bit in case there are any more comments.
> > > > >
> > > > > I was a little worried that taking vendor extensions isn't the right
> > > > > move, as we might get stuck with a large number of them. But this is
> > > > > pretty self contained and I think with the growing RISC-V interest
> > > > > it's something we will eventually need to support.
> > > > >
> > > > > I'm going to update the QEMU RISC-V wiki page with this to make the
> > > > > position clear (comments very welcome)
> > > > >
> > > > > === RISC-V Extensions ===
> > > > > As RISC-V has a range of possible extensions, QEMU has guidelines for
> > > > > supporting them all.
> > > > >
> > > > > If an extension is frozen or ratified by the RISC-V foundation, it can
> > > > > be supported in QEMU.
> > > > >
> > > > > If an official RISC-V foundation extension is in a reasonable draft
> > > > > state, that is not too many changes are still expected, it can be
> > > > > supported experimentally by QEMU. Experimental support means it must
> > > > > be disabled by default and marked with a "x-" in the properties. QEMU
> > > > > will only support the latest version of patches submitted for a draft
> > > > > extension. A draft extension can also be removed at any time if it
> > > > > conflicts with other extensions.
> > > > >
> > > > > QEMU will also support vendor extensions. Vendor extensions must be
> > > > > disabled by default, but can be enabled for specific vendor CPUs and
> > > > > boards. Vendor extensions must be maintained and tested by the vendor.
> > > >
> > > > I guess I should create a v3 with appropriate paths in the MAINTAINERS file?
> > >
> > > Hmm... Good point. I don't think you have to if you don't want to.
> > >
> > > My point here was more to just make it clear that upstream QEMU is not
> > > a dumping ground for vendor extensions to get them maintained by
> > > someone else. Obviously we won't purposely break things just for fun.
> > > There is an expectation that the vendor tests their extensions and
> > > responds to bug reports and things like that.
> > >
> > > >
> > > > > Vendor extensions can not interfere with other extensions and can not
> > > > > be obtrusive to the RISC-V target code.
> > > >
> > > > I know that there is some interest to have the XtheadV (the
> > > > instructions previously known as vectors 0.7.1-draft) supported and we
> > > > have the reality of a deployed base that implements it in hardware.
> > > > This would conflict with the opcode space used by the standard RISC-V
> > > > vectors, so it makes for an interesting test case (even if just to
> > > > clarify our intent)...
> > > > Personally, I would like to avoid precluding inclusion of something
> > > > useful (of course, "Vendor extensions must be maintained and tested by
> > > > the vendor." has to apply!), if a vendor was going to step up and also
> > > > offers to maintain it.
> > >
> > > Yeah... this is unfortunate. I agree that having the 0.7.1-draft
> > > extensions supported would be great. There is hardware that supports
> > > it.
> > >
> > > I think this point still stands though. IF the XtheadV implementation
> > > is self contained and doesn't interfere with the vector extensions,
> > > then that's great and we can support it. If instead it adds a large
> > > amount of conditionals to the released vector extension code then I
> > > don't think we can take it.
> > >
> > > There is some wiggle room, but the RISC-V tree already has enough
> > > going on and very little reviewers. If in the future we get more
> > > reviewers and testers we can re-evaulate what is acceptable, but for
> > > now I think we need to be a little strict. (Hint to any companies to
> > > give developers time to review)
> > >
> > > >
> > > > So let's assume such a (very significant) addition were factored out
> > > > similarly, interfacing just through a hook in decode_op and
> > > > argument-parsing logic that ensures that the conflicting
> > > > standard-extension is turned off: would this still be acceptable under
> > > > this policy — or would it trip the "obtrusive" condition?
> > >
> > > I think that would be acceptable, I wouldn't say that is obtrusive as
> > > it's self contained.
> >
> > Ok, take two:
> >
> > === RISC-V Foundation Extensions ===
> > As RISC-V has a range of possible extensions, QEMU has guidelines for
> > supporting them all.
> >
> > If an extension is frozen or ratified by the RISC-V foundation, it can
> > be supported in QEMU. Generally we will try to support as many versions
> > as feasible, following the usual QEMU deprecation policy to remove old
> > versions.
> >
> > If an official RISC-V foundation extension is in a reasonable draft
> > state, that is not too many changes are still expected, it can be
> > supported experimentally by QEMU. Experimental support means it must
> > be disabled by default and marked with a "x-" in the CPU/board properties.
> > Draft extensions can be enabled by specific CPUs or boards if the hardware
> > supports that extension.
>
> Should we include a version number on experimental versions?
>
> LLVM requires users to fully specify the version, when using
> experimental versions.
> This may be a useful stereotype also for QEmu, as it ensures that
> users are aware that the underlying specification version has changed
> (e.g., when someone requests x-zbb-0p92 and our implementation moves
> to x-zbb-0p93 (there was a difference in encoding of the
> minimum/maximum operations in-between)), an error will be raised early
> instead of having a computation go wrong later.
We traditionally haven't and it's up to anyone using an experimental
extension to check the version.
It's probably worth including though, I have added a sentence there.
Thanks for the feedback, I have added that to the wiki
Alistair
>
> > QEMU will only support the latest version of patches submitted for a draft
> > extension. A draft extension can also be removed at any time and does not
> > follow QEMU's deprecation policy.
> >
> > === RISC-V Custom Extensions/Instructions ===
> > Support for custom instruction set extensions are an important part of RISC-V,
> > with large encoding spaces reserved of vendor extensions.
> >
> > QEMU follows similar rules to the RISC-V toolchain convention, as described
> > here: https://github.com/riscv-non-isa/riscv-toolchain-conventions/pull/17
> >
> > QEMU will support vendor extensions. Vendor extensions must be
> > disabled by default, but can be enabled for specific vendor CPUs and
> > boards. The vendor extensions should use prefixes and names as described in
> > https://github.com/riscv-non-isa/riscv-toolchain-conventions
> >
> > Vendor extensions must be maintained and tested by the vendor. Upstream will
> > take efforts to not break extensions, but testing and bug fixes should be
> > done by the vendor. Patches to add support for open source toolchains are
> > unlikely to be accepted without specification documents being made available
> > publicly.
> >
> > Vendor extensions can not interfere with other extensions and can not
> > be obtrusive to the core RISC-V target code.
> >
> > If you are looking to add support for vendor extensions, it is recommended
> > that you get involved in the QEMU review process. It is also recommended that
> > you send your patches as early as possible to get community feedback before
> > they are fully implemented. This is especially important if you are modifying
> > core code.
> >
> > Alistair
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension
@ 2022-01-21 3:02 ` Alistair Francis
0 siblings, 0 replies; 26+ messages in thread
From: Alistair Francis @ 2022-01-21 3:02 UTC (permalink / raw)
To: Philipp Tomsich
Cc: qemu-devel@nongnu.org Developers, open list:RISC-V, Bin Meng,
Greg Favor, Palmer Dabbelt, Alistair Francis, Kito Cheng
On Fri, Jan 21, 2022 at 1:38 AM Philipp Tomsich
<philipp.tomsich@vrull.eu> wrote:
>
> Thanks for taking the time to write this up!
>
> On Wed, 19 Jan 2022 at 02:30, Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Wed, Jan 19, 2022 at 11:19 AM Alistair Francis <alistair23@gmail.com> wrote:
> > >
> > > On Wed, Jan 19, 2022 at 9:22 AM Philipp Tomsich
> > > <philipp.tomsich@vrull.eu> wrote:
> > > >
> > > > Alistair,
> > > >
> > > > Some of us (the merit almost exclusively goes to Kito) have been
> > > > working towards a similar policy for GCC/binutils and LLVM.
> > > > This currently lives in:
> > > > https://github.com/riscv-non-isa/riscv-toolchain-conventions/pull/17
> > >
> > > Ah cool! We can use that as a good starting point.
> > >
> > > >
> > > > A few comments & a question below.
> > > >
> > > > Thanks,
> > > > Philipp.
> > > >
> > > > On Tue, 18 Jan 2022 at 23:53, Alistair Francis <alistair23@gmail.com> wrote:
> > > > >
> > > > > On Fri, Jan 14, 2022 at 6:22 AM Philipp Tomsich
> > > > > <philipp.tomsich@vrull.eu> wrote:
> > > > > >
> > > > > > This adds the decoder and translation for the XVentanaCondOps custom
> > > > > > extension (vendor-defined by Ventana Micro Systems), which is
> > > > > > documented at https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.0/ventana-custom-extensions-v1.0.0.pdf
> > > > > >
> > > > > > This commit then also adds a guard-function (has_XVentanaCondOps_p)
> > > > > > and the decoder function to the table of decoders, enabling the
> > > > > > support for the XVentanaCondOps extension.
> > > > > >
> > > > > > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> > > > >
> > > > > This looks reasonable to me.
> > > > >
> > > > > I'm going to leave this for a bit in case there are any more comments.
> > > > >
> > > > > I was a little worried that taking vendor extensions isn't the right
> > > > > move, as we might get stuck with a large number of them. But this is
> > > > > pretty self contained and I think with the growing RISC-V interest
> > > > > it's something we will eventually need to support.
> > > > >
> > > > > I'm going to update the QEMU RISC-V wiki page with this to make the
> > > > > position clear (comments very welcome)
> > > > >
> > > > > === RISC-V Extensions ===
> > > > > As RISC-V has a range of possible extensions, QEMU has guidelines for
> > > > > supporting them all.
> > > > >
> > > > > If an extension is frozen or ratified by the RISC-V foundation, it can
> > > > > be supported in QEMU.
> > > > >
> > > > > If an official RISC-V foundation extension is in a reasonable draft
> > > > > state, that is not too many changes are still expected, it can be
> > > > > supported experimentally by QEMU. Experimental support means it must
> > > > > be disabled by default and marked with a "x-" in the properties. QEMU
> > > > > will only support the latest version of patches submitted for a draft
> > > > > extension. A draft extension can also be removed at any time if it
> > > > > conflicts with other extensions.
> > > > >
> > > > > QEMU will also support vendor extensions. Vendor extensions must be
> > > > > disabled by default, but can be enabled for specific vendor CPUs and
> > > > > boards. Vendor extensions must be maintained and tested by the vendor.
> > > >
> > > > I guess I should create a v3 with appropriate paths in the MAINTAINERS file?
> > >
> > > Hmm... Good point. I don't think you have to if you don't want to.
> > >
> > > My point here was more to just make it clear that upstream QEMU is not
> > > a dumping ground for vendor extensions to get them maintained by
> > > someone else. Obviously we won't purposely break things just for fun.
> > > There is an expectation that the vendor tests their extensions and
> > > responds to bug reports and things like that.
> > >
> > > >
> > > > > Vendor extensions can not interfere with other extensions and can not
> > > > > be obtrusive to the RISC-V target code.
> > > >
> > > > I know that there is some interest to have the XtheadV (the
> > > > instructions previously known as vectors 0.7.1-draft) supported and we
> > > > have the reality of a deployed base that implements it in hardware.
> > > > This would conflict with the opcode space used by the standard RISC-V
> > > > vectors, so it makes for an interesting test case (even if just to
> > > > clarify our intent)...
> > > > Personally, I would like to avoid precluding inclusion of something
> > > > useful (of course, "Vendor extensions must be maintained and tested by
> > > > the vendor." has to apply!), if a vendor was going to step up and also
> > > > offers to maintain it.
> > >
> > > Yeah... this is unfortunate. I agree that having the 0.7.1-draft
> > > extensions supported would be great. There is hardware that supports
> > > it.
> > >
> > > I think this point still stands though. IF the XtheadV implementation
> > > is self contained and doesn't interfere with the vector extensions,
> > > then that's great and we can support it. If instead it adds a large
> > > amount of conditionals to the released vector extension code then I
> > > don't think we can take it.
> > >
> > > There is some wiggle room, but the RISC-V tree already has enough
> > > going on and very little reviewers. If in the future we get more
> > > reviewers and testers we can re-evaulate what is acceptable, but for
> > > now I think we need to be a little strict. (Hint to any companies to
> > > give developers time to review)
> > >
> > > >
> > > > So let's assume such a (very significant) addition were factored out
> > > > similarly, interfacing just through a hook in decode_op and
> > > > argument-parsing logic that ensures that the conflicting
> > > > standard-extension is turned off: would this still be acceptable under
> > > > this policy — or would it trip the "obtrusive" condition?
> > >
> > > I think that would be acceptable, I wouldn't say that is obtrusive as
> > > it's self contained.
> >
> > Ok, take two:
> >
> > === RISC-V Foundation Extensions ===
> > As RISC-V has a range of possible extensions, QEMU has guidelines for
> > supporting them all.
> >
> > If an extension is frozen or ratified by the RISC-V foundation, it can
> > be supported in QEMU. Generally we will try to support as many versions
> > as feasible, following the usual QEMU deprecation policy to remove old
> > versions.
> >
> > If an official RISC-V foundation extension is in a reasonable draft
> > state, that is not too many changes are still expected, it can be
> > supported experimentally by QEMU. Experimental support means it must
> > be disabled by default and marked with a "x-" in the CPU/board properties.
> > Draft extensions can be enabled by specific CPUs or boards if the hardware
> > supports that extension.
>
> Should we include a version number on experimental versions?
>
> LLVM requires users to fully specify the version, when using
> experimental versions.
> This may be a useful stereotype also for QEmu, as it ensures that
> users are aware that the underlying specification version has changed
> (e.g., when someone requests x-zbb-0p92 and our implementation moves
> to x-zbb-0p93 (there was a difference in encoding of the
> minimum/maximum operations in-between)), an error will be raised early
> instead of having a computation go wrong later.
We traditionally haven't and it's up to anyone using an experimental
extension to check the version.
It's probably worth including though, I have added a sentence there.
Thanks for the feedback, I have added that to the wiki
Alistair
>
> > QEMU will only support the latest version of patches submitted for a draft
> > extension. A draft extension can also be removed at any time and does not
> > follow QEMU's deprecation policy.
> >
> > === RISC-V Custom Extensions/Instructions ===
> > Support for custom instruction set extensions are an important part of RISC-V,
> > with large encoding spaces reserved of vendor extensions.
> >
> > QEMU follows similar rules to the RISC-V toolchain convention, as described
> > here: https://github.com/riscv-non-isa/riscv-toolchain-conventions/pull/17
> >
> > QEMU will support vendor extensions. Vendor extensions must be
> > disabled by default, but can be enabled for specific vendor CPUs and
> > boards. The vendor extensions should use prefixes and names as described in
> > https://github.com/riscv-non-isa/riscv-toolchain-conventions
> >
> > Vendor extensions must be maintained and tested by the vendor. Upstream will
> > take efforts to not break extensions, but testing and bug fixes should be
> > done by the vendor. Patches to add support for open source toolchains are
> > unlikely to be accepted without specification documents being made available
> > publicly.
> >
> > Vendor extensions can not interfere with other extensions and can not
> > be obtrusive to the core RISC-V target code.
> >
> > If you are looking to add support for vendor extensions, it is recommended
> > that you get involved in the QEMU review process. It is also recommended that
> > you send your patches as early as possible to get community feedback before
> > they are fully implemented. This is especially important if you are modifying
> > core code.
> >
> > Alistair
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/2] target/riscv: iterate over a table of decoders
2022-01-13 20:20 ` Philipp Tomsich
` (2 preceding siblings ...)
(?)
@ 2022-01-25 21:28 ` Richard Henderson
-1 siblings, 0 replies; 26+ messages in thread
From: Richard Henderson @ 2022-01-25 21:28 UTC (permalink / raw)
To: Philipp Tomsich, qemu-devel
Cc: qemu-riscv, Bin Meng, Greg Favor, Palmer Dabbelt,
Alistair Francis, Kito Cheng
On 1/14/22 7:20 AM, Philipp Tomsich wrote:
> +static inline bool always_true_p(CPURISCVState *env __attribute__((__unused__)),
> + DisasContext *ctx __attribute__((__unused__)))
> +{
> + return true;
> +}
Drop the inline; the function will be instantiated so that it can be put in the table.
Drop the env pointer. Everything this hook should examine must be in DisasContext.
> static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
> {
> - /* check for compressed insn */
> + /* If not handled, we'll raise an illegal instruction exception */
> + bool handled = false;
> +
> + /*
> + * A table with predicate (i.e., guard) functions and decoder functions
> + * that are tested in-order until a decoder matches onto the opcode.
> + */
> + const struct {
static const.
> + bool (*guard_func)(CPURISCVState *, DisasContext *);
> + bool (*decode_func)(DisasContext *, uint32_t);
> + } decoders[] = {
> + { always_true_p, decode_insn32 },
> + };
> +
> + /* Check for compressed insn */
> if (extract16(opcode, 0, 2) != 3) {
> if (!has_ext(ctx, RVC)) {
> gen_exception_illegal(ctx);
> } else {
> ctx->opcode = opcode;
> ctx->pc_succ_insn = ctx->base.pc_next + 2;
> - if (!decode_insn16(ctx, opcode)) {
> - gen_exception_illegal(ctx);
> - }
> + handled = decode_insn16(ctx, opcode);
> }
> } else {
> uint32_t opcode32 = opcode;
> @@ -862,10 +880,18 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
> ctx->base.pc_next + 2));
> ctx->opcode = opcode32;
> ctx->pc_succ_insn = ctx->base.pc_next + 4;
> - if (!decode_insn32(ctx, opcode32)) {
> - gen_exception_illegal(ctx);
> +
> + for (size_t i = 0; i < ARRAY_SIZE(decoders); ++i) {
> + if (!decoders[i].guard_func(env, ctx))
> + continue;
> +
> + if ((handled = decoders[i].decode_func(ctx, opcode32)))
Never put an assignment in an if like this.
I think it would be cleaner to just do
if (decode()) {
return;
}
and drop the handled variable entirely.
r~
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension
2022-01-13 20:20 ` Philipp Tomsich
` (2 preceding siblings ...)
(?)
@ 2022-01-25 21:42 ` Richard Henderson
-1 siblings, 0 replies; 26+ messages in thread
From: Richard Henderson @ 2022-01-25 21:42 UTC (permalink / raw)
To: Philipp Tomsich, qemu-devel
Cc: qemu-riscv, Bin Meng, Greg Favor, Palmer Dabbelt,
Alistair Francis, Kito Cheng
On 1/14/22 7:20 AM, Philipp Tomsich wrote:
> new file mode 100644
> index 0000000000..b8a5d031b5
> --- /dev/null
> +++ b/target/riscv/insn_trans/trans_xventanacondops.inc
The filename suffix should be ".c.inc".
> +static bool gen_condmask(DisasContext *ctx, arg_r *a, TCGCond cond)
> +{
> + TCGv dest = dest_gpr(ctx, a->rd);
> + TCGv src1 = get_gpr(ctx, a->rs1, EXT_NONE);
> + TCGv src2 = get_gpr(ctx, a->rs2, EXT_NONE);
> +
> + tcg_gen_movcond_tl(cond, dest, src2, ctx->zero, src1, ctx->zero);
> +
> + gen_set_gpr(ctx, a->rd, dest);
> + return true;
> +}
> +
> +static bool trans_vt_maskc(DisasContext *ctx, arg_r *a)
> +{
> + return gen_condmask(ctx, a, TCG_COND_NE);
> +}
> +
> +static bool trans_vt_maskcn(DisasContext *ctx, arg_r *a)
> +{
> + return gen_condmask(ctx, a, TCG_COND_EQ);
> +}
Implementation looks good.
> +#define MATERIALISE_EXT_PREDICATE(ext) \
> + static inline bool has_ ## ext ## _p(CPURISCVState *env, \
> + DisasContext *ctx __attribute__((__unused__))) \
> + { \
> + return RISCV_CPU(ctx->cs)->cfg.ext_ ## ext ; \
> + }
Again, no inline.
Don't look back to RISCV_CPU here. We shouldn't even have access to that here, as it
leads to temptation to do invalid things at translation time (this isn't one of them,
since it only accesses constant state). What we have been doing is copying ext_foo into
DisasContext in riscv_tr_init_disas_context. Though it might be time to revisit that.
Perhaps give the cpu->cfg structure type a name, e.g. RISCVCPUConfig. Add "const
RISCVCPUConfig *cfg" to DisasContext and copy the pointer across in init_disas_context.
r~
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2022-01-25 21:47 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-13 20:20 [PATCH v2 1/2] target/riscv: iterate over a table of decoders Philipp Tomsich
2022-01-13 20:20 ` Philipp Tomsich
2022-01-13 20:20 ` [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension Philipp Tomsich
2022-01-13 20:20 ` Philipp Tomsich
2022-01-18 22:53 ` Alistair Francis
2022-01-18 22:53 ` Alistair Francis
2022-01-18 23:21 ` Philipp Tomsich
2022-01-18 23:21 ` Philipp Tomsich
2022-01-19 1:19 ` Alistair Francis
2022-01-19 1:19 ` Alistair Francis
2022-01-19 1:30 ` Alistair Francis
2022-01-19 1:30 ` Alistair Francis
2022-01-20 15:37 ` Philipp Tomsich
2022-01-20 15:37 ` Philipp Tomsich
2022-01-21 3:02 ` Alistair Francis
2022-01-21 3:02 ` Alistair Francis
2022-01-19 11:17 ` Philippe Mathieu-Daudé via
2022-01-19 11:17 ` Philippe Mathieu-Daudé
2022-01-20 15:24 ` Philipp Tomsich
2022-01-20 15:24 ` Philipp Tomsich
2022-01-25 21:42 ` Richard Henderson
2022-01-19 11:30 ` [PATCH v2 1/2] target/riscv: iterate over a table of decoders Philippe Mathieu-Daudé via
2022-01-19 11:30 ` Philippe Mathieu-Daudé
2022-01-20 20:05 ` Philipp Tomsich
2022-01-20 20:05 ` Philipp Tomsich
2022-01-25 21:28 ` Richard Henderson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.