Re: [PATCH] target/riscv: Smepmp: Skip applying default rules when address matches

[LIU Zhiwei]

On 2023/2/13 13:21, Himanshu Chauhan wrote:

>       
>       On 13/02/23 09:52, LIU Zhiwei wrote:
>       
> >         
> >         On 2023/2/9 13:52, Himanshu Chauhan wrote:
> >         
> > > When MSECCFG.MML is set, after checking
> > >           the address range in PMP if the
> > >           
> > >           asked permissions are not same as programmed in PMP, the
> > >           default
> > >           
> > >           permissions are applied. This should only be the case when
> > >           there
> > >           
> > >           is no matching address is found.
> > >           
> > >           
> > >           This patch skips applying default rules when matching address
> > >           range
> > >           
> > >           is found. It returns the index of the match PMP entry.
> > >           
> > >           
> > >           fixes: 824cac681c3 (target/riscv: Fix PMP propagation for tlb)
> > >           
> > >           
> > >           Signed-off-by: Himanshu Chauhan
> > >           <hchauhan@ventanamicro.com>
> > >           
> > >           ---
> > >           
> > >             target/riscv/pmp.c | 9 ++++++---
> > >           
> > >             1 file changed, 6 insertions(+), 3 deletions(-)
> > >           
> > >           
> > >           diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> > >           
> > >           index d85ad07caa..0dfdb35828 100644
> > >           
> > >           --- a/target/riscv/pmp.c
> > >           
> > >           +++ b/target/riscv/pmp.c
> > >           
> > >           @@ -446,9 +446,12 @@ int pmp_hart_has_privs(CPURISCVState
> > >           *env, target_ulong addr,
> > >           
> > >                             }
> > >           
> > >                         }
> > >           
> > >             -            if ((privs & *allowed_privs) == privs) {
> > >           
> > >           -                ret = i;
> > >           
> > >           -            }
> > >           
> > >           +            /*
> > >           
> > >           +             * If matching address range was found, the
> > >           protection bits
> > >           
> > >           +             * defined with PMP must be used. We shouldn't
> > >           fallback on
> > >           
> > >           +             * finding default privileges.
> > >           
> > >           +             */
> > >           
> > >           +            ret = i;
> > >           
> > >         
> >         
> >         Notice the return value is the matching rule index, which
> >         includes
> >         
> >         
> >         1) the address range is matching.
> >         
> >         
> >         2) the permission of the PMP rule and the memory access type are
> >         matching.
> >         
> >         
> >         
> >         So we can't simply remove the second check.  I think the right
> >         fix is:
> >         
> >         
> >                    if ((privs & *allowed_privs) == privs) {
> >         
> >                         ret = i;
> >         
> >          -         }
> >         
> >          +         } else {
> >         
> >          +        ret = -2;
> >         
> >          +         }
> >         
> >         
> >         The -2 return value avoids finding the default privileges. And
> >         it implies no matching rule is found.
> >         
> >         
> >         Zhiwei
> >         
> >       
>       
>       Hi Zhiwei,
>       
>       
>       In case the address range is matched and MSECCFG.MML is set, the
>       permission in *allowed_privs* are binding. 
    Yes.
> So
>       if the index matching is returned, the binding permissions are
>       applied by the caller function.
>       
>     
    Yes. And the index will also be used. So we should check both
    conditions in this function.
>       
>       Which case does my patch break? 
>

Look at the get_physical_address_pmp which calls pmp_hart_has_privs,

    pmp_index = pmp_hart_has_privs(env, addr, size, 1 << access_type,
                                   &pmp_priv, mode);
    if (pmp_index < 0) {
        *prot = 0;
        return TRANSLATE_PMP_FAIL;
    }
    *prot = pmp_priv_to_page_prot(pmp_priv);
    if ((tlb_size != NULL) && pmp_index != MAX_RISCV_PMPS) {
        target_ulong tlb_sa = addr & ~(TARGET_PAGE_SIZE - 1);
        target_ulong tlb_ea = tlb_sa + TARGET_PAGE_SIZE - 1;
        *tlb_size = pmp_get_tlb_size(env, pmp_index, tlb_sa, tlb_ea);
    }
        return TRANSLATE_SUCCESS;

You break the pmp_index < 0 condition.  If ((privs & *allowed_privs) != privs,  the get_physical_address_pmp should return fail.

Zhiwei

>       
>       - Himanshu
>       
>       
> >         
> > >               break;
> > >           
> > >                     }
> > >           
> > >                 }
> > >           
> > >         
> >       
>