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

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

[cp0613]

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.

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

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

[Anup Patel]

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.

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

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

[Samuel Holland]

Hi Anup,

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.

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

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

[Anup Patel]

On Thu, Oct 30, 2025 at 4:02 AM Samuel Holland
<samuel.holland@sifive.com> wrote:
>
> Hi Anup,
>
> 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.

Regards,
Anup

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

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

[Samuel Holland]

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

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

[Anup Patel]

On Thu, Oct 30, 2025 at 4:06 PM Samuel Holland
<samuel.holland@sifive.com> wrote:
>
> 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().

The reserved PMP entry under consideration is disabled while in S-mode
before sbi_hart_map_saddr() and sbi_hart_unmap_saddr() so there is no
way the reserved PMP entry impacts S-mode. In other words, we are
not changing any PMP entry which impacts address ranges accessed by
S-mode.

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

This is a fair point but we don't need to nuke the entire TLB. To address this,
sfence.vma to a particular address range is sufficient. Plus, hfence is not
required at all. I think this patch needs a complete re-write with
clear comments
for sfence.vma after PMP entry change in sbi_hart_map_saddr().

Regards,
Anup

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

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

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

[cp0613]

On 2025-10-30 10:36 AM, Samuel Holland wrote:

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

Hi Samuel,

Thank you so much for participating in the discussion.
You pointed out the key points, this is indeed a case.

Thanks,
Pei

> > Regards,
> > Samuel

> >>> Further, flushing TLB entries in sbi_hart_map/unmap_saddr() also
> >>> slows-down many SBI calls which use shared memory.

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

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

[cp0613]

On 2025-10-31 03:49 AM, Anup Patel wrote:

> > 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().
> 
> The reserved PMP entry under consideration is disabled while in S-mode
> before sbi_hart_map_saddr() and sbi_hart_unmap_saddr() so there is no
> way the reserved PMP entry impacts S-mode. In other words, we are
> not changing any PMP entry which impacts address ranges accessed by
> S-mode.
> 
> >
> > 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()).
> 
> This is a fair point but we don't need to nuke the entire TLB. To address this,
> sfence.vma to a particular address range is sufficient. Plus, hfence is not
> required at all. I think this patch needs a complete re-write with
> clear comments
> for sfence.vma after PMP entry change in sbi_hart_map_saddr().
> 
> Regards,
> Anup

Thank you very much for your review. The issue you mentioned earlier, which slows
down many SBI calls that use shared memory, is indeed something that needs to be
considered. I will try to make further modifications based on your suggestions.

Thanks,
Pei

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