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

[Alistair Francis <alistair23@gmail.com>]

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
>
>