[PATCH 2/3] lib: sbi: Add pmp_set_tor for setting TOR regions
Bo Gan
ganboing at gmail.com
Wed Nov 12 02:50:32 PST 2025
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.
>>
>> 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
>>>>
>>>
>
More information about the opensbi
mailing list