Re: [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension
On 1/14/22 7:20 AM, Philipp Tomsich wrote:
new file mode 100644
index 00..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~
Re: [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension
On Fri, Jan 21, 2022 at 1:38 AM Philipp Tomsich wrote: > > Thanks for taking the time to write this up! > > On Wed, 19 Jan 2022 at 02:30, Alistair Francis wrote: > > > > On Wed, Jan 19, 2022 at 11:19 AM Alistair Francis > > wrote: > > > > > > On Wed, Jan 19, 2022 at 9:22 AM Philipp Tomsich > > > 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 > > > > wrote: > > > > > > > > > > On Fri, Jan 14, 2022 at 6:22 AM 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 > > > > > > > > > > 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'
Re: [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension
On Wed, 19 Jan 2022 at 12:17, Philippe Mathieu-Daudé 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
> >
> > ---
> >
> > 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 */
>
Re: [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension
Thanks for taking the time to write this up! On Wed, 19 Jan 2022 at 02:30, Alistair Francis wrote: > > On Wed, Jan 19, 2022 at 11:19 AM Alistair Francis > wrote: > > > > On Wed, Jan 19, 2022 at 9:22 AM Philipp Tomsich > > 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 > > > wrote: > > > > > > > > On Fri, Jan 14, 2022 at 6:22 AM 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 > > > > > > > > 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 l
Re: [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension
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
---
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 */
Re: [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension
On Wed, Jan 19, 2022 at 11:19 AM Alistair Francis wrote: > > On Wed, Jan 19, 2022 at 9:22 AM Philipp Tomsich > 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 wrote: > > > > > > On Fri, Jan 14, 2022 at 6:22 AM 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 > > > > > > 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
Re: [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension
On Wed, Jan 19, 2022 at 9:22 AM Philipp Tomsich 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 wrote: > > > > On Fri, Jan 14, 2022 at 6:22 AM 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 > > > > 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 decod
Re: [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension
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 wrote: > > On Fri, Jan 14, 2022 at 6:22 AM 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 > > 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 00..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.toms...@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 > > +%rd7:5 > > + > > +# Argument sets > > +&rrd rs1 rs2 !extern > > + >
Re: [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension
On Fri, Jan 14, 2022 at 6:22 AM 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
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 00..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.toms...@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
> +%rd7:5
> +
> +# Argument sets
> +&rrd rs1 rs2 !extern
> +
> +# Formats
> +@r ... . . ... . ... &r%rs2
> %rs1 %rd
> +
> +# *** RV64 Custom-3 Extension ***
> +vt_maskc 000 . . 110 . 011 @r
> +vt_maskcn 000 . . 111 . 011 @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 00..b8a5d031b5
> --- /dev/null
> +++ b/target/riscv/insn_trans/tra
[PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension
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
---
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 00..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.toms...@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
+%rd7:5
+
+# Argument sets
+&rrd rs1 rs2 !extern
+
+# Formats
+@r ... . . ... . ... &r%rs2 %rs1
%rd
+
+# *** RV64 Custom-3 Extension ***
+vt_maskc 000 . . 110 . 011 @r
+vt_maskcn 000 . . 111 . 011 @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 00..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
