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

Nikos Nikoleris
2024-03-04T10:01:53Z

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)