[v2] Support UXL filed in xstatus

[PATCH v2 12/14] target/riscv: Split out the vill from vtype

Posted by LIU Zhiwei 4 years, 3 months ago

We need not specially process vtype when XLEN changes.
Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
---
 target/riscv/cpu.h           |  1 +
 target/riscv/csr.c           | 15 ++++++++++++++-
 target/riscv/machine.c       |  1 +
 target/riscv/vector_helper.c |  7 ++-----
 4 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 73d7aa9ad7..e67531deab 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -121,6 +121,7 @@ struct CPURISCVState {
     target_ulong vl;
     target_ulong vstart;
     target_ulong vtype;
+    target_ulong vill;
 
     target_ulong pc;
     target_ulong load_res;
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 59e368f004..33e342f529 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -286,7 +286,20 @@ static RISCVException write_fcsr(CPURISCVState *env, int csrno,
 static RISCVException read_vtype(CPURISCVState *env, int csrno,
                                  target_ulong *val)
 {
-    *val = env->vtype;
+    target_ulong vill;
+    switch (cpu_get_xl(env)) {
+    case MXL_RV32:
+        vill = env->vill << 31;
+        break;
+#ifdef TARGET_RISCV64
+    case MXL_RV64:
+        vill = env->vill << 63;
+        break;
+#endif
+    default:
+        g_assert_not_reached();
+    }
+    *val = vill | env->vtype;
     return RISCV_EXCP_NONE;
 }
 
diff --git a/target/riscv/machine.c b/target/riscv/machine.c
index 19e982d3f0..cc4dda4b93 100644
--- a/target/riscv/machine.c
+++ b/target/riscv/machine.c
@@ -104,6 +104,7 @@ static const VMStateDescription vmstate_vector = {
             VMSTATE_UINTTL(env.vl, RISCVCPU),
             VMSTATE_UINTTL(env.vstart, RISCVCPU),
             VMSTATE_UINTTL(env.vtype, RISCVCPU),
+            VMSTATE_UINTTL(env.vill, RISCVCPU),
             VMSTATE_END_OF_LIST()
         }
 };
diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c
index 51bcf63d65..7d7b554789 100644
--- a/target/riscv/vector_helper.c
+++ b/target/riscv/vector_helper.c
@@ -45,11 +45,8 @@ target_ulong HELPER(vsetvl)(CPURISCVState *env, target_ulong s1,
     }
     if ((sew > cpu->cfg.elen) || vill || (ediv != 0) || (reserved != 0)) {
         /* only set vill bit. */
-        if (xlen < TARGET_LONG_BITS) {
-            env->vtype = FIELD_DP64(0, VTYPE, VILL_XLEN32, 1);
-        } else {
-            env->vtype = FIELD_DP64(0, VTYPE, VILL, 1);
-        }
+        env->vill = 1;
+        env->vtype = 0;
         env->vl = 0;
         env->vstart = 0;
         return 0;
-- 
2.25.1

Re: [PATCH v2 12/14] target/riscv: Split out the vill from vtype

Posted by Richard Henderson 4 years, 3 months ago

On 11/10/21 8:04 AM, LIU Zhiwei wrote:
> We need not specially process vtype when XLEN changes.
> Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
> ---
>   target/riscv/cpu.h           |  1 +
>   target/riscv/csr.c           | 15 ++++++++++++++-
>   target/riscv/machine.c       |  1 +
>   target/riscv/vector_helper.c |  7 ++-----
>   4 files changed, 18 insertions(+), 6 deletions(-)

This patch should come before patch 6, which is over-complicated.

> +    target_ulong vill;

This could be bool, though there's no good place to slot it that does not result in unused 
padding.  Comments should be added to show that this bit is now missing from vtype.

> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> index 19e982d3f0..cc4dda4b93 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -104,6 +104,7 @@ static const VMStateDescription vmstate_vector = {
>               VMSTATE_UINTTL(env.vl, RISCVCPU),
>               VMSTATE_UINTTL(env.vstart, RISCVCPU),
>               VMSTATE_UINTTL(env.vtype, RISCVCPU),
> +            VMSTATE_UINTTL(env.vill, RISCVCPU),
>               VMSTATE_END_OF_LIST()

This will need a bump to version.

> @@ -45,11 +45,8 @@ target_ulong HELPER(vsetvl)(CPURISCVState *env, target_ulong s1,
>       }
>       if ((sew > cpu->cfg.elen) || vill || (ediv != 0) || (reserved != 0)) {
>           /* only set vill bit. */
> -        if (xlen < TARGET_LONG_BITS) {
> -            env->vtype = FIELD_DP64(0, VTYPE, VILL_XLEN32, 1);
> -        } else {
> -            env->vtype = FIELD_DP64(0, VTYPE, VILL, 1);
> -        }
> +        env->vill = 1;
> +        env->vtype = 0;

This is fine.

You're missing the updates to cpu_get_tb_cpu_state.


r~

Re: [PATCH v2 12/14] target/riscv: Split out the vill from vtype

Posted by LIU Zhiwei 4 years, 3 months ago

On 2021/11/10 下午7:23, Richard Henderson wrote:
> On 11/10/21 8:04 AM, LIU Zhiwei wrote:
>> We need not specially process vtype when XLEN changes.
>> Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
>> ---
>>   target/riscv/cpu.h           |  1 +
>>   target/riscv/csr.c           | 15 ++++++++++++++-
>>   target/riscv/machine.c       |  1 +
>>   target/riscv/vector_helper.c |  7 ++-----
>>   4 files changed, 18 insertions(+), 6 deletions(-)
>
> This patch should come before patch 6, which is over-complicated.

Agree.

One question here. Even come before patch 6, we don't have a simple way 
to choose vill and reserved fields from s2 register in patch 6.

>
>> +    target_ulong vill;
>
> This could be bool, though there's no good place to slot it that does 
> not result in unused padding.

As env->vill will be used in read_vtype,  we still need to covert 
env->vill type to target_ulong there.
Is there any benefit to use bool instead of target_ulong?

> Comments should be added to show that this bit is now missing from vtype.
>
>> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
>> index 19e982d3f0..cc4dda4b93 100644
>> --- a/target/riscv/machine.c
>> +++ b/target/riscv/machine.c
>> @@ -104,6 +104,7 @@ static const VMStateDescription vmstate_vector = {
>>               VMSTATE_UINTTL(env.vl, RISCVCPU),
>>               VMSTATE_UINTTL(env.vstart, RISCVCPU),
>>               VMSTATE_UINTTL(env.vtype, RISCVCPU),
>> +            VMSTATE_UINTTL(env.vill, RISCVCPU),
>>               VMSTATE_END_OF_LIST()
>
> This will need a bump to version.
>
>> @@ -45,11 +45,8 @@ target_ulong HELPER(vsetvl)(CPURISCVState *env, 
>> target_ulong s1,
>>       }
>>       if ((sew > cpu->cfg.elen) || vill || (ediv != 0) || (reserved 
>> != 0)) {
>>           /* only set vill bit. */
>> -        if (xlen < TARGET_LONG_BITS) {
>> -            env->vtype = FIELD_DP64(0, VTYPE, VILL_XLEN32, 1);
>> -        } else {
>> -            env->vtype = FIELD_DP64(0, VTYPE, VILL, 1);
>> -        }
>> +        env->vill = 1;
>> +        env->vtype = 0;
>
> This is fine.
>
> You're missing the updates to cpu_get_tb_cpu_state.

Yes.

Thanks,
Zhiwei

>
>
> r~

Re: [PATCH v2 12/14] target/riscv: Split out the vill from vtype

Posted by Richard Henderson 4 years, 3 months ago

On 11/10/21 3:26 PM, LIU Zhiwei wrote:
> One question here. Even come before patch 6, we don't have a simple way to choose vill and 
> reserved fields from s2 register in patch 6.

You can certainly split out vill before you create a new way to select it based on xlen. 
In fact, you *should* do that as a separate patch before extending helper_vsetvl to handle 
multiple xlen.

Note that vill is always at the msb, so it's easy to find without necessarily defining an 
XLEN32 field.

As for the reserved "field"... how about

     reserved = MAKE_64BIT_MASK(R_VTYPE_RESERVED_START,
                                xlen - 1 - R_VTYPE_RESERVED_START);
     reserved &= s2;

> As env->vill will be used in read_vtype,  we still need to covert env->vill type to 
> target_ulong there.

A cast in read_vtype will do.  Explicit casts to uint32_t/uint64_t would remove the need 
for an ifdef in that function as well.

> Is there any benefit to use bool instead of target_ulong?

Self-documentation?

r~