[PATCH 1/2] target/riscv: Preliminary textra trigger CSR writting support

Alvin Chang via posted 2 patches 4 months, 3 weeks ago
Maintainers: Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Bin Meng <bmeng.cn@gmail.com>, Weiwei Li <liwei1518@gmail.com>, Daniel Henrique Barboza <dbarboza@ventanamicro.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>
There is a newer version of this series
[PATCH 1/2] target/riscv: Preliminary textra trigger CSR writting support
Posted by Alvin Chang via 4 months, 3 weeks ago
This commit allows program to write textra trigger CSR for type 2, 3, 6
triggers. In this preliminary patch, the textra.MHVALUE and the
textra.MHSELECT fields are allowed to be configured. Other fields, such
as textra.SBYTEMASK, textra.SVALUE, and textra.SSELECT, are hardwired to
zero for now.

For textra.MHSELECT field, the only legal values are 0 (ignore) and 4
(mcontext). Writing 1~3 into textra.MHSELECT will be changed to 0, and
writing 5~7 into textra.MHSELECT will be changed to 4. This behavior is
aligned to RISC-V SPIKE simulator.

Signed-off-by: Alvin Chang <alvinga@andestech.com>
---
 target/riscv/cpu_bits.h | 10 +++++
 target/riscv/debug.c    | 81 ++++++++++++++++++++++++++++++++++++++---
 2 files changed, 85 insertions(+), 6 deletions(-)

diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index c257c5ed7d..0530b4f9f4 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -906,6 +906,16 @@ typedef enum RISCVException {
 #define JVT_BASE                           (~0x3F)
 
 /* Debug Sdtrig CSR masks */
+#define TEXTRA32_MHVALUE                   0xFC000000
+#define TEXTRA32_MHSELECT                  0x03800000
+#define TEXTRA32_SBYTEMASK                 0x000C0000
+#define TEXTRA32_SVALUE                    0x0003FFFC
+#define TEXTRA32_SSELECT                   0x00000003
+#define TEXTRA64_MHVALUE                   0xFFF8000000000000ULL
+#define TEXTRA64_MHSELECT                  0x0007000000000000ULL
+#define TEXTRA64_SBYTEMASK                 0x000000F000000000ULL
+#define TEXTRA64_SVALUE                    0x00000003FFFFFFFCULL
+#define TEXTRA64_SSELECT                   0x0000000000000003ULL
 #define MCONTEXT32                         0x0000003F
 #define MCONTEXT64                         0x0000000000001FFFULL
 #define MCONTEXT32_HCONTEXT                0x0000007F
diff --git a/target/riscv/debug.c b/target/riscv/debug.c
index 0b5099ff9a..f7d8f5e320 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -217,6 +217,69 @@ static inline void warn_always_zero_bit(target_ulong val, target_ulong mask,
     }
 }
 
+static target_ulong textra_validate(CPURISCVState *env, target_ulong tdata3)
+{
+    target_ulong mhvalue, mhselect;
+    target_ulong mhselect_new;
+    target_ulong textra;
+    const uint32_t mhselect_no_rvh[8] = { 0, 0, 0, 0, 4, 4, 4, 4 };
+
+    switch (riscv_cpu_mxl(env)) {
+    case MXL_RV32:
+        mhvalue  = get_field(tdata3, TEXTRA32_MHVALUE);
+        mhselect = get_field(tdata3, TEXTRA32_MHSELECT);
+        /* Validate unimplemented (always zero) bits */
+        warn_always_zero_bit(tdata3, (target_ulong)TEXTRA32_SBYTEMASK,
+                             "sbytemask");
+        warn_always_zero_bit(tdata3, (target_ulong)TEXTRA32_SVALUE,
+                             "svalue");
+        warn_always_zero_bit(tdata3, (target_ulong)TEXTRA32_SSELECT,
+                             "sselect");
+        break;
+    case MXL_RV64:
+    case MXL_RV128:
+        mhvalue  = get_field(tdata3, TEXTRA64_MHVALUE);
+        mhselect = get_field(tdata3, TEXTRA64_MHSELECT);
+        /* Validate unimplemented (always zero) bits */
+        warn_always_zero_bit(tdata3, (target_ulong)TEXTRA64_SBYTEMASK,
+                             "sbytemask");
+        warn_always_zero_bit(tdata3, (target_ulong)TEXTRA64_SVALUE,
+                             "svalue");
+        warn_always_zero_bit(tdata3, (target_ulong)TEXTRA64_SSELECT,
+                             "sselect");
+        break;
+    default:
+        g_assert_not_reached();
+    }
+
+    /* Validate mhselect. */
+    mhselect_new = mhselect_no_rvh[mhselect];
+
+    /* Write legal values into textra */
+    textra = 0;
+    switch (riscv_cpu_mxl(env)) {
+    case MXL_RV32:
+        textra = set_field(textra, TEXTRA32_MHVALUE,  mhvalue);
+        textra = set_field(textra, TEXTRA32_MHSELECT, mhselect_new);
+        break;
+    case MXL_RV64:
+    case MXL_RV128:
+        textra = set_field(textra, TEXTRA64_MHVALUE,  mhvalue);
+        textra = set_field(textra, TEXTRA64_MHSELECT, mhselect_new);
+        break;
+    default:
+        g_assert_not_reached();
+    }
+
+    if (textra != tdata3) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "different value 0x" TARGET_FMT_lx " write to tdata3\n",
+                      textra);
+    }
+
+    return textra;
+}
+
 static void do_trigger_action(CPURISCVState *env, target_ulong trigger_index)
 {
     trigger_action_t action = get_trigger_action(env, trigger_index);
@@ -441,8 +504,10 @@ static void type2_reg_write(CPURISCVState *env, target_ulong index,
         }
         break;
     case TDATA3:
-        qemu_log_mask(LOG_UNIMP,
-                      "tdata3 is not supported for type 2 trigger\n");
+        new_val = textra_validate(env, val);
+        if (new_val != env->tdata3[index]) {
+            env->tdata3[index] = new_val;
+        }
         break;
     default:
         g_assert_not_reached();
@@ -558,8 +623,10 @@ static void type6_reg_write(CPURISCVState *env, target_ulong index,
         }
         break;
     case TDATA3:
-        qemu_log_mask(LOG_UNIMP,
-                      "tdata3 is not supported for type 6 trigger\n");
+        new_val = textra_validate(env, val);
+        if (new_val != env->tdata3[index]) {
+            env->tdata3[index] = new_val;
+        }
         break;
     default:
         g_assert_not_reached();
@@ -741,8 +808,10 @@ static void itrigger_reg_write(CPURISCVState *env, target_ulong index,
                       "tdata2 is not supported for icount trigger\n");
         break;
     case TDATA3:
-        qemu_log_mask(LOG_UNIMP,
-                      "tdata3 is not supported for icount trigger\n");
+        new_val = textra_validate(env, val);
+        if (new_val != env->tdata3[index]) {
+            env->tdata3[index] = new_val;
+        }
         break;
     default:
         g_assert_not_reached();
-- 
2.34.1
Re: [PATCH 1/2] target/riscv: Preliminary textra trigger CSR writting support
Posted by Alistair Francis 4 months, 2 weeks ago
On Thu, Jul 4, 2024 at 2:03 PM Alvin Chang via <qemu-devel@nongnu.org> wrote:
>
> This commit allows program to write textra trigger CSR for type 2, 3, 6
> triggers. In this preliminary patch, the textra.MHVALUE and the
> textra.MHSELECT fields are allowed to be configured. Other fields, such
> as textra.SBYTEMASK, textra.SVALUE, and textra.SSELECT, are hardwired to
> zero for now.
>
> For textra.MHSELECT field, the only legal values are 0 (ignore) and 4
> (mcontext). Writing 1~3 into textra.MHSELECT will be changed to 0, and
> writing 5~7 into textra.MHSELECT will be changed to 4. This behavior is
> aligned to RISC-V SPIKE simulator.
>
> Signed-off-by: Alvin Chang <alvinga@andestech.com>
> ---
>  target/riscv/cpu_bits.h | 10 +++++
>  target/riscv/debug.c    | 81 ++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 85 insertions(+), 6 deletions(-)
>
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index c257c5ed7d..0530b4f9f4 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -906,6 +906,16 @@ typedef enum RISCVException {
>  #define JVT_BASE                           (~0x3F)
>
>  /* Debug Sdtrig CSR masks */
> +#define TEXTRA32_MHVALUE                   0xFC000000
> +#define TEXTRA32_MHSELECT                  0x03800000
> +#define TEXTRA32_SBYTEMASK                 0x000C0000
> +#define TEXTRA32_SVALUE                    0x0003FFFC
> +#define TEXTRA32_SSELECT                   0x00000003
> +#define TEXTRA64_MHVALUE                   0xFFF8000000000000ULL
> +#define TEXTRA64_MHSELECT                  0x0007000000000000ULL
> +#define TEXTRA64_SBYTEMASK                 0x000000F000000000ULL
> +#define TEXTRA64_SVALUE                    0x00000003FFFFFFFCULL
> +#define TEXTRA64_SSELECT                   0x0000000000000003ULL
>  #define MCONTEXT32                         0x0000003F
>  #define MCONTEXT64                         0x0000000000001FFFULL
>  #define MCONTEXT32_HCONTEXT                0x0000007F
> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> index 0b5099ff9a..f7d8f5e320 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -217,6 +217,69 @@ static inline void warn_always_zero_bit(target_ulong val, target_ulong mask,
>      }
>  }
>
> +static target_ulong textra_validate(CPURISCVState *env, target_ulong tdata3)
> +{
> +    target_ulong mhvalue, mhselect;
> +    target_ulong mhselect_new;
> +    target_ulong textra;
> +    const uint32_t mhselect_no_rvh[8] = { 0, 0, 0, 0, 4, 4, 4, 4 };
> +
> +    switch (riscv_cpu_mxl(env)) {
> +    case MXL_RV32:
> +        mhvalue  = get_field(tdata3, TEXTRA32_MHVALUE);
> +        mhselect = get_field(tdata3, TEXTRA32_MHSELECT);
> +        /* Validate unimplemented (always zero) bits */
> +        warn_always_zero_bit(tdata3, (target_ulong)TEXTRA32_SBYTEMASK,
> +                             "sbytemask");
> +        warn_always_zero_bit(tdata3, (target_ulong)TEXTRA32_SVALUE,
> +                             "svalue");
> +        warn_always_zero_bit(tdata3, (target_ulong)TEXTRA32_SSELECT,
> +                             "sselect");
> +        break;
> +    case MXL_RV64:
> +    case MXL_RV128:
> +        mhvalue  = get_field(tdata3, TEXTRA64_MHVALUE);
> +        mhselect = get_field(tdata3, TEXTRA64_MHSELECT);
> +        /* Validate unimplemented (always zero) bits */
> +        warn_always_zero_bit(tdata3, (target_ulong)TEXTRA64_SBYTEMASK,
> +                             "sbytemask");
> +        warn_always_zero_bit(tdata3, (target_ulong)TEXTRA64_SVALUE,
> +                             "svalue");
> +        warn_always_zero_bit(tdata3, (target_ulong)TEXTRA64_SSELECT,
> +                             "sselect");
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
> +
> +    /* Validate mhselect. */
> +    mhselect_new = mhselect_no_rvh[mhselect];

I don't follow, we don't check if the H extension is supported.

The spec states: "mhselect and sselect may only support 0 (ignore)" so
it sounds like we either support non or all of them (but that isn't
clear).

Either way we should at least log that we don't support
mcontext_select and vmid_select if the Hypervisor extension is enabled

> +
> +    /* Write legal values into textra */
> +    textra = 0;
> +    switch (riscv_cpu_mxl(env)) {
> +    case MXL_RV32:
> +        textra = set_field(textra, TEXTRA32_MHVALUE,  mhvalue);
> +        textra = set_field(textra, TEXTRA32_MHSELECT, mhselect_new);
> +        break;
> +    case MXL_RV64:
> +    case MXL_RV128:
> +        textra = set_field(textra, TEXTRA64_MHVALUE,  mhvalue);
> +        textra = set_field(textra, TEXTRA64_MHSELECT, mhselect_new);
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
> +
> +    if (textra != tdata3) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "different value 0x" TARGET_FMT_lx " write to tdata3\n",
> +                      textra);
> +    }
> +
> +    return textra;
> +}
> +
>  static void do_trigger_action(CPURISCVState *env, target_ulong trigger_index)
>  {
>      trigger_action_t action = get_trigger_action(env, trigger_index);
> @@ -441,8 +504,10 @@ static void type2_reg_write(CPURISCVState *env, target_ulong index,
>          }
>          break;
>      case TDATA3:
> -        qemu_log_mask(LOG_UNIMP,
> -                      "tdata3 is not supported for type 2 trigger\n");
> +        new_val = textra_validate(env, val);
> +        if (new_val != env->tdata3[index]) {
> +            env->tdata3[index] = new_val;

You don't need the if() you can just have a single line

env->tdata3[index] = textra_validate(env, val);

Alistair

> +        }
>          break;
>      default:
>          g_assert_not_reached();
> @@ -558,8 +623,10 @@ static void type6_reg_write(CPURISCVState *env, target_ulong index,
>          }
>          break;
>      case TDATA3:
> -        qemu_log_mask(LOG_UNIMP,
> -                      "tdata3 is not supported for type 6 trigger\n");
> +        new_val = textra_validate(env, val);
> +        if (new_val != env->tdata3[index]) {
> +            env->tdata3[index] = new_val;
> +        }
>          break;
>      default:
>          g_assert_not_reached();
> @@ -741,8 +808,10 @@ static void itrigger_reg_write(CPURISCVState *env, target_ulong index,
>                        "tdata2 is not supported for icount trigger\n");
>          break;
>      case TDATA3:
> -        qemu_log_mask(LOG_UNIMP,
> -                      "tdata3 is not supported for icount trigger\n");
> +        new_val = textra_validate(env, val);
> +        if (new_val != env->tdata3[index]) {
> +            env->tdata3[index] = new_val;
> +        }
>          break;
>      default:
>          g_assert_not_reached();
> --
> 2.34.1
>
>