diff mbox series

[PULL,53/61] target/riscv: Split out the vill from vtype

Message ID 20220121055830.3164408-54-alistair.francis@opensource.wdc.com (mailing list archive)
State New, archived
Headers show
Series [PULL,01/61] hw: timer: ibex_timer: Fixup reading w/o register | expand

Commit Message

Alistair Francis Jan. 21, 2022, 5:58 a.m. UTC 
From: LIU Zhiwei <zhiwei_liu@c-sky.com>

We need not specially process vtype when XLEN changes.

Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Message-id: 20220120122050.41546-16-zhiwei_liu@c-sky.com
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.h           |  1 +
 target/riscv/cpu_helper.c    |  3 +--
 target/riscv/csr.c           | 13 ++++++++++++-
 target/riscv/machine.c       |  5 +++--
 target/riscv/vector_helper.c |  3 ++-
 5 files changed, 19 insertions(+), 6 deletions(-)

Comments

Peter Maydell Jan. 28, 2022, 4:10 p.m. UTC | #1 
On Fri, 21 Jan 2022 at 09:42, Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: LIU Zhiwei <zhiwei_liu@c-sky.com>
>
> We need not specially process vtype when XLEN changes.
>
> Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Message-id: 20220120122050.41546-16-zhiwei_liu@c-sky.com
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

Odd thing I noticed looking at this code: as far as I can see we
may set env->vill to 1 in the vsetvl helper, but there is nowhere
that we set it to 0, so once it transitions to 1 it's stuck there
until the system is reset. Is this really right?

> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 89621e1996..6c740b92c1 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -125,6 +125,7 @@ struct CPURISCVState {
>      target_ulong vl;
>      target_ulong vstart;
>      target_ulong vtype;
> +    bool vill;
>
>      target_ulong pc;
>      target_ulong load_res;
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 502aee84ab..327a2c4f1d 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -60,8 +60,7 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
>          uint32_t maxsz = vlmax << sew;
>          bool vl_eq_vlmax = (env->vstart == 0) && (vlmax == env->vl) &&
>                             (maxsz >= 8);
> -        flags = FIELD_DP32(flags, TB_FLAGS, VILL,
> -                    FIELD_EX64(env->vtype, VTYPE, VILL));
> +        flags = FIELD_DP32(flags, TB_FLAGS, VILL, env->vill);
>          flags = FIELD_DP32(flags, TB_FLAGS, SEW, sew);
>          flags = FIELD_DP32(flags, TB_FLAGS, LMUL,
>                      FIELD_EX64(env->vtype, VTYPE, VLMUL));
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 292f7e1624..b11d92b51b 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -283,7 +283,18 @@ static RISCVException write_fcsr(CPURISCVState *env, int csrno,
>  static RISCVException read_vtype(CPURISCVState *env, int csrno,
>                                   target_ulong *val)
>  {
> -    *val = env->vtype;
> +    uint64_t vill;
> +    switch (env->xl) {
> +    case MXL_RV32:
> +        vill = (uint32_t)env->vill << 31;
> +        break;
> +    case MXL_RV64:
> +        vill = (uint64_t)env->vill << 63;
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
> +    *val = (target_ulong)vill | env->vtype;
>      return RISCV_EXCP_NONE;
>  }
>
> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> index a4b7859c2a..740e11fcff 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -124,8 +124,8 @@ static bool vector_needed(void *opaque)
>
>  static const VMStateDescription vmstate_vector = {
>      .name = "cpu/vector",
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> +    .version_id = 2,
> +    .minimum_version_id = 2,
>      .needed = vector_needed,
>      .fields = (VMStateField[]) {
>              VMSTATE_UINT64_ARRAY(env.vreg, RISCVCPU, 32 * RV_VLEN_MAX / 64),
> @@ -134,6 +134,7 @@ static const VMStateDescription vmstate_vector = {
>              VMSTATE_UINTTL(env.vl, RISCVCPU),
>              VMSTATE_UINTTL(env.vstart, RISCVCPU),
>              VMSTATE_UINTTL(env.vtype, RISCVCPU),
> +            VMSTATE_BOOL(env.vill, RISCVCPU),
>              VMSTATE_END_OF_LIST()
>          }
>  };
> diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c
> index ad505ec9b2..a9484c22ea 100644
> --- a/target/riscv/vector_helper.c
> +++ b/target/riscv/vector_helper.c
> @@ -52,7 +52,8 @@ target_ulong HELPER(vsetvl)(CPURISCVState *env, target_ulong s1,
>          || (ediv != 0)
>          || (reserved != 0)) {
>          /* only set vill bit. */
> -        env->vtype = FIELD_DP64(0, VTYPE, VILL, 1);
> +        env->vill = 1;
> +        env->vtype = 0;
>          env->vl = 0;
>          env->vstart = 0;
>          return 0;
> --
> 2.31.1

thanks
-- PMM
Alistair Francis Feb. 1, 2022, 2:12 a.m. UTC | #2 
On Sat, Jan 29, 2022 at 2:10 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 21 Jan 2022 at 09:42, Alistair Francis
> <alistair.francis@opensource.wdc.com> wrote:
> >
> > From: LIU Zhiwei <zhiwei_liu@c-sky.com>
> >
> > We need not specially process vtype when XLEN changes.
> >
> > Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> > Message-id: 20220120122050.41546-16-zhiwei_liu@c-sky.com
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>
> Odd thing I noticed looking at this code: as far as I can see we
> may set env->vill to 1 in the vsetvl helper, but there is nowhere
> that we set it to 0, so once it transitions to 1 it's stuck there
> until the system is reset. Is this really right?

This is really confusing. It implies that you can't set vill from
software, but that just seems to be confusing wording.

Reading https://lists.riscv.org/g/tech-vector-ext/topic/reliably_set_vtype_vill/86745728
it seems that this is a QEMU bug and the guest should be able to set
the bit as part of vsetvl

@LIU Zhiwei are you able to fix this up?


Alistair
LIU Zhiwei Feb. 1, 2022, 6:47 a.m. UTC | #3 
On 2022/2/1 10:12, Alistair Francis wrote:
> On Sat, Jan 29, 2022 at 2:10 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>> On Fri, 21 Jan 2022 at 09:42, Alistair Francis
>> <alistair.francis@opensource.wdc.com> wrote:
>>> From: LIU Zhiwei <zhiwei_liu@c-sky.com>
>>>
>>> We need not specially process vtype when XLEN changes.
>>>
>>> Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>>> Message-id: 20220120122050.41546-16-zhiwei_liu@c-sky.com
>>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>> Odd thing I noticed looking at this code: as far as I can see we
>> may set env->vill to 1 in the vsetvl helper, but there is nowhere
>> that we set it to 0, so once it transitions to 1 it's stuck there
>> until the system is reset. Is this really right?
> This is really confusing. It implies that you can't set vill from
> software, but that just seems to be confusing wording.
>
> Reading https://lists.riscv.org/g/tech-vector-ext/topic/reliably_set_vtype_vill/86745728
> it seems that this is a QEMU bug and the guest should be able to set
> the bit as part of vsetvl
>
> @LIU Zhiwei are you able to fix this up?

Thanks for pointing it out. I have sent a patch to fix this up.

Thanks,
Zhiwei

>
>
> Alistair
diff mbox series

Patch

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 89621e1996..6c740b92c1 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -125,6 +125,7 @@  struct CPURISCVState {
     target_ulong vl;
     target_ulong vstart;
     target_ulong vtype;
+    bool vill;
 
     target_ulong pc;
     target_ulong load_res;
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 502aee84ab..327a2c4f1d 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -60,8 +60,7 @@  void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
         uint32_t maxsz = vlmax << sew;
         bool vl_eq_vlmax = (env->vstart == 0) && (vlmax == env->vl) &&
                            (maxsz >= 8);
-        flags = FIELD_DP32(flags, TB_FLAGS, VILL,
-                    FIELD_EX64(env->vtype, VTYPE, VILL));
+        flags = FIELD_DP32(flags, TB_FLAGS, VILL, env->vill);
         flags = FIELD_DP32(flags, TB_FLAGS, SEW, sew);
         flags = FIELD_DP32(flags, TB_FLAGS, LMUL,
                     FIELD_EX64(env->vtype, VTYPE, VLMUL));
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 292f7e1624..b11d92b51b 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -283,7 +283,18 @@  static RISCVException write_fcsr(CPURISCVState *env, int csrno,
 static RISCVException read_vtype(CPURISCVState *env, int csrno,
                                  target_ulong *val)
 {
-    *val = env->vtype;
+    uint64_t vill;
+    switch (env->xl) {
+    case MXL_RV32:
+        vill = (uint32_t)env->vill << 31;
+        break;
+    case MXL_RV64:
+        vill = (uint64_t)env->vill << 63;
+        break;
+    default:
+        g_assert_not_reached();
+    }
+    *val = (target_ulong)vill | env->vtype;
     return RISCV_EXCP_NONE;
 }
 
diff --git a/target/riscv/machine.c b/target/riscv/machine.c
index a4b7859c2a..740e11fcff 100644
--- a/target/riscv/machine.c
+++ b/target/riscv/machine.c
@@ -124,8 +124,8 @@  static bool vector_needed(void *opaque)
 
 static const VMStateDescription vmstate_vector = {
     .name = "cpu/vector",
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .needed = vector_needed,
     .fields = (VMStateField[]) {
             VMSTATE_UINT64_ARRAY(env.vreg, RISCVCPU, 32 * RV_VLEN_MAX / 64),
@@ -134,6 +134,7 @@  static const VMStateDescription vmstate_vector = {
             VMSTATE_UINTTL(env.vl, RISCVCPU),
             VMSTATE_UINTTL(env.vstart, RISCVCPU),
             VMSTATE_UINTTL(env.vtype, RISCVCPU),
+            VMSTATE_BOOL(env.vill, RISCVCPU),
             VMSTATE_END_OF_LIST()
         }
 };
diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c
index ad505ec9b2..a9484c22ea 100644
--- a/target/riscv/vector_helper.c
+++ b/target/riscv/vector_helper.c
@@ -52,7 +52,8 @@  target_ulong HELPER(vsetvl)(CPURISCVState *env, target_ulong s1,
         || (ediv != 0)
         || (reserved != 0)) {
         /* only set vill bit. */
-        env->vtype = FIELD_DP64(0, VTYPE, VILL, 1);
+        env->vill = 1;
+        env->vtype = 0;
         env->vl = 0;
         env->vstart = 0;
         return 0;