[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 01/15] RISC-V: Adding XTheadCmo ISA extension
|
From: |
Christoph Müllner |
|
Subject: |
Re: [PATCH v2 01/15] RISC-V: Adding XTheadCmo ISA extension |
|
Date: |
Mon, 30 Jan 2023 15:04:52 +0100 |
On Sun, Jan 29, 2023 at 11:40 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Wed, Jan 25, 2023 at 5:51 AM Christoph Müllner
> <christoph.muellner@vrull.eu> wrote:
> >
> >
> >
> > On Tue, Jan 24, 2023 at 6:31 PM Christoph Müllner
> > <christoph.muellner@vrull.eu> wrote:
> >>
> >>
> >>
> >> On Mon, Jan 23, 2023 at 11:50 PM Alistair Francis <alistair23@gmail.com>
> >> wrote:
> >>>
> >>> On Sat, Dec 24, 2022 at 4:09 AM Christoph Muellner
> >>> <christoph.muellner@vrull.eu> wrote:
> >>> >
> >>> > From: Christoph Müllner <christoph.muellner@vrull.eu>
> >>> >
> >>> > This patch adds support for the XTheadCmo ISA extension.
> >>> > To avoid interfering with standard extensions, decoder and translation
> >>> > are in its own xthead* specific files.
> >>> > Future patches should be able to easily add additional T-Head extension.
> >>> >
> >>> > The implementation does not have much functionality (besides accepting
> >>> > the instructions and not qualifying them as illegal instructions if
> >>> > the hart executes in the required privilege level for the instruction),
> >>> > as QEMU does not model CPU caches and instructions are documented
> >>> > to not raise any exceptions.
> >>> >
> >>> > Changes in v2:
> >>> > - Add ISA_EXT_DATA_ENTRY()
> >>> > - Explicit test for PRV_U
> >>> > - Encapsule access to env-priv in inline function
> >>> > - Use single decoder for XThead extensions
> >>> >
> >>> > Co-developed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
> >>> > Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
> >>> > ---
> >>> > target/riscv/cpu.c | 2 +
> >>> > target/riscv/cpu.h | 1 +
> >>> > target/riscv/insn_trans/trans_xthead.c.inc | 89 ++++++++++++++++++++++
> >>> > target/riscv/meson.build | 1 +
> >>> > target/riscv/translate.c | 15 +++-
> >>> > target/riscv/xthead.decode | 38 +++++++++
> >>> > 6 files changed, 143 insertions(+), 3 deletions(-)
> >>> > create mode 100644 target/riscv/insn_trans/trans_xthead.c.inc
> >>> > create mode 100644 target/riscv/xthead.decode
> >>> >
> >>> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> >>> > index 6fe176e483..a90b82c5c5 100644
> >>> > --- a/target/riscv/cpu.c
> >>> > +++ b/target/riscv/cpu.c
> >>> > @@ -108,6 +108,7 @@ static const struct isa_ext_data isa_edata_arr[] = {
> >>> > ISA_EXT_DATA_ENTRY(svinval, true, PRIV_VERSION_1_12_0,
> >>> > ext_svinval),
> >>> > ISA_EXT_DATA_ENTRY(svnapot, true, PRIV_VERSION_1_12_0,
> >>> > ext_svnapot),
> >>> > ISA_EXT_DATA_ENTRY(svpbmt, true, PRIV_VERSION_1_12_0, ext_svpbmt),
> >>> > + ISA_EXT_DATA_ENTRY(xtheadcmo, true, PRIV_VERSION_1_11_0,
> >>> > ext_xtheadcmo),
> >>> > ISA_EXT_DATA_ENTRY(xventanacondops, true, PRIV_VERSION_1_12_0,
> >>> > ext_XVentanaCondOps),
> >>> > };
> >>> >
> >>> > @@ -1060,6 +1061,7 @@ static Property riscv_cpu_extensions[] = {
> >>> > DEFINE_PROP_BOOL("zmmul", RISCVCPU, cfg.ext_zmmul, false),
> >>> >
> >>> > /* Vendor-specific custom extensions */
> >>> > + DEFINE_PROP_BOOL("xtheadcmo", RISCVCPU, cfg.ext_xtheadcmo, false),
> >>> > DEFINE_PROP_BOOL("xventanacondops", RISCVCPU,
> >>> > cfg.ext_XVentanaCondOps, false),
> >>> >
> >>> > /* These are experimental so mark with 'x-' */
> >>> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> >>> > index 443d15a47c..ad1c19f870 100644
> >>> > --- a/target/riscv/cpu.h
> >>> > +++ b/target/riscv/cpu.h
> >>> > @@ -465,6 +465,7 @@ struct RISCVCPUConfig {
> >>> > uint64_t mimpid;
> >>> >
> >>> > /* Vendor-specific custom extensions */
> >>> > + bool ext_xtheadcmo;
> >>> > bool ext_XVentanaCondOps;
> >>> >
> >>> > uint8_t pmu_num;
> >>> > diff --git a/target/riscv/insn_trans/trans_xthead.c.inc
> >>> > b/target/riscv/insn_trans/trans_xthead.c.inc
> >>> > new file mode 100644
> >>> > index 0000000000..00e75c7dca
> >>> > --- /dev/null
> >>> > +++ b/target/riscv/insn_trans/trans_xthead.c.inc
> >>> > @@ -0,0 +1,89 @@
> >>> > +/*
> >>> > + * RISC-V translation routines for the T-Head vendor extensions
> >>> > (xthead*).
> >>> > + *
> >>> > + * Copyright (c) 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/>.
> >>> > + */
> >>> > +
> >>> > +#define REQUIRE_XTHEADCMO(ctx) do { \
> >>> > + if (!ctx->cfg_ptr->ext_xtheadcmo) { \
> >>> > + return false; \
> >>> > + } \
> >>> > +} while (0)
> >>> > +
> >>> > +/* XTheadCmo */
> >>> > +
> >>> > +static inline int priv_level(DisasContext *ctx)
> >>> > +{
> >>> > +#ifdef CONFIG_USER_ONLY
> >>> > + return PRV_U;
> >>> > +#else
> >>> > + /* Priv level equals mem_idx -- see cpu_mmu_index. */
> >>> > + return ctx->mem_idx;
> >>>
> >>> This should be ANDed with TB_FLAGS_PRIV_MMU_MASK as sometimes this can
> >>> include hypervisor priv access information
> >>
> >>
> >> Ok.
> >>
> >>>
> >>>
> >>> > +#endif
> >>> > +}
> >>> > +
> >>> > +#define REQUIRE_PRIV_MHSU(ctx) \
> >>> > +do { \
> >>> > + int priv = priv_level(ctx); \
> >>> > + if (!(priv == PRV_M || \
> >>> > + priv == PRV_H || \
> >>>
> >>> PRV_H isn't used
> >>>
> >>> > + priv == PRV_S || \
> >>> > + priv == PRV_U)) { \
> >>> > + return false; \
> >>>
> >>> When would this not be the case?
> >>
> >>
> >> Ok, I will make this a macro that expands to nothing (and a comment).
> >>
> >>>
> >>>
> >>> > + } \
> >>> > +} while (0)
> >>> > +
> >>> > +#define REQUIRE_PRIV_MHS(ctx) \
> >>> > +do { \
> >>> > + int priv = priv_level(ctx); \
> >>> > + if (!(priv == PRV_M || \
> >>> > + priv == PRV_H || \
> >>>
> >>> Also not used
> >>
> >>
> >> Ok, I will remove the PRV_H.
> >>
> >>>
> >>>
> >>> > + priv == PRV_S)) { \
> >>> > + return false; \
> >>> > + } \
> >>> > +} while (0)
> >>> > +
> >>> > +#define NOP_PRIVCHECK(insn, extcheck, privcheck) \
> >>> > +static bool trans_ ## insn(DisasContext *ctx, arg_ ## insn * a) \
> >>> > +{ \
> >>> > + (void) a; \
> >>> > + extcheck(ctx); \
> >>> > + privcheck(ctx); \
> >>> > + return true; \
> >>> > +}
> >>> > +
> >>> > +NOP_PRIVCHECK(th_dcache_call, REQUIRE_XTHEADCMO, REQUIRE_PRIV_MHS)
> >>> > +NOP_PRIVCHECK(th_dcache_ciall, REQUIRE_XTHEADCMO, REQUIRE_PRIV_MHS)
> >>> > +NOP_PRIVCHECK(th_dcache_iall, REQUIRE_XTHEADCMO, REQUIRE_PRIV_MHS)
> >>> > +NOP_PRIVCHECK(th_dcache_cpa, REQUIRE_XTHEADCMO, REQUIRE_PRIV_MHS)
> >>> > +NOP_PRIVCHECK(th_dcache_cipa, REQUIRE_XTHEADCMO, REQUIRE_PRIV_MHS)
> >>> > +NOP_PRIVCHECK(th_dcache_ipa, REQUIRE_XTHEADCMO, REQUIRE_PRIV_MHS)
> >>> > +NOP_PRIVCHECK(th_dcache_cva, REQUIRE_XTHEADCMO, REQUIRE_PRIV_MHSU)
> >>> > +NOP_PRIVCHECK(th_dcache_civa, REQUIRE_XTHEADCMO, REQUIRE_PRIV_MHSU)
> >>> > +NOP_PRIVCHECK(th_dcache_iva, REQUIRE_XTHEADCMO, REQUIRE_PRIV_MHSU)
> >>> > +NOP_PRIVCHECK(th_dcache_csw, REQUIRE_XTHEADCMO, REQUIRE_PRIV_MHS)
> >>> > +NOP_PRIVCHECK(th_dcache_cisw, REQUIRE_XTHEADCMO, REQUIRE_PRIV_MHS)
> >>> > +NOP_PRIVCHECK(th_dcache_isw, REQUIRE_XTHEADCMO, REQUIRE_PRIV_MHS)
> >>> > +NOP_PRIVCHECK(th_dcache_cpal1, REQUIRE_XTHEADCMO, REQUIRE_PRIV_MHS)
> >>> > +NOP_PRIVCHECK(th_dcache_cval1, REQUIRE_XTHEADCMO, REQUIRE_PRIV_MHS)
> >>> > +
> >>> > +NOP_PRIVCHECK(th_icache_iall, REQUIRE_XTHEADCMO, REQUIRE_PRIV_MHS)
> >>> > +NOP_PRIVCHECK(th_icache_ialls, REQUIRE_XTHEADCMO, REQUIRE_PRIV_MHS)
> >>> > +NOP_PRIVCHECK(th_icache_ipa, REQUIRE_XTHEADCMO, REQUIRE_PRIV_MHS)
> >>> > +NOP_PRIVCHECK(th_icache_iva, REQUIRE_XTHEADCMO, REQUIRE_PRIV_MHSU)
> >>> > +
> >>> > +NOP_PRIVCHECK(th_l2cache_call, REQUIRE_XTHEADCMO, REQUIRE_PRIV_MHS)
> >>> > +NOP_PRIVCHECK(th_l2cache_ciall, REQUIRE_XTHEADCMO, REQUIRE_PRIV_MHS)
> >>> > +NOP_PRIVCHECK(th_l2cache_iall, REQUIRE_XTHEADCMO, REQUIRE_PRIV_MHS)
> >>> > diff --git a/target/riscv/meson.build b/target/riscv/meson.build
> >>> > index ba25164d74..5dee37a242 100644
> >>> > --- a/target/riscv/meson.build
> >>> > +++ b/target/riscv/meson.build
> >>> > @@ -2,6 +2,7 @@
> >>> > 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('xthead.decode', extra_args:
> >>> > '--static-decode=decode_xthead'),
> >>> > decodetree.process('XVentanaCondOps.decode', extra_args:
> >>> > '--static-decode=decode_XVentanaCodeOps'),
> >>> > ]
> >>> >
> >>> > diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> >>> > index db123da5ec..14d9116975 100644
> >>> > --- a/target/riscv/translate.c
> >>> > +++ b/target/riscv/translate.c
> >>> > @@ -125,13 +125,18 @@ static bool always_true_p(DisasContext *ctx
> >>> > __attribute__((__unused__)))
> >>> > return true;
> >>> > }
> >>> >
> >>> > +static bool has_xthead_p(DisasContext *ctx
> >>> > __attribute__((__unused__)))
> >>> > +{
> >>> > + return ctx->cfg_ptr->ext_xtheadcmo;
> >>> > +}
> >>> > +
> >>> > #define MATERIALISE_EXT_PREDICATE(ext) \
> >>> > static bool has_ ## ext ## _p(DisasContext *ctx) \
> >>> > { \
> >>> > return ctx->cfg_ptr->ext_ ## ext ; \
> >>> > }
> >>> >
> >>> > -MATERIALISE_EXT_PREDICATE(XVentanaCondOps);
> >>> > +MATERIALISE_EXT_PREDICATE(XVentanaCondOps)
> >>>
> >>> Do we need this change?
> >>
> >>
> >> It is indeed a drive-by cleanup, that is not necessary.
> >> In v1 we were using this macro, therefore it made sense back then.
> >> Will be dropped.
> >>
> >>>
> >>>
> >>> >
> >>> > #ifdef TARGET_RISCV32
> >>> > #define get_xl(ctx) MXL_RV32
> >>> > @@ -732,6 +737,10 @@ static int ex_rvc_shiftri(DisasContext *ctx, int
> >>> > imm)
> >>> > /* Include the auto-generated decoder for 32 bit insn */
> >>> > #include "decode-insn32.c.inc"
> >>> >
> >>> > +/* Include decoders for factored-out extensions */
> >>> > +#include "decode-xthead.c.inc"
> >>> > +#include "decode-XVentanaCondOps.c.inc"
> >>> > +
> >>> > static bool gen_logic_imm_fn(DisasContext *ctx, arg_i *a,
> >>> > void (*func)(TCGv, TCGv, target_long))
> >>> > {
> >>> > @@ -1033,12 +1042,11 @@ static uint32_t opcode_at(DisasContextBase
> >>> > *dcbase, target_ulong pc)
> >>> > #include "insn_trans/trans_rvk.c.inc"
> >>> > #include "insn_trans/trans_privileged.c.inc"
> >>> > #include "insn_trans/trans_svinval.c.inc"
> >>> > +#include "insn_trans/trans_xthead.c.inc"
> >>> > #include "insn_trans/trans_xventanacondops.c.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"
> >>>
> >>> Can we not leave these at the bottom?
> >>
> >>
> >> Ok.
> >
> >
> > I got reminded again, why this is like it is:
> > The decoder code needs to be included before the translation functions,
> > because the translation functions use types that are defined in the
> > generated decoder code.
> > And I wanted to keep all vendor extensions together.
>
> Ah ok, why not keep each extension together instead, like this:
>
> #include "decode-xthead.c.inc"
> #include "insn_trans/trans_xthead.c.inc"
>
> #include "decode-XVentanaCondOps.c.inc"
In v3 I had the decode included below decode-insn32.c.inc.
And the trans_xthead.c.inc was at the end of the trans* includes.
For v4 I will move the decode down to the inclusion of trans_xthead.c.inc.
v4 will be sent out, once we have addressed the review comments from Richard.
Thanks,
Christoph
>
>
> Alistair