Re: [PATCH v3 2/7] target/riscv: Move pmp_get_tlb_size apart from get_physical_address_pmp

[LIU Zhiwei]

On 2023/4/19 11:27, Weiwei Li wrote:
> pmp_get_tlb_size can be separated from get_physical_address_pmp and is only
> needed when ret == TRANSLATE_SUCCESS.
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---
>   target/riscv/cpu_helper.c | 21 +++++++--------------
>   target/riscv/pmp.c        |  4 ++++
>   2 files changed, 11 insertions(+), 14 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 075fc0538a..ea08ca9fbb 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -676,14 +676,11 @@ void riscv_cpu_set_mode(CPURISCVState *env, target_ulong 
> newpriv)
>    *
>    * @env: CPURISCVState
>    * @prot: The returned protection attributes
> - * @tlb_size: TLB page size containing addr. It could be modified after PMP
> - *            permission checking. NULL if not set TLB page for addr.
>    * @addr: The physical address to be checked permission
>    * @access_type: The type of MMU access
>    * @mode: Indicates current privilege level.
>    */
> -static int get_physical_address_pmp(CPURISCVState *env, int *prot,
> -                                    target_ulong *tlb_size, hwaddr addr,
> +static int get_physical_address_pmp(CPURISCVState *env, int *prot, hwaddr addr,
>                                       int size, MMUAccessType access_type,
>                                       int mode)
>   {
> @@ -703,9 +700,6 @@ static int get_physical_address_pmp(CPURISCVState *env, int 
> *prot,
>       }
>   
>       *prot = pmp_priv_to_page_prot(pmp_priv);
> -    if (tlb_size != NULL) {
> -        *tlb_size = pmp_get_tlb_size(env, addr);
> -    }
>   
>       return TRANSLATE_SUCCESS;
>   }
> @@ -905,7 +899,7 @@ restart:
>           }
>   
>           int pmp_prot;
> -        int pmp_ret = get_physical_address_pmp(env, &pmp_prot, NULL, pte_addr,
> +        int pmp_ret = get_physical_address_pmp(env, &pmp_prot, pte_addr,
>                                                  sizeof(target_ulong),
>                                                  MMU_DATA_LOAD, PRV_S);
>           if (pmp_ret != TRANSLATE_SUCCESS) {
> @@ -1300,13 +1294,12 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, 
> int size,
>               prot &= prot2;
>   
>               if (ret == TRANSLATE_SUCCESS) {
> -                ret = get_physical_address_pmp(env, &prot_pmp, &tlb_size, pa,
> +                ret = get_physical_address_pmp(env, &prot_pmp, pa,
>                                                  size, access_type, mode);
>   
>                   qemu_log_mask(CPU_LOG_MMU,
>                                 "%s PMP address=" HWADDR_FMT_plx " ret %d prot"
> -                              " %d tlb_size " TARGET_FMT_lu "\n",
> -                              __func__, pa, ret, prot_pmp, tlb_size);
We discard the tlb_size message here，which is not good.
If we really want to discard it, we should give a reason and remove it 
in a separated patch.
> +                              " %d\n", __func__, pa, ret, prot_pmp);
>   
>                   prot &= prot_pmp;
>               }
> @@ -1333,13 +1326,12 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, 
> int size,
>                         __func__, address, ret, pa, prot);
>   
>           if (ret == TRANSLATE_SUCCESS) {
> -            ret = get_physical_address_pmp(env, &prot_pmp, &tlb_size, pa,
> +            ret = get_physical_address_pmp(env, &prot_pmp, pa,
>                                              size, access_type, mode);
>   
>               qemu_log_mask(CPU_LOG_MMU,
>                             "%s PMP address=" HWADDR_FMT_plx " ret %d prot"
> -                          " %d tlb_size " TARGET_FMT_lu "\n",
> -                          __func__, pa, ret, prot_pmp, tlb_size);
> +                          " %d\n", __func__, pa, ret, prot_pmp);
>   
>               prot &= prot_pmp;
>           }
> @@ -1350,6 +1342,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int 
> size,
>       }
>   
>       if (ret == TRANSLATE_SUCCESS) {
> +        tlb_size = pmp_get_tlb_size(env, pa);
>           tlb_set_page(cs, address & ~(tlb_size - 1), pa & ~(tlb_size - 1),
>                        prot, mmu_idx, tlb_size);
>           return true;
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 22f3b3f217..d1ef9457ea 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -612,6 +612,10 @@ target_ulong pmp_get_tlb_size(CPURISCVState *env, 
> target_ulong addr)
>       target_ulong tlb_ea = tlb_sa + TARGET_PAGE_SIZE - 1;
>       int i;
>   
> +    if (!riscv_cpu_cfg(env)->pmp || !pmp_get_num_rules(env)) {
> +        return TARGET_PAGE_SIZE;
> +    }

Can we move this to the first patch? So that we have a right 
implementation when  change of the prototype of  this function。

Zhiwei

> +
>       for (i = 0; i < MAX_RISCV_PMPS; i++) {
>           if (pmp_get_a_field(env->pmp_state.pmp[i].cfg_reg) == PMP_AMATCH_OFF) 
> {
>               continue;