| Message ID | 20240721072422.1377506-3-alvinga@andestech.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RISC-V: Add preliminary textra trigger CSR functions | expand |
Hi Alistair, > -----Original Message----- > From: Alvin Che-Chia Chang(張哲嘉) <alvinga@andestech.com> > Sent: Sunday, July 21, 2024 3:24 PM > To: qemu-riscv@nongnu.org; qemu-devel@nongnu.org > Cc: alistair.francis@wdc.com; bin.meng@windriver.com; > liwei1518@gmail.com; dbarboza@ventanamicro.com; > zhiwei_liu@linux.alibaba.com; Alvin Che-Chia Chang(張哲嘉) > <alvinga@andestech.com> > Subject: [PATCH v3 2/2] target/riscv: Add textra matching condition for the > triggers > > According to RISC-V Debug specification, the optional textra32 and > textra64 trigger CSRs can be used to configure additional matching conditions > for the triggers. For example, if the textra.MHSELECT field is set to 4 > (mcontext), this trigger will only match or fire if the low bits of > mcontext/hcontext equal textra.MHVALUE field. > > This commit adds the aforementioned matching condition as common trigger > matching conditions. Currently, the only legal values of textra.MHSELECT are 0 > (ignore) and 4 (mcontext). When textra.MHSELECT is 0, we pass the checking. > When textra.MHSELECT is 4, we compare textra.MHVALUE with mcontext CSR. > The remaining fields, such as textra.SBYTEMASK, textra.SVALUE, and > textra.SSELECT, are hardwired to zero for now. Thus, we skip checking them > here. > > Signed-off-by: Alvin Chang <alvinga@andestech.com> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com> > --- > target/riscv/debug.c | 63 > +++++++++++++++++++++++++++++++++++++++++++- > target/riscv/debug.h | 3 +++ > 2 files changed, 65 insertions(+), 1 deletion(-) > > diff --git a/target/riscv/debug.c b/target/riscv/debug.c index > d6b4a06144..62bb758860 100644 > --- a/target/riscv/debug.c > +++ b/target/riscv/debug.c > @@ -364,11 +364,72 @@ static bool trigger_priv_match(CPURISCVState *env, > trigger_type_t type, > return false; > } > > +static bool trigger_textra_match(CPURISCVState *env, trigger_type_t type, > + int trigger_index) { > + target_ulong textra = env->tdata3[trigger_index]; > + target_ulong mhvalue, mhselect; > + > + if (type < TRIGGER_TYPE_AD_MATCH || type > > TRIGGER_TYPE_AD_MATCH6) { > + /* textra checking is only applicable when type is 2, 3, 4, 5, or 6 */ > + return true; > + } > + > + switch (riscv_cpu_mxl(env)) { > + case MXL_RV32: > + mhvalue = get_field(textra, TEXTRA32_MHVALUE); > + mhselect = get_field(textra, TEXTRA32_MHSELECT); > + break; > + case MXL_RV64: > + case MXL_RV128: > + mhvalue = get_field(textra, TEXTRA64_MHVALUE); > + mhselect = get_field(textra, TEXTRA64_MHSELECT); > + break; > + default: > + g_assert_not_reached(); > + } > + > + /* Check mhvalue and mhselect. */ > + switch (mhselect) { > + case MHSELECT_IGNORE: > + break; > + case MHSELECT_MCONTEXT: > + /* Match or fire if the low bits of mcontext/hcontext equal mhvalue. > */ > + if (riscv_has_ext(env, RVH)) { > + if (mhvalue != env->mcontext) { > + return false; > + } > + } else { > + switch (riscv_cpu_mxl(env)) { > + case MXL_RV32: > + if (mhvalue != (env->mcontext & MCONTEXT32)) { > + return false; > + } > + break; > + case MXL_RV64: > + case MXL_RV128: > + if (mhvalue != (env->mcontext & MCONTEXT64)) { > + return false; > + } > + break; > + default: > + g_assert_not_reached(); > + } > + } I have some new ideas on this part. Should we replace this whole if-else with just the following simple code ? case MHSELECT_MCONTEXT: /* Match if the low bits of mcontext/hcontext equal mhvalue. */ if (mhvalue != env->mcontext) { return false; } break; Those masks on mcontext have been applied in write_mcontext(). I think we can skip the masks here. What do you think ? Regards, Alvin Chang > + break; > + default: > + break; > + } > + > + return true; > +} > + > /* Common matching conditions for all types of the triggers. */ static bool > trigger_common_match(CPURISCVState *env, trigger_type_t type, > int trigger_index) { > - return trigger_priv_match(env, type, trigger_index); > + return trigger_priv_match(env, type, trigger_index) && > + trigger_textra_match(env, type, trigger_index); > } > > /* type 2 trigger */ > diff --git a/target/riscv/debug.h b/target/riscv/debug.h index > c347863578..f76b8f944a 100644 > --- a/target/riscv/debug.h > +++ b/target/riscv/debug.h > @@ -131,6 +131,9 @@ enum { > #define ITRIGGER_VU BIT(25) > #define ITRIGGER_VS BIT(26) > > +#define MHSELECT_IGNORE 0 > +#define MHSELECT_MCONTEXT 4 > + > bool tdata_available(CPURISCVState *env, int tdata_index); > > target_ulong tselect_csr_read(CPURISCVState *env); > -- > 2.34.1 CONFIDENTIALITY NOTICE: This e-mail (and its attachments) may contain confidential and legally privileged information or information protected from disclosure. If you are not the intended recipient, you are hereby notified that any disclosure, copying, distribution, or use of the information contained herein is strictly prohibited. In this case, please immediately notify the sender by return e-mail, delete the message (and any accompanying documents) and destroy all printed hard copies. Thank you for your cooperation. Copyright ANDES TECHNOLOGY CORPORATION - All Rights Reserved.
On Tue, Aug 20, 2024 at 2:00 PM Alvin Che-Chia Chang(張哲嘉) <alvinga@andestech.com> wrote: > > Hi Alistair, > > > -----Original Message----- > > From: Alvin Che-Chia Chang(張哲嘉) <alvinga@andestech.com> > > Sent: Sunday, July 21, 2024 3:24 PM > > To: qemu-riscv@nongnu.org; qemu-devel@nongnu.org > > Cc: alistair.francis@wdc.com; bin.meng@windriver.com; > > liwei1518@gmail.com; dbarboza@ventanamicro.com; > > zhiwei_liu@linux.alibaba.com; Alvin Che-Chia Chang(張哲嘉) > > <alvinga@andestech.com> > > Subject: [PATCH v3 2/2] target/riscv: Add textra matching condition for the > > triggers > > > > According to RISC-V Debug specification, the optional textra32 and > > textra64 trigger CSRs can be used to configure additional matching conditions > > for the triggers. For example, if the textra.MHSELECT field is set to 4 > > (mcontext), this trigger will only match or fire if the low bits of > > mcontext/hcontext equal textra.MHVALUE field. > > > > This commit adds the aforementioned matching condition as common trigger > > matching conditions. Currently, the only legal values of textra.MHSELECT are 0 > > (ignore) and 4 (mcontext). When textra.MHSELECT is 0, we pass the checking. > > When textra.MHSELECT is 4, we compare textra.MHVALUE with mcontext CSR. > > The remaining fields, such as textra.SBYTEMASK, textra.SVALUE, and > > textra.SSELECT, are hardwired to zero for now. Thus, we skip checking them > > here. > > > > Signed-off-by: Alvin Chang <alvinga@andestech.com> > > Reviewed-by: Alistair Francis <alistair.francis@wdc.com> > > --- > > target/riscv/debug.c | 63 > > +++++++++++++++++++++++++++++++++++++++++++- > > target/riscv/debug.h | 3 +++ > > 2 files changed, 65 insertions(+), 1 deletion(-) > > > > diff --git a/target/riscv/debug.c b/target/riscv/debug.c index > > d6b4a06144..62bb758860 100644 > > --- a/target/riscv/debug.c > > +++ b/target/riscv/debug.c > > @@ -364,11 +364,72 @@ static bool trigger_priv_match(CPURISCVState *env, > > trigger_type_t type, > > return false; > > } > > > > +static bool trigger_textra_match(CPURISCVState *env, trigger_type_t type, > > + int trigger_index) { > > + target_ulong textra = env->tdata3[trigger_index]; > > + target_ulong mhvalue, mhselect; > > + > > + if (type < TRIGGER_TYPE_AD_MATCH || type > > > TRIGGER_TYPE_AD_MATCH6) { > > + /* textra checking is only applicable when type is 2, 3, 4, 5, or 6 */ > > + return true; > > + } > > + > > + switch (riscv_cpu_mxl(env)) { > > + case MXL_RV32: > > + mhvalue = get_field(textra, TEXTRA32_MHVALUE); > > + mhselect = get_field(textra, TEXTRA32_MHSELECT); > > + break; > > + case MXL_RV64: > > + case MXL_RV128: > > + mhvalue = get_field(textra, TEXTRA64_MHVALUE); > > + mhselect = get_field(textra, TEXTRA64_MHSELECT); > > + break; > > + default: > > + g_assert_not_reached(); > > + } > > + > > + /* Check mhvalue and mhselect. */ > > + switch (mhselect) { > > + case MHSELECT_IGNORE: > > + break; > > + case MHSELECT_MCONTEXT: > > + /* Match or fire if the low bits of mcontext/hcontext equal mhvalue. > > */ > > + if (riscv_has_ext(env, RVH)) { > > + if (mhvalue != env->mcontext) { > > + return false; > > + } > > + } else { > > + switch (riscv_cpu_mxl(env)) { > > + case MXL_RV32: > > + if (mhvalue != (env->mcontext & MCONTEXT32)) { > > + return false; > > + } > > + break; > > + case MXL_RV64: > > + case MXL_RV128: > > + if (mhvalue != (env->mcontext & MCONTEXT64)) { > > + return false; > > + } > > + break; > > + default: > > + g_assert_not_reached(); > > + } > > + } > > I have some new ideas on this part. > Should we replace this whole if-else with just the following simple code ? > > case MHSELECT_MCONTEXT: > /* Match if the low bits of mcontext/hcontext equal mhvalue. */ > if (mhvalue != env->mcontext) { > return false; > } > break; > > Those masks on mcontext have been applied in write_mcontext(). > I think we can skip the masks here. > What do you think ? Yep, that would be much better Alistair > > > Regards, > Alvin Chang > > > + break; > > + default: > > + break; > > + } > > + > > + return true; > > +} > > + > > /* Common matching conditions for all types of the triggers. */ static bool > > trigger_common_match(CPURISCVState *env, trigger_type_t type, > > int trigger_index) { > > - return trigger_priv_match(env, type, trigger_index); > > + return trigger_priv_match(env, type, trigger_index) && > > + trigger_textra_match(env, type, trigger_index); > > } > > > > /* type 2 trigger */ > > diff --git a/target/riscv/debug.h b/target/riscv/debug.h index > > c347863578..f76b8f944a 100644 > > --- a/target/riscv/debug.h > > +++ b/target/riscv/debug.h > > @@ -131,6 +131,9 @@ enum { > > #define ITRIGGER_VU BIT(25) > > #define ITRIGGER_VS BIT(26) > > > > +#define MHSELECT_IGNORE 0 > > +#define MHSELECT_MCONTEXT 4 > > + > > bool tdata_available(CPURISCVState *env, int tdata_index); > > > > target_ulong tselect_csr_read(CPURISCVState *env); > > -- > > 2.34.1 > > CONFIDENTIALITY NOTICE: > > This e-mail (and its attachments) may contain confidential and legally privileged information or information protected from disclosure. If you are not the intended recipient, you are hereby notified that any disclosure, copying, distribution, or use of the information contained herein is strictly prohibited. In this case, please immediately notify the sender by return e-mail, delete the message (and any accompanying documents) and destroy all printed hard copies. Thank you for your cooperation. > > Copyright ANDES TECHNOLOGY CORPORATION - All Rights Reserved.
Hi Alistair, > -----Original Message----- > From: Alistair Francis <alistair23@gmail.com> > Sent: Monday, August 26, 2024 8:26 AM > To: Alvin Che-Chia Chang(張哲嘉) <alvinga@andestech.com> > Cc: qemu-riscv@nongnu.org; qemu-devel@nongnu.org; > alistair.francis@wdc.com; bin.meng@windriver.com; liwei1518@gmail.com; > dbarboza@ventanamicro.com; zhiwei_liu@linux.alibaba.com > Subject: Re: [PATCH v3 2/2] target/riscv: Add textra matching condition for the > triggers > > [EXTERNAL MAIL] > > On Tue, Aug 20, 2024 at 2:00 PM Alvin Che-Chia Chang(張哲嘉) > <alvinga@andestech.com> wrote: > > > > Hi Alistair, > > > > > -----Original Message----- > > > From: Alvin Che-Chia Chang(張哲嘉) <alvinga@andestech.com> > > > Sent: Sunday, July 21, 2024 3:24 PM > > > To: qemu-riscv@nongnu.org; qemu-devel@nongnu.org > > > Cc: alistair.francis@wdc.com; bin.meng@windriver.com; > > > liwei1518@gmail.com; dbarboza@ventanamicro.com; > > > zhiwei_liu@linux.alibaba.com; Alvin Che-Chia Chang(張哲嘉) > > > <alvinga@andestech.com> > > > Subject: [PATCH v3 2/2] target/riscv: Add textra matching condition > > > for the triggers > > > > > > According to RISC-V Debug specification, the optional textra32 and > > > textra64 trigger CSRs can be used to configure additional matching > > > conditions for the triggers. For example, if the textra.MHSELECT > > > field is set to 4 (mcontext), this trigger will only match or fire > > > if the low bits of mcontext/hcontext equal textra.MHVALUE field. > > > > > > This commit adds the aforementioned matching condition as common > > > trigger matching conditions. Currently, the only legal values of > > > textra.MHSELECT are 0 > > > (ignore) and 4 (mcontext). When textra.MHSELECT is 0, we pass the > checking. > > > When textra.MHSELECT is 4, we compare textra.MHVALUE with mcontext > CSR. > > > The remaining fields, such as textra.SBYTEMASK, textra.SVALUE, and > > > textra.SSELECT, are hardwired to zero for now. Thus, we skip > > > checking them here. > > > > > > Signed-off-by: Alvin Chang <alvinga@andestech.com> > > > Reviewed-by: Alistair Francis <alistair.francis@wdc.com> > > > --- > > > target/riscv/debug.c | 63 > > > +++++++++++++++++++++++++++++++++++++++++++- > > > target/riscv/debug.h | 3 +++ > > > 2 files changed, 65 insertions(+), 1 deletion(-) > > > > > > diff --git a/target/riscv/debug.c b/target/riscv/debug.c index > > > d6b4a06144..62bb758860 100644 > > > --- a/target/riscv/debug.c > > > +++ b/target/riscv/debug.c > > > @@ -364,11 +364,72 @@ static bool trigger_priv_match(CPURISCVState > > > *env, trigger_type_t type, > > > return false; > > > } > > > > > > +static bool trigger_textra_match(CPURISCVState *env, trigger_type_t > type, > > > + int trigger_index) { > > > + target_ulong textra = env->tdata3[trigger_index]; > > > + target_ulong mhvalue, mhselect; > > > + > > > + if (type < TRIGGER_TYPE_AD_MATCH || type > > > > TRIGGER_TYPE_AD_MATCH6) { > > > + /* textra checking is only applicable when type is 2, 3, 4, 5, or 6 > */ > > > + return true; > > > + } > > > + > > > + switch (riscv_cpu_mxl(env)) { > > > + case MXL_RV32: > > > + mhvalue = get_field(textra, TEXTRA32_MHVALUE); > > > + mhselect = get_field(textra, TEXTRA32_MHSELECT); > > > + break; > > > + case MXL_RV64: > > > + case MXL_RV128: > > > + mhvalue = get_field(textra, TEXTRA64_MHVALUE); > > > + mhselect = get_field(textra, TEXTRA64_MHSELECT); > > > + break; > > > + default: > > > + g_assert_not_reached(); > > > + } > > > + > > > + /* Check mhvalue and mhselect. */ > > > + switch (mhselect) { > > > + case MHSELECT_IGNORE: > > > + break; > > > + case MHSELECT_MCONTEXT: > > > + /* Match or fire if the low bits of mcontext/hcontext equal > mhvalue. > > > */ > > > + if (riscv_has_ext(env, RVH)) { > > > + if (mhvalue != env->mcontext) { > > > + return false; > > > + } > > > + } else { > > > + switch (riscv_cpu_mxl(env)) { > > > + case MXL_RV32: > > > + if (mhvalue != (env->mcontext & MCONTEXT32)) { > > > + return false; > > > + } > > > + break; > > > + case MXL_RV64: > > > + case MXL_RV128: > > > + if (mhvalue != (env->mcontext & MCONTEXT64)) { > > > + return false; > > > + } > > > + break; > > > + default: > > > + g_assert_not_reached(); > > > + } > > > + } > > > > I have some new ideas on this part. > > Should we replace this whole if-else with just the following simple code ? > > > > case MHSELECT_MCONTEXT: > > /* Match if the low bits of mcontext/hcontext equal mhvalue. */ > > if (mhvalue != env->mcontext) { > > return false; > > } > > break; > > > > Those masks on mcontext have been applied in write_mcontext(). > > I think we can skip the masks here. > > What do you think ? > > Yep, that would be much better I've submitted v4 patch to simplify this code. Please see https://lists.gnu.org/archive/html/qemu-riscv/2024-08/msg00512.html Sincerely, Alvin > > Alistair > > > > > > > Regards, > > Alvin Chang > > > > > + break; > > > + default: > > > + break; > > > + } > > > + > > > + return true; > > > +} > > > + > > > /* Common matching conditions for all types of the triggers. */ > > > static bool trigger_common_match(CPURISCVState *env, trigger_type_t > type, > > > int trigger_index) { > > > - return trigger_priv_match(env, type, trigger_index); > > > + return trigger_priv_match(env, type, trigger_index) && > > > + trigger_textra_match(env, type, trigger_index); > > > } > > > > > > /* type 2 trigger */ > > > diff --git a/target/riscv/debug.h b/target/riscv/debug.h index > > > c347863578..f76b8f944a 100644 > > > --- a/target/riscv/debug.h > > > +++ b/target/riscv/debug.h > > > @@ -131,6 +131,9 @@ enum { > > > #define ITRIGGER_VU BIT(25) > > > #define ITRIGGER_VS BIT(26) > > > > > > +#define MHSELECT_IGNORE 0 > > > +#define MHSELECT_MCONTEXT 4 > > > + > > > bool tdata_available(CPURISCVState *env, int tdata_index); > > > > > > target_ulong tselect_csr_read(CPURISCVState *env); > > > -- > > > 2.34.1 > > > > CONFIDENTIALITY NOTICE: > > > > This e-mail (and its attachments) may contain confidential and legally > privileged information or information protected from disclosure. If you are not > the intended recipient, you are hereby notified that any disclosure, copying, > distribution, or use of the information contained herein is strictly prohibited. In > this case, please immediately notify the sender by return e-mail, delete the > message (and any accompanying documents) and destroy all printed hard > copies. Thank you for your cooperation. > > > > Copyright ANDES TECHNOLOGY CORPORATION - All Rights Reserved. CONFIDENTIALITY NOTICE: This e-mail (and its attachments) may contain confidential and legally privileged information or information protected from disclosure. If you are not the intended recipient, you are hereby notified that any disclosure, copying, distribution, or use of the information contained herein is strictly prohibited. In this case, please immediately notify the sender by return e-mail, delete the message (and any accompanying documents) and destroy all printed hard copies. Thank you for your cooperation. Copyright ANDES TECHNOLOGY CORPORATION - All Rights Reserved.
diff --git a/target/riscv/debug.c b/target/riscv/debug.c index d6b4a06144..62bb758860 100644 --- a/target/riscv/debug.c +++ b/target/riscv/debug.c @@ -364,11 +364,72 @@ static bool trigger_priv_match(CPURISCVState *env, trigger_type_t type, return false; } +static bool trigger_textra_match(CPURISCVState *env, trigger_type_t type, + int trigger_index) +{ + target_ulong textra = env->tdata3[trigger_index]; + target_ulong mhvalue, mhselect; + + if (type < TRIGGER_TYPE_AD_MATCH || type > TRIGGER_TYPE_AD_MATCH6) { + /* textra checking is only applicable when type is 2, 3, 4, 5, or 6 */ + return true; + } + + switch (riscv_cpu_mxl(env)) { + case MXL_RV32: + mhvalue = get_field(textra, TEXTRA32_MHVALUE); + mhselect = get_field(textra, TEXTRA32_MHSELECT); + break; + case MXL_RV64: + case MXL_RV128: + mhvalue = get_field(textra, TEXTRA64_MHVALUE); + mhselect = get_field(textra, TEXTRA64_MHSELECT); + break; + default: + g_assert_not_reached(); + } + + /* Check mhvalue and mhselect. */ + switch (mhselect) { + case MHSELECT_IGNORE: + break; + case MHSELECT_MCONTEXT: + /* Match or fire if the low bits of mcontext/hcontext equal mhvalue. */ + if (riscv_has_ext(env, RVH)) { + if (mhvalue != env->mcontext) { + return false; + } + } else { + switch (riscv_cpu_mxl(env)) { + case MXL_RV32: + if (mhvalue != (env->mcontext & MCONTEXT32)) { + return false; + } + break; + case MXL_RV64: + case MXL_RV128: + if (mhvalue != (env->mcontext & MCONTEXT64)) { + return false; + } + break; + default: + g_assert_not_reached(); + } + } + break; + default: + break; + } + + return true; +} + /* Common matching conditions for all types of the triggers. */ static bool trigger_common_match(CPURISCVState *env, trigger_type_t type, int trigger_index) { - return trigger_priv_match(env, type, trigger_index); + return trigger_priv_match(env, type, trigger_index) && + trigger_textra_match(env, type, trigger_index); } /* type 2 trigger */ diff --git a/target/riscv/debug.h b/target/riscv/debug.h index c347863578..f76b8f944a 100644 --- a/target/riscv/debug.h +++ b/target/riscv/debug.h @@ -131,6 +131,9 @@ enum { #define ITRIGGER_VU BIT(25) #define ITRIGGER_VS BIT(26) +#define MHSELECT_IGNORE 0 +#define MHSELECT_MCONTEXT 4 + bool tdata_available(CPURISCVState *env, int tdata_index); target_ulong tselect_csr_read(CPURISCVState *env);