On Wed, Jan 14, 2026 at 02:46:37PM +0800, Nicholas Piggin wrote:
> The check_itrigger_priv() did not check privilege bits properly. Move
> all priv checks into functions and have the icount check follow the same
> form as the others.
>
> Signed-off-by: Nicholas Piggin <[email protected]>
> ---
> target/riscv/debug.c | 82 ++++++++++++++++++++------------------------
> 1 file changed, 37 insertions(+), 45 deletions(-)
>
> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> index c92bd9860e..2effbb49af 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -306,48 +306,50 @@ 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 type2_priv_match(CPURISCVState *env, target_ulong tdata1)
> +{
> + /* 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 */
> + return (((tdata1 >> 3) & 0b1011) & BIT(env->priv));
> +}
> +
> +static bool type6_priv_match(CPURISCVState *env, target_ulong tdata1)
> +{
> + if (env->virt_enabled) {
> + /* check VU/VS bit against current privilege level */
> + return (((tdata1 >> 23) & 0b11) & BIT(env->priv));
> + } else {
> + /* check U/S/M bit against current privilege level */
> + return (((tdata1 >> 3) & 0b1011) & BIT(env->priv));
> + }
> +}
The new logic looks correct to me, but now it uses open-coded shifts
and masks to check the privilege bits.
Since we already have these bits defined in debug.h, like `TYPE2_U/S/M`,
`TYPE6_VU/VS/U/S/M` and `ITRIGGER_VU/VS/U/S/M`, I think it would be
better to use those masks through a small helper instead of spelling
out `>> 3`, `>> 6`, `>> 23` and `0b1011` here.
That would make the code easier to read and maintain later.
like:
```
static bool trigger_priv_bit_match(target_ulong tdata1,
target_ulong u_mask,
target_ulong s_mask,
target_ulong m_mask,
target_ulong priv)
{
switch (priv) {
case PRV_U:
return (tdata1 & u_mask) != 0;
case PRV_S:
return (tdata1 & s_mask) != 0;
case PRV_M:
return (tdata1 & m_mask) != 0;
default:
g_assert_not_reached();
}
}
static bool trigger_vpriv_bit_match(target_ulong tdata1,
target_ulong vu_mask,
target_ulong vs_mask,
target_ulong priv)
{
switch (priv) {
case PRV_U:
return (tdata1 & vu_mask) != 0;
case PRV_S:
return (tdata1 & vs_mask) != 0;
default:
return false;
}
}
```
So we can call it this way:
```
static bool type2_priv_match(CPURISCVState *env, target_ulong tdata1)
{
/* type 2 trigger cannot be fired in VU/VS mode */
if (env->virt_enabled) {
return false;
}
return trigger_priv_bit_match(tdata1, TYPE2_U, TYPE2_S, TYPE2_M,
env->priv);
}
static bool type6_priv_match(CPURISCVState *env, target_ulong tdata1)
{
if (env->virt_enabled) {
return trigger_vpriv_bit_match(tdata1, TYPE6_VU, TYPE6_VS,
env->priv);
}
return trigger_priv_bit_match(tdata1, TYPE6_U, TYPE6_S, TYPE6_M,
env->priv);
}
static bool icount_priv_match(CPURISCVState *env, target_ulong tdata1)
{
if (env->virt_enabled) {
return trigger_vpriv_bit_match(tdata1, ITRIGGER_VU, ITRIGGER_VS,
env->priv);
}
return trigger_priv_bit_match(tdata1, ITRIGGER_U, ITRIGGER_S, ITRIGGER_M,
env->priv);
}
```
And I don't mean going back to `get_field(mask) == env->priv`; that pattern
is easy to get wrong for `PRV_U == 0`. The helper should just test the mask
selected by the current privilege.
Thanks,
Chao
> +
> +static bool icount_priv_match(CPURISCVState *env, target_ulong tdata1)
> +{
> + if (env->virt_enabled) {
> + /* check VU/VS bit against current privilege level */
> + return (((tdata1 >> 25) & 0b11) & BIT(env->priv));
> + } else {
> + /* check U/S/M bit against current privilege level */
> + return (((tdata1 >> 6) & 0b1011) & BIT(env->priv));
> + }
> +}
> +
> static bool trigger_priv_match(CPURISCVState *env, trigger_type_t type,
> int trigger_index)
> {
> - target_ulong ctrl = env->tdata1[trigger_index];
> + target_ulong tdata1 = 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;
> + return type2_priv_match(env, tdata1);
> 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;
> + return type6_priv_match(env, tdata1);
> 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;
> + return icount_priv_match(env, tdata1);
> case TRIGGER_TYPE_INT:
> case TRIGGER_TYPE_EXCP:
> case TRIGGER_TYPE_EXT_SRC:
> @@ -665,17 +667,7 @@ itrigger_set_count(CPURISCVState *env, int index, int
> value)
>
> static bool check_itrigger_priv(CPURISCVState *env, int index)
> {
> - target_ulong tdata1 = env->tdata1[index];
> - if (env->virt_enabled) {
> - /* check VU/VS bit against current privilege level */
> - return (get_field(tdata1, ITRIGGER_VS) == env->priv) ||
> - (get_field(tdata1, ITRIGGER_VU) == env->priv);
> - } else {
> - /* check U/S/M bit against current privilege level */
> - return (get_field(tdata1, ITRIGGER_M) == env->priv) ||
> - (get_field(tdata1, ITRIGGER_S) == env->priv) ||
> - (get_field(tdata1, ITRIGGER_U) == env->priv);
> - }
> + return icount_priv_match(env, index);
> }
>
> bool riscv_itrigger_enabled(CPURISCVState *env)
> --
> 2.51.0
>
>
