From: Andrew Jones <ajones@ventanamicro.com>
To: Himanshu Chauhan <hchauhan@ventanamicro.com>
Cc: qemu-riscv@nongnu.org, qemu-devel@nongnu.org
Subject: Re: [PATCH v6 1/3] target/riscv: Enable mcontrol6 triggers only when sdtrig is selected
Date: Thu, 14 Mar 2024 18:07:30 +0100 [thread overview]
Message-ID: <20240314-00c015519323ddca7ac7de74@orel> (raw)
In-Reply-To: <20240314113510.477862-2-hchauhan@ventanamicro.com>
On Thu, Mar 14, 2024 at 05:05:08PM +0530, Himanshu Chauhan wrote:
> The mcontrol6 triggers are not defined in debug specification v0.13
> These triggers are defined in sdtrig ISA extension.
>
> This patch:
> * Adds ext_sdtrig capability which is used to select mcontrol6 triggers
> * Keeps the debug property. All triggers that are defined in v0.13 are
> exposed.
>
> Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
> ---
> target/riscv/cpu.c | 4 +-
> target/riscv/cpu_cfg.h | 1 +
> target/riscv/csr.c | 2 +-
> target/riscv/debug.c | 90 +++++++++++++++++++++++++-----------------
> 4 files changed, 57 insertions(+), 40 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index c160b9216b..2602aae9f5 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1008,7 +1008,7 @@ static void riscv_cpu_reset_hold(Object *obj)
> set_default_nan_mode(1, &env->fp_status);
>
> #ifndef CONFIG_USER_ONLY
> - if (cpu->cfg.debug) {
> + if (cpu->cfg.debug || cpu->cfg.ext_sdtrig) {
I still don't see the point of adding '|| cpu->cfg.ext_sdtrig'. debug must
be true when ext_sdtrig is true.
> riscv_trigger_reset_hold(env);
> }
>
> @@ -1168,7 +1168,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> riscv_cpu_register_gdb_regs_for_features(cs);
>
> #ifndef CONFIG_USER_ONLY
> - if (cpu->cfg.debug) {
> + if (cpu->cfg.debug || cpu->cfg.ext_sdtrig) {
> riscv_trigger_realize(&cpu->env);
> }
> #endif
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index 2040b90da0..0c57e1acd4 100644
> --- a/target/riscv/cpu_cfg.h
> +++ b/target/riscv/cpu_cfg.h
> @@ -114,6 +114,7 @@ struct RISCVCPUConfig {
> bool ext_zvfbfwma;
> bool ext_zvfh;
> bool ext_zvfhmin;
> + bool ext_sdtrig;
> bool ext_smaia;
> bool ext_ssaia;
> bool ext_sscofpmf;
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 726096444f..26623d3640 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -546,7 +546,7 @@ static RISCVException have_mseccfg(CPURISCVState *env, int csrno)
>
> static RISCVException debug(CPURISCVState *env, int csrno)
> {
> - if (riscv_cpu_cfg(env)->debug) {
> + if (riscv_cpu_cfg(env)->debug || riscv_cpu_cfg(env)->ext_sdtrig) {
> return RISCV_EXCP_NONE;
> }
>
> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> index e30d99cc2f..674223e966 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -100,13 +100,15 @@ static trigger_action_t get_trigger_action(CPURISCVState *env,
> target_ulong tdata1 = env->tdata1[trigger_index];
> int trigger_type = get_trigger_type(env, trigger_index);
> trigger_action_t action = DBG_ACTION_NONE;
> + const RISCVCPUConfig *cfg = riscv_cpu_cfg(env);
>
> switch (trigger_type) {
> case TRIGGER_TYPE_AD_MATCH:
> action = (tdata1 & TYPE2_ACTION) >> 12;
> break;
> case TRIGGER_TYPE_AD_MATCH6:
> - action = (tdata1 & TYPE6_ACTION) >> 12;
> + if (cfg->ext_sdtrig)
> + action = (tdata1 & TYPE6_ACTION) >> 12;
QEMU requires {}, even for single line blocks. I'm not sure if QEMU's
checkpatch is smart enough to complain about that, but if you haven't
run checkpatch, then you probably should.
> break;
> case TRIGGER_TYPE_INST_CNT:
> case TRIGGER_TYPE_INT:
> @@ -727,7 +729,12 @@ void tdata_csr_write(CPURISCVState *env, int tdata_index, target_ulong val)
> type2_reg_write(env, env->trigger_cur, tdata_index, val);
> break;
> case TRIGGER_TYPE_AD_MATCH6:
> - type6_reg_write(env, env->trigger_cur, tdata_index, val);
> + if (riscv_cpu_cfg(env)->ext_sdtrig) {
> + type6_reg_write(env, env->trigger_cur, tdata_index, val);
> + } else {
> + qemu_log_mask(LOG_UNIMP, "trigger type: %d is not supported\n",
> + trigger_type);
> + }
> break;
> case TRIGGER_TYPE_INST_CNT:
> itrigger_reg_write(env, env->trigger_cur, tdata_index, val);
> @@ -750,9 +757,14 @@ void tdata_csr_write(CPURISCVState *env, int tdata_index, target_ulong val)
>
> target_ulong tinfo_csr_read(CPURISCVState *env)
> {
> - /* assume all triggers support the same types of triggers */
> - return BIT(TRIGGER_TYPE_AD_MATCH) |
> - BIT(TRIGGER_TYPE_AD_MATCH6);
> + target_ulong ts = 0;
Useless initialization to zero since it's assigned in the next line.
Actually, should just do
target_ulong ts = BIT(TRIGGER_TYPE_AD_MATCH);
> +
> + ts = BIT(TRIGGER_TYPE_AD_MATCH);
> +
> + if (riscv_cpu_cfg(env)->ext_sdtrig)
> + ts |= BIT(TRIGGER_TYPE_AD_MATCH6);
Need {}
> +
> + return ts;
> }
>
> void riscv_cpu_debug_excp_handler(CPUState *cs)
> @@ -803,19 +815,21 @@ bool riscv_cpu_debug_check_breakpoint(CPUState *cs)
> }
> break;
> case TRIGGER_TYPE_AD_MATCH6:
> - ctrl = env->tdata1[i];
> - pc = env->tdata2[i];
> -
> - if ((ctrl & TYPE6_EXEC) && (bp->pc == pc)) {
> - if (env->virt_enabled) {
> - /* check VU/VS bit against current privilege level */
> - if ((ctrl >> 23) & BIT(env->priv)) {
> - return true;
> - }
> - } else {
> - /* check U/S/M bit against current privilege level */
> - if ((ctrl >> 3) & BIT(env->priv)) {
> - return true;
> + if (cpu->cfg.ext_sdtrig) {
> + ctrl = env->tdata1[i];
> + pc = env->tdata2[i];
> +
> + if ((ctrl & TYPE6_EXEC) && (bp->pc == pc)) {
> + if (env->virt_enabled) {
> + /* check VU/VS bit against current privilege level */
> + if ((ctrl >> 23) & BIT(env->priv)) {
> + return true;
> + }
> + } else {
> + /* check U/S/M bit against current privilege level */
> + if ((ctrl >> 3) & BIT(env->priv)) {
> + return true;
> + }
> }
> }
> }
> @@ -869,27 +883,29 @@ bool riscv_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
> }
> break;
> case TRIGGER_TYPE_AD_MATCH6:
> - ctrl = env->tdata1[i];
> - addr = env->tdata2[i];
> - flags = 0;
> + if (cpu->cfg.ext_sdtrig) {
> + ctrl = env->tdata1[i];
> + addr = env->tdata2[i];
> + flags = 0;
>
> - if (ctrl & TYPE6_LOAD) {
> - flags |= BP_MEM_READ;
> - }
> - if (ctrl & TYPE6_STORE) {
> - flags |= BP_MEM_WRITE;
> - }
> + if (ctrl & TYPE6_LOAD) {
> + flags |= BP_MEM_READ;
> + }
> + if (ctrl & TYPE6_STORE) {
> + flags |= BP_MEM_WRITE;
> + }
>
> - if ((wp->flags & flags) && (wp->vaddr == addr)) {
> - if (env->virt_enabled) {
> - /* check VU/VS bit against current privilege level */
> - if ((ctrl >> 23) & BIT(env->priv)) {
> - return true;
> - }
> - } else {
> - /* check U/S/M bit against current privilege level */
> - if ((ctrl >> 3) & BIT(env->priv)) {
> - return true;
> + if ((wp->flags & flags) && (wp->vaddr == addr)) {
> + if (env->virt_enabled) {
> + /* check VU/VS bit against current privilege level */
> + if ((ctrl >> 23) & BIT(env->priv)) {
> + return true;
> + }
> + } else {
> + /* check U/S/M bit against current privilege level */
> + if ((ctrl >> 3) & BIT(env->priv)) {
> + return true;
> + }
> }
> }
> }
> --
> 2.34.1
>
For the two TRIGGER_TYPE_AD_MATCH6 cases above in
riscv_cpu_debug_check_breakpoint() and riscv_cpu_debug_check_watchpoint()
I'd just put a
if (!cpu->cfg.ext_sdtrig) {
break;
}
at the top of the code, rather than indenting everything. But either way
is fine.
Thanks,
drew
next prev parent reply other threads:[~2024-03-14 17:07 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-14 11:35 [PATCH v6 0/3] Introduce sdtrig ISA extension Himanshu Chauhan
2024-03-14 11:35 ` [PATCH v6 1/3] target/riscv: Enable mcontrol6 triggers only when sdtrig is selected Himanshu Chauhan
2024-03-14 17:07 ` Andrew Jones [this message]
2024-03-14 11:35 ` [PATCH v6 2/3] target/riscv: Expose sdtrig ISA extension Himanshu Chauhan
2024-03-14 17:12 ` Andrew Jones
2024-03-14 11:35 ` [PATCH v6 3/3] target/riscv: Enable sdtrig for Ventana's Veyron CPUs Himanshu Chauhan
2024-03-14 17:13 ` Andrew Jones
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=20240314-00c015519323ddca7ac7de74@orel \
--to=ajones@ventanamicro.com \
--cc=hchauhan@ventanamicro.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.