[v3,3/4] RISC-V: support vector extension csr

Commit Message

LIU Zhiwei Jan. 3, 2020, 3:33 a.m. UTC

Until v0.7.1 specification, vector status is still not defined for
mstatus.

Signed-off-by: LIU Zhiwei <[e-mail]>
---
 target/riscv/cpu_bits.h | 15 +++++++
 target/riscv/csr.c      | 92 +++++++++++++++++++++++++++++------------
 2 files changed, 80 insertions(+), 27 deletions(-)

Comments

Richard Henderson Jan. 3, 2020, 11:14 p.m. UTC | #1

On 1/3/20 2:33 PM, LIU Zhiwei wrote:
> Until v0.7.1 specification, vector status is still not defined for
> mstatus.

Using "until" imples that v0.7.1 *does* define the vector status.

Better said as "The v0.7.1 specification does not define vector status within
mstatus.  A future revision will define the privileged portion of the vector
status."

Although lack of a privileged spec does suggest to me that perhaps we ought to
support the vector extension only for CONFIG_USER_ONLY for now.


r~

Jim Wilson Jan. 6, 2020, 10 p.m. UTC | #2

On 1/2/20 7:33 PM, LIU Zhiwei wrote:
> Until v0.7.1 specification, vector status is still not defined for
> mstatus.

The v0.8 spec does define a VS bit in mstatus.

> @@ -107,11 +112,6 @@ static int pmp(CPURISCVState *env, int csrno)
>   /* User Floating-Point CSRs */
>   static int read_fflags(CPURISCVState *env, int csrno, target_ulong *val)
>   {
> -#if !defined(CONFIG_USER_ONLY)
> -    if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
> -        return -1;
> -    }
> -#endif
>       *val = riscv_cpu_get_fflags(env);
>       return 0;
>   }

This allows reads of fflags when it doesn't exist, and hence does not 
make much sense.  Instead of removing the code, you should add a check 
for the vector extension, since the vector extension requires that fcsr 
exist even if the base architecture doesn't include FP support.  Ideally 
this should use the VS bit, but if you don't have it then you can just 
check to see if the vector extension was enabled as a command line option.

While the vector spec says that fcsr must exist, it doesn't specify that 
the FP fields in fcsr are necessarily readable or writable when there is 
no FP.  It also doesn't specify whether the other FP related shadows of 
fcsr exist, like fflags.  This appears to have been left unspecified.  I 
don't think that you should be making fflags reads and writes work for a 
target with vector but without float.  I think it would make more sense 
to have fcsr behave 3 different ways depending on whether we have only 
F, only V, or both F and V.  And then we can support reads and writes of 
only the valid fields.

Jim

LIU Zhiwei Jan. 7, 2020, 1:34 a.m. UTC | #3

On 2020/1/7 6:00, Jim Wilson wrote:
> On 1/2/20 7:33 PM, LIU Zhiwei wrote:
>> Until v0.7.1 specification, vector status is still not defined for
>> mstatus.
>
> The v0.8 spec does define a VS bit in mstatus.
>
Yes, I will also support v0.8 spec after the v0.7.1 spec.
>> @@ -107,11 +112,6 @@ static int pmp(CPURISCVState *env, int csrno)
>>   /* User Floating-Point CSRs */
>>   static int read_fflags(CPURISCVState *env, int csrno, target_ulong 
>> *val)
>>   {
>> -#if !defined(CONFIG_USER_ONLY)
>> -    if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
>> -        return -1;
>> -    }
>> -#endif
>>       *val = riscv_cpu_get_fflags(env);
>>       return 0;
>>   }
>
> This allows reads of fflags when it doesn't exist, and hence does not 
> make much sense.  Instead of removing the code, you should add a check 
> for the vector extension, since the vector extension requires that 
> fcsr exist even if the base architecture doesn't include FP support.  
> Ideally this should use the VS bit, but if you don't have it then you 
> can just check to see if the vector extension was enabled as a command 
> line option.
>
I' sorry that there is some ambiguous here. The reason to remove these 
code is that they are redundant, and has nothing to do with the vector 
extension.  I just delete them by hand.

As you can see, all float csr has a predicate function.

static int fs(CPURISCVState *env, int csrno)
{
#if !defined(CONFIG_USER_ONLY)
     if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
         return -1;
     }
#endif
     return 0;
}

