[v4] RISC-V: Add preliminary textra trigger CSR functions

[PATCH v4 2/2] target/riscv: Add textra matching condition for the triggers

Posted by Alvin Chang via 3 months ago

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>
---
 target/riscv/debug.c | 45 +++++++++++++++++++++++++++++++++++++++++++-
 target/riscv/debug.h |  3 +++
 2 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/target/riscv/debug.c b/target/riscv/debug.c
index d6b4a06144..c79b51af30 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -364,11 +364,54 @@ 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 if the low bits of mcontext/hcontext equal mhvalue. */
+        if (mhvalue != env->mcontext) {
+            return false;
+        }
+        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

Re: [PATCH v4 2/2] target/riscv: Add textra matching condition for the triggers

Posted by Alistair Francis 2 months, 2 weeks ago

On Mon, Aug 26, 2024 at 12:48 PM Alvin Chang via <qemu-devel@nongnu.org> wrote:
>
> 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>

Alistair

> ---
>  target/riscv/debug.c | 45 +++++++++++++++++++++++++++++++++++++++++++-
>  target/riscv/debug.h |  3 +++
>  2 files changed, 47 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> index d6b4a06144..c79b51af30 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -364,11 +364,54 @@ 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 if the low bits of mcontext/hcontext equal mhvalue. */
> +        if (mhvalue != env->mcontext) {
> +            return false;
> +        }
> +        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
>
>

Re: [PATCH v4 2/2] target/riscv: Add textra matching condition for the triggers

Posted by Alistair Francis 2 months, 2 weeks ago

On Mon, Aug 26, 2024 at 12:48 PM Alvin Chang via <qemu-devel@nongnu.org> wrote:

Your From is still wrong

>
> 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>
> ---
>  target/riscv/debug.c | 45 +++++++++++++++++++++++++++++++++++++++++++-
>  target/riscv/debug.h |  3 +++
>  2 files changed, 47 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> index d6b4a06144..c79b51af30 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -364,11 +364,54 @@ 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;

Shouldn't this be false?

Alistair

> +    }
> +
> +    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 if the low bits of mcontext/hcontext equal mhvalue. */
> +        if (mhvalue != env->mcontext) {
> +            return false;
> +        }
> +        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
>
>

RE: [PATCH v4 2/2] target/riscv: Add textra matching condition for the triggers

Posted by Alvin Che-Chia Chang(張哲嘉) 2 months, 2 weeks ago

Hi Alistair,

> -----Original Message-----
> From: Alistair Francis <alistair23@gmail.com>
> Sent: Friday, September 6, 2024 8:29 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 v4 2/2] target/riscv: Add textra matching condition for the
> triggers
>
> [EXTERNAL MAIL]
>
> On Mon, Aug 26, 2024 at 12:48 PM Alvin Chang via
> <qemu-devel@nongnu.org> wrote:
>
> Your From is still wrong

Oh sorry for inconvenience. Seems it randomly happens on our server.
I will ask my colleagues.

>
> >
> > 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>
> > ---
> >  target/riscv/debug.c | 45
> > +++++++++++++++++++++++++++++++++++++++++++-
> >  target/riscv/debug.h |  3 +++
> >  2 files changed, 47 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/riscv/debug.c b/target/riscv/debug.c index
> > d6b4a06144..c79b51af30 100644
> > --- a/target/riscv/debug.c
> > +++ b/target/riscv/debug.c
> > @@ -364,11 +364,54 @@ 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;
>
> Shouldn't this be false?

My idea is: only type 2, 3, 4, 5, 6 triggers need to further check textra CSR.
Other types of trigger should bypass check on textra CSR, which means they pass the check on textra CSR.
Thus, return true should be correct.


Alvin

>
> Alistair
>
> > +    }
> > +
> > +    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 if the low bits of mcontext/hcontext equal mhvalue. */
> > +        if (mhvalue != env->mcontext) {
> > +            return false;
> > +        }
> > +        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.

[PATCH v4 1/2] target/riscv: Preliminary textra trigger CSR writting support
[PATCH v4 2/2] target/riscv: Add textra matching condition for the triggers