* Sai Praneeth Prakhya <[email protected]> wrote:
> Presently, in EFI subsystem of kernel, every time kernel allocates memory for
> a
> new EFI memory map, it forgets to free the memory occupied by old EFI memory
> map.
> It does clear the mappings though (using efi_memmap_unmap()), but forgets to
> free up the memory. Also, there is another minor issue, where in the newly
> allocated memory isn't freed, should remap fail.
>
> The first issue is addressed by adding efi_memmap_free() to
> efi_memmap_install()
> and the second issue is addressed by pushing the remap code into
> efi_memmap_alloc() and there by handling the failure condition.
>
> Memory allocated to EFI memory map is leaked in below functions and hence they
> are modified to fix the issue. Functions that modify EFI memmap are:
> 1. efi_clean_memmap(),
> 2. efi_fake_memmap(),
> 3. efi_arch_mem_reserve(),
> 4. efi_free_boot_services(),
> 5. and __efi_enter_virtual_mode()
>
> More detailed explanation:
> --------------------------
> A typical boot flow on EFI supported x86_64 machines might look something like
> below
> 1. EFI memory map is passed by firmware to kernel.
> 2. Kernel does a memblock_reserve() on this memory
> (see efi_memblock_x86_reserve_range()).
> 3. This memory map is checked for invalid entries in efi_clean_memmap(). If
> any
> invalid entries are found, they are omitted from EFI memory map but the
> memory occupied by these invalid EFI memory descriptors isn't freed.
> 3. To further process this memory map (see efi_fake_memmap(), efi_bgrt_init()
> and efi_esrt_init()), kernel allocates memory using efi_memmap_alloc() and
> copies the processed memory map to newly allocated memory but it forgets to
> free memory occupied by old EFI memory map.
> 4. Further, in efi_map_regions() the EFI memory map is processed again to
> include only EFI memory descriptors of type Runtime Code/Data and Boot
> Code/Data. Again, memory is allocated for this new memory map through
> realloc_pages() and the old EFI memory map is not freed.
> 5. After SetVirtualAddressMap() is done, the EFI memory map is processed again
> to have only EFI memory descriptors of type Runtime Code/Data. Again,
> memory
> is allocated for this new memory map through efi_memmap_alloc() and the old
> EFI memory map is not freed.
So it was only halfway through the changelog that I realized that by
'free the memory occupied by old EFI memory map' you didn't mean the EFI
memory map itself (the system RAM described), but the EFI memory map
*table*, this relatively small array of descriptors that we allocate and
reallocate every now and then according to the limitations of the
(largely non-existent yet) early boot memory management system...
Could we please use much more precise terminology when referring to these
entities, in changelogs and code alike? I.e.:
- 'EFI memory map' is the whole map and the contents
- 'EFI memory map table' is the table structure itself, without regard
of its contents
- 'EFI memory map table entry/descriptor' is an entry in the table.
This kind of clarity is what we are using on the e820 memory map side as
well: we have struct e820_table and struct e820_entry.
Ard, would you object to standardizing the EFI code in a similar fashion
as well: efi_mem_table and efi_mem_entry?
Parameters and local variables should be named unambiguously following
these concepts as well. There should be no opaque 'new' variables
*especially* where the primary role of the efi_memmap_insert() API call
is to add a new *entry* - it should be 'new_efi_map_table' (or
'new_table' where it's unambiguous) instead, and new entries added should
be 'new_entry' - etc.
Once this is reorganized and clarified it should be a lot more easy to
review 'table freeing' patches. I can help out with patches if there's no
conceptual objections.
Thanks,
Ingo