The read or write function must be called after the predicate return 0. 
So no need to check (!env->debugger && !(env->mstatus & MSTATUS_FS)  again.
> While the vector spec says that fcsr must exist, it doesn't specify 
> that the FP fields in fcsr are necessarily readable or writable when 
> there is no FP.  It also doesn't specify whether the other FP related 
> shadows of fcsr exist, like fflags.  This appears to have been left 
> unspecified.  I don't think that you should be making fflags reads and 
> writes work for a target with vector but without float.  I think it 
> would make more sense to have fcsr behave 3 different ways depending 
> on whether we have only F, only V, or both F and V.  And then we can 
> support reads and writes of only the valid fields.
>
Thanks. Maybe I should just only loose the check condition for fcsr.

Best Regards,
Zhiwei
> Jim

Patch

diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index 11f971ad5d..9eb43ecc1e 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)
+
+/* Vector Fixed-Point saturation flag */
+#define FSR_VXSAT_SHIFT     8
+#define FSR_VXSAT           (0x1 << FSR_VXSAT_SHIFT)
+
 /* 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 e0d4586760..506ad7b590 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -53,6 +53,11 @@  static int fs(CPURISCVState *env, int csrno)
     return 0;
 }
 
+static int vs(CPURISCVState *env, int csrno)
+{
+    return 0;
+}
+
 static int ctr(CPURISCVState *env, int csrno)
 {
 #if !defined(CONFIG_USER_ONLY)
@@ -107,11 +112,6 @@  static int pmp(CPURISCVState *env, int csrno)
 /* User Floating-Point CSRs */
 static int read_fflags(CPURISCVState *env, int csrno, target_ulong *val)
 {
-#if !defined(CONFIG_USER_ONLY)
-    if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
-        return -1;
-    }
-#endif
     *val = riscv_cpu_get_fflags(env);
     return 0;
 }
@@ -119,9 +119,6 @@  static int read_fflags(CPURISCVState *env, int csrno, target_ulong *val)
 static int write_fflags(CPURISCVState *env, int csrno, target_ulong val)
 {
 #if !defined(CONFIG_USER_ONLY)
-    if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
-        return -1;
-    }
     env->mstatus |= MSTATUS_FS;
 #endif
     riscv_cpu_set_fflags(env, val & (FSR_AEXC >> FSR_AEXC_SHIFT));
@@ -130,11 +127,6 @@  static int write_fflags(CPURISCVState *env, int csrno, target_ulong val)
 
 static int read_frm(CPURISCVState *env, int csrno, target_ulong *val)
 {
-#if !defined(CONFIG_USER_ONLY)
-    if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
-        return -1;
-    }
-#endif
     *val = env->frm;
     return 0;
 }
@@ -142,9 +134,6 @@  static int read_frm(CPURISCVState *env, int csrno, target_ulong *val)
 static int write_frm(CPURISCVState *env, int csrno, target_ulong val)
 {
 #if !defined(CONFIG_USER_ONLY)
-    if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
-        return -1;
-    }
     env->mstatus |= MSTATUS_FS;
 #endif
     env->frm = val & (FSR_RD >> FSR_RD_SHIFT);
@@ -153,29 +142,73 @@  static int write_frm(CPURISCVState *env, int csrno, target_ulong val)
 
 static int read_fcsr(CPURISCVState *env, int csrno, target_ulong *val)
 {
-#if !defined(CONFIG_USER_ONLY)
-    if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
-        return -1;
-    }
-#endif
-    *val = (riscv_cpu_get_fflags(env) << FSR_AEXC_SHIFT)
-        | (env->frm << FSR_RD_SHIFT);
+    *val = (env->vext.vxrm << FSR_VXRM_SHIFT)
+            | (env->vext.vxsat << FSR_VXSAT_SHIFT)
+            | (riscv_cpu_get_fflags(env) << FSR_AEXC_SHIFT)
+            | (env->frm << FSR_RD_SHIFT);
     return 0;
 }
 
 static int write_fcsr(CPURISCVState *env, int csrno, target_ulong val)
 {
 #if !defined(CONFIG_USER_ONLY)
-    if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
-        return -1;
-    }
     env->mstatus |= MSTATUS_FS;
 #endif
     env->frm = (val & FSR_RD) >> FSR_RD_SHIFT;
+    env->vext.vxrm = (val & FSR_VXRM) >> FSR_VXRM_SHIFT;
+    env->vext.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->vext.vtype;
+    return 0;
+}
+
+static int read_vl(CPURISCVState *env, int csrno, target_ulong *val)
+{
+    *val = env->vext.vl;
+    return 0;
+}
+
+static int read_vxrm(CPURISCVState *env, int csrno, target_ulong *val)
+{
+    *val = env->vext.vxrm;
+    return 0;
+}
+
+static int read_vxsat(CPURISCVState *env, int csrno, target_ulong *val)
+{
+    *val = env->vext.vxsat;
+    return 0;
+}
+
+static int read_vstart(CPURISCVState *env, int csrno, target_ulong *val)
+{
+    *val = env->vext.vstart;
+    return 0;
+}
+
+static int write_vxrm(CPURISCVState *env, int csrno, target_ulong val)
+{
+    env->vext.vxrm = val;
+    return 0;
+}
+
+static int write_vxsat(CPURISCVState *env, int csrno, target_ulong val)
+{
+    env->vext.vxsat = val;
+    return 0;
+}
+
+static int write_vstart(CPURISCVState *env, int csrno, target_ulong val)
+{
+    env->vext.vstart = val;
+    return 0;
+}
+
 /* User Timers and Counters */
 static int read_instret(CPURISCVState *env, int csrno, target_ulong *val)
 {
@@ -873,7 +906,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                        },