* [PATCH 1/4] target/riscv: Add functions for common matching conditions of trigger
2024-02-19 3:25 [PATCH 0/4] RISC-V: Modularize common match conditions for trigger Alvin Chang via
@ 2024-02-19 3:25 ` Alvin Chang via
2024-02-21 17:09 ` Daniel Henrique Barboza
2024-02-19 3:25 ` [PATCH 2/4] target/riscv: Apply modularized matching conditions for breakpoint Alvin Chang via
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Alvin Chang via @ 2024-02-19 3:25 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: alistair.francis, bin.meng, liwei1518, dbarboza, zhiwei_liu, Alvin Chang
According to RISC-V Debug specification, there are several common
matching conditions before firing a trigger, including the enabled
privilege levels of the trigger.
This commit adds trigger_common_match() to prepare the common matching
conditions for the type 2/3/6 triggers. For now, we just implement
trigger_priv_match() to check if the enabled privilege levels of the
trigger match CPU's current privilege level.
Signed-off-by: Alvin Chang <alvinga@andestech.com>
---
target/riscv/debug.c | 70 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 70 insertions(+)
diff --git a/target/riscv/debug.c b/target/riscv/debug.c
index e30d99cc2f..7932233073 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -241,6 +241,76 @@ static void do_trigger_action(CPURISCVState *env, target_ulong trigger_index)
}
}
+/*
+ * Check the privilege level of specific trigger matches CPU's current privilege
+ * level.
+ */
+static bool trigger_priv_match(CPURISCVState *env, trigger_type_t type,
+ int trigger_index)
+{
+ target_ulong ctrl = env->tdata1[trigger_index];
+
+ switch (type) {
+ case TRIGGER_TYPE_AD_MATCH:
+ /* type 2 trigger cannot be fired in VU/VS mode */
+ if (env->virt_enabled) {
+ return false;
+ }
+ /* check U/S/M bit against current privilege level */
+ if ((ctrl >> 3) & BIT(env->priv)) {
+ return true;
+ }
+ break;
+ case TRIGGER_TYPE_AD_MATCH6:
+ 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;
+ }
+ }
+ break;
+ case TRIGGER_TYPE_INST_CNT:
+ if (env->virt_enabled) {
+ /* check VU/VS bit against current privilege level */
+ if ((ctrl >> 25) & BIT(env->priv)) {
+ return true;
+ }
+ } else {
+ /* check U/S/M bit against current privilege level */
+ if ((ctrl >> 6) & BIT(env->priv)) {
+ return true;
+ }
+ }
+ break;
+ case TRIGGER_TYPE_INT:
+ case TRIGGER_TYPE_EXCP:
+ case TRIGGER_TYPE_EXT_SRC:
+ qemu_log_mask(LOG_UNIMP, "trigger type: %d is not supported\n", type);
+ break;
+ case TRIGGER_TYPE_NO_EXIST:
+ case TRIGGER_TYPE_UNAVAIL:
+ qemu_log_mask(LOG_GUEST_ERROR, "trigger type: %d does not exit\n",
+ type);
+ break;
+ default:
+ g_assert_not_reached();
+ }
+
+ return false;
+}
+
+/* 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);
+}
+
/* type 2 trigger */
static uint32_t type2_breakpoint_size(CPURISCVState *env, target_ulong ctrl)
--
2.34.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 1/4] target/riscv: Add functions for common matching conditions of trigger
2024-02-19 3:25 ` [PATCH 1/4] target/riscv: Add functions for common matching conditions of trigger Alvin Chang via
@ 2024-02-21 17:09 ` Daniel Henrique Barboza
0 siblings, 0 replies; 12+ messages in thread
From: Daniel Henrique Barboza @ 2024-02-21 17:09 UTC (permalink / raw)
To: Alvin Chang, qemu-riscv, qemu-devel
Cc: alistair.francis, bin.meng, liwei1518, zhiwei_liu
On 2/19/24 00:25, Alvin Chang wrote:
> According to RISC-V Debug specification, there are several common
> matching conditions before firing a trigger, including the enabled
> privilege levels of the trigger.
>
> This commit adds trigger_common_match() to prepare the common matching
> conditions for the type 2/3/6 triggers. For now, we just implement
> trigger_priv_match() to check if the enabled privilege levels of the
> trigger match CPU's current privilege level.
>
> Signed-off-by: Alvin Chang <alvinga@andestech.com>
> ---
> target/riscv/debug.c | 70 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 70 insertions(+)
>
> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> index e30d99cc2f..7932233073 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -241,6 +241,76 @@ static void do_trigger_action(CPURISCVState *env, target_ulong trigger_index)
> }
> }
>
> +/*
> + * Check the privilege level of specific trigger matches CPU's current privilege
> + * level.
> + */
> +static bool trigger_priv_match(CPURISCVState *env, trigger_type_t type,
> + int trigger_index)
> +{
> + target_ulong ctrl = env->tdata1[trigger_index];
> +
> + switch (type) {
> + case TRIGGER_TYPE_AD_MATCH:
> + /* type 2 trigger cannot be fired in VU/VS mode */
> + if (env->virt_enabled) {
> + return false;
> + }
> + /* check U/S/M bit against current privilege level */
> + if ((ctrl >> 3) & BIT(env->priv)) {
> + return true;
> + }
> + break;
> + case TRIGGER_TYPE_AD_MATCH6:
> + 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;
> + }
> + }
> + break;
> + case TRIGGER_TYPE_INST_CNT:
> + if (env->virt_enabled) {
> + /* check VU/VS bit against current privilege level */
> + if ((ctrl >> 25) & BIT(env->priv)) {
> + return true;
> + }
> + } else {
> + /* check U/S/M bit against current privilege level */
> + if ((ctrl >> 6) & BIT(env->priv)) {
> + return true;
> + }
> + }
> + break;
> + case TRIGGER_TYPE_INT:
> + case TRIGGER_TYPE_EXCP:
> + case TRIGGER_TYPE_EXT_SRC:
> + qemu_log_mask(LOG_UNIMP, "trigger type: %d is not supported\n", type);
> + break;
> + case TRIGGER_TYPE_NO_EXIST:
> + case TRIGGER_TYPE_UNAVAIL:
> + qemu_log_mask(LOG_GUEST_ERROR, "trigger type: %d does not exit\n",
> + type);
I believe you meant 'does not exist'.
Thanks,
Daniel
> + break;
> + default:
> + g_assert_not_reached();
> + }
> +
> + return false;
> +}
> +
> +/* 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);
> +}
> +
> /* type 2 trigger */
>
> static uint32_t type2_breakpoint_size(CPURISCVState *env, target_ulong ctrl)
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/4] target/riscv: Apply modularized matching conditions for breakpoint
2024-02-19 3:25 [PATCH 0/4] RISC-V: Modularize common match conditions for trigger Alvin Chang via
2024-02-19 3:25 ` [PATCH 1/4] target/riscv: Add functions for common matching conditions of trigger Alvin Chang via
@ 2024-02-19 3:25 ` Alvin Chang via
2024-02-21 17:25 ` Daniel Henrique Barboza
2024-02-19 3:25 ` [PATCH 3/4] target/riscv: Apply modularized matching conditions for watchpoint Alvin Chang via
2024-02-19 3:25 ` [PATCH 4/4] target/riscv: Apply modularized matching conditions for icount trigger Alvin Chang via
3 siblings, 1 reply; 12+ messages in thread
From: Alvin Chang via @ 2024-02-19 3:25 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: alistair.francis, bin.meng, liwei1518, dbarboza, zhiwei_liu, Alvin Chang
We have implemented trigger_common_match(), which checks if the enabled
privilege levels of the trigger match CPU's current privilege level.
Remove the related code in riscv_cpu_debug_check_breakpoint() and invoke
trigger_common_match() to check the privilege levels of the type 2 and
type 6 triggers for the breakpoints.
Only the execution bit and the executed PC should be futher checked in
riscv_cpu_debug_check_breakpoint().
Signed-off-by: Alvin Chang <alvinga@andestech.com>
---
target/riscv/debug.c | 26 ++++++--------------------
1 file changed, 6 insertions(+), 20 deletions(-)
diff --git a/target/riscv/debug.c b/target/riscv/debug.c
index 7932233073..b971ed5d7a 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -855,21 +855,17 @@ bool riscv_cpu_debug_check_breakpoint(CPUState *cs)
for (i = 0; i < RV_MAX_TRIGGERS; i++) {
trigger_type = get_trigger_type(env, i);
+ if (!trigger_common_match(env, trigger_type, i)) {
+ continue;
+ }
+
switch (trigger_type) {
case TRIGGER_TYPE_AD_MATCH:
- /* type 2 trigger cannot be fired in VU/VS mode */
- if (env->virt_enabled) {
- return false;
- }
-
ctrl = env->tdata1[i];
pc = env->tdata2[i];
if ((ctrl & TYPE2_EXEC) && (bp->pc == pc)) {
- /* check U/S/M bit against current privilege level */
- if ((ctrl >> 3) & BIT(env->priv)) {
- return true;
- }
+ return true;
}
break;
case TRIGGER_TYPE_AD_MATCH6:
@@ -877,17 +873,7 @@ bool riscv_cpu_debug_check_breakpoint(CPUState *cs)
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;
- }
- }
+ return true;
}
break;
default:
--
2.34.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 2/4] target/riscv: Apply modularized matching conditions for breakpoint
2024-02-19 3:25 ` [PATCH 2/4] target/riscv: Apply modularized matching conditions for breakpoint Alvin Chang via
@ 2024-02-21 17:25 ` Daniel Henrique Barboza
2024-02-22 1:46 ` Alvin Che-Chia Chang(張哲嘉)
0 siblings, 1 reply; 12+ messages in thread
From: Daniel Henrique Barboza @ 2024-02-21 17:25 UTC (permalink / raw)
To: Alvin Chang, qemu-riscv, qemu-devel
Cc: alistair.francis, bin.meng, liwei1518, zhiwei_liu
On 2/19/24 00:25, Alvin Chang wrote:
> We have implemented trigger_common_match(), which checks if the enabled
> privilege levels of the trigger match CPU's current privilege level.
> Remove the related code in riscv_cpu_debug_check_breakpoint() and invoke
> trigger_common_match() to check the privilege levels of the type 2 and
> type 6 triggers for the breakpoints.
>
> Only the execution bit and the executed PC should be futher checked in
typo: further
> riscv_cpu_debug_check_breakpoint().
>
> Signed-off-by: Alvin Chang <alvinga@andestech.com>
> ---
> target/riscv/debug.c | 26 ++++++--------------------
> 1 file changed, 6 insertions(+), 20 deletions(-)
>
> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> index 7932233073..b971ed5d7a 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -855,21 +855,17 @@ bool riscv_cpu_debug_check_breakpoint(CPUState *cs)
> for (i = 0; i < RV_MAX_TRIGGERS; i++) {
> trigger_type = get_trigger_type(env, i);
>
> + if (!trigger_common_match(env, trigger_type, i)) {
> + continue;
> + }
> +
I believe this will change how the function behaves. Before this patch, we would
'return false' if we have a TRIGGER_TYPE_AD_MATCH and env->virt_enabled is true.
Now, for the same case, we'll keep looping until we found a match, or return 'false'
if we run out of triggers.
This seems ok to do, but we should state in the commit msg that we're intentionally
change how the function works. If that's not the idea and we want to preserve the existing
behavior, we would need to do:
>
> + if (!trigger_common_match(env, trigger_type, i)) {
> + return false;
> + }
Instead of just continue looping. Thanks,
Daniel
> switch (trigger_type) {
> case TRIGGER_TYPE_AD_MATCH:
> - /* type 2 trigger cannot be fired in VU/VS mode */
> - if (env->virt_enabled) {
> - return false;
> - }
> -
> ctrl = env->tdata1[i];
> pc = env->tdata2[i];
>
> if ((ctrl & TYPE2_EXEC) && (bp->pc == pc)) {
> - /* check U/S/M bit against current privilege level */
> - if ((ctrl >> 3) & BIT(env->priv)) {
> - return true;
> - }
> + return true;
> }
> break;
> case TRIGGER_TYPE_AD_MATCH6:
> @@ -877,17 +873,7 @@ bool riscv_cpu_debug_check_breakpoint(CPUState *cs)
> 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;
> - }
> - }
> + return true;
> }
> break;
> default:
^ permalink raw reply [flat|nested] 12+ messages in thread* RE: [PATCH 2/4] target/riscv: Apply modularized matching conditions for breakpoint
2024-02-21 17:25 ` Daniel Henrique Barboza
@ 2024-02-22 1:46 ` Alvin Che-Chia Chang(張哲嘉)
2024-02-22 12:37 ` Daniel Henrique Barboza
0 siblings, 1 reply; 12+ messages in thread
From: Alvin Che-Chia Chang(張哲嘉) @ 2024-02-22 1:46 UTC (permalink / raw)
To: Daniel Henrique Barboza, qemu-riscv, qemu-devel
Cc: alistair.francis, bin.meng, liwei1518, zhiwei_liu
Hi Daniel,
> -----Original Message-----
> From: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Sent: Thursday, February 22, 2024 1:26 AM
> To: Alvin Che-Chia Chang(張哲嘉) <alvinga@andestech.com>;
> qemu-riscv@nongnu.org; qemu-devel@nongnu.org
> Cc: alistair.francis@wdc.com; bin.meng@windriver.com;
> liwei1518@gmail.com; zhiwei_liu@linux.alibaba.com
> Subject: Re: [PATCH 2/4] target/riscv: Apply modularized matching conditions
> for breakpoint
>
>
>
> On 2/19/24 00:25, Alvin Chang wrote:
> > We have implemented trigger_common_match(), which checks if the
> > enabled privilege levels of the trigger match CPU's current privilege level.
> > Remove the related code in riscv_cpu_debug_check_breakpoint() and
> > invoke
> > trigger_common_match() to check the privilege levels of the type 2 and
> > type 6 triggers for the breakpoints.
> >
> > Only the execution bit and the executed PC should be futher checked in
>
> typo: further
Thanks! Will fix it.
>
> > riscv_cpu_debug_check_breakpoint().
> >
> > Signed-off-by: Alvin Chang <alvinga@andestech.com>
> > ---
> > target/riscv/debug.c | 26 ++++++--------------------
> > 1 file changed, 6 insertions(+), 20 deletions(-)
> >
> > diff --git a/target/riscv/debug.c b/target/riscv/debug.c index
> > 7932233073..b971ed5d7a 100644
> > --- a/target/riscv/debug.c
> > +++ b/target/riscv/debug.c
> > @@ -855,21 +855,17 @@ bool riscv_cpu_debug_check_breakpoint(CPUState
> *cs)
> > for (i = 0; i < RV_MAX_TRIGGERS; i++) {
> > trigger_type = get_trigger_type(env, i);
> >
> > + if (!trigger_common_match(env, trigger_type, i)) {
> > + continue;
> > + }
> > +
>
> I believe this will change how the function behaves. Before this patch, we
> would 'return false' if we have a TRIGGER_TYPE_AD_MATCH and
> env->virt_enabled is true.
>
> Now, for the same case, we'll keep looping until we found a match, or return
> 'false'
> if we run out of triggers.
Oh! I didn't notice that the behavior is changed.. thank you.
Image that CPU supports both type 2 and type 6 triggers, and the debugger sets two identical breakpoints.(this should be a mistake, but we should not restrict the debugger)
One of them is type 2 breakpoint at trigger index 0, and the other is type 6 breakpoint at trigger index 1.
Now if the system runs in VS/VU modes, it could match type 6 breakpoint, so the looping is necessary.
If the system runs in M/HS/U modes, it could match type 2 breakpoint.
Is my understanding correct?
Sincerely,
Alvin
>
> This seems ok to do, but we should state in the commit msg that we're
> intentionally change how the function works. If that's not the idea and we want
> to preserve the existing behavior, we would need to do:
>
> >
> > + if (!trigger_common_match(env, trigger_type, i)) {
> > + return false;
> > + }
>
> Instead of just continue looping. Thanks,
>
>
> Daniel
>
> > switch (trigger_type) {
> > case TRIGGER_TYPE_AD_MATCH:
> > - /* type 2 trigger cannot be fired in VU/VS mode */
> > - if (env->virt_enabled) {
> > - return false;
> > - }
> > -
> > ctrl = env->tdata1[i];
> > pc = env->tdata2[i];
> >
> > if ((ctrl & TYPE2_EXEC) && (bp->pc == pc)) {
> > - /* check U/S/M bit against current privilege level
> */
> > - if ((ctrl >> 3) & BIT(env->priv)) {
> > - return true;
> > - }
> > + return true;
> > }
> > break;
> > case TRIGGER_TYPE_AD_MATCH6:
> > @@ -877,17 +873,7 @@ bool riscv_cpu_debug_check_breakpoint(CPUState
> *cs)
> > 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;
> > - }
> > - }
> > + return true;
> > }
> > break;
> > default:
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 2/4] target/riscv: Apply modularized matching conditions for breakpoint
2024-02-22 1:46 ` Alvin Che-Chia Chang(張哲嘉)
@ 2024-02-22 12:37 ` Daniel Henrique Barboza
0 siblings, 0 replies; 12+ messages in thread
From: Daniel Henrique Barboza @ 2024-02-22 12:37 UTC (permalink / raw)
To: Alvin Che-Chia Chang(張哲嘉), qemu-riscv, qemu-devel
Cc: alistair.francis, bin.meng, liwei1518, zhiwei_liu
On 2/21/24 22:46, Alvin Che-Chia Chang(張哲嘉) wrote:
> Hi Daniel,
>
>> -----Original Message-----
>> From: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> Sent: Thursday, February 22, 2024 1:26 AM
>> To: Alvin Che-Chia Chang(張哲嘉) <alvinga@andestech.com>;
>> qemu-riscv@nongnu.org; qemu-devel@nongnu.org
>> Cc: alistair.francis@wdc.com; bin.meng@windriver.com;
>> liwei1518@gmail.com; zhiwei_liu@linux.alibaba.com
>> Subject: Re: [PATCH 2/4] target/riscv: Apply modularized matching conditions
>> for breakpoint
>>
>>
>>
>> On 2/19/24 00:25, Alvin Chang wrote:
>>> We have implemented trigger_common_match(), which checks if the
>>> enabled privilege levels of the trigger match CPU's current privilege level.
>>> Remove the related code in riscv_cpu_debug_check_breakpoint() and
>>> invoke
>>> trigger_common_match() to check the privilege levels of the type 2 and
>>> type 6 triggers for the breakpoints.
>>>
>>> Only the execution bit and the executed PC should be futher checked in
>>
>> typo: further
>
> Thanks! Will fix it.
>
>>
>>> riscv_cpu_debug_check_breakpoint().
>>>
>>> Signed-off-by: Alvin Chang <alvinga@andestech.com>
>>> ---
>>> target/riscv/debug.c | 26 ++++++--------------------
>>> 1 file changed, 6 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/target/riscv/debug.c b/target/riscv/debug.c index
>>> 7932233073..b971ed5d7a 100644
>>> --- a/target/riscv/debug.c
>>> +++ b/target/riscv/debug.c
>>> @@ -855,21 +855,17 @@ bool riscv_cpu_debug_check_breakpoint(CPUState
>> *cs)
>>> for (i = 0; i < RV_MAX_TRIGGERS; i++) {
>>> trigger_type = get_trigger_type(env, i);
>>>
>>> + if (!trigger_common_match(env, trigger_type, i)) {
>>> + continue;
>>> + }
>>> +
>>
>> I believe this will change how the function behaves. Before this patch, we
>> would 'return false' if we have a TRIGGER_TYPE_AD_MATCH and
>> env->virt_enabled is true.
>>
>> Now, for the same case, we'll keep looping until we found a match, or return
>> 'false'
>> if we run out of triggers.
>
> Oh! I didn't notice that the behavior is changed.. thank you.
>
> Image that CPU supports both type 2 and type 6 triggers, and the debugger sets two identical breakpoints.(this should be a mistake, but we should not restrict the debugger)
> One of them is type 2 breakpoint at trigger index 0, and the other is type 6 breakpoint at trigger index 1.
> Now if the system runs in VS/VU modes, it could match type 6 breakpoint, so the looping is necessary.
> If the system runs in M/HS/U modes, it could match type 2 breakpoint.
> Is my understanding correct?
It looks correct to me. We just need to mention in the commit msg that the behavior
change is intentional, not an accident.
Thanks,
Daniel
>
>
> Sincerely,
> Alvin
>
>>
>> This seems ok to do, but we should state in the commit msg that we're
>> intentionally change how the function works. If that's not the idea and we want
>> to preserve the existing behavior, we would need to do:
>>
>>>
>>> + if (!trigger_common_match(env, trigger_type, i)) {
>>> + return false;
>>> + }
>>
>> Instead of just continue looping. Thanks,
>>
>>
>> Daniel
>>
>>> switch (trigger_type) {
>>> case TRIGGER_TYPE_AD_MATCH:
>>> - /* type 2 trigger cannot be fired in VU/VS mode */
>>> - if (env->virt_enabled) {
>>> - return false;
>>> - }
>>> -
>>> ctrl = env->tdata1[i];
>>> pc = env->tdata2[i];
>>>
>>> if ((ctrl & TYPE2_EXEC) && (bp->pc == pc)) {
>>> - /* check U/S/M bit against current privilege level
>> */
>>> - if ((ctrl >> 3) & BIT(env->priv)) {
>>> - return true;
>>> - }
>>> + return true;
>>> }
>>> break;
>>> case TRIGGER_TYPE_AD_MATCH6:
>>> @@ -877,17 +873,7 @@ bool riscv_cpu_debug_check_breakpoint(CPUState
>> *cs)
>>> 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;
>>> - }
>>> - }
>>> + return true;
>>> }
>>> break;
>>> default:
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/4] target/riscv: Apply modularized matching conditions for watchpoint
2024-02-19 3:25 [PATCH 0/4] RISC-V: Modularize common match conditions for trigger Alvin Chang via
2024-02-19 3:25 ` [PATCH 1/4] target/riscv: Add functions for common matching conditions of trigger Alvin Chang via
2024-02-19 3:25 ` [PATCH 2/4] target/riscv: Apply modularized matching conditions for breakpoint Alvin Chang via
@ 2024-02-19 3:25 ` Alvin Chang via
2024-02-21 17:31 ` Daniel Henrique Barboza
2024-02-19 3:25 ` [PATCH 4/4] target/riscv: Apply modularized matching conditions for icount trigger Alvin Chang via
3 siblings, 1 reply; 12+ messages in thread
From: Alvin Chang via @ 2024-02-19 3:25 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: alistair.francis, bin.meng, liwei1518, dbarboza, zhiwei_liu, Alvin Chang
We have implemented trigger_common_match(), which checks if the enabled
privilege levels of the trigger match CPU's current privilege level.
Remove the related code in riscv_cpu_debug_check_watchpoint() and invoke
trigger_common_match() to check the privilege levels of the type 2 and
type 6 triggers for the watchpoints.
Only load/store bits and loaded/stored address should be further checked
in riscv_cpu_debug_check_watchpoint().
Signed-off-by: Alvin Chang <alvinga@andestech.com>
---
target/riscv/debug.c | 26 ++++++--------------------
1 file changed, 6 insertions(+), 20 deletions(-)
diff --git a/target/riscv/debug.c b/target/riscv/debug.c
index b971ed5d7a..67ba19c966 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -899,13 +899,12 @@ bool riscv_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
for (i = 0; i < RV_MAX_TRIGGERS; i++) {
trigger_type = get_trigger_type(env, i);
+ if (!trigger_common_match(env, trigger_type, i)) {
+ continue;
+ }
+
switch (trigger_type) {
case TRIGGER_TYPE_AD_MATCH:
- /* type 2 trigger cannot be fired in VU/VS mode */
- if (env->virt_enabled) {
- return false;
- }
-
ctrl = env->tdata1[i];
addr = env->tdata2[i];
flags = 0;
@@ -918,10 +917,7 @@ bool riscv_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
}
if ((wp->flags & flags) && (wp->vaddr == addr)) {
- /* check U/S/M bit against current privilege level */
- if ((ctrl >> 3) & BIT(env->priv)) {
- return true;
- }
+ return true;
}
break;
case TRIGGER_TYPE_AD_MATCH6:
@@ -937,17 +933,7 @@ bool riscv_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
}
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;
- }
- }
+ return true;
}
break;
default:
--
2.34.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 3/4] target/riscv: Apply modularized matching conditions for watchpoint
2024-02-19 3:25 ` [PATCH 3/4] target/riscv: Apply modularized matching conditions for watchpoint Alvin Chang via
@ 2024-02-21 17:31 ` Daniel Henrique Barboza
0 siblings, 0 replies; 12+ messages in thread
From: Daniel Henrique Barboza @ 2024-02-21 17:31 UTC (permalink / raw)
To: Alvin Chang, qemu-riscv, qemu-devel
Cc: alistair.francis, bin.meng, liwei1518, zhiwei_liu
On 2/19/24 00:25, Alvin Chang wrote:
> We have implemented trigger_common_match(), which checks if the enabled
> privilege levels of the trigger match CPU's current privilege level.
> Remove the related code in riscv_cpu_debug_check_watchpoint() and invoke
> trigger_common_match() to check the privilege levels of the type 2 and
> type 6 triggers for the watchpoints.
>
> Only load/store bits and loaded/stored address should be further checked
> in riscv_cpu_debug_check_watchpoint().
>
> Signed-off-by: Alvin Chang <alvinga@andestech.com>
> ---
> target/riscv/debug.c | 26 ++++++--------------------
> 1 file changed, 6 insertions(+), 20 deletions(-)
>
> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> index b971ed5d7a..67ba19c966 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -899,13 +899,12 @@ bool riscv_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
> for (i = 0; i < RV_MAX_TRIGGERS; i++) {
> trigger_type = get_trigger_type(env, i);
>
> + if (!trigger_common_match(env, trigger_type, i)) {
> + continue;
> + }
> +
The same comments I made in patch 2 also applies here. It's ok to change
how the function behaves as long as we're doing it on purpose and explaining
why in the commit message. Otherwise this if (!trigger_common_match()"
conditional should "return false" instead of keep looping to maintain
the existing behavior.
Thanks,
Daniel
> switch (trigger_type) {
> case TRIGGER_TYPE_AD_MATCH:
> - /* type 2 trigger cannot be fired in VU/VS mode */
> - if (env->virt_enabled) {
> - return false;
> - }
> -
> ctrl = env->tdata1[i];
> addr = env->tdata2[i];
> flags = 0;
> @@ -918,10 +917,7 @@ bool riscv_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
> }
>
> if ((wp->flags & flags) && (wp->vaddr == addr)) {
> - /* check U/S/M bit against current privilege level */
> - if ((ctrl >> 3) & BIT(env->priv)) {
> - return true;
> - }
> + return true;
> }
> break;
> case TRIGGER_TYPE_AD_MATCH6:
> @@ -937,17 +933,7 @@ bool riscv_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
> }
>
> 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;
> - }
> - }
> + return true;
> }
> break;
> default:
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 4/4] target/riscv: Apply modularized matching conditions for icount trigger
2024-02-19 3:25 [PATCH 0/4] RISC-V: Modularize common match conditions for trigger Alvin Chang via
` (2 preceding siblings ...)
2024-02-19 3:25 ` [PATCH 3/4] target/riscv: Apply modularized matching conditions for watchpoint Alvin Chang via
@ 2024-02-19 3:25 ` Alvin Chang via
2024-02-21 18:05 ` Daniel Henrique Barboza
3 siblings, 1 reply; 12+ messages in thread
From: Alvin Chang via @ 2024-02-19 3:25 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: alistair.francis, bin.meng, liwei1518, dbarboza, zhiwei_liu, Alvin Chang
We have implemented trigger_common_match(), which checks if the enabled
privilege levels of the trigger match CPU's current privilege level. We
can invoke trigger_common_match() to check the privilege levels of the
type 3 triggers.
Signed-off-by: Alvin Chang <alvinga@andestech.com>
---
target/riscv/debug.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/riscv/debug.c b/target/riscv/debug.c
index 67ba19c966..de996a393c 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -624,7 +624,7 @@ void helper_itrigger_match(CPURISCVState *env)
if (get_trigger_type(env, i) != TRIGGER_TYPE_INST_CNT) {
continue;
}
- if (check_itrigger_priv(env, i)) {
+ if (!trigger_common_match(env, TRIGGER_TYPE_INST_CNT, i)) {
continue;
}
count = itrigger_get_count(env, i);
--
2.34.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 4/4] target/riscv: Apply modularized matching conditions for icount trigger
2024-02-19 3:25 ` [PATCH 4/4] target/riscv: Apply modularized matching conditions for icount trigger Alvin Chang via
@ 2024-02-21 18:05 ` Daniel Henrique Barboza
2024-02-22 2:05 ` Alvin Che-Chia Chang(張哲嘉)
0 siblings, 1 reply; 12+ messages in thread
From: Daniel Henrique Barboza @ 2024-02-21 18:05 UTC (permalink / raw)
To: Alvin Chang, qemu-riscv, qemu-devel
Cc: alistair.francis, bin.meng, liwei1518, zhiwei_liu
On 2/19/24 00:25, Alvin Chang wrote:
> We have implemented trigger_common_match(), which checks if the enabled
> privilege levels of the trigger match CPU's current privilege level. We
> can invoke trigger_common_match() to check the privilege levels of the
> type 3 triggers.
>
> Signed-off-by: Alvin Chang <alvinga@andestech.com>
> ---
> target/riscv/debug.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> index 67ba19c966..de996a393c 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -624,7 +624,7 @@ void helper_itrigger_match(CPURISCVState *env)
> if (get_trigger_type(env, i) != TRIGGER_TYPE_INST_CNT) {
> continue;
> }
> - if (check_itrigger_priv(env, i)) {
> + if (!trigger_common_match(env, TRIGGER_TYPE_INST_CNT, i)) {
> continue;
> }
Looks good. Shouldn't we also change riscv_itrigger_enabled() to also use
trigger_common_match()? riscv_itrigger_enabled() is remarkably similar to
helper_itrigger_match() so I believe we can also use the new function
there.
Thanks,
Daniel
> count = itrigger_get_count(env, i);
^ permalink raw reply [flat|nested] 12+ messages in thread* RE: [PATCH 4/4] target/riscv: Apply modularized matching conditions for icount trigger
2024-02-21 18:05 ` Daniel Henrique Barboza
@ 2024-02-22 2:05 ` Alvin Che-Chia Chang(張哲嘉)
0 siblings, 0 replies; 12+ messages in thread
From: Alvin Che-Chia Chang(張哲嘉) @ 2024-02-22 2:05 UTC (permalink / raw)
To: Daniel Henrique Barboza, qemu-riscv, qemu-devel
Cc: alistair.francis, bin.meng, liwei1518, zhiwei_liu
Hi Daniel,
> -----Original Message-----
> From: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Sent: Thursday, February 22, 2024 2:06 AM
> To: Alvin Che-Chia Chang(張哲嘉) <alvinga@andestech.com>;
> qemu-riscv@nongnu.org; qemu-devel@nongnu.org
> Cc: alistair.francis@wdc.com; bin.meng@windriver.com;
> liwei1518@gmail.com; zhiwei_liu@linux.alibaba.com
> Subject: Re: [PATCH 4/4] target/riscv: Apply modularized matching conditions
> for icount trigger
>
>
>
> On 2/19/24 00:25, Alvin Chang wrote:
> > We have implemented trigger_common_match(), which checks if the
> > enabled privilege levels of the trigger match CPU's current privilege
> > level. We can invoke trigger_common_match() to check the privilege
> > levels of the type 3 triggers.
> >
> > Signed-off-by: Alvin Chang <alvinga@andestech.com>
> > ---
> > target/riscv/debug.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/target/riscv/debug.c b/target/riscv/debug.c index
> > 67ba19c966..de996a393c 100644
> > --- a/target/riscv/debug.c
> > +++ b/target/riscv/debug.c
> > @@ -624,7 +624,7 @@ void helper_itrigger_match(CPURISCVState *env)
> > if (get_trigger_type(env, i) != TRIGGER_TYPE_INST_CNT) {
> > continue;
> > }
> > - if (check_itrigger_priv(env, i)) {
> > + if (!trigger_common_match(env, TRIGGER_TYPE_INST_CNT, i)) {
> > continue;
> > }
>
>
> Looks good. Shouldn't we also change riscv_itrigger_enabled() to also use
> trigger_common_match()? riscv_itrigger_enabled() is remarkably similar to
> helper_itrigger_match() so I believe we can also use the new function there.
I think we might not want to apply trigger_common_match() into riscv_itrigger_enabled().
The trigger_common_match() is used to check if the trigger can be matched in current privilege level.
It will check many conditions: trigger privilege levels, textra, tcontrol, etc.
The riscv_itrigger_enabled() is used to check if any icount trigger is enabled by checking vs/vu/count/s/u fields of tdata1 only.
In fact, we found the comparisons between tdata1 bit-fields and env->priv in check_itrigger_priv() are bugs.
And we have a patch to fix that:
bool riscv_itrigger_enabled(CPURISCVState *env)
{
int count;
for (int i = 0; i < RV_MAX_TRIGGERS; i++) {
if (get_trigger_type(env, i) != TRIGGER_TYPE_INST_CNT) {
continue;
}
- if (check_itrigger_priv(env, i)) {
+ if ((env->tdata1[i] & ITRIGGER_VS) == 0 &&
+ (env->tdata1[i] & ITRIGGER_VU) == 0 &&
+ (env->tdata1[i] & ITRIGGER_U) == 0 &&
+ (env->tdata1[i] & ITRIGGER_S) == 0 &&
+ (env->tdata1[i] & ITRIGGER_M) == 0 ) {
continue;
}
count = itrigger_get_count(env, i);
if (!count) {
continue;
}
return true;
}
return false;
}
Sincerely,
Alvin Chang
>
>
> Thanks,
>
> Daniel
>
>
>
>
>
> > count = itrigger_get_count(env, i);
^ permalink raw reply [flat|nested] 12+ messages in thread