Re: [PATCH 1/3] lib: sbi: allow platform to override PMP configuration

Xiang W
2025-11-11T05:46:12Z

在 2025-11-10一的 19:41 -0800，Bo Gan写道：
> Platform sometimes wants to override the entire PMP configuration phase,
> not just performing additional work for each individual PMP entry. This
> allows platforms to insert SoC/core specific PMP entries in a way that
> works together with the existing ones set by lib/ code, not conflicting.
> platform can also choose to merge or skip memory regions in a reasonable
> way in case there's a shortage of PMP entries.
>
> In addition, `sbi_hart_oldpmp_configure` is made public and a callback
> function `skip` is added, so platform code can built on top of it to
> simplify PMP configuration override.
>
> Signed-off-by: Bo Gan <ganboing@gmail.com>
> ---
>  include/sbi/sbi_hart.h     |  9 +++++++++
>  include/sbi/sbi_platform.h | 33 +++++++++++++++++++++++++++++++++
>  lib/sbi/sbi_hart.c         | 27 +++++++++++++++++++--------
>  3 files changed, 61 insertions(+), 8 deletions(-)
>
> diff --git a/include/sbi/sbi_hart.h b/include/sbi/sbi_hart.h
> index e66dd52f..e5a221bd 100644
> --- a/include/sbi/sbi_hart.h
> +++ b/include/sbi/sbi_hart.h
> @@ -133,6 +133,7 @@ struct sbi_hart_features {
>  };
>
>  struct sbi_scratch;
> +struct sbi_domain_memregion;
>
>  int sbi_hart_reinit(struct sbi_scratch *scratch);
>  int sbi_hart_init(struct sbi_scratch *scratch, bool cold_boot);
> @@ -148,6 +149,14 @@ unsigned int sbi_hart_pmp_addrbits(struct sbi_scratch *scratch);
>  unsigned int sbi_hart_mhpm_bits(struct sbi_scratch *scratch);
>  bool sbi_hart_smepmp_is_fw_region(unsigned int pmp_idx);
>  int sbi_hart_pmp_configure(struct sbi_scratch *scratch);
> +int sbi_hart_oldpmp_configure(struct sbi_scratch *scratch,
> +       unsigned int pmp_start,
It is recommended to change the declaration from pmp_start to pmp_reserved.
Otherwise, it might give the misleading impression that the last pmp is
pmp_start + pmp_count - 1.

Regards,
Xiang W
> +       unsigned int pmp_count,
> +       unsigned int pmp_log2gran,
> +       unsigned long pmp_addr_max,
> +       bool (*skip)(struct sbi_domain_memregion *reg,
> +    void *data),
> +       void *data);
>  int sbi_hart_map_saddr(unsigned long base, unsigned long size);
>  int sbi_hart_unmap_saddr(void);
>  int sbi_hart_priv_version(struct sbi_scratch *scratch);
> diff --git a/include/sbi/sbi_platform.h b/include/sbi/sbi_platform.h
> index d75c12de..c6fc137f 100644
> --- a/include/sbi/sbi_platform.h
> +++ b/include/sbi/sbi_platform.h
> @@ -146,6 +146,11 @@ struct sbi_platform_operations {
>   unsigned long log2len);
>   /** platform specific pmp disable on current HART */
>   void (*pmp_disable)(unsigned int n);
> +
> + /** platform pmp configure override on current HART */
> + int (*pmp_configure)(unsigned int pmp_count,
> +      unsigned int pmp_log2gran,
> +      unsigned long pmp_addr_max);
>  };
>
>  /** Platform default per-HART stack size for exception/interrupt handling */
> @@ -666,6 +671,34 @@ static inline void sbi_platform_pmp_disable(const struct sbi_platform *plat,
>   sbi_platform_ops(plat)->pmp_disable(n);
>  }
>
> +/**
> + * Check if platform wants to override PMP configuration
> + *
> + * @param plat pointer to struct sbi_platform
> + */
> +static inline bool sbi_platform_pmp_override(const struct sbi_platform *plat)
> +{
> + return plat && sbi_platform_ops(plat)->pmp_configure;
> +}
> +
> +/**
> + * Platform PMP configuration override
> + *
> + * @param plat pointer to struct sbi_platform
> + * @param pmp_count number of PMP entries
> + * @param pmp_log2gran PMP granularity
> + * @param pmp_addr_max largest value pmpaddr(x) can hold
> + */
> +static inline int sbi_platform_pmp_configure(const struct sbi_platform *plat,
> +      unsigned int pmp_count,
> +      unsigned int pmp_log2gran,
> +      unsigned long pmp_addr_max)
> +{
> + return sbi_platform_ops(plat)->pmp_configure(pmp_count,
> +      pmp_log2gran,
> +      pmp_addr_max);
> +}
> +
>  #endif
>
>  #endif
> diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c
> index a91703b4..4425f36b 100644
> --- a/lib/sbi/sbi_hart.c
> +++ b/lib/sbi/sbi_hart.c
> @@ -433,18 +433,24 @@ static int sbi_hart_smepmp_configure(struct sbi_scratch *scratch,
>   return 0;
>  }
>
> -static int sbi_hart_oldpmp_configure(struct sbi_scratch *scratch,
> -      unsigned int pmp_count,
> -      unsigned int pmp_log2gran,
> -      unsigned long pmp_addr_max)
> +int sbi_hart_oldpmp_configure(struct sbi_scratch *scratch,
> +       unsigned int pmp_start,
> +       unsigned int pmp_count,
> +       unsigned int pmp_log2gran,
> +       unsigned long pmp_addr_max,
> +       bool (*skip)(struct sbi_domain_memregion *reg,
> +    void *data),
> +       void *data)
>  {
>   struct sbi_domain_memregion *reg;
>   struct sbi_domain *dom = sbi_domain_thishart_ptr();
> - unsigned int pmp_idx = 0;
> + unsigned int pmp_idx = pmp_start;
>   unsigned int pmp_flags;
>   unsigned long pmp_addr;
>
>   sbi_domain_for_each_memregion(dom, reg) {
> + if (skip && skip(reg, data))
> + continue;
>   if (!is_valid_pmp_idx(pmp_count, pmp_idx))
>   return SBI_EFAIL;
>
> @@ -534,6 +540,7 @@ int sbi_hart_pmp_configure(struct sbi_scratch *scratch)
>   unsigned int pmp_bits, pmp_log2gran;
>   unsigned int pmp_count = sbi_hart_pmp_count(scratch);
>   unsigned long pmp_addr_max;
> + const struct sbi_platform *plat = sbi_platform_ptr(scratch);
>
>   if (!pmp_count)
>   return 0;
> @@ -542,12 +549,16 @@ int sbi_hart_pmp_configure(struct sbi_scratch *scratch)
>   pmp_bits = sbi_hart_pmp_addrbits(scratch) - 1;
>   pmp_addr_max = (1UL << pmp_bits) | ((1UL << pmp_bits) - 1);
>
> - if (sbi_hart_has_extension(scratch, SBI_HART_EXT_SMEPMP))
> + if (sbi_platform_pmp_override(plat))
> + rc = sbi_platform_pmp_configure(plat, pmp_count,
> + pmp_log2gran, pmp_addr_max);
> + else if (sbi_hart_has_extension(scratch, SBI_HART_EXT_SMEPMP))
>   rc = sbi_hart_smepmp_configure(scratch, pmp_count,
>   pmp_log2gran, pmp_addr_max);
>   else
> - rc = sbi_hart_oldpmp_configure(scratch, pmp_count,
> - pmp_log2gran, pmp_addr_max);
> + rc = sbi_hart_oldpmp_configure(scratch, 0, pmp_count,
> + pmp_log2gran, pmp_addr_max,
> + NULL, NULL);
>
>   /*
>   * As per section 3.7.2 of privileged specification v1.12,
> --
> 2.34.1