Re: [PATCH v5 04/13] target/riscv: Change the return type of pmp_hart_has_privs() to bool

[Alistair Francis]

On Sat, Apr 29, 2023 at 12:38 AM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
> We no longer need the pmp_index for matched PMP entry now.
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/cpu_helper.c |  8 ++++----
>  target/riscv/pmp.c        | 32 +++++++++++++-------------------
>  target/riscv/pmp.h        |  8 ++++----
>  3 files changed, 21 insertions(+), 27 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 83c9699a6d..1868766082 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -685,16 +685,16 @@ static int get_physical_address_pmp(CPURISCVState *env, 
> int *prot, hwaddr addr,
>                                      int mode)
>  {
>      pmp_priv_t pmp_priv;
> -    int pmp_index = -1;
> +    bool pmp_has_privs;
>
>      if (!riscv_cpu_cfg(env)->pmp) {
>          *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
>          return TRANSLATE_SUCCESS;
>      }
>
> -    pmp_index = pmp_hart_has_privs(env, addr, size, 1 << access_type,
> -                                   &pmp_priv, mode);
> -    if (pmp_index < 0) {
> +    pmp_has_privs = pmp_hart_has_privs(env, addr, size, 1 << access_type,
> +                                       &pmp_priv, mode);
> +    if (!pmp_has_privs) {
>          *prot = 0;
>          return TRANSLATE_PMP_FAIL;
>      }
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 86abe1e7cd..b5808538aa 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -296,27 +296,23 @@ static bool pmp_hart_has_privs_default(CPURISCVState 
> *env, target_ulong addr,
>
>  /*
>   * Check if the address has required RWX privs to complete desired operation
> - * Return PMP rule index if a pmp rule match
> - * Return MAX_RISCV_PMPS if default match
> - * Return negtive value if no match
> + * Return true if a pmp rule match or default match
> + * Return false if no match
>   */
> -int pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
> -                       target_ulong size, pmp_priv_t privs,
> -                       pmp_priv_t *allowed_privs, target_ulong mode)
> +bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
> +                        target_ulong size, pmp_priv_t privs,
> +                        pmp_priv_t *allowed_privs, target_ulong mode)
>  {
>      int i = 0;
> -    int ret = -1;
> +    bool ret = false;
>      int pmp_size = 0;
>      target_ulong s = 0;
>      target_ulong e = 0;
>
>      /* Short cut if no rules */
>      if (0 == pmp_get_num_rules(env)) {
> -        if (pmp_hart_has_privs_default(env, addr, size, privs,
> -                                       allowed_privs, mode)) {
> -            ret = MAX_RISCV_PMPS;
> -        }
> -        return ret;
> +        return pmp_hart_has_privs_default(env, addr, size, privs,
> +                                          allowed_privs, mode);
>      }
>
>      if (size == 0) {
> @@ -345,7 +341,7 @@ int pmp_hart_has_privs(CPURISCVState *env, target_ulong 
> addr,
>          if ((s + e) == 1) {
>              qemu_log_mask(LOG_GUEST_ERROR,
>                            "pmp violation - access is partially inside\n");
> -            ret = -1;
> +            ret = false;
>              break;
>          }
>
> @@ -453,17 +449,15 @@ int pmp_hart_has_privs(CPURISCVState *env, target_ulong 
> addr,
>               * defined with PMP must be used. We shouldn't fallback on
>               * finding default privileges.
>               */
> -            ret = i;
> +            ret = true;
>              break;
>          }
>      }
>
>      /* No rule matched */
> -    if (ret == -1) {
> -        if (pmp_hart_has_privs_default(env, addr, size, privs,
> -                                       allowed_privs, mode)) {
> -            ret = MAX_RISCV_PMPS;
> -        }
> +    if (!ret) {
> +        ret = pmp_hart_has_privs_default(env, addr, size, privs,
> +                                         allowed_privs, mode);
>      }
>
>      return ret;
> diff --git a/target/riscv/pmp.h b/target/riscv/pmp.h
> index 0a7e24750b..cf5c99f8e6 100644
> --- a/target/riscv/pmp.h
> +++ b/target/riscv/pmp.h
> @@ -72,10 +72,10 @@ target_ulong mseccfg_csr_read(CPURISCVState *env);
>  void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
>                         target_ulong val);
>  target_ulong pmpaddr_csr_read(CPURISCVState *env, uint32_t addr_index);
> -int pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
> -                       target_ulong size, pmp_priv_t privs,
> -                       pmp_priv_t *allowed_privs,
> -                       target_ulong mode);
> +bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
> +                        target_ulong size, pmp_priv_t privs,
> +                        pmp_priv_t *allowed_privs,
> +                        target_ulong mode);
>  target_ulong pmp_get_tlb_size(CPURISCVState *env, target_ulong addr);
>  void pmp_update_rule_addr(CPURISCVState *env, uint32_t pmp_index);
>  void pmp_update_rule_nums(CPURISCVState *env);
> --
> 2.25.1
>
>