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

[Alistair Francis <alistair23@gmail.com>]

On Wed, Jul 10, 2024 at 8:01 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    | 75 +++++++++++++++++++++++++++++++++++++----
>  2 files changed, 79 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..139b722a7e 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -217,6 +217,72 @@ 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];
> +    if (mhselect != mhselect_new) {
> +        qemu_log_mask(LOG_UNIMP, "mhselect only supports 0 or 4 for now\n");
> +    }
> +
> +    /* 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);
> +    }

You don't need this, you have already reported on all of the possible
differences

With the above removed

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair