Re: [PATCH 2/4] lib: sbi: Move PMP encoding into a new file

[Nicholas Piggin <npiggin@gmail.com>]

On Wed, Apr 01, 2026 at 11:20:53PM +1030, Joel Stanley wrote:
> On Tue, 10 Mar 2026 at 11:19, Nicholas Piggin <npiggin@gmail.com> wrote:
> >
> > The Tenstorrent RISC-V IOMMU PMP MMRs use the same encoding as PMP CSRs.
> > In preparation to support it, move the non hart-specific PMP operations
> > into their own file where they will also be used to build the IOMMU
> > PMPs.
> >
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> 
> > --- a/lib/sbi/riscv_asm.c
> > +++ b/lib/sbi/riscv_asm.c
> 
> > +int is_pmp_entry_mapped(unsigned int entry)
> > +{
> > +       pmp_t pmp;
> 
> > +       if (hart_pmp_read(entry, &pmp))
> > +               return pmp_enabled(&pmp);
> 
> The diff is a bit all over the place so I may have confused myself.
> The patch keeps is_pmp_entry_mapped but changes the body from
> 
>     if (pmp_get(entry, &prot, &addr, &log2len) != 0)
>         return false;
> 
> to
> 
>     if (hart_pmp_read(entry, &pmp))
>         return pmp_enabled(&pmp);
> 
> but hart_pmp_read returns SBI_OK (0) on success. So we should keep the !=0?

Yes, you're right.

> 
> > +int pmp_get(unsigned int n, unsigned long *prot_out, unsigned long *addr_out,
> > +           unsigned long *log2len)
> > +{
> > +       pmp_t pmp;
> > +       int rc;
> >
> > -       /* return details */
> > -       *prot_out    = prot;
> 
> It looks like prot_out is no longer written to after your patch. This
> hunk never made it into the new pmp_decode:
> 
>        /* decode PMP config */
>        cfgmask = (0xffUL << pmpcfg_shift);
>        pmpcfg  = csr_read_num(pmpcfg_csr) & cfgmask;
>        prot    = pmpcfg >> pmpcfg_shift;

Good catch.

> > --- /dev/null
> > +++ b/lib/sbi/sbi_pmp.c
> > @@ -0,0 +1,91 @@
> 
> > +int pmp_create(pmp_t *pmp, unsigned long prot, unsigned long addr,
> > +           unsigned long log2len)
> > +{
> > +       unsigned long addrmask, pmpaddr;
> > +
> > +       /* check parameters */
> > +       if (log2len > __riscv_xlen || log2len < PMP_SHIFT)
> > +               return SBI_EINVAL;
> 
> Here we check that log2len <=__riscv_xlen, but in pmp_decode below we
> set log2len =  __riscv_xlen + 3. Is that just how it works, or should
> pmp_decode be able to decode what pmp_create does?

That + 3 looks wrong, I'll have to dig a bit more into it. I think I
need to simplify and/or split the patch up so it's a bit more feasbile
to review (and hopefully less buggy).

Thanks,
Nick

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi