[PATCH 11/22] platform: generic: mips p8700: restrict memory region to physical memory as in DTS

[Bo Gan]

Hi Vladimir,

On 1/14/26 01:21, Vladimir Kondratiev wrote:
> SBI domain initialization add "all memory" region with unrestricted
> access. This can cause a problem: CPU can issue
> speculative prefetches to non-existent addresses. If this access
> goes to the system NOC, it is mis-interpreted as an access violation
> and error is reported, forcing system reset.
> 
> To prevent such a speculative transaction to leave a CPU cluster,
> block it using PMP, by restricting memory region to physically present
> memory.
> 
> Signed-off-by: Vladimir Kondratiev <vladimir.kondratiev at mobileye.com>
> ---
>   platform/generic/mips/p8700.c | 33 +++++++++++++++++++++++++++------
>   1 file changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/platform/generic/mips/p8700.c b/platform/generic/mips/p8700.c
> index 25e6075581e8..a3d05602a6d1 100644
> --- a/platform/generic/mips/p8700.c
> +++ b/platform/generic/mips/p8700.c
> @@ -14,6 +14,7 @@
>   #include <sbi/sbi_timer.h>
>   #include <sbi/sbi_hart_pmp.h>
>   #include <sbi/riscv_io.h>
> +#include <libfdt.h>
>   #include <sbi_utils/fdt/fdt_helper.h>
>   #include <mips/p8700.h>
>   #include <mips/mips-cm.h>
> @@ -148,14 +149,36 @@ static struct sbi_domain_memregion *find_last_memregion(const struct sbi_domain
>   	return --reg;
>   }
>   
> +static int fixup_dram_region(const struct sbi_domain *dom,
> +			     struct sbi_domain_memregion *reg)
> +{
> +	const void *fdt = fdt_get_address();
> +	int node;
> +	int ret;
> +	uint64_t mem_addr, mem_size;
> +	static const char mem_str[] = "memory";
> +
> +	if (!reg || !fdt)
> +		return SBI_EINVAL;
> +
> +	/* Find the memory range */
> +	node = fdt_node_offset_by_prop_value(fdt, -1, "device_type",
> +					     mem_str, sizeof(mem_str));
> +	ret = fdt_get_node_addr_size(fdt, node, 0, &mem_addr, &mem_size);
> +	if (ret)
> +		return ret;
> +	sbi_domain_memregion_init(mem_addr, mem_size, reg->flags, reg);> +	return 0;
> +}
> +
>   static int mips_p8700_final_init(bool cold_boot)
>   {
>   	struct sbi_scratch *scratch;
>   	const struct sbi_domain *dom;
>   	struct sbi_domain_memregion *reg;
> -	const unsigned long end_of_dram = 0x1000000000LU;
>   	unsigned long pmp_gran;
>   	int pmp_count;
> +	int ret;
>   
>   	if (!cold_boot)
>   		return 0;
> @@ -172,11 +195,9 @@ static int mips_p8700_final_init(bool cold_boot)
>   	 */
>   	dom = sbi_domain_thishart_ptr();
>   	reg = find_last_memregion(dom);
> -	sbi_printf("Fix region[%ld]: base 0x%lx order %ld flags 0x%lx\n",
> -		   reg - dom->regions, reg->base, reg->order, reg->flags);
> -	reg->order = sbi_fls(end_of_dram);

I have some doubts on this. On the code level, `reg` comes from
find_last_memregion, which exposes internal memory region data structure
from `domain`. If modifying the base/order/flags directly, that bypasses
the sanitation logic in root_add_memregion, which could invalidate the
assumption in lib/ code. In your subsequent patch, there's also direct
access to dom->regions (again internal structures). IMO, we shouldn't do
that in platform code. Perhaps Anup has better thoughts.

> -	sbi_printf("Modified order: %ld to match end of DRAM 0x%lx\n",
> -		   reg->order, end_of_dram);
> +	ret = fixup_dram_region(dom, reg);

Based on your description above, you are trying to block speculative
access above the physical memory region. I encountered similar problem
when coding up the EIC7700 platform. I also need to block access to some
regions to avoid speculative access triggering bus errors. However I
realized that just doing what you did here is not enough. For M mode, if
there's no matching PMP (traditional) entries, then by default the access
is granted. Hence, I don't see how you prevent speculation from M mode to
system NOC.

> +	if (ret)
> +		return ret;
>   	/*
>   	 * clear the rest of regions that may be set by the boot code
>   	 * SBI code don't let to disable PMP region, so configure it to

Bo