Re: [kvm-unit-tests PATCH v2 11/18] arm/arm64: Factor out some initial setup

[Nikos Nikoleris <nikos.nikoleris@arm.com>]

On 27/02/2024 19:21, Andrew Jones wrote:
> Factor out some initial setup code into separate functions in order
> to share more code between setup() and setup_efi().
> 
> Signed-off-by: Andrew Jones <andrew.jones@linux.dev>

Reviewed-by: Nikos Nikoleris <nikos.nikoleris@arm.com>

Thanks,

Nikos

> ---
>   lib/arm/setup.c | 81 ++++++++++++++++++++++++++++---------------------
>   1 file changed, 47 insertions(+), 34 deletions(-)
> 
> diff --git a/lib/arm/setup.c b/lib/arm/setup.c
> index 76aae4627a7b..f96ee04ddd68 100644
> --- a/lib/arm/setup.c
> +++ b/lib/arm/setup.c
> @@ -182,32 +182,57 @@ static void mem_init(phys_addr_t freemem_start)
>   	page_alloc_ops_enable();
>   }
>   
> -void setup(const void *fdt, phys_addr_t freemem_start)
> +static void freemem_push_fdt(void **freemem, const void *fdt)
>   {
> -	void *freemem;
> -	const char *bootargs, *tmp;
>   	u32 fdt_size;
>   	int ret;
>   
> -	assert(sizeof(long) == 8 || freemem_start < (3ul << 30));
> -	freemem = (void *)(unsigned long)freemem_start;
> -
> -	/* Move the FDT to the base of free memory */
>   	fdt_size = fdt_totalsize(fdt);
> -	ret = fdt_move(fdt, freemem, fdt_size);
> +	ret = fdt_move(fdt, *freemem, fdt_size);
>   	assert(ret == 0);
> -	ret = dt_init(freemem);
> +	ret = dt_init(*freemem);
>   	assert(ret == 0);
> -	freemem += fdt_size;
> +	*freemem += fdt_size;
> +}
> +
> +static void freemem_push_dt_initrd(void **freemem)
> +{
> +	const char *tmp;
> +	int ret;
>   
> -	/* Move the initrd to the top of the FDT */
>   	ret = dt_get_initrd(&tmp, &initrd_size);
>   	assert(ret == 0 || ret == -FDT_ERR_NOTFOUND);
>   	if (ret == 0) {
> -		initrd = freemem;
> +		initrd = *freemem;
>   		memmove(initrd, tmp, initrd_size);
> -		freemem += initrd_size;
> +		*freemem += initrd_size;
>   	}
> +}
> +
> +static void initrd_setup(void)
> +{
> +	char *env;
> +
> +	if (!initrd)
> +		return;
> +
> +	/* environ is currently the only file in the initrd */
> +	env = malloc(initrd_size);
> +	memcpy(env, initrd, initrd_size);
> +	setup_env(env, initrd_size);
> +}
> +
> +void setup(const void *fdt, phys_addr_t freemem_start)
> +{
> +	void *freemem;
> +	const char *bootargs;
> +	int ret;
> +
> +	assert(sizeof(long) == 8 || freemem_start < (3ul << 30));
> +	freemem = (void *)(unsigned long)freemem_start;
> +
> +	freemem_push_fdt(&freemem, fdt);
> +	freemem_push_dt_initrd(&freemem);
>   
>   	memregions_init(arm_mem_regions, NR_MEM_REGIONS);
>   	memregions_add_dt_regions(MAX_DT_MEM_REGIONS);
> @@ -229,12 +254,7 @@ void setup(const void *fdt, phys_addr_t freemem_start)
>   	assert(ret == 0 || ret == -FDT_ERR_NOTFOUND);
>   	setup_args_progname(bootargs);
>   
> -	if (initrd) {
> -		/* environ is currently the only file in the initrd */
> -		char *env = malloc(initrd_size);
> -		memcpy(env, initrd, initrd_size);
> -		setup_env(env, initrd_size);
> -	}
> +	initrd_setup();
>   
>   	if (!(auxinfo.flags & AUXINFO_MMU_OFF))
>   		setup_vm();
> @@ -277,7 +297,6 @@ static efi_status_t efi_mem_init(efi_bootinfo_t *efi_bootinfo)
>   	uintptr_t text = (uintptr_t)&_text, etext = ALIGN((uintptr_t)&_etext, 4096);
>   	uintptr_t data = (uintptr_t)&_data, edata = ALIGN((uintptr_t)&_edata, 4096);
>   	const void *fdt = efi_bootinfo->fdt;
> -	int fdt_size, ret;
>   
>   	/*
>   	 * Record the largest free EFI_CONVENTIONAL_MEMORY region
> @@ -344,14 +363,13 @@ static efi_status_t efi_mem_init(efi_bootinfo_t *efi_bootinfo)
>   	}
>   
>   	if (efi_bootinfo->fdt_valid) {
> -		/* Move the FDT to the base of free memory */
> -		fdt_size = fdt_totalsize(fdt);
> -		ret = fdt_move(fdt, (void *)free_mem_start, fdt_size);
> -		assert(ret == 0);
> -		ret = dt_init((void *)free_mem_start);
> -		assert(ret == 0);
> -		free_mem_start += ALIGN(fdt_size, EFI_PAGE_SIZE);
> -		free_mem_pages -= ALIGN(fdt_size, EFI_PAGE_SIZE) >> EFI_PAGE_SHIFT;
> +		unsigned long old_start = free_mem_start;
> +		void *freemem = (void *)free_mem_start;
> +
> +		freemem_push_fdt(&freemem, fdt);
> +
> +		free_mem_start = ALIGN((unsigned long)freemem, EFI_PAGE_SIZE);
> +		free_mem_pages = (free_mem_start - old_start) >> EFI_PAGE_SHIFT;
>   	}
>   
>   	__phys_end &= PHYS_MASK;
> @@ -419,13 +437,8 @@ efi_status_t setup_efi(efi_bootinfo_t *efi_bootinfo)
>   	io_init();
>   
>   	timer_save_state();
> -	if (initrd) {
> -		/* environ is currently the only file in the initrd */
> -		char *env = malloc(initrd_size);
>   
> -		memcpy(env, initrd, initrd_size);
> -		setup_env(env, initrd_size);
> -	}
> +	initrd_setup();
>   
>   	if (!(auxinfo.flags & AUXINFO_MMU_OFF))
>   		setup_vm();