Re: [PATCH v2] lib: utils: fdt_helper: simplify fdt_parse_isa_extensions() helper

[Samuel Holland <samuel.holland@sifive.com>]

Hi Peter,

On 2025-09-06 2:26 AM, Yu-Chien Peter Lin wrote:
> The fdt_isa_bitmap was previously used as a temporary array
> to store ISA extensions when parsing device-tree.
> To reduce scratch space consumption, set the ISA bitmap
> directly in hfeature->extensions, allows us to remove the
> fdt_parse_isa_all_harts() as fdt_parse_isa_extensions() can
> now handle the task directly.

Unfortunately, this change is not valid, because sbi_platform_extensions_init()
is called from the warm boot process, and it is not safe to access the FDT after
the cold boot process completes. The FDT is stored in memory accessible to
S-mode, so it could be clobbered as soon as the first supervisor domain starts.
Any refactoring must satisfy the requirement that the cold boot hart parses the
FDT nodes of all harts.

Regards,
Samuel

> 
> This change makes the helper function more versatile, even
> before scratch space is initialized.
> 
> Signed-off-by: Yu-Chien Peter Lin <peter.lin@sifive.com>
> ---
>  include/sbi_utils/fdt/fdt_helper.h |  3 +-
>  lib/utils/fdt/fdt_helper.c         | 48 ++++--------------------------
>  platform/generic/platform.c        |  2 +-
>  3 files changed, 7 insertions(+), 46 deletions(-)
> 
> diff --git a/include/sbi_utils/fdt/fdt_helper.h b/include/sbi_utils/fdt/fdt_helper.h
> index 04c850cc..24ad1a94 100644
> --- a/include/sbi_utils/fdt/fdt_helper.h
> +++ b/include/sbi_utils/fdt/fdt_helper.h
> @@ -54,8 +54,7 @@ int fdt_parse_cbom_block_size(const void *fdt, int cpu_offset, unsigned long  *c
>  
>  int fdt_parse_timebase_frequency(const void *fdt, unsigned long *freq);
>  
> -int fdt_parse_isa_extensions(const void *fdt, unsigned int hartid,
> -			     unsigned long *extensions);
> +int fdt_parse_isa_extensions(const void *fdt, unsigned long *hart_exts);
>  
>  int fdt_parse_gaisler_uart_node(const void *fdt, int nodeoffset,
>  				struct platform_uart_data *uart);
> diff --git a/lib/utils/fdt/fdt_helper.c b/lib/utils/fdt/fdt_helper.c
> index 0f4859c1..252dd459 100644
> --- a/lib/utils/fdt/fdt_helper.c
> +++ b/lib/utils/fdt/fdt_helper.c
> @@ -324,8 +324,6 @@ int fdt_parse_timebase_frequency(const void *fdt, unsigned long *freq)
>  
>  #define RISCV_ISA_EXT_NAME_LEN_MAX	32
>  
> -static unsigned long fdt_isa_bitmap_offset;
> -
>  static int fdt_parse_isa_one_hart(const char *isa, unsigned long *extensions)
>  {
>  	size_t i, j, isa_len;
> @@ -405,15 +403,13 @@ static void fdt_parse_isa_extensions_one_hart(const char *isa,
>  	}
>  }
>  
> -static int fdt_parse_isa_all_harts(const void *fdt)
> +int fdt_parse_isa_extensions(const void *fdt, unsigned long *hart_exts)
>  {
>  	u32 hartid;
>  	const fdt32_t *val;
> -	unsigned long *hart_exts;
> -	struct sbi_scratch *scratch;
>  	int err, cpu_offset, cpus_offset, len;
>  
> -	if (!fdt || !fdt_isa_bitmap_offset)
> +	if (!fdt || !hart_exts)
>  		return SBI_EINVAL;
>  
>  	cpus_offset = fdt_path_offset(fdt, "/cpus");
> @@ -425,15 +421,11 @@ static int fdt_parse_isa_all_harts(const void *fdt)
>  		if (err)
>  			continue;
>  
> -		if (!fdt_node_is_enabled(fdt, cpu_offset))
> +		if (hartid != current_hartid())
>  			continue;
>  
> -		scratch = sbi_hartid_to_scratch(hartid);
> -		if (!scratch)
> -			return SBI_ENOENT;
> -
> -		hart_exts = sbi_scratch_offset_ptr(scratch,
> -						   fdt_isa_bitmap_offset);
> +		if (!fdt_node_is_enabled(fdt, cpu_offset))
> +			continue;
>  
>  		val = fdt_getprop(fdt, cpu_offset, "riscv,isa-extensions", &len);
>  		if (val && len > 0) {
> @@ -454,36 +446,6 @@ static int fdt_parse_isa_all_harts(const void *fdt)
>  	return 0;
>  }
>  
> -int fdt_parse_isa_extensions(const void *fdt, unsigned int hartid,
> -			unsigned long *extensions)
> -{
> -	int rc, i;
> -	unsigned long *hart_exts;
> -	struct sbi_scratch *scratch;
> -
> -	if (!fdt_isa_bitmap_offset) {
> -		fdt_isa_bitmap_offset = sbi_scratch_alloc_offset(
> -					sizeof(*hart_exts) *
> -					BITS_TO_LONGS(SBI_HART_EXT_MAX));
> -		if (!fdt_isa_bitmap_offset)
> -			return SBI_ENOMEM;
> -
> -		rc = fdt_parse_isa_all_harts(fdt);
> -		if (rc)
> -			return rc;
> -	}
> -
> -	scratch = sbi_hartid_to_scratch(hartid);
> -	if (!scratch)
> -		return SBI_ENOENT;
> -
> -	hart_exts = sbi_scratch_offset_ptr(scratch, fdt_isa_bitmap_offset);
> -
> -	for (i = 0; i < BITS_TO_LONGS(SBI_HART_EXT_MAX); i++)
> -		extensions[i] |= hart_exts[i];
> -	return 0;
> -}
> -
>  static int fdt_parse_uart_node_common(const void *fdt, int nodeoffset,
>  				      struct platform_uart_data *uart,
>  				      unsigned long default_freq,
> diff --git a/platform/generic/platform.c b/platform/generic/platform.c
> index 47e771ad..2e8c6051 100644
> --- a/platform/generic/platform.c
> +++ b/platform/generic/platform.c
> @@ -251,7 +251,7 @@ int generic_final_init(bool cold_boot)
>  int generic_extensions_init(struct sbi_hart_features *hfeatures)
>  {
>  	/* Parse the ISA string from FDT and enable the listed extensions */
> -	return fdt_parse_isa_extensions(fdt_get_address(), current_hartid(),
> +	return fdt_parse_isa_extensions(fdt_get_address(),
>  					hfeatures->extensions);
>  }
>  


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