Re: [PATCH v5 3/4] target/riscv: support vector extension csr

[LIU Zhiwei]

On 2020/2/27 2:42, Alistair Francis wrote:
> On Fri, Feb 21, 2020 at 1:45 AM LIU Zhiwei <address@hidden> wrote:
> > The v0.7.1 specification does not define vector status within mstatus.
> > A future revision will define the privileged portion of the vector status.
> >
> > Signed-off-by: LIU Zhiwei <address@hidden>
> > ---
> >   target/riscv/cpu_bits.h | 15 +++++++++
> >   target/riscv/csr.c      | 75 ++++++++++++++++++++++++++++++++++++++++-
> >   2 files changed, 89 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> > index e99834856c..1f588ebc14 100644
> > --- a/target/riscv/cpu_bits.h
> > +++ b/target/riscv/cpu_bits.h
> > @@ -29,6 +29,14 @@
> >   #define FSR_NXA             (FPEXC_NX << FSR_AEXC_SHIFT)
> >   #define FSR_AEXC            (FSR_NVA | FSR_OFA | FSR_UFA | FSR_DZA | FSR_NXA)
> >
> > +/* Vector Fixed-Point round model */
> > +#define FSR_VXRM_SHIFT      9
> > +#define FSR_VXRM            (0x3 << FSR_VXRM_SHIFT)
> Shouldn't these be FSCR_*?
Like other fields in fcsr, they all have been named by FSR_*, so I just 
name it like before.
> > +
> > +/* Vector Fixed-Point saturation flag */
> > +#define FSR_VXSAT_SHIFT     8
> > +#define FSR_VXSAT           (0x1 << FSR_VXSAT_SHIFT)
> Same here, FCSR_*
>
> > +
> >   /* Control and Status Registers */
> >
> >   /* User Trap Setup */
> > @@ -48,6 +56,13 @@
> >   #define CSR_FRM             0x002
> >   #define CSR_FCSR            0x003
> >
> > +/* User Vector CSRs */
> > +#define CSR_VSTART          0x008
> > +#define CSR_VXSAT           0x009
> > +#define CSR_VXRM            0x00a
> > +#define CSR_VL              0xc20
> > +#define CSR_VTYPE           0xc21
> > +
> >   /* User Timers and Counters */
> >   #define CSR_CYCLE           0xc00
> >   #define CSR_TIME            0xc01
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 0e34c292c5..9cd2b418bf 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -46,6 +46,10 @@ void riscv_set_csr_ops(int csrno, riscv_csr_operations *ops)
> >   static int fs(CPURISCVState *env, int csrno)
> >   {
> >   #if !defined(CONFIG_USER_ONLY)
> > +    /* loose check condition for fcsr in vector extension */
> > +    if ((csrno == CSR_FCSR) && (env->misa & RVV)) {
> > +        return 0;
> > +    }
> >       if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
> >           return -1;
> >       }
> > @@ -53,6 +57,14 @@ static int fs(CPURISCVState *env, int csrno)
> >       return 0;
> >   }
> >
> > +static int vs(CPURISCVState *env, int csrno)
> > +{
> > +    if (env->misa & RVV) {
> > +        return 0;
> > +    }
> > +    return -1;
> > +}
> > +
> >   static int ctr(CPURISCVState *env, int csrno)
> >   {
> >   #if !defined(CONFIG_USER_ONLY)
> > @@ -160,6 +172,10 @@ static int read_fcsr(CPURISCVState *env, int csrno, 
> > target_ulong *val)
> >   #endif
> >       *val = (riscv_cpu_get_fflags(env) << FSR_AEXC_SHIFT)
> >           | (env->frm << FSR_RD_SHIFT);
> > +    if (vs(env, csrno) >= 0) {
> > +        *val |= (env->vxrm << FSR_VXRM_SHIFT)
> > +                | (env->vxsat << FSR_VXSAT_SHIFT);
> > +    }
> >       return 0;
> >   }
> >
> > @@ -172,10 +188,62 @@ static int write_fcsr(CPURISCVState *env, int csrno, 
> > target_ulong val)
> >       env->mstatus |= MSTATUS_FS;
> >   #endif
> >       env->frm = (val & FSR_RD) >> FSR_RD_SHIFT;
> > +    if (vs(env, csrno) >= 0) {
> > +        env->vxrm = (val & FSR_VXRM) >> FSR_VXRM_SHIFT;
> > +        env->vxsat = (val & FSR_VXSAT) >> FSR_VXSAT_SHIFT;
> > +    }
> >       riscv_cpu_set_fflags(env, (val & FSR_AEXC) >> FSR_AEXC_SHIFT);
> >       return 0;
> >   }
> >
> > +static int read_vtype(CPURISCVState *env, int csrno, target_ulong *val)
> > +{
> > +    *val = env->vtype;
> > +    return 0;
> > +}
> > +
> > +static int read_vl(CPURISCVState *env, int csrno, target_ulong *val)
> > +{
> > +    *val = env->vl;
> > +    return 0;
> > +}
> > +
> > +static int read_vxrm(CPURISCVState *env, int csrno, target_ulong *val)
> > +{
> > +    *val = env->vxrm;
> > +    return 0;
> > +}
> > +
> > +static int read_vxsat(CPURISCVState *env, int csrno, target_ulong *val)
> > +{
> > +    *val = env->vxsat;
> > +    return 0;
> > +}
> > +
> > +static int read_vstart(CPURISCVState *env, int csrno, target_ulong *val)
> > +{
> > +    *val = env->vstart;
> > +    return 0;
> > +}
> > +
> > +static int write_vxrm(CPURISCVState *env, int csrno, target_ulong val)
> > +{
> > +    env->vxrm = val;
> > +    return 0;
> > +}
> > +
> > +static int write_vxsat(CPURISCVState *env, int csrno, target_ulong val)
> > +{
> > +    env->vxsat = val;
> > +    return 0;
> > +}
> > +
> > +static int write_vstart(CPURISCVState *env, int csrno, target_ulong val)
> > +{
> > +    env->vstart = val;
> > +    return 0;
> > +}
> Can you keep these in read/write order? So read_vxrm() then
> write_vxrm() for example.
OK.
> Otherwise the patch looks good :)
>
> Alistair
>
> > +
> >   /* User Timers and Counters */
> >   static int read_instret(CPURISCVState *env, int csrno, target_ulong *val)
> >   {
> > @@ -877,7 +945,12 @@ static riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
> >       [CSR_FFLAGS] =              { fs,   read_fflags,      write_fflags      },
> >       [CSR_FRM] =                 { fs,   read_frm,         write_frm         },
> >       [CSR_FCSR] =                { fs,   read_fcsr,        write_fcsr        },
> > -
> > +    /* Vector CSRs */
> > +    [CSR_VSTART] =              { vs,   read_vstart,      write_vstart      },
> > +    [CSR_VXSAT] =               { vs,   read_vxsat,       write_vxsat       },
> > +    [CSR_VXRM] =                { vs,   read_vxrm,        write_vxrm        },
> > +    [CSR_VL] =                  { vs,   read_vl                             },
> > +    [CSR_VTYPE] =               { vs,   read_vtype                          },
> >       /* User Timers and Counters */
> >       [CSR_CYCLE] =               { ctr,  read_instret                        },
> >       [CSR_INSTRET] =             { ctr,  read_instret                        },
> > --
> > 2.23.0