[PATCH 2/3] lib: sbi: Add pmp_set_tor for setting TOR regions

Xiang W wangxiang at iscas.ac.cn
Wed Nov 12 03:29:44 PST 2025 

在 2025-11-12三的 02:50 -0800，Bo Gan写道：
> On 11/11/25 02:35, Xiang W wrote:
> > 在 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)
> > 
> 
> We shouldn't do pmp_set_raw(n-1, 0, addr >> PMP_SHIFT), because it'll zero-
> out the corresponding pmpcfg of n-1.
> 
> This is actually the tricky part of TOR. You can have adjacent TOR regions
> that share the same pmpaddr, i.e., the same pmpaddr forms the upper-bound
> of previous region and lower-bound of current region. If we disallow this
> sharing, then potentially we'll end up with 2x the number of TOR regions.
> Thus, if lib/ code were to use TOR, either it would have to coordinate
> neighboring PMP entries, or a single PMP entry should own 2 pmpcfg/pmpaddr
> register pairs.
Thank you for your detailed explanation.
> 
> > > 
> > > 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.
> > 
> 
> Given those scenarios I mentioned earlier, I'd like to avoid making it
> overly complicated. I.e., lib/ code never set or get TOR regions, and can
> still safely assume the 1:1 correspondence of pmp index to pmpcfg/pmpaddr
> pair. Everything related to TOR is only used by platform pmp_configure
> override. Thus, the different function name, so the caller of set_tor can
> be easily identified.
I agree that we should avoid unnecessary complexity and keep the lib/ code 
from directly handling TOR regions. To streamline things further, I suggest 
moving all TOR-related configurations to the platform code. This will help 
maintain clarity and allow the lib/ code to focus on its main responsibilities 
without the added burden of managing TOR settings.

Regards,
Xiang W
> 
> Bo
> 
> > 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