Re: [PATCH] lib: sbi: Flush cache entries after writing PMP CSRs

[Samuel Holland <samuel.holland@sifive.com>]

Hi Anup,

On 2025-10-30 12:50 AM, Anup Patel wrote:
> On Thu, Oct 30, 2025 at 4:02 AM Samuel Holland
> <samuel.holland@sifive.com> wrote:
>> On 2025-10-19 11:36 PM, Anup Patel wrote:
>>> On Thu, Sep 4, 2025 at 4:26 PM <cp0613@linux.alibaba.com> wrote:
>>>>
>>>> From: Chen Pei <cp0613@linux.alibaba.com>
>>>>
>>>> As the privileged specification states, after writing to the PMP CSRs,
>>>> a SFENCE.VMA or HFENCE.GVMA instruction should be executed with rs1=x0
>>>> and rs2=x0 to flush all address translation cache entries.
>>>>
>>>> The original implementation does not cover all possible cases. For
>>>> example, the sbi_hart_map_saddr function calls pmp_set but does not
>>>> execute the SFENCE.VMA instruction. This patch covers sbi_hart_map_saddr
>>>> and sbi_hart_unmap_saddr, ensuring that dbtr, sse and other modules can
>>>> safely update pmpcfg.
>>>
>>> The temporary mapping created using sbi_hart_map/unmap_saddr() is
>>> only accessed by M-mode and does not impact the S-mode TLB
>>> entries in anyway.
>>
>> That doesn't match my interpretation of this paragraph from the privileged ISA:
>>
>> Implementations with virtual memory are permitted to perform address
>> translations speculatively and earlier than required by an explicit memory
>> access, and are permitted to cache them in address translation cache
>> structures—including possibly caching the identity mappings from effective
>> address to physical address used in Bare translation modes and M-mode. The PMP
>> settings for the resulting physical address may be checked (and possibly cached)
>> at any point between the address translation and the explicit memory access.
>> Hence, when the PMP settings are modified, M-mode software must synchronize the
>> PMP settings with the virtual memory system and any PMP or address-translation
>> caches. This is accomplished by executing an SFENCE.VMA instruction with rs1=x0
>> and rs2=x0, after the PMP CSRs are written. See Section 19.5.3 for additional
>> synchronization requirements when the hypervisor extension is implemented.
>>
>> Specifically, note "including possibly caching the identity mappings from
>> effective address to physical address used in Bare translation modes and M-mode".
>>
>> So hardware is allowed to (speculatively) fill the TLB with entries that apply
>> to M-mode, and tag those M-mode PMP entries with the permissions from the PMPs.
>> The sfence.vma is required to clear those entries, or else accesses _even from
>> M-mode_ may erroneously use the previous PMP permissions.
> 
> If hardware is filling TLB entries for M-mode then it must tag these TLB
> entries with privilege level so that they don't match for S-mode otherwise
> a simple M-mode to S-mode switch at boot-time will allow bootloaders
> to access M-mode memory with M-mode permissions.

Yes, agreed, the TLB entries must be tagged with the privilege mode, but that's
not the problem here. The problem has nothing at all to do with privilege mode
transitions, and nothing to do with executing code in S-mode, either. The bug
can be observed purely within the scope of the function calling
sbi_hart_map_saddr().

sbi_hart_map_saddr() rewrites a PMP range to allow M-mode access to some address
range that it previously did not have permission to access. The problem is that
this PMP change may not be observed until software executes a sfence.vma
covering at least this address range. So without this patch, it is
architecturally valid to receive an access fault when M-mode attempts to access
this address range (i.e. on the very next line of code after calling
sbi_hart_map_saddr()).

Regards,
Samuel

