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

[Bo Gan <ganboing@gmail.com>]

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@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.

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

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


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