Re: [PATCH 2/4] target/riscv: Apply modularized matching conditions for breakpoint

[Daniel Henrique Barboza]

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: