[PATCH 2/3] lib: sbi: Add pmp_set_tor for setting TOR regions
Xiang W
wangxiang at iscas.ac.cn
Tue Nov 11 02:35:02 PST 2025
在 2025-11-11二的 01:45 -0800，Bo Gan写道：
> Hi Xiang,
>
> On 11/10/25 21:45, Xiang W wrote:
> > 在 2025-11-10一的 19:41 -0800，Bo Gan写道：
> > > TOR can be utilized to cover memory regions that are not aligned with
> > > their sizes. Given that the address matching is formed bt 2 consecutive
> > > pmpaddr, i.e., pmpaddr(i-1) and pmpaddr(i), TOR should not be used
> > > generically to avoid pmpaddr conflict with other NA4/NAPOT regions.
> > >
> > > It's advised to only use them in platform code where the caller can
> > > ensure the index and order of every pmp region especially when there's
> > > a mixture of TOR/NA4/NAPOT.
> > >
> > > Signed-off-by: Bo Gan <ganboing at gmail.com>
> >
> > Some suggestions:
> > 1. Merge pmp_set_addr/pmp_set_port into
> > void static void pmp_set_raw(unsigned int n,
> > unsigned long port, unsigned long pmpaddr)
> >
> > 2. Modify the declaration of pmp_set to
> > int pmp_set(bool is_tor, unsigned int n, unsigned long port,
> > unsigned long addr, unsigned long opaque)
> >
> > Regards,
> > Xiang W
>
> I assume you meant port -> prot. For 1, I don't quite like it because it
> kind of assumes you always change a pmpaddr[i]/pmpcfg[i] pair. In the TOR
> case, often times you change pmpaddr[i-1] and pmpaddr[i], then pmpcfg[i].
> An edge case is when addr == 0 and n == 0, for it we only update pmpaddr0,
> then pmpcfg0. Perhaps I'm mistaken, and you are suggesting `pmp_set_raw`
> can handle different cases based on whether prot is TOR? Then I'd say it
> probably makes the code more complicated than necessary, as the check can
> already be made on the caller side, or no TOR check is necessary with my
> different function approach.
The function pmp_set_raw is used to set pmpaddr[i]/pmpcfg[i].
Regarding the edge case you mentioned, calling pmp_set_raw once is sufficient.
For regular Tor types, call it twice:
pmp_set_raw(n-1, 0, addr >> PMP_SHIFT)
pmp_set_raw(n,prot, (addr + size) >> PMP_SHIFT)
>
> For 2, my rationale on having a different function is that I want to keep
> all existing pmp_set the same way, and try to discourage the use of TOR.
> Maybe I should have added a warning in the comment in header file. The
> reason is because TOR makes the PMP entries dependent on preceding ones,
> so care must be taken when programming them. The caller must have global
> knowledge of potentially all PMP entries to be programmed in order to use
> it properly. I don't feel comfortable unifying to the same interface for
> TOR and NA4/NAPOT.
The names pmp_set and pmp_set_tor give the impression that pmp_set is a
general-purpose method, while pmp_set_tor is for handling special cases.
In reality, pmp_set is not a general-purpose method.
Regards,
Xiang W
>
> Let me know what you think. I'm happy to listen. Thanks.
>
> Bo
>
> > > ---
> > > include/sbi/riscv_asm.h | 4 +++
> > > lib/sbi/riscv_asm.c | 75 +++++++++++++++++++++++++++++++----------
> > > 2 files changed, 62 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/include/sbi/riscv_asm.h b/include/sbi/riscv_asm.h
> > > index ef48dc89..c23feab6 100644
> > > --- a/include/sbi/riscv_asm.h
> > > +++ b/include/sbi/riscv_asm.h
> > > @@ -218,6 +218,10 @@ int is_pmp_entry_mapped(unsigned long entry);
> > > int pmp_set(unsigned int n, unsigned long prot, unsigned long addr,
> > > unsigned long log2len);
> > >
> > > +/* Top of range (TOR) matching mode. pmpaddr(n-1) will also be changed */
> > > +int pmp_set_tor(unsigned int n, unsigned long prot, unsigned long addr,
> > > + unsigned long size);
> > > +
> > > int pmp_get(unsigned int n, unsigned long *prot_out, unsigned long *addr_out,
> > > unsigned long *log2len);
> > >
> > > diff --git a/lib/sbi/riscv_asm.c b/lib/sbi/riscv_asm.c
> > > index 3e44320f..8f64f2b5 100644
> > > --- a/lib/sbi/riscv_asm.c
> > > +++ b/lib/sbi/riscv_asm.c
> > > @@ -330,16 +330,10 @@ int is_pmp_entry_mapped(unsigned long entry)
> > > return false;
> > > }
> > >
> > > -int pmp_set(unsigned int n, unsigned long prot, unsigned long addr,
> > > - unsigned long log2len)
> > > +static void pmp_set_prot(unsigned int n, unsigned long prot)
> > > {
> > > - int pmpcfg_csr, pmpcfg_shift, pmpaddr_csr;
> > > + int pmpcfg_csr, pmpcfg_shift;
> > > unsigned long cfgmask, pmpcfg;
> > > - unsigned long addrmask, pmpaddr;
> > > -
> > > - /* check parameters */
> > > - if (n >= PMP_COUNT || log2len > __riscv_xlen || log2len < PMP_SHIFT)
> > > - return SBI_EINVAL;
> > >
> > > /* calculate PMP register and offset */
> > > #if __riscv_xlen == 32
> > > @@ -351,15 +345,29 @@ int pmp_set(unsigned int n, unsigned long prot, unsigned long addr,
> > > #else
> > > # error "Unexpected __riscv_xlen"
> > > #endif
> > > - pmpaddr_csr = CSR_PMPADDR0 + n;
> > > -
> > > - /* encode PMP config */
> > > - prot &= ~PMP_A;
> > > - prot |= (log2len == PMP_SHIFT) ? PMP_A_NA4 : PMP_A_NAPOT;
> > > cfgmask = ~(0xffUL << pmpcfg_shift);
> > > pmpcfg = (csr_read_num(pmpcfg_csr) & cfgmask);
> > > pmpcfg |= ((prot << pmpcfg_shift) & ~cfgmask);
> > >
> > > + csr_write_num(pmpcfg_csr, pmpcfg);
> > > +}
> > > +
> > > +static void pmp_set_addr(unsigned int n, unsigned long pmpaddr)
> > > +{
> > > + int pmpaddr_csr = CSR_PMPADDR0 + n;
> > > +
> > > + csr_write_num(pmpaddr_csr, pmpaddr);
> > > +}
> > > +
> > > +int pmp_set(unsigned int n, unsigned long prot, unsigned long addr,
> > > + unsigned long log2len)
> > > +{
> > > + unsigned long addrmask, pmpaddr;
> > > +
> > > + /* check parameters */
> > > + if (n >= PMP_COUNT || log2len > __riscv_xlen || log2len < PMP_SHIFT)
> > > + return SBI_EINVAL;
> > > +
> > > /* encode PMP address */
> > > if (log2len == PMP_SHIFT) {
> > > pmpaddr = (addr >> PMP_SHIFT);
> > > @@ -373,10 +381,41 @@ int pmp_set(unsigned int n, unsigned long prot, unsigned long addr,
> > > }
> > > }
> > >
> > > + /* encode PMP config */
> > > + prot &= ~PMP_A;
> > > + prot |= (log2len == PMP_SHIFT) ? PMP_A_NA4 : PMP_A_NAPOT;
> > > +
> > > /* write csrs */
> > > - csr_write_num(pmpaddr_csr, pmpaddr);
> > > - csr_write_num(pmpcfg_csr, pmpcfg);
> > > + pmp_set_addr(n, pmpaddr);
> > > + pmp_set_prot(n, prot);
> > > + return 0;
> > > +}
> > > +
> > > +int pmp_set_tor(unsigned int n, unsigned long prot, unsigned long addr,
> > > + unsigned long size)
> > > +{
> > > + unsigned long pmpaddr, pmpaddrp;
> > > +
> > > + /* check parameters */
> > > + if (n >= PMP_COUNT)
> > > + return SBI_EINVAL;
> > > +
> > > + if (n == 0 && addr != 0)
> > > + return SBI_EINVAL;
> > > +
> > > + /* encode PMP address */
> > > + pmpaddrp = addr >> PMP_SHIFT;
> > > + pmpaddr = (addr + size) >> PMP_SHIFT;
> > >
> > > + /* encode PMP config */
> > > + prot &= ~PMP_A;
> > > + prot |= PMP_A_TOR;
> > > +
> > > + /* write csrs */
> > > + if (n)
> > > + pmp_set_addr(n - 1, pmpaddrp);
> > > + pmp_set_addr(n, pmpaddr);
> > > + pmp_set_prot(n, prot);
> > > return 0;
> > > }
> > >
> > > @@ -420,10 +459,12 @@ int pmp_get(unsigned int n, unsigned long *prot_out, unsigned long *addr_out,
> > > addr = (addr & ~((1UL << t1) - 1)) << PMP_SHIFT;
> > > len = (t1 + PMP_SHIFT + 1);
> > > }
> > > - } else {
> > > + } else if ((prot & PMP_A) == PMP_A_NA4) {
> > > addr = csr_read_num(pmpaddr_csr) << PMP_SHIFT;
> > > len = PMP_SHIFT;
> > > - }
> > > + } else
> > > + /* Error out for TOR region */
> > > + return SBI_EINVAL;
> > >
> > > /* return details */
> > > *prot_out = prot;
> > > --
> > > 2.34.1
> > >
> >
More information about the opensbi
mailing list