lib: sbi_platform: Add platform specific pmp_set() and pmp_disable()

From: Chao-ying Fu <cfu@wavecomp.com>

Allow platforms to implement platform specific PMP setup and
PMP disable functions which are called before actual PMP CSRs
are configured.

Also, implement pmp_set() and pmp_disable() for MIPS P8700.

Signed-off-by: Chao-ying Fu <cfu@mips.com>
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 include/sbi/sbi_platform.h    | 39 +++++++++++++++
 lib/sbi/sbi_domain_context.c  |  2 +
 lib/sbi/sbi_hart.c            | 10 ++++
 platform/generic/mips/p8700.c | 93 +++++++++++++++++++++++++++++++++++
 4 files changed, 144 insertions(+)

Hi Chao-ying Fu,


On Sat, Jun 14, 2025 at 10:58 PM Anup Patel <apatel@ventanamicro.com> wrote:
>
> From: Chao-ying Fu <cfu@wavecomp.com>
>
> Allow platforms to implement platform specific PMP setup and
> PMP disable functions which are called before actual PMP CSRs
> are configured.
>
> Also, implement pmp_set() and pmp_disable() for MIPS P8700.
>
> Signed-off-by: Chao-ying Fu <cfu@mips.com>
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>

Please try this patch at your end on top of the latest OpenSBI
and see if this helps on MIPS P8700.

Regards,
Anup

