From: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
To: Bin Meng <bmeng.cn@gmail.com>, qemu-devel@nongnu.org
Cc: Frank Chang <frank.chang@sifive.com>,
Alistair Francis <alistair.francis@wdc.com>,
Bin Meng <bin.meng@windriver.com>,
Palmer Dabbelt <palmer@dabbelt.com>,
qemu-riscv@nongnu.org
Subject: Re: [PATCH v2 1/8] target/riscv: debug: Determine the trigger type from tdata1.type
Date: Fri, 16 Sep 2022 09:53:48 +0800 [thread overview]
Message-ID: <ce34e972-aea6-dd10-6623-2630a1e40b67@linux.alibaba.com> (raw)
In-Reply-To: <20220909134215.1843865-2-bmeng.cn@gmail.com>
Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
Zhiwei
On 2022/9/9 21:42, Bin Meng wrote:
> From: Frank Chang <frank.chang@sifive.com>
>
> Current RISC-V debug assumes that only type 2 trigger is supported.
> To allow more types of triggers to be supported in the future
> (e.g. type 6 trigger, which is similar to type 2 trigger with additional
> functionality), we should determine the trigger type from tdata1.type.
>
> RV_MAX_TRIGGERS is also introduced in replacement of TRIGGER_TYPE2_NUM.
>
> Signed-off-by: Frank Chang <frank.chang@sifive.com>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> [bmeng: fixed MXL_RV128 case, and moved macros to the following patch]
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>
> ---
>
> Changes in v2:
> - fixed MXL_RV128 case
> - moved macros to patch#2
> - added log guest errors for TRIGGER_TYPE_{NO_EXIST,UNAVAIL}
>
> target/riscv/cpu.h | 2 +-
> target/riscv/debug.h | 13 +--
> target/riscv/csr.c | 2 +-
> target/riscv/debug.c | 188 +++++++++++++++++++++++++++++------------
> target/riscv/machine.c | 2 +-
> 5 files changed, 140 insertions(+), 67 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 06751e1e3e..4d82a3250b 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -324,7 +324,7 @@ struct CPUArchState {
>
> /* trigger module */
> target_ulong trigger_cur;
> - type2_trigger_t type2_trig[TRIGGER_TYPE2_NUM];
> + type2_trigger_t type2_trig[RV_MAX_TRIGGERS];
>
> /* machine specific rdtime callback */
> uint64_t (*rdtime_fn)(void *);
> diff --git a/target/riscv/debug.h b/target/riscv/debug.h
> index 27b9cac6b4..72e4edcd8c 100644
> --- a/target/riscv/debug.h
> +++ b/target/riscv/debug.h
> @@ -22,13 +22,7 @@
> #ifndef RISCV_DEBUG_H
> #define RISCV_DEBUG_H
>
> -/* trigger indexes implemented */
> -enum {
> - TRIGGER_TYPE2_IDX_0 = 0,
> - TRIGGER_TYPE2_IDX_1,
> - TRIGGER_TYPE2_NUM,
> - TRIGGER_NUM = TRIGGER_TYPE2_NUM
> -};
> +#define RV_MAX_TRIGGERS 2
>
> /* register index of tdata CSRs */
> enum {
> @@ -46,7 +40,8 @@ typedef enum {
> TRIGGER_TYPE_EXCP = 5, /* exception trigger */
> TRIGGER_TYPE_AD_MATCH6 = 6, /* new address/data match trigger */
> TRIGGER_TYPE_EXT_SRC = 7, /* external source trigger */
> - TRIGGER_TYPE_UNAVAIL = 15 /* trigger exists, but unavailable */
> + TRIGGER_TYPE_UNAVAIL = 15, /* trigger exists, but unavailable */
> + TRIGGER_TYPE_NUM
> } trigger_type_t;
>
> typedef struct {
> @@ -56,7 +51,7 @@ typedef struct {
> struct CPUWatchpoint *wp;
> } type2_trigger_t;
>
> -/* tdata field masks */
> +/* tdata1 field masks */
>
> #define RV32_TYPE(t) ((uint32_t)(t) << 28)
> #define RV32_TYPE_MASK (0xf << 28)
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index b96db1b62b..3d0d8e0340 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -3065,7 +3065,7 @@ static RISCVException read_tdata(CPURISCVState *env, int csrno,
> target_ulong *val)
> {
> /* return 0 in tdata1 to end the trigger enumeration */
> - if (env->trigger_cur >= TRIGGER_NUM && csrno == CSR_TDATA1) {
> + if (env->trigger_cur >= RV_MAX_TRIGGERS && csrno == CSR_TDATA1) {
> *val = 0;
> return RISCV_EXCP_NONE;
> }
> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> index fc6e13222f..9dd468753a 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -52,8 +52,15 @@
> /* tdata availability of a trigger */
> typedef bool tdata_avail[TDATA_NUM];
>
> -static tdata_avail tdata_mapping[TRIGGER_NUM] = {
> - [TRIGGER_TYPE2_IDX_0 ... TRIGGER_TYPE2_IDX_1] = { true, true, false },
> +static tdata_avail tdata_mapping[TRIGGER_TYPE_NUM] = {
> + [TRIGGER_TYPE_NO_EXIST] = { false, false, false },
> + [TRIGGER_TYPE_AD_MATCH] = { true, true, true },
> + [TRIGGER_TYPE_INST_CNT] = { true, false, true },
> + [TRIGGER_TYPE_INT] = { true, true, true },
> + [TRIGGER_TYPE_EXCP] = { true, true, true },
> + [TRIGGER_TYPE_AD_MATCH6] = { true, true, true },
> + [TRIGGER_TYPE_EXT_SRC] = { true, false, false },
> + [TRIGGER_TYPE_UNAVAIL] = { true, true, true }
> };
>
> /* only breakpoint size 1/2/4/8 supported */
> @@ -67,6 +74,27 @@ static int access_size[SIZE_NUM] = {
> [6 ... 15] = -1,
> };
>
> +static inline target_ulong extract_trigger_type(CPURISCVState *env,
> + target_ulong tdata1)
> +{
> + switch (riscv_cpu_mxl(env)) {
> + case MXL_RV32:
> + return extract32(tdata1, 28, 4);
> + case MXL_RV64:
> + case MXL_RV128:
> + return extract64(tdata1, 60, 4);
> + default:
> + g_assert_not_reached();
> + }
> +}
> +
> +static inline target_ulong get_trigger_type(CPURISCVState *env,
> + target_ulong trigger_index)
> +{
> + target_ulong tdata1 = env->type2_trig[trigger_index].mcontrol;
> + return extract_trigger_type(env, tdata1);
> +}
> +
> static inline target_ulong trigger_type(CPURISCVState *env,
> trigger_type_t type)
> {
> @@ -89,15 +117,17 @@ static inline target_ulong trigger_type(CPURISCVState *env,
>
> bool tdata_available(CPURISCVState *env, int tdata_index)
> {
> + int trigger_type = get_trigger_type(env, env->trigger_cur);
> +
> if (unlikely(tdata_index >= TDATA_NUM)) {
> return false;
> }
>
> - if (unlikely(env->trigger_cur >= TRIGGER_NUM)) {
> + if (unlikely(env->trigger_cur >= RV_MAX_TRIGGERS)) {
> return false;
> }
>
> - return tdata_mapping[env->trigger_cur][tdata_index];
> + return tdata_mapping[trigger_type][tdata_index];
> }
>
> target_ulong tselect_csr_read(CPURISCVState *env)
> @@ -137,6 +167,7 @@ static target_ulong tdata1_validate(CPURISCVState *env, target_ulong val,
> qemu_log_mask(LOG_GUEST_ERROR,
> "ignoring type write to tdata1 register\n");
> }
> +
> if (dmode != 0) {
> qemu_log_mask(LOG_UNIMP, "debug mode is not supported\n");
> }
> @@ -261,9 +292,8 @@ static void type2_breakpoint_remove(CPURISCVState *env, target_ulong index)
> }
>
> static target_ulong type2_reg_read(CPURISCVState *env,
> - target_ulong trigger_index, int tdata_index)
> + target_ulong index, int tdata_index)
> {
> - uint32_t index = trigger_index - TRIGGER_TYPE2_IDX_0;
> target_ulong tdata;
>
> switch (tdata_index) {
> @@ -280,10 +310,9 @@ static target_ulong type2_reg_read(CPURISCVState *env,
> return tdata;
> }
>
> -static void type2_reg_write(CPURISCVState *env, target_ulong trigger_index,
> +static void type2_reg_write(CPURISCVState *env, target_ulong index,
> int tdata_index, target_ulong val)
> {
> - uint32_t index = trigger_index - TRIGGER_TYPE2_IDX_0;
> target_ulong new_val;
>
> switch (tdata_index) {
> @@ -309,35 +338,64 @@ static void type2_reg_write(CPURISCVState *env, target_ulong trigger_index,
> return;
> }
>
> -typedef target_ulong (*tdata_read_func)(CPURISCVState *env,
> - target_ulong trigger_index,
> - int tdata_index);
> -
> -static tdata_read_func trigger_read_funcs[TRIGGER_NUM] = {
> - [TRIGGER_TYPE2_IDX_0 ... TRIGGER_TYPE2_IDX_1] = type2_reg_read,
> -};
> -
> -typedef void (*tdata_write_func)(CPURISCVState *env,
> - target_ulong trigger_index,
> - int tdata_index,
> - target_ulong val);
> -
> -static tdata_write_func trigger_write_funcs[TRIGGER_NUM] = {
> - [TRIGGER_TYPE2_IDX_0 ... TRIGGER_TYPE2_IDX_1] = type2_reg_write,
> -};
> -
> target_ulong tdata_csr_read(CPURISCVState *env, int tdata_index)
> {
> - tdata_read_func read_func = trigger_read_funcs[env->trigger_cur];
> + int trigger_type = get_trigger_type(env, env->trigger_cur);
> +
> + switch (trigger_type) {
> + case TRIGGER_TYPE_AD_MATCH:
> + return type2_reg_read(env, env->trigger_cur, tdata_index);
> + break;
> + case TRIGGER_TYPE_INST_CNT:
> + case TRIGGER_TYPE_INT:
> + case TRIGGER_TYPE_EXCP:
> + case TRIGGER_TYPE_AD_MATCH6:
> + case TRIGGER_TYPE_EXT_SRC:
> + qemu_log_mask(LOG_UNIMP, "trigger type: %d is not supported\n",
> + trigger_type);
> + break;
> + case TRIGGER_TYPE_NO_EXIST:
> + case TRIGGER_TYPE_UNAVAIL:
> + qemu_log_mask(LOG_GUEST_ERROR, "trigger type: %d does not exit\n",
> + trigger_type);
> + break;
> + default:
> + g_assert_not_reached();
> + }
>
> - return read_func(env, env->trigger_cur, tdata_index);
> + return 0;
> }
>
> void tdata_csr_write(CPURISCVState *env, int tdata_index, target_ulong val)
> {
> - tdata_write_func write_func = trigger_write_funcs[env->trigger_cur];
> + int trigger_type;
>
> - return write_func(env, env->trigger_cur, tdata_index, val);
> + if (tdata_index == TDATA1) {
> + trigger_type = extract_trigger_type(env, val);
> + } else {
> + trigger_type = get_trigger_type(env, env->trigger_cur);
> + }
> +
> + switch (trigger_type) {
> + case TRIGGER_TYPE_AD_MATCH:
> + type2_reg_write(env, env->trigger_cur, tdata_index, val);
> + break;
> + case TRIGGER_TYPE_INST_CNT:
> + case TRIGGER_TYPE_INT:
> + case TRIGGER_TYPE_EXCP:
> + case TRIGGER_TYPE_AD_MATCH6:
> + case TRIGGER_TYPE_EXT_SRC:
> + qemu_log_mask(LOG_UNIMP, "trigger type: %d is not supported\n",
> + trigger_type);
> + break;
> + case TRIGGER_TYPE_NO_EXIST:
> + case TRIGGER_TYPE_UNAVAIL:
> + qemu_log_mask(LOG_GUEST_ERROR, "trigger type: %d does not exit\n",
> + trigger_type);
> + break;
> + default:
> + g_assert_not_reached();
> + }
> }
>
> void riscv_cpu_debug_excp_handler(CPUState *cs)
> @@ -364,18 +422,28 @@ bool riscv_cpu_debug_check_breakpoint(CPUState *cs)
> CPUBreakpoint *bp;
> target_ulong ctrl;
> target_ulong pc;
> + int trigger_type;
> int i;
>
> QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
> - for (i = 0; i < TRIGGER_TYPE2_NUM; i++) {
> - ctrl = env->type2_trig[i].mcontrol;
> - pc = env->type2_trig[i].maddress;
> -
> - if ((ctrl & TYPE2_EXEC) && (bp->pc == pc)) {
> - /* check U/S/M bit against current privilege level */
> - if ((ctrl >> 3) & BIT(env->priv)) {
> - return true;
> + for (i = 0; i < RV_MAX_TRIGGERS; i++) {
> + trigger_type = get_trigger_type(env, i);
> +
> + switch (trigger_type) {
> + case TRIGGER_TYPE_AD_MATCH:
> + ctrl = env->type2_trig[i].mcontrol;
> + pc = env->type2_trig[i].maddress;
> +
> + if ((ctrl & TYPE2_EXEC) && (bp->pc == pc)) {
> + /* check U/S/M bit against current privilege level */
> + if ((ctrl >> 3) & BIT(env->priv)) {
> + return true;
> + }
> }
> + break;
> + default:
> + /* other trigger types are not supported or irrelevant */
> + break;
> }
> }
> }
> @@ -389,26 +457,36 @@ bool riscv_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
> CPURISCVState *env = &cpu->env;
> target_ulong ctrl;
> target_ulong addr;
> + int trigger_type;
> int flags;
> int i;
>
> - for (i = 0; i < TRIGGER_TYPE2_NUM; i++) {
> - ctrl = env->type2_trig[i].mcontrol;
> - addr = env->type2_trig[i].maddress;
> - flags = 0;
> + for (i = 0; i < RV_MAX_TRIGGERS; i++) {
> + trigger_type = get_trigger_type(env, i);
>
> - if (ctrl & TYPE2_LOAD) {
> - flags |= BP_MEM_READ;
> - }
> - if (ctrl & TYPE2_STORE) {
> - flags |= BP_MEM_WRITE;
> - }
> + switch (trigger_type) {
> + case TRIGGER_TYPE_AD_MATCH:
> + ctrl = env->type2_trig[i].mcontrol;
> + addr = env->type2_trig[i].maddress;
> + flags = 0;
>
> - if ((wp->flags & flags) && (wp->vaddr == addr)) {
> - /* check U/S/M bit against current privilege level */
> - if ((ctrl >> 3) & BIT(env->priv)) {
> - return true;
> + if (ctrl & TYPE2_LOAD) {
> + flags |= BP_MEM_READ;
> + }
> + if (ctrl & TYPE2_STORE) {
> + flags |= BP_MEM_WRITE;
> + }
> +
> + if ((wp->flags & flags) && (wp->vaddr == addr)) {
> + /* check U/S/M bit against current privilege level */
> + if ((ctrl >> 3) & BIT(env->priv)) {
> + return true;
> + }
> }
> + break;
> + default:
> + /* other trigger types are not supported */
> + break;
> }
> }
>
> @@ -417,11 +495,11 @@ bool riscv_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
>
> void riscv_trigger_init(CPURISCVState *env)
> {
> - target_ulong type2 = trigger_type(env, TRIGGER_TYPE_AD_MATCH);
> + target_ulong tdata1 = trigger_type(env, TRIGGER_TYPE_AD_MATCH);
> int i;
>
> - /* type 2 triggers */
> - for (i = 0; i < TRIGGER_TYPE2_NUM; i++) {
> + /* init to type 2 triggers */
> + for (i = 0; i < RV_MAX_TRIGGERS; i++) {
> /*
> * type = TRIGGER_TYPE_AD_MATCH
> * dmode = 0 (both debug and M-mode can write tdata)
> @@ -435,7 +513,7 @@ void riscv_trigger_init(CPURISCVState *env)
> * chain = 0 (unimplemented, always 0)
> * match = 0 (always 0, when any compare value equals tdata2)
> */
> - env->type2_trig[i].mcontrol = type2;
> + env->type2_trig[i].mcontrol = tdata1;
> env->type2_trig[i].maddress = 0;
> env->type2_trig[i].bp = NULL;
> env->type2_trig[i].wp = NULL;
> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> index 41098f6ad0..b8173394a2 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -247,7 +247,7 @@ static const VMStateDescription vmstate_debug = {
> .needed = debug_needed,
> .fields = (VMStateField[]) {
> VMSTATE_UINTTL(env.trigger_cur, RISCVCPU),
> - VMSTATE_STRUCT_ARRAY(env.type2_trig, RISCVCPU, TRIGGER_TYPE2_NUM,
> + VMSTATE_STRUCT_ARRAY(env.type2_trig, RISCVCPU, RV_MAX_TRIGGERS,
> 0, vmstate_debug_type2, type2_trigger_t),
> VMSTATE_END_OF_LIST()
> }
next prev parent reply other threads:[~2022-09-16 1:55 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-09 13:42 [PATCH v2 0/8] target/riscv: Improve RISC-V Debug support Bin Meng
2022-09-09 13:42 ` [PATCH v2 1/8] target/riscv: debug: Determine the trigger type from tdata1.type Bin Meng
2022-09-16 1:53 ` LIU Zhiwei [this message]
2022-09-16 2:42 ` LIU Zhiwei
2022-09-09 13:42 ` [PATCH v2 2/8] target/riscv: debug: Introduce build_tdata1() to build tdata1 register content Bin Meng
2022-09-16 1:55 ` LIU Zhiwei
2022-09-09 13:42 ` [PATCH v2 3/8] target/riscv: debug: Introduce tdata1, tdata2, and tdata3 CSRs Bin Meng
2022-09-16 1:58 ` LIU Zhiwei
2022-09-09 13:42 ` [PATCH v2 4/8] target/riscv: debug: Restrict the range of tselect value can be written Bin Meng
2022-09-16 1:59 ` LIU Zhiwei
2022-09-09 13:42 ` [PATCH v2 5/8] target/riscv: debug: Introduce tinfo CSR Bin Meng
2022-09-16 2:26 ` LIU Zhiwei
2022-09-09 13:42 ` [PATCH v2 6/8] target/riscv: debug: Create common trigger actions function Bin Meng
2022-09-16 2:40 ` LIU Zhiwei
2022-09-09 13:42 ` [PATCH v2 7/8] target/riscv: debug: Check VU/VS modes for type 2 trigger Bin Meng
2022-09-09 13:42 ` [PATCH v2 8/8] target/riscv: debug: Add initial support of type 6 trigger Bin Meng
2022-09-23 4:46 ` [PATCH v2 0/8] target/riscv: Improve RISC-V Debug support Alistair Francis
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ce34e972-aea6-dd10-6623-2630a1e40b67@linux.alibaba.com \
--to=zhiwei_liu@linux.alibaba.com \
--cc=alistair.francis@wdc.com \
--cc=bin.meng@windriver.com \
--cc=bmeng.cn@gmail.com \
--cc=frank.chang@sifive.com \
--cc=palmer@dabbelt.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-riscv@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).