Re: [PATCH v5 4/4] target/riscv: add vector configure instruction

[LIU Zhiwei <zhiwei_liu@c-sky.com>]

On 2020/2/27 3:20, Alistair Francis wrote:
>   On Fri, Feb 21, 2020 at 1:45 AM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote:
>> vsetvl and vsetvli are two configure instructions for vl, vtype. TB flags
>> should update after configure instructions. The (ill, lmul, sew ) of vtype
>> and the bit of (VSTART == 0 && VL == VLMAX) will be placed within tb_flags.
>>
>> Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
>> ---
>>   MAINTAINERS                             |  1 +
>>   target/riscv/Makefile.objs              |  2 +-
>>   target/riscv/cpu.h                      | 61 +++++++++++++++++++---
>>   target/riscv/helper.h                   |  2 +
>>   target/riscv/insn32.decode              |  5 ++
>>   target/riscv/insn_trans/trans_rvv.inc.c | 69 +++++++++++++++++++++++++
>>   target/riscv/translate.c                | 17 +++++-
>>   target/riscv/vector_helper.c            | 53 +++++++++++++++++++
>>   8 files changed, 199 insertions(+), 11 deletions(-)
>>   create mode 100644 target/riscv/insn_trans/trans_rvv.inc.c
>>   create mode 100644 target/riscv/vector_helper.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 1740a4fddc..cd2e200db9 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -266,6 +266,7 @@ M: Palmer Dabbelt <palmer@dabbelt.com>
>>   M: Alistair Francis <Alistair.Francis@wdc.com>
>>   M: Sagar Karandikar <sagark@eecs.berkeley.edu>
>>   M: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
>> +M: LIU Zhiwei <zhiwei_liu@c-sky.com>
> I don't think you should add yourself here. MAINTAINERS is more for
> people doing active patch review.
OK.
> RISC-V QEMU can really do with more maintainers though, so if you do
> want to be involved you could help review patches.
Actually my main job is to maintain and develop QEMU code，so I'd like to 
review target/riscv code,
however vector upstream takes a lot time .
>>   L: qemu-riscv@nongnu.org
>>   S: Supported
>>   F: target/riscv/
>> diff --git a/target/riscv/Makefile.objs b/target/riscv/Makefile.objs
>> index ff651f69f6..ff38df6219 100644
>> --- a/target/riscv/Makefile.objs
>> +++ b/target/riscv/Makefile.objs
>> @@ -1,4 +1,4 @@
>> -obj-y += translate.o op_helper.o cpu_helper.o cpu.o csr.o fpu_helper.o gdbstub.o
>> +obj-y += translate.o op_helper.o cpu_helper.o cpu.o csr.o fpu_helper.o vector_helper.o gdbstub.o
>>   obj-$(CONFIG_SOFTMMU) += pmp.o
>>
>>   ifeq ($(CONFIG_SOFTMMU),y)
>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> index 748bd557f9..f7003edb86 100644
>> --- a/target/riscv/cpu.h
>> +++ b/target/riscv/cpu.h
>> @@ -21,6 +21,7 @@
>>   #define RISCV_CPU_H
>>
>>   #include "hw/core/cpu.h"
>> +#include "hw/registerfields.h"
>>   #include "exec/cpu-defs.h"
>>   #include "fpu/softfloat-types.h"
>>
>> @@ -98,6 +99,12 @@ typedef struct CPURISCVState CPURISCVState;
>>
>>   #define RV_VLEN_MAX 512
>>
>> +FIELD(VTYPE, LMUL, 0, 2)
> Shouldn't this be VLMUL?
OK. The same with VSEW and VEDIV.
>
>> +FIELD(VTYPE, SEW, 2, 3)
> VSEW?
>
>> +FIELD(VTYPE, EDIV, 5, 2)
> VEDIV?
>
>> +FIELD(VTYPE, RESERVED, 7, sizeof(target_ulong) * 8 - 9)
>> +FIELD(VTYPE, VILL, sizeof(target_ulong) * 8 - 2, 1)
>> +
>>   struct CPURISCVState {
>>       target_ulong gpr[32];
>>       uint64_t fpr[32]; /* assume both F and D extensions */
>> @@ -302,16 +309,59 @@ void riscv_cpu_set_fflags(CPURISCVState *env, target_ulong);
>>   #define TB_FLAGS_MMU_MASK   3
>>   #define TB_FLAGS_MSTATUS_FS MSTATUS_FS
>>
>> +typedef CPURISCVState CPUArchState;
>> +typedef RISCVCPU ArchCPU;
>> +#include "exec/cpu-all.h"
> Why do you need this? Shouldn't the TB_FLAGS fields work without this.
Because env_archcpu in cpu_get_tb_cpu_state will use it.
>> +
>> +FIELD(TB_FLAGS, VL_EQ_VLMAX, 2, 1)
>> +FIELD(TB_FLAGS, LMUL, 3, 2)
>> +FIELD(TB_FLAGS, SEW, 5, 3)
>> +FIELD(TB_FLAGS, VILL, 8, 1)
> These should probably be defined with the other TB_FLAGS (or if you
> need them here you can move the others up here).
I'd like to put other TB_FLAGS in other separate patch.
>
>> +
>> +/*
>> + * A simplification for VLMAX
>> + * = (1 << LMUL) * VLEN / (8 * (1 << SEW))
>> + * = (VLEN << LMUL) / (8 << SEW)
>> + * = (VLEN << LMUL) >> (SEW + 3)
>> + * = VLEN >> (SEW + 3 - LMUL)
>> + */
>> +static inline uint32_t vext_get_vlmax(RISCVCPU *cpu, target_ulong vtype)
>> +{
>> +    uint8_t sew, lmul;
>> +
>> +    sew = FIELD_EX64(vtype, VTYPE, SEW);
>> +    lmul = FIELD_EX64(vtype, VTYPE, LMUL);
>> +    return cpu->cfg.vlen >> (sew + 3 - lmul);
> Shouldn't we assert this isn't over RV_VLEN_MAX?
I don't think so.  VLEN is vector register length in bits. It is checked
against RV_VLEN_MAX in cpu realize function. If it is over RV_VLEN_MAX,
it will exits before translate any tb.

Zhiwei

>
> Alistair
>
>> +}
>> +
>>   static inline void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
>> -                                        target_ulong *cs_base, uint32_t *flags)
>> +                                        target_ulong *cs_base, uint32_t *pflags)
>>   {
>> +    uint32_t flags = 0;
>> +
>>       *pc = env->pc;
>>       *cs_base = 0;
>> +
>> +    if (env->misa & RVV) {
>> +        uint32_t vlmax = vext_get_vlmax(env_archcpu(env), env->vtype);
>> +        bool vl_eq_vlmax = (env->vstart == 0) && (vlmax == env->vl);
>> +        flags = FIELD_DP32(flags, TB_FLAGS, VILL,
>> +                    FIELD_EX64(env->vtype, VTYPE, VILL));
>> +        flags = FIELD_DP32(flags, TB_FLAGS, SEW,
>> +                    FIELD_EX64(env->vtype, VTYPE, SEW));
>> +        flags = FIELD_DP32(flags, TB_FLAGS, LMUL,
>> +                    FIELD_EX64(env->vtype, VTYPE, LMUL));
>> +        flags = FIELD_DP32(flags, TB_FLAGS, VL_EQ_VLMAX, vl_eq_vlmax);
>> +    } else {
>> +        flags = FIELD_DP32(flags, TB_FLAGS, VILL, 1);
>> +    }
>> +
>>   #ifdef CONFIG_USER_ONLY
>> -    *flags = TB_FLAGS_MSTATUS_FS;
>> +    flags |= TB_FLAGS_MSTATUS_FS;
>>   #else
>> -    *flags = cpu_mmu_index(env, 0) | (env->mstatus & MSTATUS_FS);
>> +    flags |= cpu_mmu_index(env, 0) | (env->mstatus & MSTATUS_FS);
>>   #endif
>> +    *pflags = flags;
>>   }
>>
>>   int riscv_csrrw(CPURISCVState *env, int csrno, target_ulong *ret_value,
>> @@ -352,9 +402,4 @@ void riscv_set_csr_ops(int csrno, riscv_csr_operations *ops);
>>
>>   void riscv_cpu_register_gdb_regs_for_features(CPUState *cs);
>>
>> -typedef CPURISCVState CPUArchState;
>> -typedef RISCVCPU ArchCPU;
>> -
>> -#include "exec/cpu-all.h"
>> -
>>   #endif /* RISCV_CPU_H */
>> diff --git a/target/riscv/helper.h b/target/riscv/helper.h
>> index debb22a480..3c28c7e407 100644
>> --- a/target/riscv/helper.h
>> +++ b/target/riscv/helper.h
>> @@ -76,3 +76,5 @@ DEF_HELPER_2(mret, tl, env, tl)
>>   DEF_HELPER_1(wfi, void, env)
>>   DEF_HELPER_1(tlb_flush, void, env)
>>   #endif
>> +/* Vector functions */
>> +DEF_HELPER_3(vsetvl, tl, env, tl, tl)
>> diff --git a/target/riscv/insn32.decode b/target/riscv/insn32.decode
>> index 77f794ed70..5dc009c3cd 100644
>> --- a/target/riscv/insn32.decode
>> +++ b/target/riscv/insn32.decode
>> @@ -62,6 +62,7 @@
>>   @r_rm    .......   ..... ..... ... ..... ....... %rs2 %rs1 %rm %rd
>>   @r2_rm   .......   ..... ..... ... ..... ....... %rs1 %rm %rd
>>   @r2      .......   ..... ..... ... ..... ....... %rs1 %rd
>> +@r2_zimm . zimm:11  ..... ... ..... ....... %rs1 %rd
>>
>>   @sfence_vma ....... ..... .....   ... ..... ....... %rs2 %rs1
>>   @sfence_vm  ....... ..... .....   ... ..... ....... %rs1
>> @@ -203,3 +204,7 @@ fcvt_w_d   1100001  00000 ..... ... ..... 1010011 @r2_rm
>>   fcvt_wu_d  1100001  00001 ..... ... ..... 1010011 @r2_rm
>>   fcvt_d_w   1101001  00000 ..... ... ..... 1010011 @r2_rm
>>   fcvt_d_wu  1101001  00001 ..... ... ..... 1010011 @r2_rm
>> +
>> +# *** RV32V Extension ***
>> +vsetvli         0 ........... ..... 111 ..... 1010111  @r2_zimm
>> +vsetvl          1000000 ..... ..... 111 ..... 1010111  @r
>> diff --git a/target/riscv/insn_trans/trans_rvv.inc.c b/target/riscv/insn_trans/trans_rvv.inc.c
>> new file mode 100644
>> index 0000000000..da82c72bbf
>> --- /dev/null
>> +++ b/target/riscv/insn_trans/trans_rvv.inc.c
>> @@ -0,0 +1,69 @@
>> +/*
>> + * RISC-V translation routines for the RVV Standard Extension.
>> + *
>> + * Copyright (c) 2020 C-SKY Limited. All rights reserved.
>> + *
>> + * 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 trans_vsetvl(DisasContext *ctx, arg_vsetvl * a)
>> +{
>> +    TCGv s1, s2, dst;
>> +    s2 = tcg_temp_new();
>> +    dst = tcg_temp_new();
>> +
>> +    /* Using x0 as the rs1 register specifier, encodes an infinite AVL */
>> +    if (a->rs1 == 0) {
>> +        /* As the mask is at least one bit, RV_VLEN_MAX is >= VLMAX */
>> +        s1 = tcg_const_tl(RV_VLEN_MAX);
>> +    } else {
>> +        s1 = tcg_temp_new();
>> +        gen_get_gpr(s1, a->rs1);
>> +    }
>> +    gen_get_gpr(s2, a->rs2);
>> +    gen_helper_vsetvl(dst, cpu_env, s1, s2);
>> +    gen_set_gpr(a->rd, dst);
>> +    tcg_gen_movi_tl(cpu_pc, ctx->pc_succ_insn);
>> +    exit_tb(ctx);
>> +    ctx->base.is_jmp = DISAS_NORETURN;
>> +
>> +    tcg_temp_free(s1);
>> +    tcg_temp_free(s2);
>> +    tcg_temp_free(dst);
>> +    return true;
>> +}
>> +
>> +static bool trans_vsetvli(DisasContext *ctx, arg_vsetvli * a)
>> +{
>> +    TCGv s1, s2, dst;
>> +    s2 = tcg_const_tl(a->zimm);
>> +    dst = tcg_temp_new();
>> +
>> +    /* Using x0 as the rs1 register specifier, encodes an infinite AVL */
>> +    if (a->rs1 == 0) {
>> +        /* As the mask is at least one bit, RV_VLEN_MAX is >= VLMAX */
>> +        s1 = tcg_const_tl(RV_VLEN_MAX);
>> +    } else {
>> +        s1 = tcg_temp_new();
>> +        gen_get_gpr(s1, a->rs1);
>> +    }
>> +    gen_helper_vsetvl(dst, cpu_env, s1, s2);
>> +    gen_set_gpr(a->rd, dst);
>> +    gen_goto_tb(ctx, 0, ctx->pc_succ_insn);
>> +    ctx->base.is_jmp = DISAS_NORETURN;
>> +
>> +    tcg_temp_free(s1);
>> +    tcg_temp_free(s2);
>> +    tcg_temp_free(dst);
>> +    return true;
>> +}
>> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
>> index 14dc71156b..cc356aabd8 100644
>> --- a/target/riscv/translate.c
>> +++ b/target/riscv/translate.c
>> @@ -55,6 +55,12 @@ typedef struct DisasContext {
>>          to reset this known value.  */
>>       int frm;
>>       bool ext_ifencei;
>> +    /* vector extension */
>> +    bool vill;
>> +    uint8_t lmul;
>> +    uint8_t sew;
>> +    uint16_t vlen;
>> +    bool vl_eq_vlmax;
>>   } DisasContext;
>>
>>   #ifdef TARGET_RISCV64
>> @@ -704,6 +710,7 @@ static bool gen_shift(DisasContext *ctx, arg_r *a,
>>   #include "insn_trans/trans_rva.inc.c"
>>   #include "insn_trans/trans_rvf.inc.c"
>>   #include "insn_trans/trans_rvd.inc.c"
>> +#include "insn_trans/trans_rvv.inc.c"
>>   #include "insn_trans/trans_privileged.inc.c"
>>
>>   /* Include the auto-generated decoder for 16 bit insn */
>> @@ -735,14 +742,20 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>>       DisasContext *ctx = container_of(dcbase, DisasContext, base);
>>       CPURISCVState *env = cs->env_ptr;
>>       RISCVCPU *cpu = RISCV_CPU(cs);
>> +    uint32_t tb_flags = ctx->base.tb->flags;
>>
>>       ctx->pc_succ_insn = ctx->base.pc_first;
>> -    ctx->mem_idx = ctx->base.tb->flags & TB_FLAGS_MMU_MASK;
>> -    ctx->mstatus_fs = ctx->base.tb->flags & TB_FLAGS_MSTATUS_FS;
>> +    ctx->mem_idx = tb_flags & TB_FLAGS_MMU_MASK;
>> +    ctx->mstatus_fs = tb_flags & TB_FLAGS_MSTATUS_FS;
>>       ctx->priv_ver = env->priv_ver;
>>       ctx->misa = env->misa;
>>       ctx->frm = -1;  /* unknown rounding mode */
>>       ctx->ext_ifencei = cpu->cfg.ext_ifencei;
>> +    ctx->vlen = cpu->cfg.vlen;
>> +    ctx->vill = FIELD_EX32(tb_flags, TB_FLAGS, VILL);
>> +    ctx->sew = FIELD_EX32(tb_flags, TB_FLAGS, SEW);
>> +    ctx->lmul = FIELD_EX32(tb_flags, TB_FLAGS, LMUL);
>> +    ctx->vl_eq_vlmax = FIELD_EX32(tb_flags, TB_FLAGS, VL_EQ_VLMAX);
>>   }
>>
>>   static void riscv_tr_tb_start(DisasContextBase *db, CPUState *cpu)
>> diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c
>> new file mode 100644
>> index 0000000000..07db704656
>> --- /dev/null
>> +++ b/target/riscv/vector_helper.c
>> @@ -0,0 +1,53 @@
>> +/*
>> + * RISC-V Vector Extension Helpers for QEMU.
>> + *
>> + * Copyright (c) 2020 C-SKY Limited. All rights reserved.
>> + *
>> + * 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/>.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "cpu.h"
>> +#include "exec/exec-all.h"
>> +#include "exec/helper-proto.h"
>> +#include <math.h>
>> +
>> +target_ulong HELPER(vsetvl)(CPURISCVState *env, target_ulong s1,
>> +    target_ulong s2)
>> +{
>> +    int vlmax, vl;
>> +    RISCVCPU *cpu = env_archcpu(env);
>> +    uint16_t sew = 1 << FIELD_EX64(s2, VTYPE, SEW);
>> +    uint8_t ediv = FIELD_EX64(s2, VTYPE, EDIV);
>> +    bool vill = FIELD_EX64(s2, VTYPE, VILL);
>> +    target_ulong reserved = FIELD_EX64(s2, VTYPE, RESERVED);
>> +
>> +    if ((sew > cpu->cfg.elen) || vill || (ediv != 0) || (reserved != 0)) {
>> +        /* only set vill bit. */
>> +        env->vtype = FIELD_DP64(0, VTYPE, VILL, 1);
>> +        env->vl = 0;
>> +        env->vstart = 0;
>> +        return 0;
>> +    }
>> +
>> +    vlmax = vext_get_vlmax(cpu, s2);
>> +    if (s1 <= vlmax) {
>> +        vl = s1;
>> +    } else {
>> +        vl = vlmax;
>> +    }
>> +    env->vl = vl;
>> +    env->vtype = s2;
>> +    env->vstart = 0;
>> +    return vl;
>> +}
>> --
>> 2.23.0
>>