Re: [kvm-unit-tests PATCH v2 13/18] arm64: Simplify efi_mem_init

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

On 27/02/2024 19:21, Andrew Jones wrote:
> Reduce the EFI mem_map loop to only setting flags and finding the
> largest free memory region. Then, apply memregions_split() for
> the code/data region split and do the rest of the things that
> used to be done in the EFI mem_map loop in a separate mem_region
> loop.
> 
> Signed-off-by: Andrew Jones <andrew.jones@linux.dev>

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

Thanks,

Nikos

> ---
>   lib/arm/setup.c | 45 ++++++++++++++++++++-------------------------
>   1 file changed, 20 insertions(+), 25 deletions(-)
> 
> diff --git a/lib/arm/setup.c b/lib/arm/setup.c
> index d0be4c437708..631597b343f1 100644
> --- a/lib/arm/setup.c
> +++ b/lib/arm/setup.c
> @@ -301,9 +301,7 @@ static efi_status_t efi_mem_init(efi_bootinfo_t *efi_bootinfo)
>   	struct efi_boot_memmap *map = &(efi_bootinfo->mem_map);
>   	efi_memory_desc_t *buffer = *map->map;
>   	efi_memory_desc_t *d = NULL;
> -	struct mem_region r;
> -	uintptr_t text = (uintptr_t)&_text, etext = ALIGN((uintptr_t)&_etext, 4096);
> -	uintptr_t data = (uintptr_t)&_data, edata = ALIGN((uintptr_t)&_edata, 4096);
> +	struct mem_region r, *code, *data;
>   	const void *fdt = efi_bootinfo->fdt;
>   
>   	/*
> @@ -337,21 +335,7 @@ static efi_status_t efi_mem_init(efi_bootinfo_t *efi_bootinfo)
>   			r.flags = MR_F_IO;
>   			break;
>   		case EFI_LOADER_CODE:
> -			if (r.start <= text && r.end > text) {
> -				/* This is the unit test region. Flag the code separately. */
> -				phys_addr_t tmp = r.end;
> -
> -				assert(etext <= data);
> -				assert(edata <= r.end);
> -				r.flags = MR_F_CODE;
> -				r.end = data;
> -				memregions_add(&r);
> -				r.start = data;
> -				r.end = tmp;
> -				r.flags = 0;
> -			} else {
> -				r.flags = MR_F_RESERVED;
> -			}
> +			r.flags = MR_F_CODE;
>   			break;
>   		case EFI_CONVENTIONAL_MEMORY:
>   			if (free_mem_pages < d->num_pages) {
> @@ -361,15 +345,27 @@ static efi_status_t efi_mem_init(efi_bootinfo_t *efi_bootinfo)
>   			break;
>   		}
>   
> -		if (!(r.flags & MR_F_IO)) {
> -			if (r.start < __phys_offset)
> -				__phys_offset = r.start;
> -			if (r.end > __phys_end)
> -				__phys_end = r.end;
> -		}
>   		memregions_add(&r);
>   	}
>   
> +	memregions_split((unsigned long)&_etext, &code, &data);
> +	assert(code && (code->flags & MR_F_CODE));
> +	if (data)
> +		data->flags &= ~MR_F_CODE;
> +
> +	for (struct mem_region *m = mem_regions; m->end; ++m) {
> +		if (m != code && (m->flags & MR_F_CODE))
> +			m->flags = MR_F_RESERVED;
> +
> +		if (!(m->flags & MR_F_IO)) {
> +			if (m->start < __phys_offset)
> +				__phys_offset = m->start;
> +			if (m->end > __phys_end)
> +				__phys_end = m->end;
> +		}
> +	}
> +	__phys_end &= PHYS_MASK;
> +
>   	if (efi_bootinfo->fdt_valid) {
>   		unsigned long old_start = free_mem_start;
>   		void *freemem = (void *)free_mem_start;
> @@ -380,7 +376,6 @@ static efi_status_t efi_mem_init(efi_bootinfo_t *efi_bootinfo)
>   		free_mem_pages = (free_mem_start - old_start) >> EFI_PAGE_SHIFT;
>   	}
>   
> -	__phys_end &= PHYS_MASK;
>   	asm_mmu_disable();
>   
>   	if (free_mem_pages == 0)