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

[Weiwei Li]

On 2023/4/20 21:19, LIU Zhiwei wrote:
> 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.

We don't need the tlb size if the translation fails. so I move the 
pmp_get_tlb_size to the following code.

In this way, we don't get the tlb size here, so  it's delete from the 
above message. It seems not very good to separate it alone.

> > +                              " %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。

I didn't add this in the first patch, because this check is unnecessary 
if we don't move this pmp_get_tlb_size() apart from 
get_physical_address_pmp().

It have been checked in get_physical_address_pmp() before calling 
pmp_get_tlb_size().

Regards,

Weiwei Li

> 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;