>>> Further, flushing TLB entries in sbi_hart_map/unmap_saddr() also
>>> slows-down many SBI calls which use shared memory.
>>>
>>> NACK to this patch.
>>>
>>> Regards,
>>> Anup
>>>
>>>>
>>>> Signed-off-by: Chen Pei <cp0613@linux.alibaba.com>
>>>> ---
>>>>  lib/sbi/sbi_hart.c | 54 ++++++++++++++++++++++++++++------------------
>>>>  1 file changed, 33 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c
>>>> index 6a2d7d6..33bba38 100644
>>>> --- a/lib/sbi/sbi_hart.c
>>>> +++ b/lib/sbi/sbi_hart.c
>>>> @@ -364,6 +364,30 @@ static unsigned int sbi_hart_get_smepmp_flags(struct sbi_scratch *scratch,
>>>>         return pmp_flags;
>>>>  }
>>>>
>>>> +static void pmp_flush(void)
>>>> +{
>>>> +       /*
>>>> +        * As per section 3.7.2 of privileged specification v1.12,
>>>> +        * virtual address translations can be speculatively performed
>>>> +        * (even before actual access). These, along with PMP traslations,
>>>> +        * can be cached. This can pose a problem with CPU hotplug
>>>> +        * and non-retentive suspend scenario because PMP states are
>>>> +        * not preserved.
>>>> +        * It is advisable to flush the caching structures under such
>>>> +        * conditions.
>>>> +        */
>>>> +       if (misa_extension('S')) {
>>>> +               __asm__ __volatile__("sfence.vma");
>>>> +
>>>> +               /*
>>>> +                * If hypervisor mode is supported, flush caching
>>>> +                * structures in guest mode too.
>>>> +                */
>>>> +               if (misa_extension('H'))
>>>> +                       __sbi_hfence_gvma_all();
>>>> +       }
>>>> +}
>>>> +
>>>>  static void sbi_hart_smepmp_set(struct sbi_scratch *scratch,
>>>>                                 struct sbi_domain *dom,
>>>>                                 struct sbi_domain_memregion *reg,
>>>> @@ -543,18 +567,25 @@ int sbi_hart_map_saddr(unsigned long addr, unsigned long size)
>>>>                              pmp_flags, base, order);
>>>>         pmp_set(SBI_SMEPMP_RESV_ENTRY, pmp_flags, base, order);
>>>>
>>>> +       pmp_flush();
>>>> +
>>>>         return SBI_OK;
>>>>  }
>>>>
>>>>  int sbi_hart_unmap_saddr(void)
>>>>  {
>>>> +       int rc;
>>>>         struct sbi_scratch *scratch = sbi_scratch_thishart_ptr();
>>>>
>>>>         if (!sbi_hart_has_extension(scratch, SBI_HART_EXT_SMEPMP))
>>>>                 return SBI_OK;
>>>>
>>>>         sbi_platform_pmp_disable(sbi_platform_ptr(scratch), SBI_SMEPMP_RESV_ENTRY);
>>>> -       return pmp_disable(SBI_SMEPMP_RESV_ENTRY);
>>>> +       rc = pmp_disable(SBI_SMEPMP_RESV_ENTRY);
>>>> +
>>>> +       pmp_flush();
>>>> +
>>>> +       return rc;
>>>>  }
>>>>
>>>>  int sbi_hart_pmp_configure(struct sbi_scratch *scratch)
>>>> @@ -578,26 +609,7 @@ int sbi_hart_pmp_configure(struct sbi_scratch *scratch)
>>>>                 rc = sbi_hart_oldpmp_configure(scratch, pmp_count,
>>>>                                                 pmp_log2gran, pmp_addr_max);
>>>>
>>>> -       /*
>>>> -        * As per section 3.7.2 of privileged specification v1.12,
>>>> -        * virtual address translations can be speculatively performed
>>>> -        * (even before actual access). These, along with PMP traslations,
>>>> -        * can be cached. This can pose a problem with CPU hotplug
>>>> -        * and non-retentive suspend scenario because PMP states are
>>>> -        * not preserved.
>>>> -        * It is advisable to flush the caching structures under such
>>>> -        * conditions.
>>>> -        */
>>>> -       if (misa_extension('S')) {
>>>> -               __asm__ __volatile__("sfence.vma");
>>>> -
>>>> -               /*
>>>> -                * If hypervisor mode is supported, flush caching
>>>> -                * structures in guest mode too.
>>>> -                */
>>>> -               if (misa_extension('H'))
>>>> -                       __sbi_hfence_gvma_all();
>>>> -       }
>>>> +       pmp_flush();
>>>>
>>>>         return rc;
>>>>  }
>>>> --
>>>> 2.49.0
>>>>
>>>>
>>>> --
>>>> opensbi mailing list
>>>> opensbi@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/opensbi
>>>
>>


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