> ---
>  include/sbi/sbi_platform.h    | 39 +++++++++++++++
>  lib/sbi/sbi_domain_context.c  |  2 +
>  lib/sbi/sbi_hart.c            | 10 ++++
>  platform/generic/mips/p8700.c | 93 +++++++++++++++++++++++++++++++++++
>  4 files changed, 144 insertions(+)
>
> diff --git a/include/sbi/sbi_platform.h b/include/sbi/sbi_platform.h
> index 08ece320..c6d30080 100644
> --- a/include/sbi/sbi_platform.h
> +++ b/include/sbi/sbi_platform.h
> @@ -142,6 +142,13 @@ struct sbi_platform_operations {
>         /** platform specific handler to fixup store fault */
>         int (*emulate_store)(int wlen, unsigned long addr,
>                              union sbi_ldst_data in_val);
> +
> +       /** platform specific pmp setup on current HART */
> +       void (*pmp_set)(unsigned int n, unsigned long flags,
> +                       unsigned long prot, unsigned long addr,
> +                       unsigned long log2len);
> +       /** platform specific pmp disable on current HART */
> +       void (*pmp_disable)(unsigned int n);
>  };
>
>  /** Platform default per-HART stack size for exception/interrupt handling */
> @@ -644,6 +651,38 @@ static inline int sbi_platform_emulate_store(const struct sbi_platform *plat,
>         return SBI_ENOTSUPP;
>  }
>
> +/**
> + * Platform specific PMP setup on current HART
> + *
> + * @param plat pointer to struct sbi_platform
> + * @param n index of the pmp entry
> + * @param flags domain memregion flags
> + * @param prot attribute of the pmp entry
> + * @param addr address of the pmp entry
> + * @param log2len size of the pmp entry as power-of-2
> + */
> +static inline void sbi_platform_pmp_set(const struct sbi_platform *plat,
> +                                       unsigned int n, unsigned long flags,
> +                                       unsigned long prot, unsigned long addr,
> +                                       unsigned long log2len)
> +{
> +       if (plat && sbi_platform_ops(plat)->pmp_set)
> +               sbi_platform_ops(plat)->pmp_set(n, flags, prot, addr, log2len);
> +}
> +
> +/**
> + * Platform specific PMP disable on current HART
> + *
> + * @param plat pointer to struct sbi_platform
> + * @param n index of the pmp entry
> + */
> +static inline void sbi_platform_pmp_disable(const struct sbi_platform *plat,
> +                                           unsigned int n)
> +{
> +       if (plat && sbi_platform_ops(plat)->pmp_disable)
> +               sbi_platform_ops(plat)->pmp_disable(n);
> +}
> +
>  #endif
>
>  #endif
> diff --git a/lib/sbi/sbi_domain_context.c b/lib/sbi/sbi_domain_context.c
> index 2b19fcc7..fb04d81d 100644
> --- a/lib/sbi/sbi_domain_context.c
> +++ b/lib/sbi/sbi_domain_context.c
> @@ -15,6 +15,7 @@
>  #include <sbi/sbi_string.h>
>  #include <sbi/sbi_domain.h>
>  #include <sbi/sbi_domain_context.h>
> +#include <sbi/sbi_platform.h>
>  #include <sbi/sbi_trap.h>
>
>  /** Context representation for a hart within a domain */
> @@ -115,6 +116,7 @@ static void switch_to_next_domain_context(struct hart_context *ctx,
>
>         /* Reconfigure PMP settings for the new domain */
>         for (int i = 0; i < pmp_count; i++) {
> +               sbi_platform_pmp_disable(sbi_platform_thishart_ptr(), i);
>                 pmp_disable(i);
>         }
>         sbi_hart_pmp_configure(scratch);
> diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c
> index 74ccdd84..054a405c 100644
> --- a/lib/sbi/sbi_hart.c
> +++ b/lib/sbi/sbi_hart.c
> @@ -375,6 +375,9 @@ static void sbi_hart_smepmp_set(struct sbi_scratch *scratch,
>         unsigned long pmp_addr = reg->base >> PMP_SHIFT;
>
>         if (pmp_log2gran <= reg->order && pmp_addr < pmp_addr_max) {
> +               sbi_platform_pmp_set(sbi_platform_ptr(scratch),
> +                                    pmp_idx, reg->flags, pmp_flags,
> +                                    reg->base, reg->order);
>                 pmp_set(pmp_idx, pmp_flags, reg->base, reg->order);
>         } else {
>                 sbi_printf("Can not configure pmp for domain %s because"
> @@ -492,6 +495,9 @@ static int sbi_hart_oldpmp_configure(struct sbi_scratch *scratch,
>
>                 pmp_addr = reg->base >> PMP_SHIFT;
>                 if (pmp_log2gran <= reg->order && pmp_addr < pmp_addr_max) {
> +                       sbi_platform_pmp_set(sbi_platform_ptr(scratch),
> +                                            pmp_idx, reg->flags, pmp_flags,
> +                                            reg->base, reg->order);
>                         pmp_set(pmp_idx++, pmp_flags, reg->base, reg->order);
>                 } else {
>                         sbi_printf("Can not configure pmp for domain %s because"
> @@ -532,6 +538,9 @@ int sbi_hart_map_saddr(unsigned long addr, unsigned long size)
>                 }
>         }
>
> +       sbi_platform_pmp_set(sbi_platform_ptr(scratch), SBI_SMEPMP_RESV_ENTRY,
> +                            SBI_DOMAIN_MEMREGION_SHARED_SURW_MRW,
> +                            pmp_flags, base, order);
>         pmp_set(SBI_SMEPMP_RESV_ENTRY, pmp_flags, base, order);
>
>         return SBI_OK;
> @@ -544,6 +553,7 @@ int sbi_hart_unmap_saddr(void)
>         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);
>  }
>
> diff --git a/platform/generic/mips/p8700.c b/platform/generic/mips/p8700.c
> index 6b687179..888a45c6 100644
> --- a/platform/generic/mips/p8700.c
> +++ b/platform/generic/mips/p8700.c
> @@ -16,6 +16,98 @@
>  #include <mips/p8700.h>
>  #include <mips/mips-cm.h>
>
> +static unsigned long mips_csr_read_num(int csr_num)
> +{
> +#define switchcase_csr_read(__csr_num, __val)          \
> +       case __csr_num:                                 \
> +               __val = csr_read(__csr_num);            \
> +               break;
> +#define switchcase_csr_read_2(__csr_num, __val)                \
> +       switchcase_csr_read(__csr_num + 0, __val)       \
> +       switchcase_csr_read(__csr_num + 1, __val)
> +#define switchcase_csr_read_4(__csr_num, __val)                \
> +       switchcase_csr_read_2(__csr_num + 0, __val)     \
> +       switchcase_csr_read_2(__csr_num + 2, __val)
> +#define switchcase_csr_read_8(__csr_num, __val)                \
> +       switchcase_csr_read_4(__csr_num + 0, __val)     \
> +       switchcase_csr_read_4(__csr_num + 4, __val)
> +#define switchcase_csr_read_16(__csr_num, __val)       \
> +       switchcase_csr_read_8(__csr_num + 0, __val)     \
> +       switchcase_csr_read_8(__csr_num + 8, __val)
> +
> +       unsigned long ret = 0;
> +
> +       switch(csr_num) {
> +       switchcase_csr_read_16(CSR_MIPSPMACFG0, ret)
> +
> +       default:
> +               sbi_panic("%s: Unknown CSR %#x", __func__, csr_num);
> +               break;
> +       }
> +
> +       return ret;
> +
> +#undef switchcase_csr_read_16
> +#undef switchcase_csr_read_8
> +#undef switchcase_csr_read_4
> +#undef switchcase_csr_read_2
> +#undef switchcase_csr_read
> +}
> +
> +static void mips_csr_write_num(int csr_num, unsigned long val)
> +{
> +#define switchcase_csr_write(__csr_num, __val)         \
> +       case __csr_num:                                 \
> +               csr_write(__csr_num, __val);            \
> +               break;
> +#define switchcase_csr_write_2(__csr_num, __val)       \
> +       switchcase_csr_write(__csr_num + 0, __val)      \
> +       switchcase_csr_write(__csr_num + 1, __val)
> +#define switchcase_csr_write_4(__csr_num, __val)       \
> +       switchcase_csr_write_2(__csr_num + 0, __val)    \
> +       switchcase_csr_write_2(__csr_num + 2, __val)
> +#define switchcase_csr_write_8(__csr_num, __val)       \
> +       switchcase_csr_write_4(__csr_num + 0, __val)    \
> +       switchcase_csr_write_4(__csr_num + 4, __val)
> +#define switchcase_csr_write_16(__csr_num, __val)      \
> +       switchcase_csr_write_8(__csr_num + 0, __val)    \
> +       switchcase_csr_write_8(__csr_num + 8, __val)
> +
> +       switch(csr_num) {
> +       switchcase_csr_write_16(CSR_MIPSPMACFG0, val)
> +
> +       default:
> +               sbi_panic("%s: Unknown CSR %#x", __func__, csr_num);
> +               break;
> +       }
> +
> +#undef switchcase_csr_write_16
> +#undef switchcase_csr_write_8
> +#undef switchcase_csr_write_4
> +#undef switchcase_csr_write_2
> +#undef switchcase_csr_write
> +}
> +
> +static void mips_p8700_pmp_set(unsigned int n, unsigned long flags,
> +                              unsigned long prot, unsigned long addr,
> +                              unsigned long log2len)
> +{
> +       int pmacfg_csr, pmacfg_shift;
> +       unsigned long cfgmask;
> +       unsigned long pmacfg, cca;
> +
> +       pmacfg_csr = (CSR_MIPSPMACFG0 + (n >> 2)) & ~1;
> +       pmacfg_shift = (n & 7) << 3;
> +       cfgmask = ~(0xffUL << pmacfg_shift);
> +
> +       /* Read pmacfg to change cacheability */
> +       pmacfg = (mips_csr_read_num(pmacfg_csr) & cfgmask);
> +       cca = (flags & SBI_DOMAIN_MEMREGION_MMIO) ? CCA_CACHE_DISABLE :
> +                                 CCA_CACHE_ENABLE | PMA_SPECULATION;
> +       pmacfg |= ((cca << pmacfg_shift) & ~cfgmask);
> +       mips_csr_write_num(pmacfg_csr, pmacfg);
> +}
> +
>  #if CLUSTERS_IN_PLATFORM > 1
>  static void power_up_other_cluster(u32 hartid)
>  {
> @@ -255,6 +347,7 @@ static int mips_p8700_platform_init(const void *fdt, int nodeoff, const struct f
>         generic_platform_ops.early_init = mips_p8700_early_init;
>         generic_platform_ops.final_init = mips_p8700_final_init;
>         generic_platform_ops.nascent_init = mips_p8700_nascent_init;
> +       generic_platform_ops.pmp_set = mips_p8700_pmp_set;
>
>         return 0;
>  }
> --
> 2.43.0
>

Hi Anup,

> Hi Chao-ying Fu,


> On Sat, Jun 14, 2025 at 10:58 PM Anup Patel <apatel@ventanamicro.com> wrote:
> >
> > From: Chao-ying Fu <cfu@wavecomp.com>
> >
> > Allow platforms to implement platform specific PMP setup and
> > PMP disable functions which are called before actual PMP CSRs
> > are configured.
> >
> > Also, implement pmp_set() and pmp_disable() for MIPS P8700.
> >
> > Signed-off-by: Chao-ying Fu <cfu@mips.com>
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>

> Please try this patch at your end on top of the latest OpenSBI
> and see if this helps on MIPS P8700.

> Regards,
> Anup

  Yes, this patch works functionally the same as what the old version does on FPGA and QEMU.
Note: I added two more patches from Vladimir to deal with MMIO regions on top of this patch.

  Thanks a lot!

Regards,
Chao-ying

Hi Chao-ying Fu,

On Mon, Jun 16, 2025 at 9:47 PM Chao-ying Fu <cfu@mips.com> wrote:
>
> Hi Anup,
>
> > Hi Chao-ying Fu,
>
>
> > On Sat, Jun 14, 2025 at 10:58 PM Anup Patel <apatel@ventanamicro.com> wrote:
> > >
> > > From: Chao-ying Fu <cfu@wavecomp.com>
> > >
> > > Allow platforms to implement platform specific PMP setup and
> > > PMP disable functions which are called before actual PMP CSRs
> > > are configured.
> > >
> > > Also, implement pmp_set() and pmp_disable() for MIPS P8700.
> > >
> > > Signed-off-by: Chao-ying Fu <cfu@mips.com>
> > > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
>
> > Please try this patch at your end on top of the latest OpenSBI
> > and see if this helps on MIPS P8700.
>
> > Regards,
> > Anup
>
>   Yes, this patch works functionally the same as what the old version does on FPGA and QEMU.

Thanks, I will apply this patch to the riscv/opensbi repo.

> Note: I added two more patches from Vladimir to deal with MMIO regions on top of this patch.

With this patch, we don't need the PMP_MMIO flag added by Vladimir's patches.
Is there any other patch ?

Regards,
Anup

Hi Anup,

> Hi Chao-ying Fu,
> 
> On Mon, Jun 16, 2025 at 9:47 PM Chao-ying Fu <cfu@mips.com> wrote:
> >
> > Hi Anup,
> >
> > > Hi Chao-ying Fu,
> >
> >
> > > On Sat, Jun 14, 2025 at 10:58 PM Anup Patel <apatel@ventanamicro.com> wrote:
> > > >
> > > > From: Chao-ying Fu <cfu@wavecomp.com>
> > > >
> > > > Allow platforms to implement platform specific PMP setup and
> > > > PMP disable functions which are called before actual PMP CSRs
> > > > are configured.
> > > >
> > > > Also, implement pmp_set() and pmp_disable() for MIPS P8700.
> > > >
> > > > Signed-off-by: Chao-ying Fu <cfu@mips.com>
> > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> >
> > > Please try this patch at your end on top of the latest OpenSBI
> > > and see if this helps on MIPS P8700.
> >
> > > Regards,
> > > Anup
> >
> >   Yes, this patch works functionally the same as what the old version does on FPGA and QEMU.

> Thanks, I will apply this patch to the riscv/opensbi repo.

> > Note: I added two more patches from Vladimir to deal with MMIO regions on top of this patc h.

> With this patch, we don't need the PMP_MMIO flag added by Vladimir's patches.
> Is there any other patch ?

  Sorry, I didn't notice that there is a parameter: flags.
With "flags", we can check SBI_DOMAIN_MEMREGION_MMIO in mips_p8700_pmp_set().
We don't need Vladirmir's patch that creates PMP_MMIO and propagates to the parameter: prop.

  However, we may still need Vladimir's patch for the fdt fixup code that tries to disable the aplic node.
With this patch, we can have the MMIO type for the region that covers the aplic node, and
mips_p8700_pmp_set() can set up correct cacheability for the MMIO or non-MMIO region.
(This patch may affect the existing dts files that don't have the MMIO type for the region that covers the aplic node.)
--
From: Vladimir Kondratiev <vladimir.kondratiev at mobileye.com>

When applying FDT fixups, request device memory to be of
MMIO type

Signed-off-by: Vladimir Kondratiev <vladimir.kondratiev at mobileye.com>
---
 lib/utils/fdt/fdt_fixup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/utils/fdt/fdt_fixup.c b/lib/utils/fdt/fdt_fixup.c
index f3fe8af..b0ed20c 100644
--- a/lib/utils/fdt/fdt_fixup.c
+++ b/lib/utils/fdt/fdt_fixup.c
@@ -185,7 +185,7 @@ static void fdt_domain_based_fixup_one(void *fdt, int nodeoff)
 		return;
 
 	if (!sbi_domain_check_addr(dom, reg_addr, dom->next_mode,
-				    SBI_DOMAIN_READ | SBI_DOMAIN_WRITE)) {
+				    SBI_DOMAIN_READ | SBI_DOMAIN_WRITE | SBI_DOMAIN_MMIO)) {
 		rc = fdt_open_into(fdt, fdt, fdt_totalsize(fdt) + 32);
 		if (rc < 0)
 			return;

On Tue, Jun 17, 2025 at 8:16 PM Chao-ying Fu <cfu@mips.com> wrote:
>
> Hi Anup,
>
> > Hi Chao-ying Fu,
> >
> > On Mon, Jun 16, 2025 at 9:47 PM Chao-ying Fu <cfu@mips.com> wrote:
> > >
> > > Hi Anup,
> > >
> > > > Hi Chao-ying Fu,
> > >
> > >
> > > > On Sat, Jun 14, 2025 at 10:58 PM Anup Patel <apatel@ventanamicro.com> wrote:
> > > > >
> > > > > From: Chao-ying Fu <cfu@wavecomp.com>
> > > > >
> > > > > Allow platforms to implement platform specific PMP setup and
> > > > > PMP disable functions which are called before actual PMP CSRs
> > > > > are configured.
> > > > >
> > > > > Also, implement pmp_set() and pmp_disable() for MIPS P8700.
> > > > >
> > > > > Signed-off-by: Chao-ying Fu <cfu@mips.com>
> > > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > >
> > > > Please try this patch at your end on top of the latest OpenSBI
> > > > and see if this helps on MIPS P8700.
> > >
> > > > Regards,
> > > > Anup
> > >
> > >   Yes, this patch works functionally the same as what the old version does on FPGA and QEMU.
>
> > Thanks, I will apply this patch to the riscv/opensbi repo.
>
> > > Note: I added two more patches from Vladimir to deal with MMIO regions on top of this patc h.
>
> > With this patch, we don't need the PMP_MMIO flag added by Vladimir's patches.
> > Is there any other patch ?
>
>   Sorry, I didn't notice that there is a parameter: flags.
> With "flags", we can check SBI_DOMAIN_MEMREGION_MMIO in mips_p8700_pmp_set().
> We don't need Vladirmir's patch that creates PMP_MMIO and propagates to the parameter: prop.
>
>   However, we may still need Vladimir's patch for the fdt fixup code that tries to disable the aplic node.
> With this patch, we can have the MMIO type for the region that covers the aplic node, and
> mips_p8700_pmp_set() can set up correct cacheability for the MMIO or non-MMIO region.
> (This patch may affect the existing dts files that don't have the MMIO type for the region that covers the aplic node.)

OpenSBI is already disabling APLIC root domain which is meant
for M-mode.

This can be easily verified on QEMU virt,aia=aplic-imsic machine
from Linux user-space using following command:
$ cat /proc/device-tree/soc/interrupt-controller@c000000/status

> --
> From: Vladimir Kondratiev <vladimir.kondratiev at mobileye.com>
>
> When applying FDT fixups, request device memory to be of
> MMIO type
>
> Signed-off-by: Vladimir Kondratiev <vladimir.kondratiev at mobileye.com>
> ---
>  lib/utils/fdt/fdt_fixup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/utils/fdt/fdt_fixup.c b/lib/utils/fdt/fdt_fixup.c
> index f3fe8af..b0ed20c 100644
> --- a/lib/utils/fdt/fdt_fixup.c
> +++ b/lib/utils/fdt/fdt_fixup.c
> @@ -185,7 +185,7 @@ static void fdt_domain_based_fixup_one(void *fdt, int nodeoff)
>                 return;
>
>         if (!sbi_domain_check_addr(dom, reg_addr, dom->next_mode,
> -                                   SBI_DOMAIN_READ | SBI_DOMAIN_WRITE)) {
> +                                   SBI_DOMAIN_READ | SBI_DOMAIN_WRITE | SBI_DOMAIN_MMIO)) {
>                 rc = fdt_open_into(fdt, fdt, fdt_totalsize(fdt) + 32);
>                 if (rc < 0)
>                         return;
> --
> 2.47.1
>

Regards,
Anup