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

[Bo Gan <ganboing@gmail.com>]

Hi Xiang,

On 11/10/25 21:45, Xiang W wrote:
> 在 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

Thanks for the review. Yes, this pmp_start/pmp_count is indeed misleading.
I'll address it in the next version.

Bo

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


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