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 <alvi...@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
>
>
