diff mbox series

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

Message ID 20240227192109.487402-33-andrew.jones@linux.dev (mailing list archive)
State New, archived
Headers show
Series arm64: EFI improvements | expand

Commit Message

Andrew Jones Feb. 27, 2024, 7:21 p.m. UTC 
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>
---
 lib/arm/setup.c | 45 ++++++++++++++++++++-------------------------
 1 file changed, 20 insertions(+), 25 deletions(-)

Comments

Nikos Nikoleris March 4, 2024, 8:10 a.m. UTC | #1 
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)
Andrew Jones March 4, 2024, 9:55 a.m. UTC | #2 
On Mon, Mar 04, 2024 at 08:10:40AM +0000, Nikos Nikoleris wrote:
> 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. While skimming this patch now to remind myself about it for v3,
I see the etext = ALIGN() below which I forgot to consider. We certainly
need the end of the text section to be on a page boundary, but that
doesn't seem to be the case right now. I think we need to add this
change

diff --git a/arm/efi/elf_aarch64_efi.lds b/arm/efi/elf_aarch64_efi.lds
index 836d98255d88..7a4192b77900 100644
--- a/arm/efi/elf_aarch64_efi.lds
+++ b/arm/efi/elf_aarch64_efi.lds
@@ -13,6 +13,7 @@ SECTIONS
     *(.rodata*)
     . = ALIGN(16);
   }
+  . = ALIGN(4096);
   _etext = .;
   _text_size = . - _text;
   .dynamic  : { *(.dynamic) }

Thanks,
drew

> 
> 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)
Nikos Nikoleris March 4, 2024, 10:01 a.m. UTC | #3 
On 04/03/2024 09:55, Andrew Jones wrote:
> On Mon, Mar 04, 2024 at 08:10:40AM +0000, Nikos Nikoleris wrote:
>> 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. While skimming this patch now to remind myself about it for v3,
> I see the etext = ALIGN() below which I forgot to consider. We certainly
> need the end of the text section to be on a page boundary, but that
> doesn't seem to be the case right now. I think we need to add this
> change

I missed and it didn't manifest in any of the runs I did. However, it 
makes sense.

Thanks,

Nikos

> 
> diff --git a/arm/efi/elf_aarch64_efi.lds b/arm/efi/elf_aarch64_efi.lds
> index 836d98255d88..7a4192b77900 100644
> --- a/arm/efi/elf_aarch64_efi.lds
> +++ b/arm/efi/elf_aarch64_efi.lds
> @@ -13,6 +13,7 @@ SECTIONS
>       *(.rodata*)
>       . = ALIGN(16);
>     }
> +  . = ALIGN(4096);
>     _etext = .;
>     _text_size = . - _text;
>     .dynamic  : { *(.dynamic) }
> 
> Thanks,
> drew
> 
>>
>> 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)
diff mbox series

Patch

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)